fix(codex): reap app helpers stranded by the detach grace - #665
Conversation
The idle reaper's owner check was a bare kill(pid, 0), which answers "does a process hold this integer", never "is this still my launcher". A recycled PID at one tick pushes the deadline forward 12 hours, and the deadline only ever moves forward, so one false positive is never corrected — helpers were observed 33 hours past their timeout, 183 concurrent, 5.6 GB RSS. Owner liveness is now PID plus the launcher's kernel start time (read under LC_ALL=C so locale cannot disable the check), re-verified at most once a minute; a failed re-read keeps the previous verdict instead of declaring a live owner dead, and where no start time is known the check degrades to bare liveness. An absolute lifetime ceiling (24h default, CODEX_MULTI_AUTH_APP_ROTATION_MAX_LIFETIME_MS) bounds the leak if activity accounting is ever wrong again. Status telemetry is per helper PID instead of N writers last-writer- winning one file at 1 Hz, published on change plus heartbeat; readers prefer the newest live helper and still read the legacy path, and app-bind unbind walks every per-PID candidate through the same ownership-verified stop it applied to the shared file. Helpers remove their owner file on exit; launchers sweep metadata whose helper PID is dead — or provably recycled, by comparing the PID's kernel start time against the file's own timestamps — before spawning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FW2tPyLdcRXrnGsYVVJeEj
Shared per-PID status discovery moves next to its filename constant (listRuntimeHelperStatusPaths) and all three readers use it; rotation status derives selection and live count from one scan; only "running" counts as running so max-lifetime/error stamps read as terminal. The helper's identity probe is async and single-flight so a wedged ps stalls a background probe, never the proxy event loop. Metadata deletions retry transient Windows locks; the launch-path sweep memoizes identity probes per PID and caps them per sweep. app-bind unbind logs when it cannot enumerate per-PID files and retries the readdir. Tests: multi-helper and ownership-preservation unbind cases, publish-rate regression, sweep retry regression, max-lifetime status case, POSIX gate plus a Windows companion for the identity-unavailable degradation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FW2tPyLdcRXrnGsYVVJeEj
…er-independent Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FW2tPyLdcRXrnGsYVVJeEj
The detach grace hands a helper off optimistically: any launcher that exits cleanly within the window leaves its helper running. Nothing then checks whether a consumer actually took the handoff, so every short forwarded command strands a helper that holds the full idle timeout — 12h by default — with a dead owner, no traffic, and nothing connected. Owner death only stopped refreshing the idle clock; it never shortened it. Observed locally at ~3 stranded helpers per 15 minutes, ~1.1GB resident across 23 of them. Once the owner is confirmed dead the deadline becomes CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS (default 15m, 0 restores the previous behavior), and it fires only while the proxy reports zero open client connections — so a consumer that really did take the handoff, such as `codex app` giving the desktop app its proxy, is never reaped out from under. The proxy already tracked its socket set for shutdown; it now reports the count. A revived owner verdict clears the detached clock rather than ratcheting it, and the published idleExpiresAt reports whichever deadline is actually enforced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gn9SDorCbELwn5wchTnyL8
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (16)docs/**/*.md📄 CodeRabbit inference engine (docs/STYLE_GUIDE.md)
Files:
docs/reference/**/*.md📄 CodeRabbit inference engine (docs/STYLE_GUIDE.md)
Files:
docs/**/*.{md,mdx}📄 CodeRabbit inference engine (docs/troubleshooting.md)
Files:
docs/**⚙️ CodeRabbit configuration file
Files:
docs/{index.md,getting-started.md,faq.md,architecture.md,features.md,configuration.md,troubleshooting.md,privacy.md,upgrade.md}📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Files:
**/*.{js,ts,mjs,cjs}📄 CodeRabbit inference engine (README.md)
Files:
**/*.{ts,js,mjs}📄 CodeRabbit inference engine (AGENTS.md)
Files:
scripts/codex*.js📄 CodeRabbit inference engine (AGENTS.md)
Files:
scripts/**/*.{js,mjs}📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/development/**/*.md📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Files:
docs/development/**/*📄 CodeRabbit inference engine (docs/development/CONFIG_FLOW.md)
Files:
docs/development/**/*.{md,mdx}📄 CodeRabbit inference engine (docs/development/TESTING.md)
Files:
test/**/*.test.ts📄 CodeRabbit inference engine (test/AGENTS.md)
Files:
test/**/codex-bin-wrapper.test.ts📄 CodeRabbit inference engine (test/AGENTS.md)
Files:
test/**/*.{ts,js,mjs}📄 CodeRabbit inference engine (AGENTS.md)
Files:
test/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2026-06-04T06:14:18.093ZApplied to files:
📚 Learning: 2026-06-04T06:14:24.975ZApplied to files:
🪛 ast-grep (0.45.1)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. (detect-child-process-typescript) [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. (detect-child-process-typescript) [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. (detect-child-process-typescript) [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. (detect-child-process-typescript) [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. (detect-child-process-typescript) [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. (detect-child-process-typescript) 🪛 LanguageTooldocs/development/ARCHITECTURE.md[style] ~209-~209: Consider using “who” when you are referring to a person instead of an object. (THAT_WHO) 🔇 Additional comments (6)
📝 Walkthroughseverity: minor. this pr bounds runtime-helper lifetime after launcher detachment and adds connection-aware cleanup. no security or data-loss risk is evident. regression tests cover detached cleanup, active connections, invalid connection counts, windows owner death, readiness-failure cleanup, and metadata cleanup. reviewers should focus on owner PID/start-time validation and the per-process status-file model in
Walkthroughruntime rotation helpers now use PID-scoped status and owner files. helpers validate process identity, enforce lifetime limits, track connections, publish terminal state, and remove stale metadata. status consumers and unbind cleanup now handle multiple helpers. Changesruntime helper lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Launcher
participant Helper
participant Proxy
participant StatusFiles
Launcher->>Helper: start with owner PID and start time
Helper->>Proxy: query traffic and open connections
Helper->>StatusFiles: write PID-scoped status heartbeat
Helper->>StatusFiles: write terminal state and remove owner metadata
Launcher->>StatusFiles: sweep stale metadata before spawning
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
test/codex-bin-wrapper.test.tsOops! Something went wrong! :( ESLint: 10.8.1 Error: The 'jiti' library is required for loading TypeScript configuration files. Make sure to install it. 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. Comment |
A proxy shape whose getOpenConnectionCount() returns a non-finite value compared unequal to 0 and blocked the detached reap forever — failing in the one direction this fix cannot afford, and silently restoring the leak for that shape. Unknown now degrades exactly like a missing method does. Also proves the other half of the contract with a test: a detached consumer that reconnects per request, holding no socket between them, keeps its helper alive on the traffic alone and loses it once the traffic stops. The fixture proxy grows a ramped request counter, because a static counter cannot express "traffic is still arriving". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gn9SDorCbELwn5wchTnyL8
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/codex-manager/commands/rotation.ts (1)
620-651: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick windrop the default parameters so the single-scan invariant cannot be bypassed.
status = readAppRuntimeHelperStatus()andliveHelperCount = status ? 1 : 0are evaluated independently. a caller that passes neither triggers a fresh directory scan and then reports a live count of1, even when three helpers are running. that is the exact mismatch the comment at lines 706-707 says the single scan prevents. the only production caller already passes both arguments.make both parameters required.
♻️ proposed refactor
function formatAppRuntimeHelperStatus( now: number, - status = readAppRuntimeHelperStatus(), - liveHelperCount = status ? 1 : 0, + status: AppRuntimeHelperStatus | null, + liveHelperCount: number, ): string {🤖 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 `@lib/codex-manager/commands/rotation.ts` around lines 620 - 651, Update formatAppRuntimeHelperStatus so status and liveHelperCount are required parameters with no default initializers. Preserve the existing single-scan contract by requiring callers to provide both the scanned helper status and matching live-helper count, including the existing production caller.
🤖 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.
Inline comments:
In `@AGENTS.md`:
- Line 139: Update the “App helper status” storage note in AGENTS.md to also
document the per-helper owner file runtime-rotation-app-helper-owner.<pid>.json,
alongside the existing status file entry, while retaining the legacy un-suffixed
status-file note.
In `@docs/configuration.md`:
- Around line 76-77: Update the settings inventory in docs/reference/settings.md
alongside CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS to document
CODEX_MULTI_AUTH_APP_ROTATION_MAX_LIFETIME_MS and
CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS, including their defaults and 0
semantics. Check the platform support for these settings and add Windows
coverage to the detached-helper tests around the relevant test block in
test/codex-bin-wrapper.test.ts if they support win32.
In `@docs/development/ARCHITECTURE.md`:
- Around line 200-201: Restructure the helper self-reaping paragraph into a
brief introductory summary followed by scan-friendly bullets covering identity
verification, recheck/degraded behavior, lifetime ceiling, detached idle
handling and connection gating, and per-process telemetry. In the internal
environment-variable enumeration, add
CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS alongside
CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID, keeping the documented names aligned
with runtime usage.
In `@docs/development/CONFIG_FIELDS.md`:
- Around line 270-273: Add CODEX_MULTI_AUTH_APP_ROTATION_MAX_LIFETIME_MS and
CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS to the public settings reference,
documenting their 24h/15m defaults and 0 semantics. Keep
CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID and
CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS documented only in the
development configuration table. Extend the wrapper test coverage for Windows
detached-helper scenarios and add a concurrent app-helper lifecycle regression
covering helper metadata and reaping.
In `@docs/privacy.md`:
- Line 33: Add the missing runtime-rotation-app-helper-owner artifact to both
storage inventories: in docs/privacy.md at lines 33-33, add a canonical
local-files row describing
~/.codex/multi-auth/runtime-rotation-app-helper-owner.<pid>.json as the owner
identity token with launcher PID, removed on helper exit and swept when the PID
is dead; in AGENTS.md at lines 139-139, add the corresponding storage note
beside the existing app helper status entry.
In `@lib/codex-manager/commands/rotation.ts`:
- Around line 576-587: The helper-selection rule must be centralized and verify
process identity, not only PID liveness. In
lib/codex-manager/commands/rotation.ts:576-587, parse startedAt in
readAppRuntimeHelperStatusFile and replace
liveAppRuntimeHelpers/selectAppRuntimeHelperStatus with the shared
identity-aware selector; in lib/runtime/runtime-current-account.ts:168-188, use
that same selector instead of the local recency sort and live filter. Add a
regression test with two live per-PID files and a stale recycled-PID legacy
file, asserting both readers select the same helper.
In `@lib/runtime/app-bind.ts`:
- Line 1652: Update the cleanup loop that processes helperCleanupPaths to wrap
each unlinkIfExists operation with the existing withFileOperationRetry helper,
matching the retry behavior used by the nearby readdir and rm operations while
preserving the loop’s best-effort error handling.
- Around line 1560-1598: Make helper status and owner removal
ownership-consistent in the unbind logic around the running and non-running
branches: preserve both files and emit the existing mismatch warning whenever
helperOwnershipMatches is false, rather than deleting status alone; retain
removal of both files when ownership matches and the process is confirmed dead.
Add Vitest coverage in app-bind tests for mismatched-token non-running records,
dead-PID running records, and non-ENOENT readdir failures verifying warning
emission and legacy-path checking.
In `@scripts/codex.js`:
- Around line 4322-4329: Update the explanatory comment above
countOpenConnections to accurately state that a proxy without
getOpenConnectionCount returns zero and therefore allows detached reaping once
the deadline expires; note that production proxies currently implement the
method. Do not change countOpenConnections or the reaping behavior.
- Around line 4148-4190: Update probeStartTime and its interaction with
readProcessStartTimeMs so a platform-level probe failure disables further
start-time probes by setting probeBudget to zero, while PID-specific failures
remain memoized without disabling probing. Preserve the existing per-PID
memoization and dead-process handling, ensuring unsupported platforms avoid
repeated synchronous ps spawn attempts during the sweep.
In `@test/app-bind.test.ts`:
- Around line 1143-1144: The dead-helper fixtures use out-of-range PIDs, making
liveness results platform-dependent. In test/app-bind.test.ts:1143-1144,
including the related sentinels at line 1187, replace 2_147_483_646,
2_147_483_645, and 2_147_483_644 with in-range unused PIDs or explicitly assert
the liveness classification; make the same change for 99999999 in
test/codex-manager-rotation-command.test.ts:484-494 and 99999998/99999999 in
test/runtime-current-account.test.ts:573-598, preserving the expected
dead-record counts and fallback behavior.
- Around line 1196-1199: Replace the manually constructed ownerPath filename
with the existing resolveRuntimeHelperOwnerPath helper, passing the appropriate
directory and deadPids[0] arguments. Preserve the current path location and
downstream behavior while centralizing the runtime helper owner naming contract.
In `@test/codex-bin-wrapper.test.ts`:
- Around line 3197-3227: Update the readiness Promise inside
spawnDirectAppHelper to terminate the spawned helper before rejecting on both
the timeout and close-before-ready paths. Ensure cleanup is performed before
each rejection while preserving the existing error messages and successful ready
resolution.
- Around line 3590-3600: The metadata cleanup fault injector controlled by
CODEX_MULTI_AUTH_TEST_HELPER_METADATA_CLEANUP_BUSY_FAILURES must not affect
published production invocations. Remove this hook from scripts/codex.js or
require an explicit test-only gate before reading it, and add a regression case
in the relevant codex-bin-wrapper tests confirming production execution ignores
the environment variable.
In `@test/codex-manager-rotation-command.test.ts`:
- Around line 471-483: Replace process.ppid in the runtime-rotation-app-helper
fixture with a test-owned sleeper process, retain its PID as the deterministic
second running process, and terminate that sleeper in a finally block after the
assertions so cleanup occurs on success or failure.
In `@test/runtime-current-account.test.ts`:
- Around line 545-571: Add coverage to the test for readAppRuntimeHelperStatus
by creating a second live helper record with a newer updatedAt than the
process.pid fixture, using a sleeper child process to provide a deterministic
live PID, and assert that the newer helper’s lastAccountId is selected. Ensure
the child process is cleaned up after the assertion.
---
Outside diff comments:
In `@lib/codex-manager/commands/rotation.ts`:
- Around line 620-651: Update formatAppRuntimeHelperStatus so status and
liveHelperCount are required parameters with no default initializers. Preserve
the existing single-scan contract by requiring callers to provide both the
scanned helper status and matching live-helper count, including the existing
production caller.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7e1d1a3-4362-4ed2-9a07-ff7595ab3a93
📒 Files selected for processing (17)
AGENTS.mddocs/configuration.mddocs/development/ARCHITECTURE.mddocs/development/CONFIG_FIELDS.mddocs/privacy.mddocs/reference/storage-paths.mdlib/codex-manager/commands/rotation.tslib/runtime-constants.tslib/runtime-rotation-proxy.tslib/runtime/app-bind.tslib/runtime/rotation-server-types.tslib/runtime/runtime-current-account.tsscripts/codex.jstest/app-bind.test.tstest/codex-bin-wrapper.test.tstest/codex-manager-rotation-command.test.tstest/runtime-current-account.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (26)
docs/**/*.md
📄 CodeRabbit inference engine (docs/STYLE_GUIDE.md)
docs/**/*.md: User-facing documentation should follow the page template: Title and one-line lead, Quick path commands, Core operational workflow, Troubleshooting or failure handling, and Related links
Use short sections and scan-friendly tables in documentation where they improve clarity
Prefer direct, actionable language in documentation
Use runnable command examples in documentation
Explain expected outcomes after critical commands in documentation
Keep terminology consistent with runtime names in documentation
Avoid speculative language when behavior is deterministic in documentation
Put the user problem in the first paragraph before implementation detail
Use descriptive page titles such ascodex-multi-auth Featuresinstead of generic titles on public docs
Do not repeat keyword lists in every section; search terms should appear only where they help a developer understand the page
Canonical command family iscodex-multi-auth ...
Canonical runtime root is~/.codex/multi-auth
Runtime rotation must be described as default-on unless the release policy changes
Legacy command/path references belong only in migration contexts in documentation
Compatibility aliases (codex multi auth,codex multi-auth,codex multiauth) belong only in command reference, troubleshooting, or migration contexts
Keep command flags aligned with runtime usage text in documentation
Avoid non-runnable command snippets in documentation
Avoid conflicting path guidance across documentation
Avoid legacy-first onboarding language in documentationOrganize repository documentation according to the defined layers: product entry, user operations, reference, and development.
docs/**/*.md: Do not describecodex-multi-authas replacing@openai/codexor publishing the globalcodexbinary; preserve the official CLI's ownership ofcodex.
Usecodex-multi-authfor account management, and reservecodex-multi-auth-codexormcodexfor intentionally forwarding official Codex commands th...
Files:
docs/configuration.mddocs/privacy.mddocs/development/CONFIG_FIELDS.mddocs/reference/storage-paths.mddocs/development/ARCHITECTURE.md
docs/{index.md,getting-started.md,faq.md,architecture.md,features.md,configuration.md,troubleshooting.md,privacy.md,upgrade.md}
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Keep the listed public documentation pages as the canonical sources for operator onboarding, FAQ, architecture, features, configuration, troubleshooting, privacy, and upgrades.
Files:
docs/configuration.mddocs/privacy.md
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (docs/troubleshooting.md)
Document that
codex-multi-auth-codexis the optional forwarding wrapper, whilecodex-multi-authis the canonical account-manager command family; the package does not publish a globalcodexbinary.Document the canonical command names, runtime paths, configuration precedence, storage migration behavior, and upgrade procedures consistently across the referenced documentation.
Files:
docs/configuration.mddocs/privacy.mddocs/development/CONFIG_FIELDS.mddocs/reference/storage-paths.mddocs/development/ARCHITECTURE.md
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Project-owned state defaults to
~/.codex/multi-auth, while official Codex state remains under~/.codex; do not conflate the two storage locations.
Files:
docs/configuration.mddocs/privacy.mdtest/runtime-current-account.test.tstest/app-bind.test.tsdocs/development/CONFIG_FIELDS.mdtest/codex-manager-rotation-command.test.tsAGENTS.mddocs/reference/storage-paths.mdlib/runtime/rotation-server-types.tslib/runtime-constants.tslib/runtime-rotation-proxy.tslib/runtime/app-bind.tslib/runtime/runtime-current-account.tslib/codex-manager/commands/rotation.tsscripts/codex.jsdocs/development/ARCHITECTURE.mdtest/codex-bin-wrapper.test.ts
docs/**/*
📄 CodeRabbit inference engine (docs/privacy.md)
docs/**/*: Keep account and session state local under the configured runtime root; honorCODEX_MULTI_AUTH_DIRandCODEX_MULTI_AUTH_CONFIG_PATHoverrides.
Do not add custom analytics, a project-owned remote database, or network destinations beyond required OpenAI OAuth/backend/update endpoints and GitHub raw/releases endpoints.
Runtime rotation listeners and the optional local bridge must be loopback-only; the bridge must expose only/health,/v1/models, and/v1/responsesand require a local bearer token by default.
Store local bridge client tokens as SHA-256 hashes with prefixes and labels; never persist plaintext tokens except when displaying them during creation or rotation.
Do not log raw request or response bodies by default. WhenCODEX_PLUGIN_LOG_BODIES=1enables body logging, treat the resulting logs as sensitive data and support appropriate rotation or deletion.
Usage ledger records must contain only local request metadata summaries; email must be hashed, and prompts, authorization headers, and raw sensitive account IDs must not be stored.
Cleanup functionality must remove multi-auth-owned data from both default and resolved override roots, including settings, account data, caches, leases, usage, backups, projects, logs, app-bind state, and configured override files.
Files:
docs/configuration.mddocs/privacy.mddocs/development/CONFIG_FIELDS.mddocs/reference/storage-paths.mddocs/development/ARCHITECTURE.md
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/configuration.mddocs/privacy.mddocs/development/CONFIG_FIELDS.mddocs/reference/storage-paths.mddocs/development/ARCHITECTURE.md
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/runtime-current-account.test.tstest/app-bind.test.tstest/codex-manager-rotation-command.test.tstest/codex-bin-wrapper.test.ts
**/*.{js,ts,mjs,cjs}
📄 CodeRabbit inference engine (README.md)
**/*.{js,ts,mjs,cjs}: Do not publish or replace a globalcodexbinary; official OpenAI installation paths must retain ownership of thecodexcommand.
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.
Responsesbackground: truecompatibility must remain opt-in; requests using it must use statefulstore=truerouting rather than statelessstore=falserouting.
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:
test/runtime-current-account.test.tstest/app-bind.test.tstest/codex-manager-rotation-command.test.tslib/runtime/rotation-server-types.tslib/runtime-constants.tslib/runtime-rotation-proxy.tslib/runtime/app-bind.tslib/runtime/runtime-current-account.tslib/codex-manager/commands/rotation.tsscripts/codex.jstest/codex-bin-wrapper.test.ts
**/*.{ts,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,js,mjs}: Use ESM syntax only; the package is configured with "type": "module" and targets Node.js >= 18.17.
Do not useas any,@ts-ignore, or@ts-expect-error.
Email deduplication must be case-insensitive by usingnormalizeEmailKey()(trim and lowercase).
Files:
test/runtime-current-account.test.tstest/app-bind.test.tstest/codex-manager-rotation-command.test.tslib/runtime/rotation-server-types.tslib/runtime-constants.tslib/runtime-rotation-proxy.tslib/runtime/app-bind.tslib/runtime/runtime-current-account.tslib/codex-manager/commands/rotation.tsscripts/codex.jstest/codex-bin-wrapper.test.ts
test/**/*.{ts,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Changes affecting runtime, storage, routing, Windows filesystem behavior, or documentation integrity should preserve and extend the corresponding Vitest, property, chaos, and docs-integrity coverage.
Files:
test/runtime-current-account.test.tstest/app-bind.test.tstest/codex-manager-rotation-command.test.tstest/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/runtime-current-account.test.tstest/app-bind.test.tstest/codex-manager-rotation-command.test.tstest/codex-bin-wrapper.test.ts
docs/development/CONFIG_FIELDS.md
📄 CodeRabbit inference engine (docs/development/RUNBOOK_ADD_CONFIG_FIELD.md)
Update
docs/development/CONFIG_FIELDS.mdwith field inventory details when adding new configuration fieldsMaintain full field inventory in
docs/development/CONFIG_FIELDS.md
Files:
docs/development/CONFIG_FIELDS.md
docs/development/**/*.md
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Keep internal architecture, configuration flow, repository ownership, testing, parity, metadata, and audit guidance in development documentation.
Prefer current architecture and reference documentation over historical plans and audit snapshots when describing the present system.
Files:
docs/development/CONFIG_FIELDS.mddocs/development/ARCHITECTURE.md
docs/development/**/*
📄 CodeRabbit inference engine (docs/development/CONFIG_FLOW.md)
docs/development/**/*: Resolve the runtime root directory in this order:CODEX_MULTI_AUTH_DIR; explicit non-defaultCODEX_HOME/multi-auth; existing account-storage roots underCODEX_HOMEor~/.codex; canonical~/.codex/multi-auth; and legacy paths only when storage signals exist.
ReaddashboardDisplaySettingsandpluginConfigfromsettings.json, while preserving legacy compatibility loading and migration.
ResolvepluginConfigvalues using this precedence: existingCODEX_MULTI_AUTH_CONFIG_PATHfile, valid unifiedsettings.jsonconfiguration, legacy compatibility configuration, thenDEFAULT_PLUGIN_CONFIG; apply environment-variable overrides afterward.
Ignore a configured but nonexistentCODEX_MULTI_AUTH_CONFIG_PATHduring loading, but create it on the first save while the variable remains set.
Resolve dashboard display values from persisteddashboardDisplaySettings, followed by normalization and fallback defaults.
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 applicable legacy project-file migration.
Normalize standalonecodex-multi-authbare subcommands toauth ...before dispatch; normalize wrapper aliases; run auth-manager commands locally; forward out-of-scope wrapper commands to the official Codex CLI.
For forwarded request-bearing commands, honor runtime rotation: resolveCODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY, thenpluginConfig.codexRuntimeRotationProxy, which defaults to enabled.
When rotation is enabled for a requesting command, use a per-process-token loopback Responses proxy, a temporary shadowCODEX_HOME, and a rewrittenconfig.toml; synchronize refreshed official Codex state on exit and remove the shadow home.
The runtime proxy must select or refresh managed accounts and rotate on rate-limit, authentication, network, or server failures before streaming begins.
The plugin host m...
Files:
docs/development/CONFIG_FIELDS.mddocs/development/ARCHITECTURE.md
docs/development/**/*.{md,mdx}
📄 CodeRabbit inference engine (docs/development/TESTING.md)
When documentation changes, verify every command snippet is runnable, path references match runtime modules, cross-links are valid, and the feature matrix matches implemented features.
Files:
docs/development/CONFIG_FIELDS.mddocs/development/ARCHITECTURE.md
docs/reference/**/*.md
📄 CodeRabbit inference engine (docs/STYLE_GUIDE.md)
New flags/settings/paths must be reflected in
docs/reference/*
docs/reference/**/*.md: Keep command, API, error-contract, settings, and storage-path details in the canonical reference documentation.
Document compatibility aliases (codex multi auth,codex multi-auth, andcodex multiauth) only in command-reference, troubleshooting, or migration sections.
Files:
docs/reference/storage-paths.md
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: Route all public exports throughlib/index.tsor documented package subpaths.
Keep module dependencies acyclic and preserve the layeringtypes/constants → storage → accounts → runtime → manager/CLI; lower layers must not import higher layers.
Preserve runtime rotation pass-through semantics except for intentionally changed auth or provider headers.
Deduplicate emails usingnormalizeEmailKey(), which trims and lowercases the email.
Use classes for state requiring multiple independent instances or dependency injection, includingAccountManager,CircuitBreaker,SessionAffinityStore, and theCodexErrorhierarchy. Reserve module-level state for genuinely process-global concerns and provide a test reset helper for such state.
Never import fromdist/in source tests or library code.
Never suppress type errors.
Never patch official Codex application binaries for desktop routing.
Never use bare recursive cleanup in Windows-sensitive paths without retry handling.
Files:
lib/runtime/rotation-server-types.tslib/runtime-constants.tslib/runtime-rotation-proxy.tslib/runtime/app-bind.tslib/runtime/runtime-current-account.tslib/codex-manager/commands/rotation.ts
lib/{runtime-rotation-proxy.ts,runtime/**/*.ts}
📄 CodeRabbit inference engine (lib/AGENTS.md)
Runtime rotation must fail open to normal official Codex forwarding when startup helpers are unavailable.
Files:
lib/runtime/rotation-server-types.tslib/runtime-rotation-proxy.tslib/runtime/app-bind.tslib/runtime/runtime-current-account.ts
lib/runtime/**/*.{ts,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
lib/runtime/**/*.{ts,js,mjs}: Runtime rotation must remain default-on throughcodexRuntimeRotationProxy, with opt-out behavior preserved via the rotation command orCODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0.
Do not patch official Codex app binaries; use the reversible app-bind or launcher-helper mechanisms instead.
Files:
lib/runtime/rotation-server-types.tslib/runtime/app-bind.tslib/runtime/runtime-current-account.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/runtime/rotation-server-types.tslib/runtime-constants.tslib/runtime-rotation-proxy.tslib/runtime/app-bind.tslib/runtime/runtime-current-account.tslib/codex-manager/commands/rotation.ts
lib/{runtime-rotation-proxy.ts,local-bridge.ts,request/**/*.ts}
📄 CodeRabbit inference engine (lib/AGENTS.md)
Do not forward stale decoded
content-encodingmetadata when Node fetch has already decoded response bytes.
Files:
lib/runtime-rotation-proxy.ts
lib/{runtime-rotation-proxy.ts,local-bridge.ts}
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/{runtime-rotation-proxy.ts,local-bridge.ts}: Runtime proxy client-facing headers and responses must never expose account emails or tokens.
Never include account emails or tokens in runtime proxy client responses.
Files:
lib/runtime-rotation-proxy.ts
lib/runtime-rotation-proxy.ts
📄 CodeRabbit inference engine (AGENTS.md)
The runtime rotation proxy must be loopback-only, use a per-process client token, forward only Responses API and model-discovery requests, and never expose account emails or tokens in response headers or logs.
Files:
lib/runtime-rotation-proxy.ts
scripts/codex*.js
📄 CodeRabbit inference engine (AGENTS.md)
The wrapper must handle account-manager/auth commands locally and forward non-auth commands to the official Codex CLI; do not reimplement general Codex commands.
Files:
scripts/codex.js
scripts/**/*.{js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Windows-sensitive cleanup and write operations must retry transient
EBUSY,EPERM, andENOTEMPTYfailures; do not use bare recursive deletion where locks are possible.
Files:
scripts/codex.js
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
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:36:54.092Z
Learning: Maintain the canonical package and command names: `codex-multi-auth` and `codex-multi-auth ...`; `codex-multi-auth-codex` is the explicit wrapper and the package must not publish a global `codex` binary.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:36:54.092Z
Learning: Keep runtime rotation default-on behavior aligned with explicit release and migration documentation.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: Resolve runtime configuration sources in this order: existing CODEX_MULTI_AUTH_CONFIG_PATH file, valid unified settings.json pluginConfig, legacy compatibility files, then DEFAULT_PLUGIN_CONFIG; afterward, environment variables override individual runtime settings.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: Ignore a set-but-missing CODEX_MULTI_AUTH_CONFIG_PATH during loading, while still using it as the next save target.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: When CODEX_HOME is non-default, resolve multi-auth strictly under $CODEX_HOME/multi-auth without scanning other roots for existing pools.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: Treat CODEX_MULTI_AUTH_FORCE_ACCOUNT as ephemeral and fail hard when the runtime rotation proxy is unavailable; an explicit --account flag takes precedence over the environment variable.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: Keep recommended defaults enabled: menuAutoFetchLimits, menuSortEnabled, liveAccountSync, sessionAffinity, proactiveRefreshGuardian, and preemptiveQuotaEnabled.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: The runtime rotation proxy must preserve request bodies and streaming responses, replace outbound authentication headers with the selected managed account, remove hop-by-hop and private account metadata headers, and remove stale decoded content-encoding headers.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: On rate limits, server errors, network failures, or refresh failures, rotate accounts before streaming response bytes; if all accounts are unavailable, return a structured pool-exhaustion error pointing to rotation status.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: When an explicit OAuth revocation is detected, return the error directly instead of rotating accounts, and apply tokenInvalidationCooldownMs rather than the generic authentication-failure cooldown.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: Bound retryAllAccountsMaxRetries and retryAllAccountsMaxWaitMs so blocking retries cannot exceed the host client's request timeout.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: In sequential scheduling, retain a manual account pin, advance only when the active account is exhausted or unavailable, and intentionally ignore per-session affinity after an account change.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: Package installation scripts must remain side-effect-free; postinstall may print only a short notice and must not perform setup or mutation.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: The wrapper must never automatically run npm install or update commands; version checks may only print a manual update notice.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: First-run desktop self-healing may configure the CLI file credential store, packaged app binding, and supported user-level launcher routing, while respecting the corresponding opt-out environment variables and recording completion in ~/.codex/multi-auth/first-run-setup.json.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:06.417Z
Learning: Deprecated selectors such as gpt-5-codex and gpt-5.1-codex* must be treated as compatibility aliases and retried with the current documented Codex model after an unsupported-model response; fallback to gpt-5.4 only after a real unsupported-model response.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:19.540Z
Learning: Keep runtime rotation local, reversible, and compatible with official Codex state files.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:19.540Z
Learning: Keep local governance data—usage ledgers, budgets, account policies, and routing profiles—file-backed and opt-in at the operator command surface.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:38.459Z
Learning: Use of the project must comply with OpenAI's Terms of Use and Privacy Policy.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:48.083Z
Learning: Use `~/.codex/multi-auth` as the default storage root, overridable with `CODEX_MULTI_AUTH_DIR`; when `CODEX_HOME` is non-default, resolve storage strictly under `$CODEX_HOME/multi-auth` without scanning the default root.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:48.083Z
Learning: Use the canonical filenames and directory layout under the multi-auth root for settings, accounts, backups, WAL files, caches, logs, usage data, policies, routing profiles, leases, app-bind state, and project pools.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:48.083Z
Learning: Treat `~/.codex/accounts.json`, `~/.codex/auth.json`, and `~/.codex/config.toml` as official Codex CLI files; do not treat them as project-owned storage.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:48.083Z
Learning: Persist the top-level Codex CLI `cli_auth_credentials_store` setting as `"file"` during first-run setup, wrapper startup, and `doctor --fix`, unless enforcement is disabled with `CODEX_MULTI_AUTH_ENFORCE_CLI_FILE_AUTH_STORE=0`. Preserve line endings and TOML string style, rewrite only the top-level assignment, and leave profile-level settings unchanged.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:48.083Z
Learning: The wrapper must inject `-c cli_auth_credentials_store="file"` for forwarded non-auth commands unless the caller supplied the setting or `CODEX_MULTI_AUTH_FORCE_FILE_AUTH_STORE=0` is set.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:48.083Z
Learning: Never read or write the macOS keychain or the `security` CLI for Codex credentials.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:48.083Z
Learning: First-run setup must use an exclusive-create marker, run at most once, tolerate failures without blocking the command, and migrate pre-v2 or unreadable markers by replaying only the auth-store step.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:48.083Z
Learning: Do not use backup recovery for intentionally cleared state: exclude `.reset-intent` markers from recovery candidates and suppress flagged-account backup recovery while the flagged reset marker exists.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:48.083Z
Learning: Named backup exports must use the plugin-owned backup namespace, append `.json` when omitted, accept only `[A-Za-z0-9_-]` names, reject path traversal and `.rotate.`, `.tmp`, or `.wal` names, and avoid overwriting existing files unless an explicit force path is used.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:37:48.083Z
Learning: The local bridge must be loopback-only, expose only `/health`, `/v1/models`, and `/v1/responses`, persist token hashes rather than plaintext tokens, and show plaintext tokens only on create or rotate commands.
📚 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/runtime-current-account.test.tstest/app-bind.test.tstest/codex-manager-rotation-command.test.tstest/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/runtime-current-account.test.tstest/app-bind.test.tstest/codex-manager-rotation-command.test.tstest/codex-bin-wrapper.test.ts
🪛 ast-grep (0.45.1)
test/runtime-current-account.test.ts
[warning] 547-557: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(
join(tempDir, runtime-rotation-app-helper.${process.pid}.json),
JSON.stringify({
kind: "codex-app-runtime-rotation-helper",
state: "running",
pid: process.pid,
lastAccountId: "acc_live",
updatedAt: now - 30_000,
}),
"utf8",
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 574-584: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(
join(tempDir, "runtime-rotation-app-helper.99999998.json"),
JSON.stringify({
kind: "codex-app-runtime-rotation-helper",
state: "idle-timeout",
pid: 99999998,
lastAccountId: "acc_older",
updatedAt: now - 60_000,
}),
"utf8",
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 585-595: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(
join(tempDir, "runtime-rotation-app-helper.99999999.json"),
JSON.stringify({
kind: "codex-app-runtime-rotation-helper",
state: "stopped",
pid: 99999999,
lastAccountId: "acc_newer",
updatedAt: now - 10_000,
}),
"utf8",
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
lib/runtime-constants.ts
[warning] 26-29: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${prefix.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\.\\d+\\.json$,
"i",
)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
lib/runtime/app-bind.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 { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[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 { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[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 { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[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 { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
scripts/codex.js
[warning] 4141-4144: Detects non-literal values in regular expressions
Context: new RegExp(
^${baseName.replace(/\.json$/i, "").replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\.(\\d+)\\.json$,
"i",
)
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
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)
[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)
[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)
[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)
[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)
[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)
[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)
🪛 LanguageTool
docs/development/ARCHITECTURE.md
[style] ~200-~200: Consider using “who” when you are referring to a person instead of an object.
Context: ... open client connections, so a consumer that really did take the handoff — `codex ap...
(THAT_WHO)
🪛 OpenGrep (1.26.0)
scripts/codex.js
[ERROR] 4194-4194: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 4194-4194: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (24)
lib/codex-manager/commands/rotation.ts (1)
517-519: LGTM!Also applies to: 549-566
lib/runtime/runtime-current-account.ts (1)
126-166: LGTM!lib/runtime-constants.ts (1)
22-37: 📐 Maintainability & Code Qualitydo not add a barrel export
listRuntimeHelperStatusPathsatlib/runtime-constants.ts:22-36is an internal helper.lib/index.ts:1-53does not exposeruntime-constants.ts, and no documented public subpath exposes it. The direct internal imports are valid. The escaped regex is not a ReDoS issue.> Likely an incorrect or invalid review comment.lib/runtime/app-bind.ts (1)
1509-1533: 🗄️ Data Integrity & Integrationno change required
lib/runtime/app-bind.ts:470-471always setsbindDirto<multiAuthDir>/app-bind.scripts/codex.js:3806-3816writes helper metadata directly under<multiAuthDir>. Therefore,dirname(paths.bindDir)is the correct helper directory. Existing coverage is intest/app-bind.test.ts:1133-1215.> Likely an incorrect or invalid review comment.scripts/codex.js (11)
3-3: LGTM!Also applies to: 88-110
3802-3817: LGTM!
3889-3965: LGTM!
3975-4029: LGTM!
4031-4041: LGTM!
4076-4103: LGTM!
4255-4255: LGTM!Also applies to: 4279-4285
4306-4321: LGTM!Also applies to: 4330-4374
4397-4401: LGTM!Also applies to: 4474-4477, 4490-4521
4609-4634: LGTM!
4088-4117: 📐 Maintainability & Code Qualityno missing regression test
test/codex-bin-wrapper.test.ts:3560forces twoEBUSYfailures and asserts that stale metadata files are removed after retry.> Likely an incorrect or invalid review comment.lib/runtime-rotation-proxy.ts (1)
829-832: LGTM!lib/runtime/rotation-server-types.ts (1)
10-16: LGTM!docs/development/ARCHITECTURE.md (1)
278-279: LGTM!docs/privacy.md (1)
91-91: LGTM!Also applies to: 118-118
docs/reference/storage-paths.md (1)
42-42: LGTM!Also applies to: 162-162
test/codex-bin-wrapper.test.ts (3)
2853-2858: LGTM!Also applies to: 2875-2875, 3243-3315, 3317-3434, 3436-3469, 3471-3555, 3557-3605, 3607-3699, 3840-3844
400-404: 🗄️ Data Integrity & Integrationremove this review comment
lib/runtime/rotation-server-types.ts:16declaresgetOpenConnectionCountas optional.scripts/codex.js:4326handles its absence.lib/runtime-rotation-proxy.ts:832andtest/codex-bin-wrapper.test.ts:404expose only a numeric socket count.> Likely an incorrect or invalid review comment.
697-708: 🎯 Functional Correctnessthe owner identity paths match
test/codex-bin-wrapper.test.ts:699uses the sameps -o lstart=command,LC_ALL=C, andDate.parsepath as the wrapper.test/codex-bin-wrapper.test.ts:3093exercises the exact equality check. no tolerance mismatch exists.> Likely an incorrect or invalid review comment.test/codex-manager-rotation-command.test.ts (1)
506-530: LGTM!
| const ownerPath = join( | ||
| dirname(legacyPath), | ||
| `runtime-rotation-app-helper-owner.${deadPids[0]}.json`, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
reuse the owner-path helper instead of hand-building the filename.
test/app-bind.test.ts:95-108 already exposes resolveRuntimeHelperOwnerPath, and test/app-bind.test.ts:124-141 writes a well-formed owner record. this block re-encodes the runtime-rotation-app-helper-owner.<pid>.json naming contract as a string literal. keep that contract in one place, so a constant rename does not require editing test literals.
♻️ proposed refactor
- // An owner file beside a dead per-PID record goes with it.
- const ownerPath = join(
- dirname(legacyPath),
- `runtime-rotation-app-helper-owner.${deadPids[0]}.json`,
- );
- await writeFile(
- ownerPath,
- `${JSON.stringify({
- version: 1,
- kind: "codex-app-runtime-rotation-helper-owner",
- identityToken: "does-not-matter-for-dead-pid",
- launcherPid: 1,
- createdAt: Date.now(),
- })}\n`,
- "utf8",
- );
+ // An owner file beside a dead per-PID record goes with it.
+ const ownerPath = await writeRuntimeHelperOwner(
+ { home: root, env },
+ deadPids[0] ?? 0,
+ "does-not-matter-for-dead-pid",
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ownerPath = join( | |
| dirname(legacyPath), | |
| `runtime-rotation-app-helper-owner.${deadPids[0]}.json`, | |
| ); | |
| // An owner file beside a dead per-PID record goes with it. | |
| const ownerPath = await writeRuntimeHelperOwner( | |
| { home: root, env }, | |
| deadPids[0] ?? 0, | |
| "does-not-matter-for-dead-pid", | |
| ); |
🤖 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/app-bind.test.ts` around lines 1196 - 1199, Replace the manually
constructed ownerPath filename with the existing resolveRuntimeHelperOwnerPath
helper, passing the appropriate directory and deadPids[0] arguments. Preserve
the current path location and downstream behavior while centralizing the runtime
helper owner naming contract.
There was a problem hiding this comment.
Agreed — resolveRuntimeHelperOwnerPath already exists at test/app-bind.test.ts:95-108 and the literal re-encodes a naming contract that should live in one place. #664 test surface; filing.
There was a problem hiding this comment.
@possibilities, agreed. tracking this under #664 is appropriate. test/app-bind.test.ts:95 already owns the owner-path naming contract.
You are interacting with an AI system.
| await writeFile( | ||
| join(root, "runtime-rotation-app-helper.json"), | ||
| `${JSON.stringify({ | ||
| version: 1, | ||
| kind: "codex-app-runtime-rotation-helper", | ||
| state: "running", | ||
| pid: process.ppid, | ||
| totalRequests: 1, | ||
| rotations: 0, | ||
| updatedAt: now - 5_000, | ||
| })}\n`, | ||
| "utf8", | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
process.ppid makes the "+1 more running" count environment-dependent.
the (+1 more running) assertion at test/codex-manager-rotation-command.test.ts:503 depends on the vitest worker's parent being alive for the duration of the test. that parent is the pool process, not something this test controls. if the worker is reparented — an orphan is adopted by pid 1 on linux, and the pool topology differs between threads, forks, and --no-file-parallelism — the count still holds only by luck, and a parent that exits mid-run flips the assertion to (+0 more running).
spawn a sleeper the test owns and kill it in finally, so the second live pid is deterministic.
♻️ proposed change
+ const sleeper = spawn(process.execPath, ["-e", "setTimeout(() => {}, 30000)"], {
+ stdio: "ignore",
+ });
await writeFile(
join(root, "runtime-rotation-app-helper.json"),
`${JSON.stringify({
version: 1,
kind: "codex-app-runtime-rotation-helper",
state: "running",
- pid: process.ppid,
+ pid: sleeper.pid,
totalRequests: 1,
rotations: 0,
updatedAt: now - 5_000,
})}\n`,
"utf8",
);kill sleeper in a finally block after the assertions.
🤖 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-manager-rotation-command.test.ts` around lines 471 - 483, Replace
process.ppid in the runtime-rotation-app-helper fixture with a test-owned
sleeper process, retain its PID as the deterministic second running process, and
terminate that sleeper in a finally block after the assertions so cleanup occurs
on success or failure.
Source: Path instructions
There was a problem hiding this comment.
Agreed that process.ppid makes the count environment-dependent, and the reparenting argument is the convincing one — the assertion holds by luck, not by construction. It is #664 test surface, so filing rather than folding in. Same for the readAppRuntimeHelperStatus several-live-helpers gap on the sibling thread: the live.sort(byRecency) half of that selector genuinely is never exercised.
There was a problem hiding this comment.
@possibilities, agreed. the current pr can keep its scope.
the follow-up should cover the deterministic second live pid in test/codex-manager-rotation-command.test.ts:471 and the multiple-live-helper recency selection for readAppRuntimeHelperStatus in lib/runtime/runtime-current-account.ts.
would you like me to open a GitHub follow-up issue and assign it to you?
You are interacting with an AI system.
| it("prefers a live per-PID helper over a fresher record with a dead PID", async () => { | ||
| const now = Date.now(); | ||
| // Per-PID file for a live process (this test), older updatedAt. | ||
| await fs.writeFile( | ||
| join(tempDir, `runtime-rotation-app-helper.${process.pid}.json`), | ||
| JSON.stringify({ | ||
| kind: "codex-app-runtime-rotation-helper", | ||
| state: "running", | ||
| pid: process.pid, | ||
| lastAccountId: "acc_live", | ||
| updatedAt: now - 30_000, | ||
| }), | ||
| "utf8", | ||
| ); | ||
| // Legacy shared file naming a dead PID, fresher updatedAt: recency must | ||
| // not outrank liveness. | ||
| await writeStatusFile( | ||
| JSON.stringify({ | ||
| kind: "codex-app-runtime-rotation-helper", | ||
| state: "running", | ||
| pid: 99999999, | ||
| lastAccountId: "acc_dead", | ||
| updatedAt: now, | ||
| }), | ||
| ); | ||
| expect(readAppRuntimeHelperStatus()?.lastAccountId).toBe("acc_live"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
add the "several live helpers" case for readAppRuntimeHelperStatus.
lib/runtime/runtime-current-account.ts:168-188 sorts the live candidates by recency before returning the first. this test supplies exactly one live candidate, so the live.sort(byRecency) half of that selector is never exercised. the equivalent case exists for the rotation command at test/codex-manager-rotation-command.test.ts:449, but that is a different selector, and this one drives runtime account resolution.
add a third fixture: a second live record with a newer updatedAt than the process.pid one, and assert its lastAccountId wins. use a sleeper child for the second live pid so the case stays deterministic.
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 547-557: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(
join(tempDir, runtime-rotation-app-helper.${process.pid}.json),
JSON.stringify({
kind: "codex-app-runtime-rotation-helper",
state: "running",
pid: process.pid,
lastAccountId: "acc_live",
updatedAt: now - 30_000,
}),
"utf8",
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/runtime-current-account.test.ts` around lines 545 - 571, Add coverage to
the test for readAppRuntimeHelperStatus by creating a second live helper record
with a newer updatedAt than the process.pid fixture, using a sleeper child
process to provide a deterministic live PID, and assert that the newer helper’s
lastAccountId is selected. Ensure the child process is cleaned up after the
assertion.
Source: Path instructions
There was a problem hiding this comment.
See the previous thread — agreed, live.sort(byRecency) is never exercised by a single live candidate, and a deterministic sleeper child is the right fixture. #664 test surface; filing as a follow-up rather than growing this PR.
There was a problem hiding this comment.
@possibilities, acknowledged. keeping this pr scoped is reasonable.
the follow-up should add two live helper records and verify that readAppRuntimeHelperStatus selects the newest updatedAt value from lib/runtime/runtime-current-account.ts:168. it should use a deterministic sleeper child pid and clean it up on every test path in test/runtime-current-account.test.ts:545.
You are interacting with an AI system.
…n Windows CodeRabbit round 1. The comment on `countOpenConnections` described the fallback backwards: a proxy that cannot report connections reads as zero, and zero is the value that *satisfies* the owner-gone condition, so the missing-method case fails open into reaping rather than being "carried by activity alone". The behavior was the intended one; only the comment was wrong, and it documented a fail-open decision as fail-closed. Now it says which direction it fails in and why that direction is the safe one. The detached-window tests simulated owner death with an unmatchable start time, which only POSIX can evaluate, so the reap had no Windows coverage. A genuinely dead owner PID is readable through the degraded bare-liveness check on every platform: the new test owns a child, kills it, waits for the kernel to agree, and hands that PID to a helper. It runs on win32. `spawnDirectAppHelper` leaked a helper per readiness failure: the rejection throws before the caller reaches its try/finally, so nothing called `stopDirectAppHelper`. A leak-fix harness that leaks helpers is its own bug report. It now kills the child on the way out. Docs: both new lifetime overrides added to the settings reference, the per-helper owner file added to the AGENTS.md and privacy.md inventories, `CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS` added to the internal env list, and the ~500-word self-reaping paragraph split into a rule table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gn9SDorCbELwn5wchTnyL8
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@scripts/codex.js`:
- Around line 4326-4334: Update countOpenConnections so it returns the raw value
only for positive safe integer connection counts; normalize negative values,
zero, non-integers, NaN, Infinity, and other invalid results to 0. Add
regression coverage for -1, NaN, and Infinity in the existing codex wrapper
tests, preserving the owner-gone helper reaping behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6dbdc665-2853-4123-b865-b7698e4d36d8
📒 Files selected for processing (2)
scripts/codex.jstest/codex-bin-wrapper.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,ts,mjs,cjs}
📄 CodeRabbit inference engine (README.md)
**/*.{js,ts,mjs,cjs}: Do not publish or replace a globalcodexbinary; official OpenAI installation paths must retain ownership of thecodexcommand.
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.
Responsesbackground: truecompatibility must remain opt-in; requests using it must use statefulstore=truerouting rather than statelessstore=falserouting.
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.jstest/codex-bin-wrapper.test.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,js}: Use ESM only; the package is configured with"type": "module".
Do not useas any,@ts-ignore, or@ts-expect-error.
Files:
scripts/codex.jstest/codex-bin-wrapper.test.ts
scripts/*.js
📄 CodeRabbit inference engine (AGENTS.md)
scripts/*.js: Keep CLI entrypoint responsibilities separated:mcodexmust forward tocodex.jsand must not reimplement account-manager logic; wrapper commands must forward non-auth commands to the official Codex CLI.
Windows-sensitive cleanup and write operations must retry transientEBUSY,EPERM, andENOTEMPTYfailures where applicable.
Files:
scripts/codex.js
scripts/codex*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not bypass the official Codex CLI by reimplementing general Codex commands in the wrapper; only auth commands are handled locally and other commands are forwarded.
Files:
scripts/codex.js
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,js}
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*.{ts,js}: Windows-sensitive tests and cleanup helpers must not use bare recursive deletion without retry handling.
Tests are Vitest suites; maintain coverage for documented behavior, including property, chaos, and documentation-integrity tests where relevant.
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-11T20:48:01.260Z
Learning: The canonical package name is `codex-multi-auth`, and the canonical command family is `codex-multi-auth ...`.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:01.260Z
Learning: Official Codex state must remain under `~/.codex`, while project-owned multi-auth state defaults to `~/.codex/multi-auth`.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: Resolve runtime configuration sources in this order: existing `CODEX_MULTI_AUTH_CONFIG_PATH`, valid unified `settings.json` `pluginConfig`, legacy compatibility files, then `DEFAULT_PLUGIN_CONFIG`; afterward apply environment-variable overrides.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: Ignore a set-but-missing `CODEX_MULTI_AUTH_CONFIG_PATH` during load, but use it as the save target when the environment variable is set.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: When `CODEX_HOME` is non-default, resolve multi-auth strictly to `$CODEX_HOME/multi-auth` without scanning other roots for existing pools.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: Treat deprecated selectors such as `gpt-5-codex` and `gpt-5.1-codex*` as compatibility aliases, retrying them with the current documented Codex model after rejection.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: Only fall back to `gpt-5.4` after receiving a real ChatGPT Codex unsupported-model response.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: Package install scripts must remain side-effect-free; postinstall may print only a short notice and must not perform setup or updates.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: The wrapper must never run npm install or update commands automatically; version checks may only print a manual notice, and notices are limited to TTY sessions or `CODEX_MULTI_AUTH_DEBUG=1`.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: The runtime rotation proxy must preserve request bodies and streaming responses, replace outbound authentication headers with the selected managed account, remove hop-by-hop and private account metadata headers, and remove stale decoded `content-encoding` headers.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: A per-invocation account pin must be ephemeral, fail hard when the rotation proxy is disabled, and must not modify persisted switch pins or leak across concurrent sessions.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: When all accounts are unavailable, return a structured pool-exhaustion error directing users to `codex-multi-auth rotation status` instead of silently selecting an invalid account.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: Token revocation responses must be returned directly instead of rotating accounts; the affected account receives the configured token-invalidation cooldown.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: Keep bounded retry and wait budgets when retrying all rate-limited accounts so blocking waits do not exceed the host client's request timeout.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: Package installation must not patch official Codex app files; persistent app binding must use the supported router/configuration mechanism.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:12.213Z
Learning: First-run self-healing must be skipped for `npx` and project-local installs so they do not consume the first-run marker.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: When `CODEX_HOME` is set to a non-default directory, resolve storage strictly under `$CODEX_HOME/multi-auth` and do not scan `~/.codex/multi-auth` for existing pools.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: Treat `~/.codex/multi-auth` as project-owned storage, while `~/.codex/accounts.json`, `~/.codex/auth.json`, and `~/.codex/config.toml` remain managed by the official Codex CLI.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: Never read or write the system keychain or the `security` CLI. File-backed authentication must use the official Codex CLI files and reconcile the persisted top-level `cli_auth_credentials_store` value to `"file"` unless explicitly disabled.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: Only rewrite the top-level `cli_auth_credentials_store` assignment in `config.toml`; preserve profile-level assignments, existing line endings, and either TOML string form.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: Use atomic writes for configuration and marker updates, with retry handling for Windows `EPERM`/`EBUSY` failures; failures that cannot be recovered must not block command forwarding.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: First-run setup must claim its marker with exclusive creation so concurrent invocations run setup at most once. Failed auth-store setup remains pre-v2 for retry; unreadable or truncated markers migrate as pre-v2; migration must not rerun app binding or launcher installation.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: Exclude `.reset-intent` markers and cache-like artifacts from recovery candidates. Do not restore flagged-account backups while the flagged reset marker exists; use `settings.json.bak` only when `settings.json` exists but is unreadable.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: Named backup export names may contain only letters, numbers, `_`, and `-`; reject path separators, `..`, `.rotate.`, `.tmp`, and `.wal`, append `.json` when omitted, and do not overwrite existing files unless explicitly forced.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: The local bridge must remain loopback-only, persist token hashes rather than plaintext tokens, and display plaintext tokens only during explicit create or rotate commands.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: Enforce account policy pause and drain entries at account-selection time through `evaluateRuntimePolicy`, excluding blocked accounts from hybrid rotation.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: Interactive TUI sessions must use the canonical `CODEX_HOME` with `-c` provider overrides instead of a temporary shadow home; no lock should be taken for concurrent sessions.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-08-11T20:48:54.783Z
Learning: Validate storage and recovery changes with `npm run build` and the specified unified-settings, storage-recovery, and flagged-account tests before shipping.
📚 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.1)
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)
[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 (1)
test/codex-bin-wrapper.test.ts (1)
335-349: LGTM!Also applies to: 412-417, 3415-3458
…umer CodeRabbit round 2, and correct: Number.isFinite admits negatives, so a proxy answering -1 compared unequal to 0 and pinned a stranded helper alive forever — the same failure the previous commit claimed to close, one value short. Only a positive safe integer now counts as attached; negative, fractional, NaN, and Infinity all read as "nothing attached" and let the detached window run. Regression cases for -1, NaN, and Infinity, which needed the fixture proxy to be able to express a garbage reading at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gn9SDorCbELwn5wchTnyL8
|
Round 1 answered. Head is Taken (5):
Deferred to follow-ups, on scope rather than merit. Everything else CodeRabbit raised lands on #664 code — approved and awaiting merge — and this PR is deliberately one behavior change stacked on it. Folding them in would mean editing an approved PR through the back door or conflicting with it. Filed with the reasoning intact:
One finding I checked and pushed back on rather than accepting: the out-of-range PID sentinels cannot invert here, because every liveness check in the tree treats non- @ndycode — happy to fold any of the three into this PR instead if you would rather have them together. |
|
@coderabbitai review All of round 1 is answered at |
|
✅ Action performedReview finished.
|
|
@ndycode — status, since the red X on this PR is misleading. This PR is not waiting on any change from me. The The 7 open threads are deliberate, not forgotten. Every one of them is #664 code, not this PR's. Each has a reply explaining the reasoning and a filed issue:
I left them open rather than resolving them because they are your call, not mine: #664 is approved and awaiting merge, and folding these in here would either edit an approved PR sideways or conflict with it. Say the word and I will fold any or all of them into this branch instead — that is the one decision this PR is actually waiting on. Everything else is green: Greptile passes, CodeRabbit's check passes, the branch is mergeable, and the local gate ( |
The detached reap added in ndycode#665 could kill a live `codex app` session. `codex app` relies on the detach grace rather than an explicit `detachOnExit`, so its launcher exits inside the grace window and the helper's owner is dead from the first tick. From then on the only thing standing between the desktop app and a dead proxy was `countOpenConnections() === 0` — and the proxy never sets `server.keepAliveTimeout`, so Node closes idle client sockets after its 5s default. A user who stops typing for the length of the detached window has zero sockets and no new requests, so the helper exits `owner-gone`, and the next message gets ECONNREFUSED against a dead localhost port with nothing left to restart it. Pre-ndycode#665 that session survived for the full 12h idle timeout. Gate the reap on the helper having *never* served a request. Every leaked helper in the ndycode#663 report had `totalRequests: 0`, so the leak is entirely a never-served phenomenon and the narrower gate closes it in full; a helper that served anything was genuinely handed off and falls back to the idle timeout and the 24h lifetime ceiling, which is where it sat before the detached window existed. Two more lifecycle fixes in the same tick: - The owner verdict is now three-valued. "No owner PID was recorded" and "the owner is confirmed dead" are different facts, and collapsing them into one `false` started the detached clock on the first tick for any helper launched without an owner PID — invoked directly, which is the documented reproduction in ndycode#663, or spawned by a pre-upgrade launcher — and reaped it silently 15 minutes later. `unknown` fires neither branch, which is what the pre-ndycode#664 `ownerPid && isAlive(ownerPid)` guard did. - The status heartbeat now accounts for the detached window. `publishToken` zeroes `idleExpiresAt`, so the published deadline only catches up on a heartbeat; pinned to the idle window alone, `rotation status` kept advertising a 12h deadline for a helper seconds from exiting, and under a short DETACHED_IDLE_MS override it never caught up at all. Also in this commit, both from the same review pass: - The metadata sweep runs after the helper spawn instead of before it. It is synchronous and unbounded — readdir, a readFileSync per live candidate, rmSync with a blocking backoff, bounded `ps` probes — and the state it cleans up is exactly the state that makes it slow, so it sat in front of `codex app` and TUI startup. Nothing about spawning depends on it. The launch timeout is armed after it either way. - Sweep deletions are guarded by an mtime re-check. Classifying a file as stale and deleting it are two moments, and a PID freed between them can be handed to a helper starting right now, which republishes that exact path before the delete lands. - The published wrapper's fault injectors need an explicit CODEX_MULTI_AUTH_TEST_FAULT_INJECTION=1 opt-in and a strict digits-only parse. `Number.parseInt` reads "2abc" as 2 and "1e3" as 1, so a value that was never meant to be a count could arm an injector in a user's install and silently defeat the first N metadata deletions of every sweep (ndycode#668). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015b4Lew3oHNEYmTZtg7zEWz
2 participants
Summary
owner is dead stops refreshing its idle clock but still serves out the
full idle timeout — 12h by default — before exiting.
the window leaves its helper running, on the assumption a consumer took the
handoff. Nothing verifies that assumption, so every short forwarded command
strands a helper.
minutes, 23 concurrent, ~1.1 GB resident, each with a dead owner, 0–3
lifetime requests, and nothing connected. Steady state at that rate is
~140 helpers, the same order as the 183 that motivated fix(codex): stop runtime helpers from leaking past their idle timeout (#663) #664.
What Changed
CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS(default 15m;0restoresthe previous behavior) instead of the full idle timeout.
connections, so a consumer that really did take the handoff —
codex appgiving the desktop app its proxy — is never reaped out from under. Traffic
after the owner dies also pushes the deadline out by another window, so an
intermittently-used detached helper stays up on its own evidence.
startRuntimeRotationProxynow reportsgetOpenConnectionCount(). Thesocket set already existed for shutdown; this only exposes its size. The
method is optional on
RuntimeRotationProxyServer, and a proxy shapewithout it degrades to activity-only accounting.
it — the same failure mode fix(codex): stop runtime helpers from leaking past their idle timeout (#663) #664 fixed for the idle clock.
idleExpiresAtnow reports whichever deadline is actuallyenforced, so
rotation statuscannot advertise 12h to a helper minutesfrom being reaped. Terminal state is
owner-gone.Behavior change worth calling out
A helper spawned with no owner PID in its environment reads as
owner-dead from its first tick, so it now exits after the detached window
rather than the full idle timeout. No in-tree launcher does this —
startRuntimeRotationAppHelperalways passes PID and start time — but adirect manual invocation would see the shorter life.
Validation
npm run lintnpm run typechecknpm test— see note belownpm test -- test/documentation.test.ts(32 passed)npm run buildThree new tests in
test/codex-bin-wrapper.test.ts, all POSIX-gated the sameway as #664's identity tests (owner death is simulated by an unmatchable
start time, which Windows cannot evaluate):
timeout — verified failing without the
scripts/codex.jschangedisabled (
0)npm testis not green on my machine, and is not green on the base brancheither:
test/paths.test.ts,test/runtime-paths.test.ts,test/install-codex-auth.test.tsand friends fail on Windows-pathexpectations under macOS,
test/named-backup-export.test.tsfails on abackup-root check, and several
codex-bin-wrapperprocess tests areload-flaky (the failing set changes run to run, on both branches). Every test
touching this change passes. Happy to chase any of the pre-existing failures
separately if they are not already known.
Docs and Governance Checklist
docs/configuration.md— new env vardocs/development/CONFIG_FIELDS.md— new env vardocs/development/ARCHITECTURE.md— the detached-window paragraphis a bound on an already-documented behavior rather than a new knob a
user reaches for
docs/upgrade.md— no migration; the change is a shorter timeout forhelpers that have no owner
Risk and Rollback
kernel start time, zero open client connections, and no traffic for the
whole window.
CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS=0restorestoday's behavior without a release.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Gn9SDorCbELwn5wchTnyL8
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
this pr bounds detached runtime-helper lifetimes while preserving active handoffs and improves concurrent helper observability.
Confidence Score: 5/5
the pr appears safe to merge.
no blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD T[helper tick] --> O{owner identity alive?} O -->|yes| R[refresh idle activity and clear detached clock] O -->|no| D[start or retain detached clock] R --> M{maximum lifetime reached?} D --> I{full idle deadline reached?} I -->|yes| X[idle-timeout shutdown] I -->|no| C{detached deadline reached and zero connections?} C -->|yes| G[owner-gone shutdown] C -->|no| M M -->|yes| L[max-lifetime shutdown] M -->|no| TReviews (4): Last reviewed commit: "fix(codex): accept only a positive socke..." | Re-trigger Greptile
Context used (3)