fix(codex): route resume/fork through the canonical home and bound helper shutdown (#647)

[ndycode]

Summary

• Fixes mcodex resume / mcodex fork hanging on a blank TUI when runtime rotation is enabled, by routing both through the canonical-home app-helper transport instead of the ephemeral shadow home.
• Makes detached app-helper shutdown bounded, so the wrapper always returns to the shell after an interrupted or nonzero Codex exit.
• Forwards every request command's --help straight to the official CLI, so help never starts a proxy, a shadow home, or a helper.
• Closes [bug] mcodex resume hangs with runtime rotation, and helper can prevent exit #647.

What Changed

Routing (scripts/codex.js). resume and fork are interactive TUI entry points, but they carry a forwarded subcommand, so isCodexInteractiveTuiCommand — which only matches a bare invocation — missed them and they fell through to the shadow-home proxy path. The shadow mirror deliberately omits the runtime SQLite state (isRuntimeRotationShadowHomeOmittedEntry), so a shadow home only ever holds a partial thread index rebuilt from the linked sessions directory, and the requested thread was simply absent. That matches the reporter's evidence exactly: canonical state_5.sqlite held 257 threads including the target, while the two shadow databases held 126 and 128 and did not.

A dedicated predicate isCodexInteractiveResumeCommand now classifies them, and createRuntimeRotationProxyContextIfEnabled routes them to createRuntimeRotationAppHelperContext with the same { detachOnExit: true, useCanonicalHome: true } the bare TUI already uses. isCodexInteractiveTuiCommand keeps its exact meaning, and shouldUseRuntimeRoutingForForwardedArgs is untouched — resume/fork stay in requestCommands, so account rotation stays enabled. This is the transport change only; nothing is dropped by leaving the shadow path, since proxyAppServerAccountRead is only set for app-server, which these are not.

Shutdown (scripts/codex.js). stopRuntimeRotationAppHelper sent SIGTERM, stopped waiting after the 2s graceful window, and returned with the helper and its piped stdio still referenced. Because the helper is spawned stdio: ["ignore", "pipe", "pipe"], those pipes keep the wrapper's event loop alive on their own — the wrapper sets process.exitCode and relies on the loop draining, so it never exited. Shutdown now escalates to SIGKILL past the graceful window and then unconditionally destroys and unrefs the helper's streams. The stream teardown is the load-bearing part on Windows, where the signals are emulated as unconditional termination. waitForRuntimeRotationAppHelperExit also removes its own close listener when the timeout wins, and an already-exited helper now short-circuits instead of waiting out the full window.

Help short-circuit (scripts/codex.js). Moving these commands to the interactive branch had a side effect: that branch passes detachOnExit: true, and help always exits 0, so resume --help would spawn a detached helper and leave it idling for its full timeout. Before this PR the same invocation built a shadow home and a proxy, but both were torn down when the wrapper exited — so this was a new leak, not a pre-existing cost. shouldUseRuntimeRoutingForForwardedArgs now short-circuits the whole transport for the help form of any request command, matching the app --help and app-server --help precedent already in that function. Raised by Greptile in review, then extended from app/resume/fork to exec/review for consistency: those never stranded a helper, but they did mirror an entire shadow Codex home just to print help. The rule is now one sentence — a request command that is only printing help makes no model requests, so it needs no transport — and it is keyed off the help flag rather than the command, so real runs are untouched.

Docs. docs/development/ARCHITECTURE.md transport table and docs/development/CONFIG_FLOW.md §6 list resume/fork under the canonical-home path, with the reasoning and the bounded-shutdown contract recorded.

Validation

• npm run lint
• npm run typecheck (plus npm run typecheck:scripts)
• npm test
• npm test -- test/documentation.test.ts (32/32)
• npm run build

Fifteen regression tests were added, and each was confirmed to fail against the unfixed wrapper rather than merely pass against the fixed one:

Test	Pre-fix behavior
canonical home for resume	RESUME_HOME_IS_ORIGINAL:false — took the shadow path
canonical home for fork	RESUME_HOME_IS_ORIGINAL:false — took the shadow path
exec stays on shadow home	passes before and after (guards against over-widening)
review stays on shadow home	passes before and after (guards against over-widening)
wrapper returns when helper stdio outlives the window	hung 25.1s until the process holding the pipes exited
force-stops a helper that ignores SIGTERM (POSIX only)	run never completed; had to be killed
resume --help / resume -h start no transport	started a helper that idled on after exit
fork --help / fork -h start no transport	started a helper that idled on after exit
exec --help / exec -h start no transport	built a shadow home and started a proxy
review --help / review -h start no transport	built a shadow home and started a proxy
resume <session-id> still starts the proxy	passes before and after (guards the short-circuit from over-matching)

Full suite, both platforms:

• Windows: 5288 passed, 4 skipped, 0 failed.
• Linux (node:24 container): 5267 passed, 19 failed. The unmodified origin/main tree scores 5252 passed, the same 19 failed in that container, so those are pre-existing environment failures (platform-specific and root-user tests), not regressions. The delta is exactly +15 tests, all passing.

Both shutdown tests bound their spawnSync call (runWrapper's opt-in timeoutMs) and assert the wrapper actually returned. spawnSync blocks the worker thread, so Vitest's testTimeout cannot interrupt it — without the bound, a shutdown regression hangs the whole run instead of failing. Against the unfixed wrapper both now fail in ~12s naming the cause. Only these two tests opt in, so the rest of the file is unchanged.

The SIGTERM-escalation test is skipped on win32: child.kill() there is always an unconditional terminate, so a helper cannot ignore it and the escalation is unreachable. The cross-platform stdio test covers the same failure on Windows. Both were verified on real Linux, not just reasoned about.

Docs and Governance Checklist

• README updated (if user-visible behavior changed)
• docs/getting-started.md updated (if onboarding flow changed)
• docs/features.md updated (if capability surface changed)
• relevant docs/reference/* pages updated — maintainer docs ARCHITECTURE.md and CONFIG_FLOW.md updated; no user-facing command, setting, or path changed
• docs/upgrade.md updated (if migration behavior changed)
• SECURITY.md and CONTRIBUTING.md reviewed for alignment

No CHANGELOG entry: this repo's changelog is written by chore(release) commits, matching #639, the PR that introduced this canonical-home path.

Risk and Rollback

• Risk level: low-to-moderate. The transport for two commands changes. resume/fork now read and write the real CODEX_HOME in place rather than a per-session copy, so a crash mid-session can no longer be discarded with the shadow directory. That is how the stock CLI already behaves, and how the bare interactive TUI has behaved since fix(codex): avoid runtime shadow reindex on TUI startup #639.
• Concurrency, called out deliberately: the detached helper and the resumed session both run against the canonical home. This is not new — the interactive TUI branch has done exactly this since fix(codex): avoid runtime shadow reindex on TUI startup #639, and test/codex-bin-wrapper.test.ts already proves two overlapping canonical sessions each keep their state. This PR widens that existing exposure to two more commands rather than introducing it.
• Rollback: revert the commit. The change is self-contained in scripts/codex.js; no persisted state, config schema, or on-disk format changes, so no migration is involved.

Additional Notes

The issue reporter diagnosed this correctly and their patch shape is what landed, with two deliberate differences. First, the routing uses a separate predicate rather than widening isCodexInteractiveTuiCommand, so that function's name stays accurate and the new branch is independently testable. Second, the stream teardown is unconditional and an already-exited helper short-circuits the wait, which avoids charging a 2s delay on the common path.

One thing worth flagging for reviewers: the original proposed patch guarded on helper.killed and returned early, which would skip teardown entirely on a second stop call. The version here keys off exitCode/signalCode instead, so a helper that is killed but still alive continues to escalation.

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

the pr routes interactive resume and fork commands through the canonical codex home and makes helper shutdown bounded, including windows stream cleanup. it also fixes the prior help-only helper leak without changing real request routing; concurrent canonical-home sessions remain explicitly supported.

• adds canonical-home routing for resume and fork.
• escalates stalled helper shutdown and releases piped stdio.
• bypasses runtime transport for request-command help forms.
• adds vitest coverage for routing, help forwarding, shutdown bounds, and concurrent behavior.

Confidence Score: 5/5

the pr appears safe to merge.

the prior help-only helper leak is fixed by bypassing runtime transport for help forms, while real resume, fork, exec, and review invocations retain their intended routing; no blocking failure remains.

Important Files Changed

Filename	Overview
scripts/codex.js	routes interactive resume and fork through the canonical home, safely short-circuits help, and bounds helper shutdown with cross-platform stream release.
test/codex-bin-wrapper.test.ts	adds focused vitest coverage for canonical versus shadow routing, help forwarding, posix escalation, windows-compatible stdio release, and bounded wrapper return.
docs/development/ARCHITECTURE.md	documents canonical-home routing, bounded shutdown, windows behavior, and the concurrency contract.
docs/development/CONFIG_FLOW.md	updates the runtime transport selection flow for resume and fork.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  args[forwarded codex arguments] --> help{help-only request command?}
  help -->|yes| direct[official cli without runtime transport]
  help -->|no| interactive{bare tui, resume, or fork?}
  interactive -->|yes| canonical[canonical home app helper]
  interactive -->|no| shadow[shadow home runtime proxy]
  canonical --> exit{clean cli exit?}
  exit -->|yes| detach[detach helper]
  exit -->|no| stop[sigterm, bounded wait, sigkill, release streams]

Loading

_{Reviews (4): Last reviewed commit: "fix(codex): skip the rotation transport ..." | Re-trigger Greptile}

Context used:

• Knowledge Base — Build and Packaging

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

Warning

Review limit reached

@ndycode, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 069c548e-3f4a-4d85-9856-e84924c320ad

📥 Commits

Reviewing files that changed from the base of the PR and between 9467b2f and afeb427.

📒 Files selected for processing (2)

• scripts/codex.js
• test/codex-bin-wrapper.test.ts

📝 Walkthrough

major fix with no stated security or data-loss risk. scripts/codex.js routes interactive resume and fork through canonical CODEX_HOME, preventing blank tuis during runtime rotation. detached helper shutdown escalates from sigterm to sigkill and releases streams within a bounded time.

regression tests in test/codex-bin-wrapper.test.ts cover canonical-home routing, help bypasses, open stdio, and forced shutdown. reviewers should focus on canonical-home transport, helper lifecycle, windows signal behavior, and concurrent helper termination.

Walkthrough

runtime rotation now routes interactive resume and fork commands through canonical CODEX_HOME. app-helper shutdown now uses bounded graceful and forceful termination with unconditional resource cleanup. tests cover routing, shadow-home preservation, open stdio, and force-kill behavior.

Changes

runtime rotation lifecycle

Layer / File(s)	Summary
canonical interactive command routing scripts/codex.js:4210, scripts/codex.js:4417, scripts/codex.js:4442, scripts/codex.js:4471, docs/development/ARCHITECTURE.md:170, docs/development/CONFIG_FLOW.md:77, test/codex-bin-wrapper.test.ts:2953	resume and fork now use canonical-home transport. help-only invocations bypass runtime startup. tests also preserve shadow-home routing for exec and review.
bounded app-helper shutdown scripts/codex.js:4006, docs/development/ARCHITECTURE.md:184, test/codex-bin-wrapper.test.ts:3151	shutdown removes listeners, waits for SIGTERM, escalates to SIGKILL, destroys stdio, and unreferences the child. tests cover inherited stdio and helpers that ignore graceful termination.
shutdown test infrastructure test/codex-bin-wrapper.test.ts:311, test/codex-bin-wrapper.test.ts:360, test/codex-bin-wrapper.test.ts:648	fixtures can spawn detached grandchildren and block close operations. wrapper execution now enforces bounded synchronous timeouts.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• ndycode/codex-multi-auth#462: overlaps in runtime-rotation routing and app-helper lifecycle.
• ndycode/codex-multi-auth#639: extends canonical-home routing into resume and fork.
• ndycode/codex-multi-auth#643: shares canonical CODEX_HOME routing and wrapper test coverage.

Suggested labels: bug

Suggested reviewers: fnmendez

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	the title describes the main routing and shutdown changes, but it is 89 characters and exceeds the 72-character limit.	remove the issue suffix and shorten the summary to 72 characters or fewer, for example: fix(codex): route resume/fork through canonical home.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	the changes address both issue objectives: canonical-home routing and bounded helper shutdown, with targeted regression coverage in test/codex-bin-wrapper.test.ts:1.
Out of Scope Changes check	✅ Passed	the changes remain within issue #647: runtime routing, helper shutdown, related documentation, and targeted tests in test/codex-bin-wrapper.test.ts:1.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	the description covers the required sections, validation, regression tests, windows cleanup, concurrency risks, documentation, and rollback details.

📋 Issue Planner

Built with CodeRabbit's Coding Plans for faster development and fewer bugs.

View plan used: #647

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/resume-fork-canonical-home

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix/resume-fork-canonical-home

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test/codex-bin-wrapper.test.ts (1)

3128-3172: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

bound the subprocess tests.

the new shutdown regressions intentionally exercise helpers that keep stdio open or ignore SIGTERM. the shared runWrapper() at test/codex-bin-wrapper.test.ts:644-656 calls spawnSync() without a timeout. if cleanup regresses, Vitest hangs instead of reporting a failed assertion. add a timeout slightly above the documented shutdown bound and assert that the child did not time out.

As per path instructions, tests must stay deterministic.

Also applies to: 3174-3232

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/codex-bin-wrapper.test.ts` around lines 3128 - 3172, Update the shared
runWrapper() helper used by the shutdown regression tests to pass spawnSync() a
timeout slightly longer than the documented shutdown window, then assert that
the returned status is not the timeout indicator before existing status and
elapsed-time assertions. Apply this protection to both affected subprocess tests
while preserving their current deterministic expectations.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@test/codex-bin-wrapper.test.ts`:
- Around line 3128-3172: Update the shared runWrapper() helper used by the
shutdown regression tests to pass spawnSync() a timeout slightly longer than the
documented shutdown window, then assert that the returned status is not the
timeout indicator before existing status and elapsed-time assertions. Apply this
protection to both affected subprocess tests while preserving their current
deterministic expectations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: dca29391-6415-4c0d-ac00-2b19f7856af7

📥 Commits

Reviewing files that changed from the base of the PR and between 3c44603 and 57cfd1f.

📒 Files selected for processing (2)

• scripts/codex.js
• test/codex-bin-wrapper.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout. (1)

• GitHub Check: Greptile Review

🧰 Additional context used

📓 Path-based instructions (9)

**/*.{ts,js,mjs}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,js,mjs}: Use ESM modules throughout the project; the package is configured with "type": "module".
Do not use as any, @ts-ignore, or @ts-expect-error.

Files:

• scripts/codex.js
• test/codex-bin-wrapper.test.ts

scripts/codex*.js

📄 CodeRabbit inference engine (AGENTS.md)

The wrapper must not reimplement general Codex commands; authentication commands are handled locally and non-authentication commands must forward to the official Codex CLI.

Files:

• scripts/codex.js

scripts/**/*.js

📄 CodeRabbit inference engine (AGENTS.md)

Windows-sensitive cleanup and write operations must retry transient EBUSY, EPERM, and ENOTEMPTY failures where applicable.

Files:

• scripts/codex.js

**/*

📄 CodeRabbit inference engine (AGENTS.md)

Source changes belong in index.ts, lib/, and scripts/; dist/ is generated output and local temporary/cache directories must not be edited.

Files:

• scripts/codex.js
• test/codex-bin-wrapper.test.ts

**/*.{js,ts,mjs,cjs}

📄 CodeRabbit inference engine (README.md)

**/*.{js,ts,mjs,cjs}: Do not publish or replace a global codex binary; official OpenAI installation paths must retain ownership of the codex command.
Keep OAuth credentials local and restrict runtime rotation and local bridges to loopback interfaces.
Require hashed local client tokens to protect the optional loopback bridge.
Responses background: true compatibility must remain opt-in; requests using it must use stateful store=true routing rather than stateless store=false routing.
Never run npm install or update commands automatically; only display a manual upgrade notice when appropriate.
Experimental synchronization and backup flows must be non-destructive by default: preview before applying sync, preserve destination-only accounts, and fail safely on backup filename collisions.
Keep account storage project-scoped under the configured multi-auth root when operating in repo-specific workflows.

Files:

• scripts/codex.js
• test/codex-bin-wrapper.test.ts

test/**/*.test.ts

📄 CodeRabbit inference engine (test/AGENTS.md)

test/**/*.test.ts: Write Vitest test suites with globals enabled (describe, it, expect)
Maintain 80%+ coverage threshold across statements, branches, functions, and lines
Use removeWithRetry() for Windows filesystem cleanup instead of bare fs.rm to handle EBUSY, EPERM, and ENOTEMPTY errors
Do not rely on dist/ in tests; use source files instead
Do not skip tests without justification
Relax lint rules for test files as configured in eslint.config.js

Files:

• test/codex-bin-wrapper.test.ts

test/**/codex-bin-wrapper.test.ts

📄 CodeRabbit inference engine (test/AGENTS.md)

Test bin wrapper lazy-load and missing dist handling with concurrent invocations in codex-bin-wrapper.test.ts

Files:

• test/codex-bin-wrapper.test.ts

test/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Windows-sensitive filesystem tests and helpers must use retry handling for transient lock-related cleanup and write failures.

Files:

• test/codex-bin-wrapper.test.ts

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

• test/codex-bin-wrapper.test.ts

🧠 Learnings (3)

📓 Common learnings

Learnt from: CR
Repo: ndycode/codex-multi-auth

Timestamp: 2026-08-02T07:54:39.822Z
Learning: Runtime rotation must remain enabled by default, local, reversible, and loopback-only.

Learnt from: CR
Repo: ndycode/codex-multi-auth

Timestamp: 2026-08-02T07:54:48.418Z
Learning: Resolve the runtime root directory in this order: `CODEX_MULTI_AUTH_DIR`; an explicit non-default `CODEX_HOME` at `$CODEX_HOME/multi-auth`; existing account-storage roots under `CODEX_HOME` or `~/.codex`; canonical `~/.codex/multi-auth`; and legacy paths only when storage signals exist.

Learnt from: CR
Repo: ndycode/codex-multi-auth

Timestamp: 2026-08-02T07:54:48.418Z
Learning: Read `dashboardDisplaySettings` and `pluginConfig` from `settings.json`, while retaining legacy compatibility loading and migration.

Learnt from: CR
Repo: ndycode/codex-multi-auth

Timestamp: 2026-08-02T07:54:48.418Z
Learning: Resolve `pluginConfig` values using this precedence: existing `CODEX_MULTI_AUTH_CONFIG_PATH` file, valid unified `settings.json` configuration, legacy compatibility configuration, and `DEFAULT_PLUGIN_CONFIG`; then apply environment-variable overrides per setting.

Learnt from: CR
Repo: ndycode/codex-multi-auth

Timestamp: 2026-08-02T07:54:48.418Z
Learning: Ignore a configured but nonexistent `CODEX_MULTI_AUTH_CONFIG_PATH` during load, while creating it on the first save if the variable remains set. Resolve dashboard display values from persisted settings followed by normalization and defaults.

Learnt from: CR
Repo: ndycode/codex-multi-auth

Timestamp: 2026-08-02T07:54:48.418Z
Learning: Resolve account storage by selecting the root directory, using the global accounts file by default, using a project-namespaced path when project-scoped mode is active, and attempting legacy project-file migration when applicable.

Learnt from: CR
Repo: ndycode/codex-multi-auth

Timestamp: 2026-08-02T07:54:48.418Z
Learning: Normalize standalone and wrapper command forms before dispatch; run auth-manager commands locally, forward out-of-scope wrapper commands to the official Codex CLI, and check runtime rotation for forwarded request-bearing commands.

Learnt from: CR
Repo: ndycode/codex-multi-auth

Timestamp: 2026-08-02T07:54:48.418Z
Learning: For runtime rotation, honor `CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY` before `pluginConfig.codexRuntimeRotationProxy`, bypass rotation when disabled or for help/non-requesting commands, use a per-process token, select the appropriate canonical or shadow `CODEX_HOME` transport, forward to official Codex, rotate on relevant failures before streaming, and clean up or synchronize shadow state on exit.

Learnt from: CR
Repo: ndycode/codex-multi-auth

Timestamp: 2026-08-02T07:54:48.418Z
Learning: For plugin-host requests, transform requests for Codex compatibility, resolve candidates using health/cooldown/quota/affinity, apply timeout and retry policy, perform failover and rotation decisions, and persist account, cache, and session updates.

Learnt from: CR
Repo: ndycode/codex-multi-auth

Timestamp: 2026-08-02T07:54:48.418Z
Learning: For unsupported model or entitlement failures, record entitlement-cache state, penalize the account/model capability pair, apply fallback-model policy when enabled, and re-evaluate scoring and retries.

Learnt from: CR
Repo: ndycode/codex-multi-auth

Timestamp: 2026-08-02T07:54:48.418Z
Learning: Detect account-file updates with a watcher, debounce and reload the in-memory account manager, and keep session-affinity and guardian processes synchronized with the updated state.

📚 Learning: 2026-06-04T06:14:18.093Z

Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/scheduling-strategy-config.test.ts:1-1
Timestamp: 2026-06-04T06:14:18.093Z
Learning: In ndycode/codex-multi-auth, do not flag explicit imports from "vitest" (e.g., describe, it, expect, beforeEach/afterEach, etc.) in test files as issues—even if the Vitest config sets `globals: true`. The repo’s established convention is to keep these imports for consistency with neighboring tests; removing them would make files outliers.

Applied to files:

• test/codex-bin-wrapper.test.ts

📚 Learning: 2026-06-04T06:14:24.975Z

Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/runtime-rotation-proxy.test.ts:2478-2491
Timestamp: 2026-06-04T06:14:24.975Z
Learning: In ndycode/codex-multi-auth test files (e.g. `test/*.test.ts`), when creating V3 storage fixtures for accounts, it’s an intentional convention to use `as never` for deliberately minimal stored-account objects that only include `refreshToken`, `addedAt`, and `lastUsed`. Do not treat `as never` here as a type-safety problem: optional/other fields are expected to be populated by the runtime during execution, and the cast is used solely to keep the fixture minimal and consistent across existing tests.

Applied to files:

• test/codex-bin-wrapper.test.ts

🪛 ast-grep (0.45.0)

test/codex-bin-wrapper.test.ts

[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { type SpawnSyncReturns, spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(detect-child-process-typescript)

🔇 Additional comments (4)

scripts/codex.js (2)

4006-4058: LGTM!

---

4210-4213: LGTM!

Also applies to: 4417-4427, 4442-4445, 4471-4479

test/codex-bin-wrapper.test.ts (2)

3174-3232: 🩺 Stability & Availability

verify the force-kill path on windows.

test/codex-bin-wrapper.test.ts:3174-3232 is posix-only, but the wrapper also runs on windows. verify that the SIGTERM wait, SIGKILL escalation, child-exit detection, and stream cleanup in scripts/codex.js:4006-4058 remain bounded on windows. add a windows case or an explicit platform-specific implementation if signal semantics differ.

As per path instructions, tests under test/** must cover windows-sensitive behavior.

Source: Path instructions

---

311-311: LGTM!

Also applies to: 360-396, 2930-3001, 3003-3049, 3051-3088, 3090-3126

[ndycode]

Addressed in 9467b2f.

Valid and worth fixing. spawnSync blocks the worker thread, so Vitest's testTimeout cannot interrupt it — this is not hypothetical, it stalled a verification run of this branch against the unfixed wrapper and had to be killed manually.

runWrapper now takes an opt-in timeoutMs (with killSignal: "SIGKILL"), and the two shutdown tests assert result.error is unset before their status and elapsed checks. Only those two pass it, so every other test in the file keeps its existing unbounded behavior rather than inheriting a new global timeout.

Verified against the unfixed wrapper on both platforms — each now fails in ~12s with a message naming the cause, instead of hanging:

× returns to the shell when the app helper's stdio outlives its shutdown window (#647) 12035ms
  → wrapper never returned within 12000ms: a leaked grandchild still holds its stdio

× force-stops an app helper that ignores SIGTERM (#647) 12024ms
  → wrapper never returned within 12000ms: the helper ignored SIGTERM

The second of those previously hung indefinitely.

Full suite after the change: Windows 5284 passed / 0 failed; Linux 5263 passed with the same 19 failures the unmodified origin/main tree produces in that container.

[ndycode]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.