fix: v2.2.0 deep-audit remediation (4 HIGH, 6 MEDIUM)#505
Conversation
Deep audit of the v2.2.0 tree (6 parallel auditors across auth, proxy/rotation, scripts, quota/policy, storage, CLI) surfaced bugs not caught pre-release. HIGH: - mcodex: rewrite the flagship launcher (#500) as a Node bin (scripts/mcodex.js). The bash script shipped as a Windows bin died with HCS_E_SERVICE_NOT_AVAILABLE when npm's shim invoked bare 'bash' and the WSL stub shadowed git-bash. The Node launcher has zero bash dependency, builds tmux/watch invocations as argv arrays (no shell interpolation), and degrades gracefully where those tools are absent. - auth/server: capture OAuth code/state in per-call closures instead of on the shared http.Server, so concurrent logins in one process can't cross-bind state. - model-capability-matrix: read capability snapshots/boosts with the entitlement key (the write key), not the sha256 account key, so the matrix stops reporting every account as supporting every model. - storage/transactions: track lock ownership via AsyncLocalStorage and persist recovered flagged backups unlocked when already inside a transaction, fixing a re-entrant deadlock (doctor restore + flagged .bak without primary) that wedged all future saves. MEDIUM: - codex-manager styleAccountDetailText: scan the whole detail for failure keywords so a 'working' prefix can't render green when 'failed' is trapped in the (NN%) quota segment (same precedence class as the prior suffix fix). - workspace command: reject fractional/trailing-garbage indices (/^\d+$/) instead of silently truncating via parseInt, matching the 'switch' command guard. - request/fetch-helpers: include NORMALIZED_UNSUPPORTED_MODEL_PATTERN in the unsupported-model classifier so 'not currently available for this chatgpt account' is treated as an entitlement block, not a transient outage. - storage/account-persistence: carry pinnedAccountIndex and affinityGeneration through the combined-transaction clone so doctor restore can't erase a manual pin. - storage + quota-cache: create secret dirs mode 0o700 (+ best-effort chmod on POSIX) instead of relying on umask. - forecast: exclude unavailable (policy-blocked/exhausted) accounts from recommendation candidates; return null with a clear reason when none are ready. Added/extended tests for every fix. Full suite: 4303 passed, 2 skipped, 0 failed; typecheck + lint clean. mcodex Node launcher live-verified in PowerShell. Findings detail: .omc/research/v2.2.0-audit-findings.md
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughthis pr centralizes oauth callback state per-invocation, replaces the bash launcher with a node-based mcodex, hardens config persistence with mtime CAS and a lockfile, adds AsyncLocalStorage guards for storage/routing locks to avoid reentrant deadlock, serializes local-client-token mutations, fixes capability entitlement-key lookups and LRU eviction, improves runtime-proxy storage-meta short-circuit and auth ordering, tightens filesystem permission handling, and adds many unit/regression tests. Changescomplete patch
estimated code review effort🎯 4 (complex) | ⏱️ ~45 minutes possibly related PRs
suggested labels
flagged issues
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| walPath, | ||
| storageBackupEnabled: storageBackupEnabled && existsSync(path), | ||
| ensureDirectory: async () => { | ||
| await fs.mkdir(dirname(path), { recursive: true }); | ||
| const dir = dirname(path); | ||
| // Account storage holds OAuth token material, so keep its directory | ||
| // owner-only on POSIX (mode is a no-op on win32 / ACL-based). | ||
| await fs.mkdir(dir, { recursive: true, mode: 0o700 }); | ||
| // mkdir's mode only applies to a freshly-created dir; a pre-existing dir | ||
| // from an earlier build keeps its old (possibly world-listable) perms, so | ||
| // re-assert 0o700 on POSIX. Best-effort: a chmod failure must not break | ||
| // the save (the atomic write still uses mode 0o600 for the file itself). | ||
| if (process.platform !== "win32") { | ||
| try { | ||
| await fs.chmod(dir, 0o700); | ||
| } catch { | ||
| // Best-effort hardening only. | ||
| } | ||
| } | ||
| }, | ||
| ensureGitignore: () => ensureGitignore(path), |
There was a problem hiding this comment.
the mode: 0o700 and best-effort chmod paths in both lib/storage.ts (account storage) and lib/quota-cache.ts (quota cache) have no test that asserts the resulting directory mode. the write behaviour is covered, but a future edit could silently regress the permission without any test failure. even a lightweight test that stubs fs.mkdir/fs.chmod and asserts they're called with 0o700 would pin the contract, consistent with the project's 80%+ coverage threshold.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 1801-1820
Comment:
**no vitest coverage for directory permission hardening (mode + chmod)**
the `mode: 0o700` and best-effort `chmod` paths in both `lib/storage.ts` (account storage) and `lib/quota-cache.ts` (quota cache) have no test that asserts the resulting directory mode. the write behaviour is covered, but a future edit could silently regress the permission without any test failure. even a lightweight test that stubs `fs.mkdir`/`fs.chmod` and asserts they're called with `0o700` would pin the contract, consistent with the project's 80%+ coverage threshold.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/model-capability-matrix.ts (1)
85-109:⚠️ Potential issue | 🟡 Minor | ⚡ Quick wincapability-policy writes use the same entitlement key format as the model-capability-matrix reads
- production computes
entitlementAccountKeyviaresolveEntitlementAccountKeyatindex.ts:1449-1454and passes it intocapabilityPolicyStore.recordUnsupported/recordFailure/recordSuccess(e.g.,index.ts:1830,index.ts:1844,index.ts:1888,index.ts:1961,index.ts:1994,index.ts:2124,index.ts:2206,index.ts:2354,index.ts:2639,index.ts:2680,index.ts:2773,index.ts:2853), so the sha256getAccountPolicyKeyformat is not used for these writes.lib/model-capability-matrix.tsreads capability snapshots/boosts usingresolveEntitlementAccountKey(lib/model-capability-matrix.ts:85-110), matching the store write key.- the key derivation is based on
accountId/indexand does not serialize refresh tokens into the key (lib/entitlement-cache.ts:65-74), andlib/capability-policy.tsjust composesaccountKey:normalizedModel(lib/capability-policy.ts:63-67).- tests cover the
recordUnsupportedalignment (test/runtime-policy.test.ts:84-95,test/model-capability-matrix.test.ts:54-114), but there’s no regression coverage thatrecordFailureandrecordSuccessalso use the entitlement-key format (important becausegetBoostdepends on failures/successes, not justunsupported).🤖 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/model-capability-matrix.ts` around lines 85 - 109, The model-capability-matrix reads capabilityPolicy snapshots/boosts using the entitlement key from resolveEntitlementAccountKey, but some capabilityPolicy writes (recordFailure/recordSuccess) still use the sha256/getAccountPolicyKey format; update the writer calls to pass the same entitlement key format used by resolveEntitlementAccountKey (the same value passed into recordUnsupported) so getSnapshot/getBoost will match (look for uses of capabilityPolicy.recordFailure, capabilityPolicy.recordSuccess, capabilityPolicyStore.recordFailure/recordSuccess and replace the key argument with the entitlementKey computed via resolveEntitlementAccountKey(accountId, email, index) used elsewhere), and add/update tests to assert recordFailure and recordSuccess produce boosts/snapshots readable by getBoost/getSnapshot in model-capability-matrix.
🤖 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 `@lib/codex-manager/commands/workspace.ts`:
- Around line 50-51: Update the error text to clarify non-digit inputs must be
numeric: where the code checks /^\d+$/.test(accountArg.trim()) and calls
logError(`Invalid account index: ${accountArg}`) change the message to
explicitly state it must be a positive integer (e.g., "Invalid account index
(must be a positive integer): {arg}"), and likewise update the workspace index
validation that currently returns "Invalid workspace index. Valid range:
1-{length}" to something like "Invalid workspace index (must be a positive
integer): {arg}. Valid range: 1-{length}" so non-digit inputs are clearly
distinguished from out-of-range numbers.
In `@scripts/mcodex.js`:
- Around line 304-313: The direct-run gate in isDirectRun incorrectly fails for
symlinked npm bin entries because it compares process.argv[1] to import.meta.url
without resolving symlinks; update the check in the isDirectRun IIFE to
canonicalize both sides (use fs.realpath or the existing resolvePath on
fileURLToPath(import.meta.url) and on process.argv[1]) before comparing, so the
comparison reliably detects symlinked invocations and still falls back to false
on errors; ensure the comparison logic lives with the existing
resolvePath/fileURLToPath/import.meta.url usage and keeps the try/catch
behavior, then add a deterministic vitest regression that creates a symlink (and
a Windows junction variant) to the entry and asserts runMcodex() executes
(including a concurrency test with multiple parallel invocations) to cover the
symlink and race scenarios.
---
Outside diff comments:
In `@lib/model-capability-matrix.ts`:
- Around line 85-109: The model-capability-matrix reads capabilityPolicy
snapshots/boosts using the entitlement key from resolveEntitlementAccountKey,
but some capabilityPolicy writes (recordFailure/recordSuccess) still use the
sha256/getAccountPolicyKey format; update the writer calls to pass the same
entitlement key format used by resolveEntitlementAccountKey (the same value
passed into recordUnsupported) so getSnapshot/getBoost will match (look for uses
of capabilityPolicy.recordFailure, capabilityPolicy.recordSuccess,
capabilityPolicyStore.recordFailure/recordSuccess and replace the key argument
with the entitlementKey computed via resolveEntitlementAccountKey(accountId,
email, index) used elsewhere), and add/update tests to assert recordFailure and
recordSuccess produce boosts/snapshots readable by getBoost/getSnapshot in
model-capability-matrix.
🪄 Autofix (Beta)
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
Run ID: 9bbaf487-4f09-4da5-b703-4cc4b1ffbc65
📒 Files selected for processing (24)
lib/auth/server.tslib/codex-manager.tslib/codex-manager/commands/workspace.tslib/forecast.tslib/model-capability-matrix.tslib/quota-cache.tslib/request/fetch-helpers.tslib/storage.tslib/storage/account-persistence.tslib/storage/transactions.tspackage.jsonscripts/mcodexscripts/mcodex.jstest/account-persistence.test.tstest/codex-manager-detail-tone.test.tstest/codex-manager-workspace-command.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/forecast.test.tstest/mcodex-launcher.test.tstest/model-capability-matrix.test.tstest/server.unit.test.tstest/storage-flagged.test.tstest/transactions.test.ts
💤 Files with no reviewable changes (1)
- scripts/mcodex
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (15)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/forecast.tslib/codex-manager/commands/workspace.tslib/storage/account-persistence.tslib/auth/server.tslib/storage/transactions.tslib/model-capability-matrix.tslib/codex-manager.tslib/quota-cache.tslib/storage.tslib/request/fetch-helpers.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
lib/forecast.tstest/fetch-helpers.test.tstest/model-capability-matrix.test.tslib/codex-manager/commands/workspace.tstest/forecast.test.tstest/codex-manager-detail-tone.test.tstest/account-persistence.test.tstest/storage-flagged.test.tslib/storage/account-persistence.tstest/documentation.test.tslib/auth/server.tslib/storage/transactions.tstest/transactions.test.tslib/model-capability-matrix.tstest/codex-manager-workspace-command.test.tslib/codex-manager.tslib/quota-cache.tslib/storage.tstest/mcodex-launcher.test.tslib/request/fetch-helpers.tstest/server.unit.test.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax exclusively; the project is ESM-only with
"type": "module"
Files:
lib/forecast.tstest/fetch-helpers.test.tstest/model-capability-matrix.test.tslib/codex-manager/commands/workspace.tstest/forecast.test.tstest/codex-manager-detail-tone.test.tstest/account-persistence.test.tstest/storage-flagged.test.tslib/storage/account-persistence.tstest/documentation.test.tslib/auth/server.tslib/storage/transactions.tstest/transactions.test.tslib/model-capability-matrix.tstest/codex-manager-workspace-command.test.tslib/codex-manager.tslib/quota-cache.tslib/storage.tstest/mcodex-launcher.test.tslib/request/fetch-helpers.tstest/server.unit.test.tsscripts/mcodex.js
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/forecast.tslib/codex-manager/commands/workspace.tslib/storage/account-persistence.tslib/auth/server.tslib/storage/transactions.tslib/model-capability-matrix.tslib/codex-manager.tslib/quota-cache.tslib/storage.tslib/request/fetch-helpers.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/fetch-helpers.test.tstest/model-capability-matrix.test.tstest/forecast.test.tstest/codex-manager-detail-tone.test.tstest/account-persistence.test.tstest/storage-flagged.test.tstest/documentation.test.tstest/transactions.test.tstest/codex-manager-workspace-command.test.tstest/mcodex-launcher.test.tstest/server.unit.test.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors where tests cover Windows locks
Files:
test/fetch-helpers.test.tstest/model-capability-matrix.test.tstest/forecast.test.tstest/codex-manager-detail-tone.test.tstest/account-persistence.test.tstest/storage-flagged.test.tstest/documentation.test.tstest/transactions.test.tstest/codex-manager-workspace-command.test.tstest/mcodex-launcher.test.tstest/server.unit.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/fetch-helpers.test.tstest/model-capability-matrix.test.tstest/forecast.test.tstest/codex-manager-detail-tone.test.tstest/account-persistence.test.tstest/storage-flagged.test.tstest/documentation.test.tstest/transactions.test.tstest/codex-manager-workspace-command.test.tstest/mcodex-launcher.test.tstest/server.unit.test.ts
package.json
📄 CodeRabbit inference engine (SECURITY.md)
package.json: Pinhonoto4.12.18or higher (but below4.12.0-4.12.1) to avoid GHSA-xh87-mx6m-69f3 authentication bypass vulnerability
Pinrollupto^4.59.0or higher to avoid vulnerable versions below4.59.0in Vite and Vitest transitive dependencies
Files:
package.json
lib/storage/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/storage/**/*.ts: Worktree storage usesresolveProjectStorageIdentityRoot; never derive project pools from raw worktree paths
Never use bare recursive cleanup in Windows-sensitive paths without retry handling
Files:
lib/storage/account-persistence.tslib/storage/transactions.ts
test/**/documentation.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test documentation parity including command flags, config precedence, changelog policy, and governance rules
Files:
test/documentation.test.ts
lib/auth/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Never hardcode OAuth ports; use the existing auth constants/helpers
Files:
lib/auth/server.ts
lib/auth/server.ts
📄 CodeRabbit inference engine (AGENTS.md)
OAuth callback port must remain 1455 and use canonical callback URL
http://127.0.0.1:1455/auth/callback
Files:
lib/auth/server.ts
lib/storage.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use case-insensitive email dedup via
normalizeEmailKey()with trim and lowercase normalization
Files:
lib/storage.ts
lib/request/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Node fetch returns decoded response bytes while preserving upstream
content-encoding; do not forward stale decoded encoding metadata to local clients
Files:
lib/request/fetch-helpers.ts
scripts/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors
Files:
scripts/mcodex.js
🔇 Additional comments (26)
test/documentation.test.ts (1)
581-594: LGTM!lib/quota-cache.ts (1)
238-252: lgtm!lib/forecast.ts (1)
379-406: lgtm!lib/codex-manager.ts (1)
422-427: lgtm!test/forecast.test.ts (1)
714-753: lgtm!test/codex-manager-detail-tone.test.ts (1)
73-82: lgtm!lib/auth/server.ts (1)
41-83: oauth callback isolation: concurrent regression test is present
the per-call closure capture in lib/auth/server.ts keeps callback state independent across concurrentstartLocalOAuthServercalls (lib/auth/server.ts:41-83, lib/auth/server.ts:105-127). test/server.unit.test.ts includes the high-severity regression for this exact scenario: it assertswaitForCode('state-a')andwaitForCode('state-b')return the correct codes for two concurrent servers (test/server.unit.test.ts:226-246). it also covers oauth callback edge cases like state mismatch and duplicate callbacks (test/server.unit.test.ts:157-169, test/server.unit.test.ts:194-222). windows/fs and 429/ebusy behavior are covered in other suites (test/device-auth.test.ts:305-376, test/uninstall-ebusy-retry.test.ts:57-73).test/server.unit.test.ts (4)
53-72: LGTM!
194-205: LGTM!
207-224: LGTM!
226-246: LGTM!lib/storage/transactions.ts (2)
13-24: LGTM!
45-47: LGTM!lib/storage.ts (2)
1804-1818: LGTM!
2099-2114: LGTM!lib/storage/account-persistence.ts (1)
6-25: LGTM!test/account-persistence.test.ts (2)
28-44: LGTM!
46-55: LGTM!test/storage-flagged.test.ts (1)
1074-1142: LGTM!test/transactions.test.ts (1)
206-240: LGTM!test/codex-manager-workspace-command.test.ts (1)
83-109: LGTM!lib/request/fetch-helpers.ts (2)
218-229: LGTM!
242-248: LGTM!test/model-capability-matrix.test.ts (2)
54-87: LGTM!
89-114: LGTM!test/fetch-helpers.test.ts (1)
825-851: LGTM!
- mcodex: canonicalize both sides of the direct-run gate (realpath) so the
launcher is reached when invoked via an npm-created symlink, not just the real
path; extract isDirectRunInvocation + add symlink/missing-argv regression tests
- workspace: clarify invalid-index errors ('must be a positive integer') so a
non-digit input isn't mistaken for an out-of-range number
- model-capability-matrix: add regression coverage that recordFailure/recordSuccess
(which drive getBoost) align on the entitlement key, not just recordUnsupported
- quota-cache: add a POSIX test asserting the cache dir is 0o700 (pins M5)
From the same audit pass (.omc/research/v2.2.0-audit-findings.md). 9 code fixes, 2 documented-as-intentional, 1 deferred-with-reason. Code-fixed: - local-client-tokens: route the full read-modify-write through the write queue so concurrent revoke/add/verify can't lose updates (L2) - codex-cli/state: honor forceRefresh even when a non-forced load is in flight (L7) - config: apply mtime compare-and-swap + ESTALE retry to the env-path config save, matching the unified-settings store (L8) - storage/record-utils: floor clampIndex so a fractional activeIndex can't persist or yield undefined on array access (L9) - runtime-rotation-proxy: stat-based mtime/size short-circuit so storage isn't re-read+hashed on every request (L3, settle-window guarded); check auth before path/method so unknown callers always get 401, not 404 (L5) - capability-policy: delete-then-set on record* so eviction is LRU, not FIFO (L6) - codex-bin-resolver: skip any PATH candidate resolving inside the wrapper's own dir, not just the exact codex.js realpath (L12) - codex-manager: clamp styleQuotaSummary percent to [0,100] (L11) Documented as intentional (not code-changed): - device-auth: 403/404 ARE the pending signal for this non-RFC-8628 endpoint; making them terminal would abort every login before approval. Loop already exits at expires_at/timeout. Comment added (L1) - runtime-policy: budgets are soft/eventually-consistent under concurrency; a hard cap needs cross-process reservation, out of scope for LOW. Comment added (L10) Deferred (needs design, documented inline): - runtime-rotation-proxy: routing the synchronous chooseAccount cursor mutations through the async routing mutex would force chooseAccount async across ~15 call sites and risk deadlock; documented the gap + fix direction (L4) Tests added/extended for every code fix. Full suite green.
|
Pushed 2 commits addressing the review + expanding scope to the full audit.
Full suite: 4325 passed, 2 skipped, 0 failed; typecheck + lint clean. @coderabbitai full review |
|
one open question: the triggering full review now. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@lib/codex-cli/state.ts`:
- Around line 396-402: The current logic lets forceRefresh start a fresh disk
read while an earlier non-forced inFlightLoadPromise can still resolve later and
unconditionally write cache/cacheLoadedAt (variables referenced in
loadCodexCliState and the inFlightLoadPromise flow), which allows stale reads to
overwrite fresh results; fix by introducing a load generation token (e.g.,
incrementing loadGen or attaching a unique loadId) checked before any assignment
to cache/cacheLoadedAt inside the promise completion paths so only the most
recent load commits results, ensure inFlightLoadPromise coalescing still occurs
for non-forced calls, and update the test file test/codex-cli-state.test.ts
(around the existing concurrency tests) to add a deterministic regression case
that starts a forced refresh, lets a stalePromise resolve later, then asserts
the subsequent non-forced load still returns the fresh snapshot (acc_b) to
prevent regressions.
In `@lib/local-client-tokens.ts`:
- Around line 148-177: The rename failure on Windows can return ENOTEMPTY and is
not currently retried; update isRetryableFsError to include "ENOTEMPTY"
alongside "EBUSY" and "EPERM" so writeStoreToDisk's retry loop will treat that
error as transient, and add a vitest regression in
test/local-client-tokens.test.ts that stubs fs.rename (or the platform-specific
wrapper) to throw ENOTEMPTY for a few attempts then succeed, verifying
add/rotate/revoke/verify operations still complete (mirror the transient-rename
test pattern used in quota-cache.test.ts).
In `@test/codex-bin-wrapper.test.ts`:
- Around line 4318-4349: The test mixes Windows drive-letter paths with
platform: "linux", causing PATH splitting to be host-dependent; update the
fixture so the path model matches the platform used by resolveRealCodexBin:
either (A) keep platform: "linux" and change wrapperScriptPath, wrapperDir,
wrapperSiblingCodexPath, nativeCodexPath and the PATH env entries to POSIX-style
(e.g., "/test-root/...") so existsSyncImpl and realpathSyncImpl see the intended
candidates, or (B) switch platform to "win32" and build the PATH string with
win32-style paths and delimiters so the PATH split logic behaves
deterministically; ensure the symbolic names referenced in the test
(wrapperScriptPath, wrapperDir, wrapperSiblingCodexPath, nativeCodexPath,
platform, PATH env, existsSyncImpl, realpathSyncImpl, and resolveRealCodexBin)
are updated consistently.
In `@test/codex-manager-detail-tone.test.ts`:
- Around line 84-94: Add a unit test for the 0% lower-bound by calling
styleAccountDetailText("acct (5h 0%)") and assert the rendered text contains
"0%" (i.e., the percent is clamped to 0) and does not contain any negative sign
or malformed percent like "-0%" or other bogus values; place the test alongside
the existing clamping test so it verifies the lower-bound behavior of the
clamping logic in styleAccountDetailText.
In `@test/issue-474-affinity-invalidation.test.ts`:
- Around line 218-271: The afterEach teardown in
test/issue-474-affinity-invalidation.test.ts currently uses a bare recursive fs
cleanup that flakes on Windows; replace that teardown with an awaited call to
removeWithRetry so the cleanup backoffs on EBUSY/EPERM/ENOTEMPTY. Specifically,
locate the afterEach block that removes the temp dir (the current fs.rm/fs.rmdir
call) and change it to: await removeWithRetry(tempDir) (ensure removeWithRetry
is imported/available in the test file); keep the same temp-dir variable name
used by makeTmpStoragePath so callers like the two new tests remain unchanged.
In `@test/mcodex-launcher.test.ts`:
- Around line 137-150: Replace the bare fs.rm call used in the finally block
with the shared retrying remover to avoid transient Windows
EBUSY/EPERM/ENOTEMPTY flakes: import and call removeWithRetry instead of rm
(i.e., replace await rm(tmp, { recursive: true, force: true }) with await
removeWithRetry(tmp)), keeping the existing mkdtemp/symlink/setup and references
to tmp, link, mcodexPath, isDirectRunInvocation and selfUrl unchanged so cleanup
uses exponential backoff for the temp symlink fixture.
In `@test/quota-cache.test.ts`:
- Around line 66-75: The test is passing even if saveQuotaCache() never hardens
an existing directory because tempDir is pre-created; modify the test so it
verifies the hardening behavior from a permissive starting state by either (A)
create tempDir with permissive mode (e.g. chmod 0o755) before calling
saveQuotaCache() and then stat to assert it becomes 0o700, or (B) set
CODEX_MULTI_AUTH_DIR (or the env used by saveQuotaCache) to a
nested/non-existent path under tempDir so saveQuotaCache must create the
directory and apply 0o700, referencing tempDir and the saveQuotaCache import to
locate and change the setup in test/quota-cache.test.ts.
In `@test/record-utils.test.ts`:
- Around line 1-2: Remove the explicit import of describe, it, and expect from
"vitest" in the test file and rely on the Vitest globals instead; keep the
existing import of clampIndex and isRecord from "../lib/storage/record-utils.js"
so the test still imports those functions, and update the top of
test/record-utils.test.ts to only import clampIndex and isRecord while using
describe/it/expect as globals.
🪄 Autofix (Beta)
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
Run ID: c54fa0f7-2560-43fa-a97e-af9fa5ce5103
📒 Files selected for processing (25)
lib/auth/device-auth.tslib/capability-policy.tslib/codex-cli/state.tslib/codex-manager.tslib/codex-manager/commands/workspace.tslib/config.tslib/local-client-tokens.tslib/policy/runtime-policy.tslib/runtime-rotation-proxy.tslib/storage/record-utils.tsscripts/codex-bin-resolver.jsscripts/mcodex.jstest/capability-policy.test.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/codex-manager-detail-tone.test.tstest/codex-manager-workspace-command.test.tstest/config-save.test.tstest/issue-474-affinity-invalidation.test.tstest/local-client-tokens.test.tstest/mcodex-launcher.test.tstest/model-capability-matrix.test.tstest/quota-cache.test.tstest/record-utils.test.tstest/runtime-rotation-proxy.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (12)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/policy/runtime-policy.tslib/auth/device-auth.tslib/storage/record-utils.tslib/capability-policy.tslib/codex-manager.tslib/codex-manager/commands/workspace.tslib/config.tslib/local-client-tokens.tslib/codex-cli/state.tslib/runtime-rotation-proxy.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
lib/policy/runtime-policy.tslib/auth/device-auth.tstest/record-utils.test.tstest/capability-policy.test.tstest/local-client-tokens.test.tslib/storage/record-utils.tstest/runtime-rotation-proxy.test.tslib/capability-policy.tstest/issue-474-affinity-invalidation.test.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/model-capability-matrix.test.tstest/quota-cache.test.tstest/codex-manager-workspace-command.test.tslib/codex-manager.tslib/codex-manager/commands/workspace.tslib/config.tstest/codex-manager-detail-tone.test.tslib/local-client-tokens.tstest/config-save.test.tslib/codex-cli/state.tslib/runtime-rotation-proxy.tstest/mcodex-launcher.test.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax exclusively; the project is ESM-only with
"type": "module"
Files:
lib/policy/runtime-policy.tslib/auth/device-auth.tstest/record-utils.test.tstest/capability-policy.test.tstest/local-client-tokens.test.tsscripts/codex-bin-resolver.jslib/storage/record-utils.tstest/runtime-rotation-proxy.test.tslib/capability-policy.tstest/issue-474-affinity-invalidation.test.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/model-capability-matrix.test.tstest/quota-cache.test.tstest/codex-manager-workspace-command.test.tslib/codex-manager.tslib/codex-manager/commands/workspace.tslib/config.tstest/codex-manager-detail-tone.test.tslib/local-client-tokens.tstest/config-save.test.tslib/codex-cli/state.tslib/runtime-rotation-proxy.tstest/mcodex-launcher.test.tsscripts/mcodex.js
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/policy/runtime-policy.tslib/auth/device-auth.tslib/storage/record-utils.tslib/capability-policy.tslib/codex-manager.tslib/codex-manager/commands/workspace.tslib/config.tslib/local-client-tokens.tslib/codex-cli/state.tslib/runtime-rotation-proxy.ts
lib/auth/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Never hardcode OAuth ports; use the existing auth constants/helpers
Files:
lib/auth/device-auth.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/record-utils.test.tstest/capability-policy.test.tstest/local-client-tokens.test.tstest/runtime-rotation-proxy.test.tstest/issue-474-affinity-invalidation.test.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/model-capability-matrix.test.tstest/quota-cache.test.tstest/codex-manager-workspace-command.test.tstest/codex-manager-detail-tone.test.tstest/config-save.test.tstest/mcodex-launcher.test.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors where tests cover Windows locks
Files:
test/record-utils.test.tstest/capability-policy.test.tstest/local-client-tokens.test.tstest/runtime-rotation-proxy.test.tstest/issue-474-affinity-invalidation.test.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/model-capability-matrix.test.tstest/quota-cache.test.tstest/codex-manager-workspace-command.test.tstest/codex-manager-detail-tone.test.tstest/config-save.test.tstest/mcodex-launcher.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/record-utils.test.tstest/capability-policy.test.tstest/local-client-tokens.test.tstest/runtime-rotation-proxy.test.tstest/issue-474-affinity-invalidation.test.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/model-capability-matrix.test.tstest/quota-cache.test.tstest/codex-manager-workspace-command.test.tstest/codex-manager-detail-tone.test.tstest/config-save.test.tstest/mcodex-launcher.test.ts
scripts/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors
Files:
scripts/codex-bin-resolver.jsscripts/mcodex.js
lib/storage/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/storage/**/*.ts: Worktree storage usesresolveProjectStorageIdentityRoot; never derive project pools from raw worktree paths
Never use bare recursive cleanup in Windows-sensitive paths without retry handling
Files:
lib/storage/record-utils.ts
test/**/codex-bin-wrapper.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test bin wrapper lazy-loading, missing dist handling, and concurrent invocations
Files:
test/codex-bin-wrapper.test.ts
lib/runtime-rotation-proxy.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/runtime-rotation-proxy.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responsesDo not expose account emails or tokens in runtime proxy client response headers or logs
Files:
lib/runtime-rotation-proxy.ts
🔇 Additional comments (14)
lib/auth/device-auth.ts (1)
170-184: no actionable issue in this hunk. this is a documentation-only clarification and is consistent with current pending-state logic inlib/auth/device-auth.ts:185-191.scripts/mcodex.js (1)
4-4: LGTM!Also applies to: 304-328
scripts/codex-bin-resolver.js (1)
4-4: LGTM!Also applies to: 28-41, 110-115, 178-183
lib/codex-manager.ts (2)
395-404: LGTM!
429-434: LGTM!lib/codex-manager/commands/workspace.ts (1)
51-51: LGTM!Also applies to: 96-98
test/codex-manager-workspace-command.test.ts (1)
79-79: LGTM!Also applies to: 88-88, 96-96, 106-106, 164-164
lib/capability-policy.ts (1)
76-78: LGTM!Also applies to: 94-94, 110-110
test/capability-policy.test.ts (1)
107-124: LGTM!test/model-capability-matrix.test.ts (1)
116-137: LGTM!Also applies to: 139-158
lib/storage/record-utils.ts (1)
5-7: LGTM!lib/policy/runtime-policy.ts (1)
155-163: LGTM!lib/config.ts (1)
453-463: 🏗️ Heavy liftmajor concurrency risk: mtime+rename is not no-clobber across writers
lib/config.ts:453-463 uses onlymtimeMs/null, which is a weak stale signal (especially on coarse timestamp filesystems). lib/config.ts:479-490 is effectively check-then-act: it readsmtimeMsand later does the replace (fs.rename), so another writer can modify the file between the re-check and the rename and this write can still clobber. test/config-save.test.ts:127-187 covers the earlier changed-mtime path but not the post-check/pre-rename lost-update window, so the concurrency gap remains untested. if cross-process “only-if-unchanged” semantics are required, this needs a held lock/version token that’s validated across the replace (not just astat+renamesequence).lib/runtime-rotation-proxy.ts (1)
1581-1590: ⚡ Quick windrain or abort unauthorized request bodies before returning 401
lib/runtime-rotation-proxy.ts:1581-1590 returns 401 before the MAX_REQUEST_BODY_BYTES guard in readRequestBody runs, so an unauthenticated post can bypass the body cap unless the 401 path drains/discards req (req.resume/req.destroy). leaving the body unread can stall keep-alive and enable a loopback resource exhaustion pattern.
add a regression: test/runtime-rotation-proxy.test.ts:586-627 covers unauth get/delete only, and test/runtime-rotation-proxy.test.ts:1088-1104 covers oversized bodies only for authenticated requests (postRawResponses always sets authorization at test/runtime-rotation-proxy.test.ts:186-208).
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/codex-cli-state.test.ts (1)
57-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winswitch this teardown to
removeWithRetry.
test/codex-cli-state.test.ts:84still uses barerm(..., { recursive: true, force: true }), so the windows lock cases intest/codex-cli-state.test.ts:163-222and the new in-flight read case attest/codex-cli-state.test.ts:1092-1169can leave the suite flaky onEBUSY,EPERM, orENOTEMPTY. please mirror the helper already used intest/config-save.test.ts:8-25and await retrying cleanup here too. As per coding guidelines,test/**/*.test.ts: useremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handleEBUSY/EPERM/ENOTEMPTYbackoff.🤖 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-cli-state.test.ts` around lines 57 - 85, The afterEach teardown uses a bare rm(tempDir, { recursive: true, force: true }) which can leave Windows tests flaky on EBUSY/EPERM/ENOTEMPTY; replace that call with the test helper removeWithRetry and await it (same pattern used in test/config-save.test.ts) so the tempDir cleanup retries/backoffs on transient filesystem errors—update the teardown to call removeWithRetry(tempDir) instead of rm(...) and ensure the function import/visibility for removeWithRetry is present in this test file.lib/codex-manager.ts (1)
425-443:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winquota-suffix
rate-limitedstill drops to muted in the paren path.
detailHasFailurenow flips the prefix to danger atlib/codex-manager.ts:425-434, butsuffixToneatlib/codex-manager.ts:435-443still only treatsfailed|erroras danger.styleAccountDetailText("acct (12%) — rate-limited")will paint the prefix red and leave the actual status text muted, while the compact path atlib/codex-manager.ts:452-453renders the whole detail as danger. addrate-limitedto the quota-suffix danger matcher and pin it with a regression besidetest/codex-manager-detail-tone.test.ts:65-94.🤖 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.ts` around lines 425 - 443, Prefix is correctly set to danger by detailHasFailure but suffixTone's danger regex omits "rate-limited", so quota-suffixes like "(12%) — rate-limited" render muted; update the suffix danger matcher in the suffixTone logic (the /failed|error/i branch) to also match /rate-limited/i so it returns "danger", and add a regression unit asserting styleAccountDetailText("acct (12%) — rate-limited") yields a danger tone in the existing codex-manager-detail-tone test suite (the test named codex-manager-detail-tone.test) alongside the referenced cases.
♻️ Duplicate comments (5)
test/mcodex-launcher.test.ts (1)
137-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winuse retrying cleanup for this symlink fixture.
test/mcodex-launcher.test.ts:137-149still does a bare recursiverm(...)in a windows-sensitive teardown path. after the assertion passes, transientEBUSY/EPERM/ENOTEMPTYcan still fail cleanup and flake the suite. switch thefinallyblock to the sharedremoveWithRetryhelper. as per coding guidelines,test/**/*.test.ts: useremoveWithRetryfor windows filesystem cleanup instead of barefs.rmto handleEBUSY/EPERM/ENOTEMPTYbackoff.🤖 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/mcodex-launcher.test.ts` around lines 137 - 149, The cleanup in the finally block uses a bare rm which can flake on Windows; replace the direct rm(tmp, { recursive: true, force: true }) call with the shared removeWithRetry helper to perform backoff on EBUSY/EPERM/ENOTEMPTY. Locate the teardown around mkdtemp/symlink (variables tmp and link) in test/mcodex-launcher.test.ts and call removeWithRetry(tmp) (import it if not already imported) instead of using rm so the fixture removal retries on transient Windows filesystem errors.test/codex-bin-wrapper.test.ts (1)
4312-4355:⚠️ Potential issue | 🟠 Major | ⚡ Quick winthis regression is still host-os dependent.
test/codex-bin-wrapper.test.ts:4318-4335builds windows drive-letter fixtures but passesplatform: "linux". on linux/mac runners, thePATHvalue gets split on theC:colons beforescripts/codex-bin-resolver.js:70-78can evaluate either candidate, so this test is not actually isolating the self-loop guard. keep the whole fixture posix-style, or switch the resolver setup to a real win32 model with;-joined PATH entries. as per coding guidelines,test/**/*.test.ts: tests must stay deterministic and use vitest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/codex-bin-wrapper.test.ts` around lines 4312 - 4355, The test constructs Windows-style drive-letter paths but passes platform: "linux", which causes PATH to be split on ':' and makes the test non-deterministic; update the test that calls resolveRealCodexBin to be self-consistent by either (A) converting all fixture paths to POSIX-style (no "C:" drive letters) so platform: "linux" is correct, or (B) switch to a real Win32 model by setting platform: "win32" and join the PATH entries with ';' (and keep the drive-letter paths), and ensure existsSyncImpl/realpathSyncImpl and argv use the same platform-style paths so resolveRealCodexBin, existsSyncImpl and realpathSyncImpl see the intended candidates deterministically.test/record-utils.test.ts (1)
1-1: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick windrop the explicit vitest import.
test/record-utils.test.ts:1importsdescribe,expect,itfromvitest, but this repo enables those as globals for*.test.ts. keep the source import on line 2 only.suggested cleanup
-import { describe, expect, it } from "vitest"; import { clampIndex, isRecord } from "../lib/storage/record-utils.js";As per coding guidelines,
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports.🤖 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/record-utils.test.ts` at line 1, Remove the explicit import of Vitest globals by deleting the line "import { describe, expect, it } from 'vitest';" so the test uses the repo-provided globals, leaving the existing second import on the file intact; ensure no other references to that import remain (e.g., inside test/record-utils.test.ts remove the named import and rely on global describe/it/expect).test/quota-cache.test.ts (1)
66-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winthis regression still passes even if the directory hardening never runs.
tempDiris created byfs.mkdtemp(test/quota-cache.test.ts:21) which is already0o700on posix, andtest/quota-cache.test.ts:72only stats that same dir. so the assertion stays green even ifsaveQuotaCache()never re-asserts perms on a pre-existing world-listable dir — which is precisely the upgrade pathlib/quota-cache.ts:246-252was added for. prime a permissive mode first (e.g.0o755) so the test actually exercises the chmod.proposed test tightening
if (process.platform === "win32") return; + await fs.chmod(tempDir, 0o755); const { saveQuotaCache } = await import("../lib/quota-cache.js");As per coding guidelines,
test/**: demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 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/quota-cache.test.ts` around lines 66 - 75, The test uses fs.mkdtemp which already creates tempDir with 0o700 on POSIX, so the assertion on stats.mode never verifies that saveQuotaCache actually hardens perms; modify the test to explicitly set the directory to a permissive mode (e.g. chmod tempDir to 0o755) before calling saveQuotaCache, then call saveQuotaCache({ byAccountId: {}, byEmail: {} }) and assert stats.mode & 0o777 === 0o700; update references to tempDir, saveQuotaCache and the initial fs.mkdtemp setup in the test so the behavior exercises the chmod path rather than relying on the default mkdir mode.lib/local-client-tokens.ts (1)
30-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
enotemptystill dropped from the token write queue's retry set.
lib/local-client-tokens.ts:30is still["EBUSY", "EPERM"], so the serialized rename loop atlib/local-client-tokens.ts:159-168will fail add/rotate/revoke/verify immediately on a transientENOTEMPTYwindows replacement race — the same class you already cover for the quota cache intest/quota-cache.test.ts:490-525. addENOTEMPTYand a matching vitest regression intest/local-client-tokens.test.ts.minimal hardening
-const RETRYABLE_FS_CODES = new Set(["EBUSY", "EPERM"]); +const RETRYABLE_FS_CODES = new Set(["EBUSY", "EPERM", "ENOTEMPTY"]);As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.🤖 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/local-client-tokens.ts` at line 30, Update the retryable filesystem error set by adding "ENOTEMPTY" to RETRYABLE_FS_CODES so the token write queue (used by add/rotate/revoke/verify and the serialized rename loop) will retry on Windows replacement races; then add a Vitest regression in test/local-client-tokens.test.ts that reproduces the ENOTEMPTY transient rename failure and asserts the queue correctly retries/recovers (mirror the approach used in test/quota-cache.test.ts for ENOTEMPTY coverage).
🤖 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 `@lib/config.ts`:
- Around line 453-463: The getConfigFileMtimeMs helper currently does a single
fs.stat and can abort save-path flows on transient Windows locks; update
getConfigFileMtimeMs to call the existing bounded retry wrapper used for
reads/renames (use the same retry policy and backoff function referenced
elsewhere in lib/config.ts) and treat EBUSY and EPERM as retryable (also surface
429-style transient conditions if your retry wrapper supports it), returning
null only on ENOENT and rethrowing other terminal errors; add a vitest
regression adjacent to test/config-save.test.ts that simulates a one-shot stat
lock to ensure saves take the retry path, update any relevant queues to
explicitly handle EBUSY/429 retry semantics, and audit logging in the changed
code paths to avoid emitting tokens or emails.
- Around line 475-490: The mtime check (getConfigFileMtimeMs) is a TOCTOU
preflight and must be replaced with a proper cross-process coordination: acquire
a short-lived lock/lease before the mtime check and hold it through the rename
(e.g., create a lock file like `${filePath}.lock` via fs.open with
O_CREAT|O_EXCL or use an OS-level flock if available, retry/backoff on EBUSY and
surface transient retry behavior), perform the mtime check while holding the
lock, then do fs.rename(tempPath, filePath) and release the lock; update the
retry loop around fs.rename to handle EBUSY/429 per guidelines and ensure log
messages do not include secrets. Add a vitest regression in
test/config-save.test.ts that mocks/intercepts fs.rename to inject a concurrent
write interleaving and assert the lock prevents lost updates; cite the test when
committing changes.
In `@lib/forecast.ts`:
- Around line 379-404: The filter that builds candidates currently only excludes
results with availability === "unavailable" but misses quota-exhausted accounts
which are classified as availability === "delayed" earlier; update the
candidates filter (the results -> candidates logic) to also exclude
quota-exhausted entries (e.g., check the same signal used in the earlier
classification for quota exhaustion—such as a delay reason or quota flag
alongside availability === "delayed") so exhausted accounts are not recommended,
and add a regression test in test/forecast.test.ts asserting an all-exhausted
pool yields recommendedIndex: null and the "blocked or exhausted" reason; keep
the existing hasBlockedOrExhausted logic to distinguish unavailable vs
disabled/hardFailed.
In `@lib/local-client-tokens.ts`:
- Line 151: The mkdir call that creates the multi-auth directory (await
fs.mkdir(getCodexMultiAuthDir(), { recursive: true, mode: 0o700 })) only sets
mode at creation and is ineffective for existing dirs and subject to umask;
update lib/local-client-tokens.ts to mirror the quota-cache pattern by
performing a best-effort fs.chmod re-assert after mkdir (use
getCodexMultiAuthDir() to locate the path), catching and ignoring ENOENT/EACCES
as the quota-cache does so the token-hash directory permissions are enforced
consistently at rest.
- Around line 259-268: Avoid per-request disk writes in
verifyLocalClientBearerToken: keep the lookup serialized via enqueue and
loadLocalClientTokenStore, but update record.lastUsedAt in-memory and coalesce
persistence. Gate writes to writeStoreToDisk behind a debounce/threshold (e.g.,
only when lastUsedAt advanced sufficiently or on periodic flush), and ensure
monotonic updates by applying max(existing.lastUsedAt, pendingLastUsedAt) per
tokenHash during flush. Schedule the flush outside the hot path (single shared
timer/queue) and ensure it doesn’t serialize all verifies behind other
mutations. Add tests to cover debounced persistence on repeated
verifyLocalClientBearerToken calls, monotonic lastUsedAt under concurrency, and
Windows fs.rename EBUSY/EPERM on the verify path.
In `@lib/request/fetch-helpers.ts`:
- Around line 218-220: handleErrorResponse() still uses
isUnsupportedCodexModelForChatGpt() which only matches legacy phrasing, so
update that predicate to also check the new normalized patterns (e.g.,
NORMALIZED_UNSUPPORTED_MODEL_PATTERN and
CHATGPT_CODEX_UNSUPPORTED_MODEL_PATTERN) or reuse getUnsupportedCodexModelInfo()
to detect unsupported-model wording; modify
isUnsupportedCodexModelForChatGpt()/handleErrorResponse() to consult the same
normalization logic and ensure entitlement-error rewrite and fallback guidance
run for bodies like "the model 'gpt-5.3-codex' is not currently available...";
add a regression test alongside test/fetch-helpers.test.ts (around existing
825-851 tests) that sends a 400 with the new wording and asserts the entitlement
rewrite/fallback behavior.
In `@lib/runtime-rotation-proxy.ts`:
- Around line 1113-1130: The comment points out a race where chooseAccount
mutates cursor via markSwitched() outside the routing mutex while
persistRuntimeActiveAccount uses markSwitchedLocked/withRoutingMutex later; fix
by making selection+commit atomic: convert chooseAccount -> async, move the
cursor commit into an awaited critical section that calls
withRoutingMutex/markSwitchedLocked and persistRuntimeActiveAccount together (so
selection and commit happen under the same runExclusive), replacing direct
markSwitched() calls in chooseAccount and the session-affinity/round-robin
branches with the new guarded commit path; update every caller of chooseAccount
to await the new signature and adjust tests, and add a deterministic vitest
regression in test/runtime-rotation-proxy.test.ts that simulates concurrent
requests with routingMutex="enabled" to assert no double-stamping/reorder,
ensure new queue code handles EBUSY/429 retries, and audit logging in these
paths to avoid leaking tokens/emails.
In `@scripts/mcodex.js`:
- Around line 129-147: The spawned child processes in forwardToCodexWrapper (and
the other spawn path around lines 149-171) are not receiving SIGTERM, which can
leave orphaned subprocesses; add a process.once("SIGTERM", handler) in each
spawn function that forwards the signal to the child (child.kill("SIGTERM") or
child.kill(signal)), and ensure the handler is removed when the child exits by
calling process.removeListener("SIGTERM", handler) (or using a closure and
process.once cleanup) inside the child's "close" or "exit" listener so the
parent relays SIGTERM while the child is running and stops listening after the
child terminates.
In `@test/record-utils.test.ts`:
- Around line 16-33: clampIndex currently propagates NaN (Math.trunc(NaN) =>
NaN) causing callers to get NaN; update the clampIndex implementation in
lib/storage/record-utils.ts (the function named clampIndex) to coerce or guard
NaN to 0 before truncation (e.g., normalize using Number.isNaN or isFinite and
default to 0), then truncate and clamp as before; also add a regression test in
test/record-utils.test.ts asserting expect(clampIndex(Number.NaN, 5)).toBe(0)
alongside the existing fractional tests.
In `@test/storage-flagged.test.ts`:
- Around line 1118-1128: The deadlock-guard timer created with setTimeout
(stored in timeout) is never cleared when run wins the Promise.race, leaving a
dangling timer; modify the test around the Promise.race (the timeout, run, and
recoveredFlagged variables) to capture the timer handle (from setTimeout) and
ensure you clearTimeout(handle) in a finally block after awaiting Promise.race
so the timer is always cleared whether run resolves or the timeout rejects.
---
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 425-443: Prefix is correctly set to danger by detailHasFailure but
suffixTone's danger regex omits "rate-limited", so quota-suffixes like "(12%) —
rate-limited" render muted; update the suffix danger matcher in the suffixTone
logic (the /failed|error/i branch) to also match /rate-limited/i so it returns
"danger", and add a regression unit asserting styleAccountDetailText("acct (12%)
— rate-limited") yields a danger tone in the existing codex-manager-detail-tone
test suite (the test named codex-manager-detail-tone.test) alongside the
referenced cases.
In `@test/codex-cli-state.test.ts`:
- Around line 57-85: The afterEach teardown uses a bare rm(tempDir, { recursive:
true, force: true }) which can leave Windows tests flaky on
EBUSY/EPERM/ENOTEMPTY; replace that call with the test helper removeWithRetry
and await it (same pattern used in test/config-save.test.ts) so the tempDir
cleanup retries/backoffs on transient filesystem errors—update the teardown to
call removeWithRetry(tempDir) instead of rm(...) and ensure the function
import/visibility for removeWithRetry is present in this test file.
---
Duplicate comments:
In `@lib/local-client-tokens.ts`:
- Line 30: Update the retryable filesystem error set by adding "ENOTEMPTY" to
RETRYABLE_FS_CODES so the token write queue (used by add/rotate/revoke/verify
and the serialized rename loop) will retry on Windows replacement races; then
add a Vitest regression in test/local-client-tokens.test.ts that reproduces the
ENOTEMPTY transient rename failure and asserts the queue correctly
retries/recovers (mirror the approach used in test/quota-cache.test.ts for
ENOTEMPTY coverage).
In `@test/codex-bin-wrapper.test.ts`:
- Around line 4312-4355: The test constructs Windows-style drive-letter paths
but passes platform: "linux", which causes PATH to be split on ':' and makes the
test non-deterministic; update the test that calls resolveRealCodexBin to be
self-consistent by either (A) converting all fixture paths to POSIX-style (no
"C:" drive letters) so platform: "linux" is correct, or (B) switch to a real
Win32 model by setting platform: "win32" and join the PATH entries with ';' (and
keep the drive-letter paths), and ensure existsSyncImpl/realpathSyncImpl and
argv use the same platform-style paths so resolveRealCodexBin, existsSyncImpl
and realpathSyncImpl see the intended candidates deterministically.
In `@test/mcodex-launcher.test.ts`:
- Around line 137-149: The cleanup in the finally block uses a bare rm which can
flake on Windows; replace the direct rm(tmp, { recursive: true, force: true })
call with the shared removeWithRetry helper to perform backoff on
EBUSY/EPERM/ENOTEMPTY. Locate the teardown around mkdtemp/symlink (variables tmp
and link) in test/mcodex-launcher.test.ts and call removeWithRetry(tmp) (import
it if not already imported) instead of using rm so the fixture removal retries
on transient Windows filesystem errors.
In `@test/quota-cache.test.ts`:
- Around line 66-75: The test uses fs.mkdtemp which already creates tempDir with
0o700 on POSIX, so the assertion on stats.mode never verifies that
saveQuotaCache actually hardens perms; modify the test to explicitly set the
directory to a permissive mode (e.g. chmod tempDir to 0o755) before calling
saveQuotaCache, then call saveQuotaCache({ byAccountId: {}, byEmail: {} }) and
assert stats.mode & 0o777 === 0o700; update references to tempDir,
saveQuotaCache and the initial fs.mkdtemp setup in the test so the behavior
exercises the chmod path rather than relying on the default mkdir mode.
In `@test/record-utils.test.ts`:
- Line 1: Remove the explicit import of Vitest globals by deleting the line
"import { describe, expect, it } from 'vitest';" so the test uses the
repo-provided globals, leaving the existing second import on the file intact;
ensure no other references to that import remain (e.g., inside
test/record-utils.test.ts remove the named import and rely on global
describe/it/expect).
🪄 Autofix (Beta)
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
Run ID: d58b2d21-9a39-4c35-b2ab-b5cb2bf7014e
📒 Files selected for processing (42)
lib/auth/device-auth.tslib/auth/server.tslib/capability-policy.tslib/codex-cli/state.tslib/codex-manager.tslib/codex-manager/commands/workspace.tslib/config.tslib/forecast.tslib/local-client-tokens.tslib/model-capability-matrix.tslib/policy/runtime-policy.tslib/quota-cache.tslib/request/fetch-helpers.tslib/runtime-rotation-proxy.tslib/storage.tslib/storage/account-persistence.tslib/storage/record-utils.tslib/storage/transactions.tspackage.jsonscripts/codex-bin-resolver.jsscripts/mcodexscripts/mcodex.jstest/account-persistence.test.tstest/capability-policy.test.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/codex-manager-detail-tone.test.tstest/codex-manager-workspace-command.test.tstest/config-save.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/forecast.test.tstest/issue-474-affinity-invalidation.test.tstest/local-client-tokens.test.tstest/mcodex-launcher.test.tstest/model-capability-matrix.test.tstest/quota-cache.test.tstest/record-utils.test.tstest/runtime-rotation-proxy.test.tstest/server.unit.test.tstest/storage-flagged.test.tstest/transactions.test.ts
💤 Files with no reviewable changes (1)
- scripts/mcodex
📜 Review details
🧰 Additional context used
📓 Path-based instructions (17)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/capability-policy.tslib/quota-cache.tslib/storage/record-utils.tslib/local-client-tokens.tslib/auth/device-auth.tslib/forecast.tslib/request/fetch-helpers.tslib/storage/transactions.tslib/model-capability-matrix.tslib/policy/runtime-policy.tslib/auth/server.tslib/codex-cli/state.tslib/storage/account-persistence.tslib/codex-manager/commands/workspace.tslib/codex-manager.tslib/config.tslib/runtime-rotation-proxy.tslib/storage.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
lib/capability-policy.tslib/quota-cache.tstest/quota-cache.test.tslib/storage/record-utils.tslib/local-client-tokens.tslib/auth/device-auth.tslib/forecast.tslib/request/fetch-helpers.tslib/storage/transactions.tslib/model-capability-matrix.tstest/issue-474-affinity-invalidation.test.tslib/policy/runtime-policy.tstest/codex-manager-workspace-command.test.tstest/config-save.test.tslib/auth/server.tstest/fetch-helpers.test.tstest/codex-cli-state.test.tstest/codex-manager-detail-tone.test.tstest/documentation.test.tslib/codex-cli/state.tslib/storage/account-persistence.tstest/runtime-rotation-proxy.test.tslib/codex-manager/commands/workspace.tstest/local-client-tokens.test.tstest/transactions.test.tslib/codex-manager.tstest/storage-flagged.test.tstest/model-capability-matrix.test.tslib/config.tstest/account-persistence.test.tstest/forecast.test.tstest/codex-bin-wrapper.test.tstest/server.unit.test.tstest/record-utils.test.tstest/mcodex-launcher.test.tslib/runtime-rotation-proxy.tslib/storage.tstest/capability-policy.test.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax exclusively; the project is ESM-only with
"type": "module"
Files:
lib/capability-policy.tslib/quota-cache.tstest/quota-cache.test.tslib/storage/record-utils.tslib/local-client-tokens.tslib/auth/device-auth.tslib/forecast.tslib/request/fetch-helpers.tslib/storage/transactions.tslib/model-capability-matrix.tstest/issue-474-affinity-invalidation.test.tslib/policy/runtime-policy.tstest/codex-manager-workspace-command.test.tstest/config-save.test.tslib/auth/server.tsscripts/codex-bin-resolver.jstest/fetch-helpers.test.tstest/codex-cli-state.test.tstest/codex-manager-detail-tone.test.tstest/documentation.test.tslib/codex-cli/state.tslib/storage/account-persistence.tstest/runtime-rotation-proxy.test.tslib/codex-manager/commands/workspace.tstest/local-client-tokens.test.tstest/transactions.test.tslib/codex-manager.tstest/storage-flagged.test.tstest/model-capability-matrix.test.tslib/config.tstest/account-persistence.test.tstest/forecast.test.tstest/codex-bin-wrapper.test.tstest/server.unit.test.tstest/record-utils.test.tstest/mcodex-launcher.test.tslib/runtime-rotation-proxy.tslib/storage.tsscripts/mcodex.jstest/capability-policy.test.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/capability-policy.tslib/quota-cache.tslib/storage/record-utils.tslib/local-client-tokens.tslib/auth/device-auth.tslib/forecast.tslib/request/fetch-helpers.tslib/storage/transactions.tslib/model-capability-matrix.tslib/policy/runtime-policy.tslib/auth/server.tslib/codex-cli/state.tslib/storage/account-persistence.tslib/codex-manager/commands/workspace.tslib/codex-manager.tslib/config.tslib/runtime-rotation-proxy.tslib/storage.ts
package.json
📄 CodeRabbit inference engine (SECURITY.md)
package.json: Pinhonoto4.12.18or higher (but below4.12.0-4.12.1) to avoid GHSA-xh87-mx6m-69f3 authentication bypass vulnerability
Pinrollupto^4.59.0or higher to avoid vulnerable versions below4.59.0in Vite and Vitest transitive dependencies
Files:
package.json
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/quota-cache.test.tstest/issue-474-affinity-invalidation.test.tstest/codex-manager-workspace-command.test.tstest/config-save.test.tstest/fetch-helpers.test.tstest/codex-cli-state.test.tstest/codex-manager-detail-tone.test.tstest/documentation.test.tstest/runtime-rotation-proxy.test.tstest/local-client-tokens.test.tstest/transactions.test.tstest/storage-flagged.test.tstest/model-capability-matrix.test.tstest/account-persistence.test.tstest/forecast.test.tstest/codex-bin-wrapper.test.tstest/server.unit.test.tstest/record-utils.test.tstest/mcodex-launcher.test.tstest/capability-policy.test.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors where tests cover Windows locks
Files:
test/quota-cache.test.tstest/issue-474-affinity-invalidation.test.tstest/codex-manager-workspace-command.test.tstest/config-save.test.tstest/fetch-helpers.test.tstest/codex-cli-state.test.tstest/codex-manager-detail-tone.test.tstest/documentation.test.tstest/runtime-rotation-proxy.test.tstest/local-client-tokens.test.tstest/transactions.test.tstest/storage-flagged.test.tstest/model-capability-matrix.test.tstest/account-persistence.test.tstest/forecast.test.tstest/codex-bin-wrapper.test.tstest/server.unit.test.tstest/record-utils.test.tstest/mcodex-launcher.test.tstest/capability-policy.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/quota-cache.test.tstest/issue-474-affinity-invalidation.test.tstest/codex-manager-workspace-command.test.tstest/config-save.test.tstest/fetch-helpers.test.tstest/codex-cli-state.test.tstest/codex-manager-detail-tone.test.tstest/documentation.test.tstest/runtime-rotation-proxy.test.tstest/local-client-tokens.test.tstest/transactions.test.tstest/storage-flagged.test.tstest/model-capability-matrix.test.tstest/account-persistence.test.tstest/forecast.test.tstest/codex-bin-wrapper.test.tstest/server.unit.test.tstest/record-utils.test.tstest/mcodex-launcher.test.tstest/capability-policy.test.ts
lib/storage/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/storage/**/*.ts: Worktree storage usesresolveProjectStorageIdentityRoot; never derive project pools from raw worktree paths
Never use bare recursive cleanup in Windows-sensitive paths without retry handling
Files:
lib/storage/record-utils.tslib/storage/transactions.tslib/storage/account-persistence.ts
lib/auth/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Never hardcode OAuth ports; use the existing auth constants/helpers
Files:
lib/auth/device-auth.tslib/auth/server.ts
lib/request/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Node fetch returns decoded response bytes while preserving upstream
content-encoding; do not forward stale decoded encoding metadata to local clients
Files:
lib/request/fetch-helpers.ts
lib/auth/server.ts
📄 CodeRabbit inference engine (AGENTS.md)
OAuth callback port must remain 1455 and use canonical callback URL
http://127.0.0.1:1455/auth/callback
Files:
lib/auth/server.ts
scripts/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors
Files:
scripts/codex-bin-resolver.jsscripts/mcodex.js
test/**/documentation.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test documentation parity including command flags, config precedence, changelog policy, and governance rules
Files:
test/documentation.test.ts
test/**/codex-bin-wrapper.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test bin wrapper lazy-loading, missing dist handling, and concurrent invocations
Files:
test/codex-bin-wrapper.test.ts
lib/runtime-rotation-proxy.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/runtime-rotation-proxy.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responsesDo not expose account emails or tokens in runtime proxy client response headers or logs
Files:
lib/runtime-rotation-proxy.ts
lib/storage.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use case-insensitive email dedup via
normalizeEmailKey()with trim and lowercase normalization
Files:
lib/storage.ts
🔇 Additional comments (22)
lib/auth/device-auth.ts (1)
170-191: LGTM!lib/auth/server.ts (1)
41-83: moving callback capture into per-call closures (capturedCode/capturedState) instead of sharedhttp.Serverfields is the right call — concurrent server closures stay independent and the dedup atlib/auth/server.ts:76-81cleanly preserves the first code. ports still flow throughAUTH_REDIRECT, so no hardcoding regressions.one observation, not blocking: at
lib/auth/server.ts:56the urlstateis already validated against the serverstate, andlib/auth/server.ts:83setscapturedState = state. so thecapturedState !== expectedStateguard atlib/auth/server.ts:115can only fire when a caller passes a differentexpectedStatethan the server was constructed with — purely defensive, never reachable on the realrunOAuthFlowpath. fine to keep, just don't read it as validating the callback's own state.test/server.unit.test.ts (2)
53-72: LGTM!
194-224: LGTM!lib/codex-cli/state.ts (1)
396-401: stale in-flight reads can still overwrite the refreshed cache.
lib/codex-cli/state.ts:396-401letsforceRefreshbypassinFlightLoadPromise, but the older read still commitscacheandcacheLoadedAtunconditionally atlib/codex-cli/state.ts:414-415,lib/codex-cli/state.ts:442-443,lib/codex-cli/state.ts:483-484,lib/codex-cli/state.ts:502-503,lib/codex-cli/state.ts:512-513, andlib/codex-cli/state.ts:515. if that stale task resolves last, later plainloadCodexCliState()calls can serve old data until the ttl expires.test/codex-cli-state.test.ts:1092-1169only proves the forced caller sees fresh data; it still needs to assert the next non-forced read stays onacc_bafterstalePromisesettles. As per coding guidelines,lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest), andtest/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.test/issue-474-affinity-invalidation.test.ts (1)
60-68: this suite still needs retrying windows cleanup.
test/issue-474-affinity-invalidation.test.ts:60-68is still doing bare recursive cleanup, so the new filesystem cases attest/issue-474-affinity-invalidation.test.ts:218-271keep the sameEBUSY/EPERM/ENOTEMPTYflake window open. this should still switch toremoveWithRetryand await it. As per coding guidelines,test/**/*.test.ts: useremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handleEBUSY/EPERM/ENOTEMPTYbackoff.test/runtime-rotation-proxy.test.ts (1)
582-627: LGTM!lib/capability-policy.ts (1)
72-120: lru refresh is implemented correctly.delete-then-setmoves the touched key to the tail of map insertion order, andevictIfNeeded(lib/capability-policy.ts:165-173) drops the head, so the least-recently-recorded entry goes first. since each record path adds at most one key, size only ever overshootsMAX_ENTRIESby one, so a single eviction per call keeps it bounded.LGTM!
test/forecast.test.ts (1)
714-753: good regression. it pins the exact gap the old recommender had:availability === "unavailable"withdisabled === false && hardFailure === false, which previously could leak into a misleading shortest-wait pick. deterministic clock, both overlay shapes (policyBlockedIndexesandlastPoolExhaustionSkipReasons) covered.LGTM!
test/capability-policy.test.ts (1)
107-124: this is the right test to lock in lru vs the old fifo behavior. filling to exactly 2048 keeps size at capacity (no eviction yet), re-recordingacc_0moves it off the head, thenacc_newovershoots and evicts the now-oldest idleacc_1. the three assertions together cleanly separate lru from fifo — fifo would have droppedacc_0.LGTM!
test/model-capability-matrix.test.ts (1)
54-158: tests now write underresolveEntitlementAccountKeyand assert the matrix surfaces it, which is exactly the read/write key contract the fix restores. boost math checks out:recordFailureatnow=100gives zero recovery → penalty 3 → boost -3 (test/model-capability-matrix.test.ts:116-137), and the follow-uprecordSuccessdecrements failures to 0 and adds a success → +2 (test/model-capability-matrix.test.ts:139-158). one suggestion: add a negative-control case keyed by the oldgetAccountPolicyKeysha256 to prove the matrix does not surface it — that would guard against a silent regression back to the dead-signal behavior.LGTM!
lib/model-capability-matrix.ts (1)
100-110: ⚡ Quick wincapability-store key contract needs a repo-wide guarantee
lib/model-capability-matrix.ts now reads capability snapshots/boosts using
entitlementKey(lib/model-capability-matrix.ts:100-110), and existing tests write unsupported signals under the same key (test/runtime-policy.test.ts:96-104; test/model-capability-matrix.test.ts:63-67). this is only safe if every non-test writer forcapabilityPolicyStore.recordUnsupported/recordFailure/recordSuccessalso keys byresolveEntitlementAccountKey(and never the oldgetAccountPolicyKey ("sha256:…")); otherwise the matrix can become inconsistent and effectively “half-dead”.
- add a vitest that exercises the real production writer(s) + reader(s) together (not just
recordUnsupportedin tests) and fails if any writer still uses the legacy key- add coverage for concurrent/interleaved updates (parallel record + matrix build) to catch queue/retry scenarios (e.g., 429/ebusy paths)
- ensure there’s no logging of tokens/emails when these keys are computed, and add windows fs coverage only if capability-store persistence uses windows path/normalization logic
lib/storage/transactions.ts (1)
13-24: LGTM!Also applies to: 45-47
lib/storage.ts (2)
1804-1818: LGTM!
2099-2114: LGTM!lib/storage/account-persistence.ts (1)
6-25: LGTM!test/account-persistence.test.ts (1)
27-56: LGTM!test/transactions.test.ts (3)
7-8: LGTM!
205-240: LGTM!
205-240: reentrancy deadlock regression test is already covered
test/storage-flagged.test.ts:1074-1142includesit("recovers a flagged .bak without deadlocking inside a held storage lock"... ), with aPromise.race+ 3000ms timeout to fail on hangs.- the lock reentrancy guard is implemented in
lib/storage.ts:2089-2114(persistRecoveredBackupusesisStorageLockHeld()to avoid re-acquiring the mutex).lib/quota-cache.ts (1)
238-252: LGTM!test/local-client-tokens.test.ts (1)
84-117: LGTM!
Two real bugs in the prior LOW fixes + test-quality hardening per repo conventions. Real bugs: - codex-cli/state: the forceRefresh-bypasses-in-flight change (L7) let a slower stale non-forced load commit its snapshot last and poison the cache. Added a load-generation guard so only the latest load commits cache/cacheLoadedAt; the caller still gets its own fresh return. Extended the race test to assert a post-stale plain read still serves fresh data. - local-client-tokens: the queued rename retry set only had EBUSY/EPERM; a transient ENOTEMPTY/EACCES/EAGAIN on Windows rename would fail the mutation. Widened to match quota-cache's taxonomy + added an ENOTEMPTY-retry regression. Test quality (CodeRabbit + repo guidelines): - codex-bin-wrapper: the L12 self-loop test mixed win32 drive-letter paths with platform:'linux'; switched to one POSIX path model end-to-end so PATH splitting is host-independent. - quota-cache + storage: the dir-perm tests now start from a 0o755 existing dir so they actually exercise the chmod-on-existing-dir hardening (were false-passes). - mcodex-launcher + issue-474: temp cleanup now uses removeWithRetry (EBUSY/EPERM/ ENOTEMPTY backoff) instead of bare fs.rm. - detail-tone: added a 0% lower-bound clamp case. - record-utils: dropped the explicit vitest import (globals enabled). Full suite: 4327 passed, 3 skipped, 0 failed; typecheck + lint clean.
|
Addressed all 8 items from the second review in Real bugs (both in the prior LOW fixes):
Test quality:
Full suite: 4327 passed, 3 skipped, 0 failed; typecheck + lint clean. @coderabbitai full review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check load-generation guard in codex-cli/state.ts
rg -n "generation|loadGeneration|cacheLoadedAt|inFlight" lib/codex-cli/state.ts -A2 -B2
# Check local-client-tokens retry set
rg -n "ENOTEMPTY|EAGAIN|EACCES|EBUSY|EPERM" lib/local-client-tokens.ts -A2 -B2
# Check clampIndex NaN guard
rg -n "clampIndex|isNaN|NaN|trunc" lib/storage/record-utils.ts -A5 -B2
# Check removeWithRetry usage in tests
rg -n "removeWithRetry" test/ -A2 -B2
# Check quota-cache + storage perm tests starting from 0o755
rg -n "0o755|chmod|0o700" test/quota-cache.test.ts test/storage-flagged.test.ts -A3 -B3
# Check detail-tone 0% lower-bound
rg -n "0%" test/codex-manager-detail-tone.test.ts -A3 -B3
# Check record-utils vitest import
rg -n "import.*vitest\|from.*vitest" test/record-utils.test.ts
# Check isDirectRunInvocation
rg -n "isDirectRunInvocation" scripts/mcodex.js -A5 -B3Length of output: 35894
confirmed clean:
one still-open item: Math.max(0, Math.min(Math.trunc(index), length - 1))
export function clampIndex(index: number, length: number): number {
if (length <= 0) return 0;
if (!Number.isFinite(index)) return 0; // guard NaN and ±Infinity
return Math.max(0, Math.min(Math.trunc(index), length - 1));
}add a test case for minor hygiene note: triggering full review now. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/codex-bin-wrapper.test.ts (1)
4332-4335:⚠️ Potential issue | 🟠 Major | ⚡ Quick winuse a linux path separator in the linux fixture.
test/codex-bin-wrapper.test.ts:4332still buildsPATHwithnode:path's hostdelimiter, but this case forcesplatform: "linux"attest/codex-bin-wrapper.test.ts:4335. on windows runners that yields;, so the resolver is no longer exercising a linux-style path scan and the regression becomes host-dependent again.suggested fix
const resolved = resolveRealCodexBin({ env: { - PATH: [wrapperDir, join("/test-root", "native", "bin")].join(delimiter), + PATH: `${wrapperDir}:${join("/test-root", "native", "bin")}`, }, argv: [process.execPath, wrapperScriptPath], platform: "linux",verify that the resolver is using the simulated platform semantics, not the host delimiter, before keeping
delimiterhere:#!/bin/bash set -euo pipefail printf '\nchanged test segment:\n' sed -n '4312,4338p' test/codex-bin-wrapper.test.ts printf '\nresolver path-splitting references:\n' rg -n -C3 'splitPathEntries|resolveCodexExecutableFromSystemPath' scripts/codex-bin-resolver.js printf '\nnode path separators:\n' node <<'NODE' const path = require('node:path'); console.log({ hostDelimiter: path.delimiter, linuxDelimiter: path.posix.delimiter, win32Delimiter: path.win32.delimiter, }); NODEas per coding guidelines, "tests must stay deterministic and use vitest."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/codex-bin-wrapper.test.ts` around lines 4332 - 4335, The linux fixture is building PATH with the host's path.delimiter (variable delimiter) while the test sets platform: "linux", causing non-deterministic behavior on Windows; change the PATH construction in the test (where PATH: [wrapperDir, join("/test-root", "native", "bin")].join(delimiter)) to use a linux separator (e.g. path.posix.delimiter or the literal ':'), referencing the same symbols wrapperDir, join, wrapperScriptPath and platform so the resolver sees linux-style path entries; ensure the resolver invocation in this test uses the simulated platform semantics rather than the host delimiter.
🤖 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 `@test/local-client-tokens.test.ts`:
- Around line 119-144: The test only simulates ENOTEMPTY but the retryable list
in addLocalClientToken (and the error handling in lib/local-client-tokens.ts)
now includes EAGAIN and EACCES; update test/local-client-tokens.test.ts to
parameterize the case over the retryable error codes (e.g., ENOTEMPTY, EAGAIN,
EACCES) so the renameSpy mockImplementation throws each code on the first
attempt and succeeds thereafter, asserting attempts > 1 and the token exists for
each code by reusing addLocalClientToken and loadLocalClientTokenStore in the
parameterized subtests or a loop.
In `@test/storage.test.ts`:
- Around line 67-70: Replace the bare fs.rm call in the afterEach block with the
test suite's retry-safe helper: call and await removeWithRetry(storageDir, {
recursive: true, force: true }) instead of await fs.rm(...); keep the existing
setStoragePathDirect(null) call, and add or ensure removeWithRetry is
imported/available in this test file so Windows EBUSY/EPERM/ENOTEMPTY transient
errors are retried like in local-client-tokens.test.ts.
---
Duplicate comments:
In `@test/codex-bin-wrapper.test.ts`:
- Around line 4332-4335: The linux fixture is building PATH with the host's
path.delimiter (variable delimiter) while the test sets platform: "linux",
causing non-deterministic behavior on Windows; change the PATH construction in
the test (where PATH: [wrapperDir, join("/test-root", "native",
"bin")].join(delimiter)) to use a linux separator (e.g. path.posix.delimiter or
the literal ':'), referencing the same symbols wrapperDir, join,
wrapperScriptPath and platform so the resolver sees linux-style path entries;
ensure the resolver invocation in this test uses the simulated platform
semantics rather than the host delimiter.
🪄 Autofix (Beta)
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
Run ID: 5927890a-2846-41e8-849f-2fe50ada8009
📒 Files selected for processing (11)
lib/codex-cli/state.tslib/local-client-tokens.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/codex-manager-detail-tone.test.tstest/issue-474-affinity-invalidation.test.tstest/local-client-tokens.test.tstest/mcodex-launcher.test.tstest/quota-cache.test.tstest/record-utils.test.tstest/storage.test.ts
💤 Files with no reviewable changes (1)
- test/record-utils.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (9)
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/quota-cache.test.tstest/codex-cli-state.test.tstest/storage.test.tstest/local-client-tokens.test.tstest/codex-bin-wrapper.test.tstest/issue-474-affinity-invalidation.test.tstest/codex-manager-detail-tone.test.tstest/mcodex-launcher.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
test/quota-cache.test.tstest/codex-cli-state.test.tstest/storage.test.tstest/local-client-tokens.test.tstest/codex-bin-wrapper.test.tstest/issue-474-affinity-invalidation.test.tslib/local-client-tokens.tstest/codex-manager-detail-tone.test.tslib/codex-cli/state.tstest/mcodex-launcher.test.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax exclusively; the project is ESM-only with
"type": "module"
Files:
test/quota-cache.test.tstest/codex-cli-state.test.tstest/storage.test.tstest/local-client-tokens.test.tstest/codex-bin-wrapper.test.tstest/issue-474-affinity-invalidation.test.tslib/local-client-tokens.tstest/codex-manager-detail-tone.test.tslib/codex-cli/state.tstest/mcodex-launcher.test.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors where tests cover Windows locks
Files:
test/quota-cache.test.tstest/codex-cli-state.test.tstest/storage.test.tstest/local-client-tokens.test.tstest/codex-bin-wrapper.test.tstest/issue-474-affinity-invalidation.test.tstest/codex-manager-detail-tone.test.tstest/mcodex-launcher.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/quota-cache.test.tstest/codex-cli-state.test.tstest/storage.test.tstest/local-client-tokens.test.tstest/codex-bin-wrapper.test.tstest/issue-474-affinity-invalidation.test.tstest/codex-manager-detail-tone.test.tstest/mcodex-launcher.test.ts
test/**/storage.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test concurrent access patterns, worktree migration, and forged pointer handling in storage tests
Files:
test/storage.test.ts
test/**/codex-bin-wrapper.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test bin wrapper lazy-loading, missing dist handling, and concurrent invocations
Files:
test/codex-bin-wrapper.test.ts
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/local-client-tokens.tslib/codex-cli/state.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/local-client-tokens.tslib/codex-cli/state.ts
🔇 Additional comments (7)
test/issue-474-affinity-invalidation.test.ts (1)
1-6: LGTM!Also applies to: 25-43, 85-90
test/codex-manager-detail-tone.test.ts (1)
95-104: LGTM!test/quota-cache.test.ts (1)
70-73: LGTM!test/storage.test.ts (1)
72-97: LGTM!lib/local-client-tokens.ts (1)
30-36: LGTM!lib/codex-cli/state.ts (1)
41-45: LGTM!Also applies to: 411-531
test/codex-cli-state.test.ts (1)
1165-1170: LGTM!
Completes partial fixes flagged in the third review, bumps to v2.2.1. Real bugs (prior fixes were incomplete): - request/fetch-helpers: isUnsupportedCodexModelForChatGpt (the handleErrorResponse path) now also matches NORMALIZED_UNSUPPORTED_MODEL_PATTERN, so a 400 'model not currently available for this chatgpt account' gets the entitlement rewrite, not generic error guidance (#13) - forecast: quota-exhausted accounts are classified 'delayed' (not 'unavailable'), so they slipped the recommendation filter; added an explicit exhausted flag and excluded it, returning null when the whole pool is exhausted (#10) - storage/record-utils: clampIndex guards NaN -> 0 (Math.trunc(NaN) propagated) (#16) - local-client-tokens: debounce lastUsedAt persistence (60s threshold) so the bearer-verify hot path stops writing to disk every request; +chmod 0o700 re-assert on the token-store dir (#12, #11) - mcodex: relay SIGTERM/SIGINT to the spawned child so it isn't orphaned (#15) Test quality: - local-client-tokens: parameterized rename-retry test over ENOTEMPTY/EAGAIN/EACCES (#18) - storage-flagged: clear the H4 deadlock-guard timer on the happy path (#17) - storage: removeWithRetry for suite cleanup (#19) Release: - bump package.json + .codex-plugin/plugin.json to 2.2.1 - add docs/releases/v2.2.1.md; point docs portal + README at it Known follow-ups (documented, deferred — need design, not rushed into a patch): config env-path save is a single-process CAS not a true cross-process lock (#8/#9); verifyLocalClientBearerToken read stays serialized but a fuller lease is future work; runtime proxy routingMutex='enabled' still has a select/commit cursor race (#14) requiring an async refactor of chooseAccount across its call sites. Full suite: 4337 passed, 3 skipped, 0 failed; typecheck + lint clean.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/request/fetch-helpers.ts (1)
192-199: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winprefer one unsupported-model text matcher.
the same three-pattern check now lives in
lib/request/fetch-helpers.ts:192-199,lib/request/fetch-helpers.ts:219-223, andlib/request/fetch-helpers.ts:243-249. this drift already caused the regression you just fixed. pull it behind one helper so the 400 rewrite path, payload parsing, and fallback detection stay locked together.Based on learnings, "normalize error message detection across all code paths (probe/forecast/report/check surfaces) to ensure consistent classification of unavailable models as entitlement blocks".possible cleanup
+function matchesUnsupportedCodexModelText(text: string): boolean { + return ( + CHATGPT_CODEX_UNSUPPORTED_MODEL_PATTERN.test(text) || + NORMALIZED_UNSUPPORTED_MODEL_PATTERN.test(text) || + MODEL_ACCESS_DENIED_PATTERN.test(text) + ); +} + function isUnsupportedCodexModelForChatGpt(status: number, bodyText: string): boolean { if (status !== HTTP_STATUS.BAD_REQUEST) return false; if (!bodyText) return false; - return ( - CHATGPT_CODEX_UNSUPPORTED_MODEL_PATTERN.test(bodyText) || - NORMALIZED_UNSUPPORTED_MODEL_PATTERN.test(bodyText) || - MODEL_ACCESS_DENIED_PATTERN.test(bodyText) - ); + return matchesUnsupportedCodexModelText(bodyText); }Also applies to: 219-223, 243-249
🤖 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/request/fetch-helpers.ts` around lines 192 - 199, Consolidate the repeated three-pattern unsupported-model checks into a single helper and replace the duplicate checks; create a shared function (e.g., isUnsupportedModelText or normalizeUnsupportedModelMatch) that encapsulates CHATGPT_CODEX_UNSUPPORTED_MODEL_PATTERN, NORMALIZED_UNSUPPORTED_MODEL_PATTERN and MODEL_ACCESS_DENIED_PATTERN testing and use it from isUnsupportedCodexModelForChatGpt plus the other call sites currently duplicating the logic (the checks around the 400 rewrite/payload parsing/fallback detection). Update isUnsupportedCodexModelForChatGpt to call that new helper and refactor the other two locations (the similar checks at the other occurrences) to call the same helper so all surfaces use a single, consistent matcher.lib/forecast.ts (1)
237-250:⚠️ Potential issue | 🟠 Major | ⚡ Quick winlet live quota override cache-only exhaustion.
evaluateForecastAccountsetsexhausted = truefromquotaCacheatlib/forecast.ts:237-253and never revisits it after processinginput.liveQuotaatlib/forecast.ts:276-314. with the new!result.exhaustedgate inlib/forecast.ts:395-400, a stale cache entry can suppress an account even when the live probe shows headroom, which can incorrectly send the pool down the null"blocked or exhausted"path. either only mark cache exhaustion when no live probe is present, or let a healthy live probe clear that flag, and add a regression intest/forecast.test.tsfor cache-exhausted + live-healthy inputs.Also applies to: 395-400
🤖 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/forecast.ts` around lines 237 - 250, The code currently sets exhausted = true based only on quotaCache inside evaluateForecastAccount (when checking quotaCache/primary/secondary) which can be stale and block accounts even if input.liveQuota shows headroom; change the logic so cache-only exhaustion is applied only when no live probe is present (i.e., only set exhausted from quotaCache if input.liveQuota is undefined/null) OR ensure that after processing input.liveQuota you clear exhausted when the live probe indicates available headroom (update the exhausted flag after liveQuota handling so the later !result.exhausted gate behaves correctly); update the logic that reads quotaCache/primary/secondary and the branch around result.exhausted to reflect this; add a regression test in test/forecast.test.ts covering cache-exhausted + live-healthy inputs to prevent regressions.scripts/mcodex.js (1)
171-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winpreserve relayed sigterm exit status
- both close handlers in
scripts/mcodex.jsonly special-casesigint; when the child closes withsignal === "sigterm"(often withcode === null), the code falls through toprocess.exit(typeof code === "number" ? code : 1), turning an intentional termination into exit code1.- add an explicit
sigtermbranch (same logic in bothforwardtocodexwrapperandrunmonitor), consistent with the existingsigint => 130mapping (sigterm => 143).- regression coverage is missing for this exit-code mapping:
test/mcodex-launcher.test.ts:187-251only validatesrelaysignalsToChildwiring/cleanup, not the launcherchild.once("close"... )exit mapping. windows signal tests are also intentionally non-cross-process there, so this is still easy to miss.proposed fix
child.once("close", (code, signal) => { removeSignalRelays(); if (signal === "SIGINT") { process.exit(130); return; } + if (signal === "SIGTERM") { + process.exit(143); + return; + } process.exit(typeof code === "number" ? code : 1); });🤖 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 `@scripts/mcodex.js` around lines 171 - 177, The close handlers that call child.once("close", ...) in scripts/mcodex.js currently only special-case SIGINT; modify both places (the close handler inside forwardToCodexWrapper and the one in runMonitor) to also check for signal === "SIGTERM" and call process.exit(143) (mirroring the SIGINT => 130 mapping), preserving removeSignalRelays() behavior before exiting; also add/adjust unit tests in test/mcodex-launcher.test.ts to cover the child close path mapping for SIGTERM so the relayed sigterm exit status is asserted.
🤖 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 `@test/mcodex-launcher.test.ts`:
- Line 5: Remove the explicit Vitest globals import line that imports describe,
expect, and it; since test files use Vitest globals, delete the import statement
referencing "describe, expect, it" (the top-level import) in
test/mcodex-launcher.test.ts so the test uses the global describe/it/expect
instead.
---
Outside diff comments:
In `@lib/forecast.ts`:
- Around line 237-250: The code currently sets exhausted = true based only on
quotaCache inside evaluateForecastAccount (when checking
quotaCache/primary/secondary) which can be stale and block accounts even if
input.liveQuota shows headroom; change the logic so cache-only exhaustion is
applied only when no live probe is present (i.e., only set exhausted from
quotaCache if input.liveQuota is undefined/null) OR ensure that after processing
input.liveQuota you clear exhausted when the live probe indicates available
headroom (update the exhausted flag after liveQuota handling so the later
!result.exhausted gate behaves correctly); update the logic that reads
quotaCache/primary/secondary and the branch around result.exhausted to reflect
this; add a regression test in test/forecast.test.ts covering cache-exhausted +
live-healthy inputs to prevent regressions.
In `@lib/request/fetch-helpers.ts`:
- Around line 192-199: Consolidate the repeated three-pattern unsupported-model
checks into a single helper and replace the duplicate checks; create a shared
function (e.g., isUnsupportedModelText or normalizeUnsupportedModelMatch) that
encapsulates CHATGPT_CODEX_UNSUPPORTED_MODEL_PATTERN,
NORMALIZED_UNSUPPORTED_MODEL_PATTERN and MODEL_ACCESS_DENIED_PATTERN testing and
use it from isUnsupportedCodexModelForChatGpt plus the other call sites
currently duplicating the logic (the checks around the 400 rewrite/payload
parsing/fallback detection). Update isUnsupportedCodexModelForChatGpt to call
that new helper and refactor the other two locations (the similar checks at the
other occurrences) to call the same helper so all surfaces use a single,
consistent matcher.
In `@scripts/mcodex.js`:
- Around line 171-177: The close handlers that call child.once("close", ...) in
scripts/mcodex.js currently only special-case SIGINT; modify both places (the
close handler inside forwardToCodexWrapper and the one in runMonitor) to also
check for signal === "SIGTERM" and call process.exit(143) (mirroring the SIGINT
=> 130 mapping), preserving removeSignalRelays() behavior before exiting; also
add/adjust unit tests in test/mcodex-launcher.test.ts to cover the child close
path mapping for SIGTERM so the relayed sigterm exit status is asserted.
🪄 Autofix (Beta)
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
Run ID: eac86f5c-485f-414c-b4ec-19c3d1cdf47c
📒 Files selected for processing (17)
.codex-plugin/plugin.jsonREADME.mddocs/README.mddocs/releases/v2.2.1.mdlib/forecast.tslib/local-client-tokens.tslib/request/fetch-helpers.tslib/storage/record-utils.tspackage.jsonscripts/mcodex.jstest/fetch-helpers.test.tstest/forecast.test.tstest/local-client-tokens.test.tstest/mcodex-launcher.test.tstest/record-utils.test.tstest/storage-flagged.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (21)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/storage/record-utils.tslib/request/fetch-helpers.tslib/forecast.tslib/local-client-tokens.ts
lib/storage/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/storage/**/*.ts: Worktree storage usesresolveProjectStorageIdentityRoot; never derive project pools from raw worktree paths
Never use bare recursive cleanup in Windows-sensitive paths without retry handling
Files:
lib/storage/record-utils.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
lib/storage/record-utils.tstest/record-utils.test.tslib/request/fetch-helpers.tstest/storage-flagged.test.tstest/storage.test.tstest/fetch-helpers.test.tstest/forecast.test.tslib/forecast.tstest/local-client-tokens.test.tstest/mcodex-launcher.test.tslib/local-client-tokens.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax exclusively; the project is ESM-only with
"type": "module"
Files:
lib/storage/record-utils.tstest/record-utils.test.tslib/request/fetch-helpers.tstest/storage-flagged.test.tstest/storage.test.tstest/fetch-helpers.test.tstest/forecast.test.tslib/forecast.tstest/local-client-tokens.test.tstest/mcodex-launcher.test.tslib/local-client-tokens.tsscripts/mcodex.js
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/storage/record-utils.tslib/request/fetch-helpers.tslib/forecast.tslib/local-client-tokens.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/record-utils.test.tstest/storage-flagged.test.tstest/storage.test.tstest/fetch-helpers.test.tstest/forecast.test.tstest/local-client-tokens.test.tstest/mcodex-launcher.test.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors where tests cover Windows locks
Files:
test/record-utils.test.tstest/storage-flagged.test.tstest/storage.test.tstest/fetch-helpers.test.tstest/forecast.test.tstest/local-client-tokens.test.tstest/mcodex-launcher.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/record-utils.test.tstest/storage-flagged.test.tstest/storage.test.tstest/fetch-helpers.test.tstest/forecast.test.tstest/local-client-tokens.test.tstest/mcodex-launcher.test.ts
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 documentationAll documentation files for the codex-multi-auth project should follow the structure and link conventions established in the documentation portal index
Files:
docs/releases/v2.2.1.mddocs/README.md
docs/releases/v*.md
📄 CodeRabbit inference engine (docs/README.md)
Release notes must be stored in the releases/ directory with filenames following the semantic versioning format (e.g., releases/v2.2.1.md)
Files:
docs/releases/v2.2.1.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/releases/v2.2.1.mddocs/README.md
lib/request/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Node fetch returns decoded response bytes while preserving upstream
content-encoding; do not forward stale decoded encoding metadata to local clients
Files:
lib/request/fetch-helpers.ts
docs/{README.md,docs/**/*.md}
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
docs/{README.md,docs/**/*.md}: Use canonical package namecodex-multi-authin all documentation
Verify internal links in documentation are valid before merge
Ensure no conflicting guidance exists between README, docs, and governance files before merge
Files:
docs/README.md
docs/{README.md,docs/getting-started.md}
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Update
README.mdanddocs/getting-started.mdfirst when runtime behavior changes
Files:
docs/README.md
docs/{README.md,docs/getting-started.md,docs/configuration.md,docs/troubleshooting.md,docs/reference/**/*.md}
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Verify every documented command is executable as written before merge
Files:
docs/README.md
docs/{docs/**/*.md,README.md}
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Verify paths in documentation match runtime modules (
lib/runtime-paths.ts,lib/storage.ts,lib/config.ts)
Files:
docs/README.md
docs/{README.md,docs/index.md,docs/README.md}
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Use accurate discoverability terms in public landing pages without keyword stuffing or ranking promises
Root README and docs landing pages should naturally include
Codex CLI,multi-account OAuth,account switching,health checks,runtime rotation,diagnostics, andrecoverywhen those topics are in scope
Files:
docs/README.md
docs/{README.md,package.json,docs/development/GITHUB_DISCOVERABILITY.md}
📄 CodeRabbit inference engine (docs/STYLE_GUIDE.md)
Keep the repository description, package description, README lead, and
docs/development/GITHUB_DISCOVERABILITY.mdaligned
Files:
docs/README.md
package.json
📄 CodeRabbit inference engine (SECURITY.md)
package.json: Pinhonoto4.12.18or higher (but below4.12.0-4.12.1) to avoid GHSA-xh87-mx6m-69f3 authentication bypass vulnerability
Pinrollupto^4.59.0or higher to avoid vulnerable versions below4.59.0in Vite and Vitest transitive dependenciesDo not publish a global
codexbinary; keepcodexowned by the official OpenAI install path and usecodex-multi-auth-codex ...only when intentionally wanting the forwarding wrapperThree distinct global binaries must ship:
codex-multi-auth(primary account manager),codex-multi-auth-codex(optional forwarding wrapper), andcodex-multi-auth-app-launcher(optional desktop launcher helper)
Files:
package.json
test/**/storage.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test concurrent access patterns, worktree migration, and forged pointer handling in storage tests
Files:
test/storage.test.ts
scripts/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors
Files:
scripts/mcodex.js
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T17:43:11.310Z
Learning: Validate configuration after changes by running: `codex-multi-auth status`, `codex-multi-auth check`, and `codex-multi-auth forecast --live`
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T17:43:11.310Z
Learning: Runtime rotation is enabled by default for request-bearing wrapper-launched Codex sessions; opt out via `CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0`, `CODEX_MULTI_AUTH_APP_BIND_INSTALL=0`, or `CODEX_MULTI_AUTH_APP_LAUNCHER_INSTALL=0`
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T17:43:11.310Z
Learning: 60-second recovery procedure: run `codex-multi-auth doctor --fix`, then `codex-multi-auth check`, then `codex-multi-auth forecast --live`
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T17:43:11.310Z
Learning: Use `codex-multi-auth login --device-auth` for remote or headless shells instead of browser-based login
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T17:43:18.882Z
Learning: Daily-use and user-facing documentation should include quick-start commands, common workflows, FAQs, troubleshooting, and feature capability maps
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T17:43:18.882Z
Learning: Documentation should clearly distinguish between user-facing guides (Getting Started, Daily Use, Repair) and maintainer-only references (development/, audits/)
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T17:43:36.202Z
Learning: Normalize error message detection across all code paths (probe/forecast/report/check surfaces) to ensure consistent classification of unavailable models as entitlement blocks
🔇 Additional comments (11)
test/fetch-helpers.test.ts (1)
1190-1210: LGTM!lib/storage/record-utils.ts (1)
7-11: LGTM!test/storage-flagged.test.ts (1)
1119-1135: LGTM!test/storage.test.ts (1)
10-10: LGTM!Also applies to: 68-71
test/record-utils.test.ts (1)
34-41: LGTM!lib/local-client-tokens.ts (3)
30-37: LGTM!
165-179: LGTM!
293-303: LGTM!test/local-client-tokens.test.ts (3)
84-128: LGTM!
130-163: LGTM!
165-193: LGTM!
Completes the deferred architectural items from the audit rather than shipping them as known follow-ups. - config (#8): wrap the CAS stat() in the bounded transient-FS retry so a Windows EBUSY/EPERM on stat no longer aborts an env-path save. - config (#9): add a cross-process file lock (modeled on RefreshLeaseCoordinator: exclusive 'wx' lockfile, pid/expiry payload, stale takeover, finally release) around the env-path read->check->merge->rename critical section, so two processes can no longer lose each other's update. In-process queue + mtime CAS retained as fast path / second-line guard. - runtime proxy routing mutex (#14): make withRoutingMutex reentrant via AsyncLocalStorage (mirrors the H4 storage-lock fix), then run selection + cursor commit inside one acquisition when routingMutex='enabled'. The nested markSwitchedLocked runs inline (reentrant) so there's no double-acquire or deadlock; persistRuntimeActiveAccount's commit is gated to legacy-only to avoid a redundant post-fetch re-advance. Legacy mode behavior + perf unchanged. Each fix ships with a deterministic regression (stat-EBUSY retry; rename-time competing-write interleave; two concurrent enabled-mode requests get distinct accounts). Fix-sensitivity confirmed: reverting the reentrancy guard deadlocks the new proxy test. Updated docs/releases/v2.2.1.md to reflect these as fixed, not deferred. Full suite: 4341 passed, 3 skipped, 0 failed; typecheck + lint clean.
|
Closed the 3 heavy concurrency gaps in
#12 (bearer per-request disk write) was resolved earlier via lastUsedAt debounce. Full suite: 4341 passed, 3 skipped, 0 failed; typecheck + lint clean. @coderabbitai full review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check config.ts - file lock, stat retry, CAS
rg -n "lockfile\|wx\|pid\|expiry\|stale\|EBUSY\|EPERM\|ESTALE\|stat\|retry\|mtime" lib/config.ts -A3 -B2 | head -120
echo "=== config.ts lock-related functions ==="
rg -n "lock\|Lock\|lease\|Lease" lib/config.ts -A5 -B2 | head -80
echo "=== config.ts full savePluginConfig ==="
ast-grep --pattern 'async function savePluginConfig($_) { $$$ }'
echo "=== routing-mutex AsyncLocalStorage ==="
rg -n "AsyncLocalStorage\|routingMutex\|withRoutingMutex\|reentrant\|isRouting" lib/runtime-rotation-proxy.ts -A4 -B2 | head -100
echo "=== clampIndex NaN guard ==="
cat lib/storage/record-utils.ts
echo "=== record-utils test NaN coverage ==="
rg -n "NaN\|Infinity\|isFinite\|clampIndex" test/record-utils.test.ts -A3 -B2
echo "=== config-save test regression ==="
rg -n "lockfile\|wx\|concurrent\|competing\|rename" test/config-save.test.ts -A5 -B2 | head -80
echo "=== removeWithRetry duplication ==="
rg -n "removeWithRetry" test/ -A5 -B2 | head -80Length of output: 6775 🏁 Script executed: #!/bin/bash
echo "=== config.ts - full file or key sections ==="
wc -l lib/config.ts
rg -n "savePluginConfig\|lockFile\|lockPath\|acquireLock\|releaseLock\|withLock\|ESTALE\|expectedMtime\|statRetry\|transientFs\|EBUSY\|EPERM\|wx\b\|\.pid\b\|expiry\|stale" lib/config.ts | head -60
echo "=== config.ts lines 1-80 ==="
sed -n '1,80p' lib/config.ts
echo "=== runtime-rotation-proxy.ts - routing mutex ==="
wc -l lib/runtime-rotation-proxy.ts
rg -n "AsyncLocalStorage\|RoutingLock\|routingLock\|isHeld\|reentrant\|withRoutingMutex\|markSwitched\|reentrancy" lib/runtime-rotation-proxy.ts | head -40
echo "=== record-utils test - NaN/Infinity coverage ==="
cat test/record-utils.test.ts
echo "=== config-save test full ==="
cat test/config-save.test.ts | head -120
echo "=== named-backup-export inline removeWithRetry ==="
sed -n '1,30p' test/named-backup-export.test.tsLength of output: 11036 🏁 Script executed: #!/bin/bash
echo "=== config.ts - savePluginConfig and lock implementation ==="
rg -n "savePluginConfig\|lockfile\|lock_\|\.lock\|exclusive\|O_EXCL\|wx\b\|pid\b\|expiry\|stale\|takeover\|cross.process\|crossProcess" lib/config.ts | head -60
echo "=== config.ts lines around savePluginConfig ==="
grep -n "savePluginConfig\|async function\|export" lib/config.ts | head -40
echo "=== runtime-rotation-proxy.ts - search different terms ==="
rg -n "Mutex\|mutex\|routing\|Routing\|held\|reentr\|selection\|cursor\|markSwitch\|withLock\|AsyncLocal" lib/runtime-rotation-proxy.ts | head -60
echo "=== config-save test - full file lines 120-250 ==="
sed -n '120,250p' test/config-save.test.ts
echo "=== config.ts around line 1000-1100 (likely savePluginConfig location) ==="
grep -n "savePlugin\|withLock\|lockFile\|lockPath\|wx\|EXCL\|pid\|stale\b" lib/config.ts | head -40
echo "=== runtime-rotation-proxy.ts lines - find routingMutex pattern ==="
grep -n "routing\|Routing\|mutex\|Mutex\|storageLock\|AsyncLocal\|markSwitch" lib/runtime-rotation-proxy.ts | head -50Length of output: 12991
main hot path: selection + cursor commit now runs inside one residual gap still documented:
one hygiene note: ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/config.ts (1)
495-518:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftthe cas still cannot protect the stat→rename gap from non-participating writers.
writeJsonFileAtomicWithRetry()only comparesmtimeMsbeforefs.rename()inlib/config.ts:498-510. a direct writer that ignores the.lockfile can still updatefilePathafter that stat and before the rename, andwithConfigFileLock()inlib/config.ts:617-675only serializes writers that also honor the lock. the new regression intest/config-save.test.ts:230-293forcesfs.rename()to throwESTALE, but a normal external write will not surface that signal, so this still leaves a lost-update window while the test gives false confidence. either narrow the contract/comments to participating writers only or add a real post-check / coordination scheme for out-of-band writers. as per coding guidelines,lib/**: 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.Also applies to: 617-675, 887-927
🤖 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/config.ts` around lines 495 - 518, The current CAS check in writeJsonFileAtomicWithRetry (uses getConfigFileMtimeMs before fs.rename) still leaves a race with non-participating writers; fix by adding a post-rename verification and clearer contract: after fs.rename succeeds in writeJsonFileAtomicWithRetry, re-read the target's mtime/content (via getConfigFileMtimeMs or reading file) and compare to the expected/temporary content — if it doesn't match, throw an ESTALE and retry the loop (or escalate after max attempts); also update withConfigFileLock comments to state it only serializes writers that honor the lock and update/extend test/config-save.test.ts to cover out-of-band writer behavior (and add handling for EBUSY/429 retry semantics where appropriate). Ensure references: writeJsonFileAtomicWithRetry, withConfigFileLock, getConfigFileMtimeMs, and test/config-save.test.ts.
🤖 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 `@lib/config.ts`:
- Around line 617-675: withConfigFileLock currently writes a static
ConfigLockPayload (expiresAt) and unlinks lockPath unconditionally, allowing
stale-takeover races; change the lock format to include a unique owner token
(e.g., uuid) in ConfigLockPayload, have withConfigFileLock write that token and
either periodically refresh the lock (heartbeat) or update isConfigLockStale to
require both age expiry and missing/invalid owner before takeover, and modify
unlinkConfigLockWithRetry to read-and-compare the current lock token before
unlinking (only unlink if tokens match) to avoid deleting another owner’s lock;
update tests referencing config save (test/config-save.test.ts) to cover
long-running saves, TTL expiry, and EBUSY/429 retry paths, and ensure no logs
print the owner token or other secrets.
In `@lib/runtime-rotation-proxy.ts`:
- Around line 1740-1788: The comment about an "L4 fix (routing mutex)" is now
stale because chooseAccount is already run inside withRoutingMutex (see
routingMutexMode check, selectAccount, withRoutingMutex, and
accountManager.markSwitchedLocked), so remove or update the old note in
lib/runtime-rotation-proxy.ts that claims selection-time cursor mutation is
unfixed; instead keep a short, accurate remark (or delete) explaining that
chooseAccount runs under the routing mutex and markSwitchedLocked re-commits the
cursor while held, and remove the misleading reference that would contradict the
serialized path asserted by the tests.
---
Duplicate comments:
In `@lib/config.ts`:
- Around line 495-518: The current CAS check in writeJsonFileAtomicWithRetry
(uses getConfigFileMtimeMs before fs.rename) still leaves a race with
non-participating writers; fix by adding a post-rename verification and clearer
contract: after fs.rename succeeds in writeJsonFileAtomicWithRetry, re-read the
target's mtime/content (via getConfigFileMtimeMs or reading file) and compare to
the expected/temporary content — if it doesn't match, throw an ESTALE and retry
the loop (or escalate after max attempts); also update withConfigFileLock
comments to state it only serializes writers that honor the lock and
update/extend test/config-save.test.ts to cover out-of-band writer behavior (and
add handling for EBUSY/429 retry semantics where appropriate). Ensure
references: writeJsonFileAtomicWithRetry, withConfigFileLock,
getConfigFileMtimeMs, and test/config-save.test.ts.
🪄 Autofix (Beta)
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
Run ID: d1cf33f2-d16e-4c3d-bebe-b5744fc17473
📒 Files selected for processing (6)
docs/releases/v2.2.1.mdlib/config.tslib/routing-mutex.tslib/runtime-rotation-proxy.tstest/config-save.test.tstest/runtime-rotation-proxy.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (12)
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 documentationDocumentation should follow the established directory structure and linkage pattern: Start Here (getting-started, index, faq, troubleshooting) → Daily Use (feature docs) → Release History (versioned release notes) → Repair (troubleshooting, upgrade) → Reference (commands, settings, storage, API, errors) → Maintainer Docs (governance, architecture, runbooks, audits) → Governance (README, CONTRIBUTING, CODE_OF_CONDUCT, SECURITY)
Files:
docs/releases/v2.2.1.md
**/*.{ts,js,md}
📄 CodeRabbit inference engine (README.md)
All command implementations must be documented in the commands reference with their flags, options, and expected output formats
Files:
docs/releases/v2.2.1.mdlib/routing-mutex.tstest/runtime-rotation-proxy.test.tslib/config.tstest/config-save.test.tslib/runtime-rotation-proxy.ts
docs/releases/*.md
📄 CodeRabbit inference engine (docs/README.md)
Release notes files should be named following semantic versioning pattern and located in the
releases/directory
Files:
docs/releases/v2.2.1.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/releases/v2.2.1.md
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/routing-mutex.tslib/config.tslib/runtime-rotation-proxy.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
lib/routing-mutex.tstest/runtime-rotation-proxy.test.tslib/config.tstest/config-save.test.tslib/runtime-rotation-proxy.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax exclusively; the project is ESM-only with
"type": "module"Always disable whole-pool replay by default when every account is rate-limited to prevent indefinite pool walking
Use a bounded outbound request budget per active request so one prompt cannot walk the full account pool indefinitely
Trigger a short cooldown on repeated cross-account 5xx bursts instead of continuing aggressive rotation
Stagger proactive refresh requests to reduce background refresh bursts
Runtime rotation must be loopback-only for the local bridge and forwarded Codex sessions to prevent external request interception
Protect local bridge endpoints (
/health,/v1/models,/v1/responses) with hashed local client tokensResponses background mode stays opt-in; only enable
backgroundResponsesin settings or viaCODEX_AUTH_BACKGROUND_RESPONSES=1for callers that intentionally sendbackground: trueUse strict Codex-oriented request/prompt compatibility with strict runtime handling and documented error contracts
Never run npm install or update commands automatically; only print a manual notice on interactive TTY or when
CODEX_MULTI_AUTH_DEBUG=1Make experimental settings non-destructive by default: sync previews before apply, preserve destination-only accounts, and fail safely on backup filename collisions
Use environment variable
CODEX_MULTI_AUTH_DIRto override the settings and accounts root directory
Files:
lib/routing-mutex.tstest/runtime-rotation-proxy.test.tslib/config.tstest/config-save.test.tslib/runtime-rotation-proxy.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/routing-mutex.tslib/config.tslib/runtime-rotation-proxy.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/runtime-rotation-proxy.test.tstest/config-save.test.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors where tests cover Windows locks
Files:
test/runtime-rotation-proxy.test.tstest/config-save.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-rotation-proxy.test.tstest/config-save.test.ts
lib/runtime-rotation-proxy.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/runtime-rotation-proxy.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responsesDo not expose account emails or tokens in runtime proxy client response headers or logs
Files:
lib/runtime-rotation-proxy.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T18:57:46.417Z
Learning: Use `codex-multi-auth-codex ...` only as an optional forwarding wrapper for official Codex CLI commands when intentionally choosing wrapper-launched sessions
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T18:57:46.417Z
Learning: Store project-scoped accounts under `~/.codex/multi-auth/projects/<project-key>/openai-codex-accounts.json` for repo-specific workflows
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T18:57:46.417Z
Learning: Credentials must stay local and never be transmitted outside the device except to official OpenAI services
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T18:58:16.166Z
Learning: Device-auth endpoint's bare 403/404 responses are intentional non-RFC-8628 signals for authorization pending (the poll already exits at the server deadline)
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T18:58:16.166Z
Learning: Runtime budgets are deliberately soft/eventually-consistent under concurrency and should not be treated as hard constraints
🔇 Additional comments (4)
docs/releases/v2.2.1.md (1)
1-3: ⚡ Quick winfix docs portal stable/prior stable links for v2.2.1
docs/README.md already maps
releases/v2.2.1.mdto “current stable release notes” andreleases/v2.2.0.mdto “prior stable release notes” (docs/README.md:35-36).lib/routing-mutex.ts (1)
29-30: LGTM!Also applies to: 145-153, 193-201
lib/runtime-rotation-proxy.ts (1)
10-10: LGTM!Also applies to: 553-575
test/runtime-rotation-proxy.test.ts (1)
11-15: LGTM!Also applies to: 292-317, 2226-2351
Real bug introduced by the round-3 config lock (#18): - the cross-process config lock fixed expiresAt once and never renewed, and released by unconditionally unlinking the lockfile. A save running longer than CONFIG_LOCK_TTL_MS could be deemed stale and stolen by another process, after which the original holder would delete the NEW owner's lock and reopen concurrent saves. Added a per-acquisition owner token (randomUUID) to the lock payload and a releaseConfigLockIfOwner() that compares-before-unlink, so a holder never deletes a lock it no longer owns. Regressions: stale-foreign-lock takeover (cleans only its own lock) + live-foreign-lock respected (times out, foreign lock untouched, no partial apply). Nits: - mcodex-launcher test: dropped the explicit vitest globals import (#17). - runtime-rotation-proxy: replaced the stale 'KNOWN GAP (L4)' comment on chooseAccount with accurate docs — the race is closed via the reentrant withRoutingMutex on the hot path (#19). Full suite: 4343 passed, 3 skipped, 0 failed; typecheck + lint clean.
|
Round 4 addressed in
The remaining items in the review are re-posts marked '✅ Addressed in commit …' (#2 generation guard, #3 ENOTEMPTY, #8/#10/#12/#13/#14/#15/#16, test nits) — all present in the branch at current line numbers. Full suite: 4343 passed, 3 skipped, 0 failed; typecheck + lint clean. @coderabbitai full review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== `#18`: owner-safe lock — releaseConfigLockIfOwner in lib/config.ts ==="
rg -n "releaseConfigLock\|ownerToken\|randomUUID\|compare-before\|owner\b" lib/config.ts -A4 -B2 | head -80
echo "=== `#18`: lock payload structure (pid/expiry/token) ==="
rg -n "owner\|token\|uuid\|UUID\|O_EXCL\|wx\b\|lockPath\|lockFile" lib/config.ts | head -40
echo "=== `#18`: regression tests for stale-foreign + live-foreign lock ==="
rg -n "foreign\|stale.*lock\|live.*lock\|takeover\|owner\|randomUUID" test/config-save.test.ts -A5 -B2 | head -100
echo "=== `#17`: mcodex test — check for vitest import ==="
rg -n "import.*vitest\|from.*vitest\|from 'vitest'\|from \"vitest\"" test/mcodex-launcher.test.ts
echo "=== `#19`: KNOWN GAP comment in runtime-rotation-proxy.ts ==="
rg -n "KNOWN GAP\|L4\|deferred\|chooseAccount\|cursor.*race\|race.*cursor" lib/runtime-rotation-proxy.ts -A3 -B2 | head -60
echo "=== removeWithRetry consolidation: check test files for inline definitions ==="
rg -n "function removeWithRetry\|const removeWithRetry\|async function removeWithRetry" test/ -A8 | head -80
echo "=== check if shared helper exists ==="
fd "remove-with-retry" test/
cat test/helpers/remove-with-retry.ts 2>/dev/null || cat test/helpers/remove-with-retry.js 2>/dev/null || echo "shared helper not found at expected path"
echo "=== config.ts releaseConfigLockIfOwner full impl ==="
ast-grep --pattern 'function releaseConfigLockIfOwner($$$) { $$$ }'
echo "=== config.ts: owner token generation and comparison ==="
sed -n '550,700p' lib/config.tsLength of output: 7684 🏁 Script executed: #!/bin/bash
echo "=== withConfigFileLock finally block ==="
sed -n '700,750p' lib/config.ts
echo "=== config-save test full content (check for owner/stale/foreign regressions) ==="
wc -l test/config-save.test.ts
grep -n "owner\|foreign\|stale\|takeover\|live.*lock\|lock.*live\|UUID\|uuid\|token" test/config-save.test.ts | head -40
echo "=== config-save test all describe/it/test names ==="
rg -n "describe\|it\(|test\(" test/config-save.test.ts | head -40
echo "=== inline removeWithRetry in test files ==="
rg -rn "async function removeWithRetry\|const removeWithRetry" test/config-save.test.ts test/storage-flagged.test.ts test/mcodex-launcher.test.ts 2>/dev/null
echo "=== config-save.test.ts lines 1-40 (imports) ==="
sed -n '1,40p' test/config-save.test.ts
echo "=== releaseConfigLockIfOwner call site in withConfigFileLock finally ==="
rg -n "releaseConfigLockIfOwner\|finally" lib/config.ts | head -20Length of output: 5804
regressions at
no
no "KNOWN GAP" or "L4" found anywhere in the file. ✅ still-open:
const RETRYABLE_REMOVE_CODES = new Set(["EBUSY", "EPERM", "ENOTEMPTY"]);the shared helper at triggering full review now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/forecast.ts (1)
34-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winkeep the new exported field backward-compatible.
ForecastAccountResultis an exported type, so makingexhaustedrequired atlib/forecast.ts:34-49is a source-breaking patch release change. any downstream ts code that constructs or mocks this type will now fail to compile even if it does not care about exhaustion. make the field optional and treatundefinedasfalseinlib/forecast.ts:395-400, or introduce a separate internal type for the enriched result shape.🤖 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/forecast.ts` around lines 34 - 49, Make the new exported field backward-compatible by changing ForecastAccountResult.exhausted to optional (exhausted?: boolean) and update places that read it (e.g., the code path around the enriched result usage in lib/forecast.ts where you check exhausted at the logic around lines ~395-400) to treat undefined as false (use a boolean coalescing check or explicit undefined -> false) so existing consumers compiling against the type won’t break; alternatively, if you prefer, create a separate internal enriched result type for internal use and leave the exported ForecastAccountResult unchanged.test/server.unit.test.ts (1)
16-16: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuedrop the dead
_lastCodemock plumbing.production no longer touches
server._lastCode(it's all closure-captured now), so the mock field attest/server.unit.test.ts:16, the type at:77, and the reset at:83are stale. leaving them suggests_lastCodeis still meaningful and invites drift for the next reader.♻️ proposed cleanup
@@ mock server const mockServer = { listen: vi.fn(), close: vi.fn(), unref: vi.fn(), on: vi.fn(), - _lastCode: undefined as string | undefined, }; @@ describe state let mockServer: ReturnType<typeof http.createServer> & { _handler?: (req: IncomingMessage, res: ServerResponse) => void; - _lastCode?: string; }; @@ beforeEach mockServer = http.createServer(() => {}) as typeof mockServer; - mockServer._lastCode = undefined;Also applies to: 77-83
🤖 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/server.unit.test.ts` at line 16, Remove the dead `_lastCode` test plumbing: delete the `_lastCode: undefined as string | undefined,` mock field declared in the test file, remove the corresponding `_lastCode` type declaration/usage (the test-local type that references `_lastCode`), and remove the reset/assignment that clears `_lastCode` in the test setup/teardown; update any tests that referenced `_lastCode` to use the closure-captured value instead and run the unit tests to ensure nothing else relies on the removed symbol.lib/storage/transactions.ts (1)
39-47: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winmake
withstoragelockitself reentrant.
lib/storage.ts:2099-2114now has a one-offisStorageLockHeld()escape hatch, butlib/storage/transactions.ts:39-47still deadlocks any future nestedwithStorageLock(...)call site that forgets that guard. short-circuiting when the current async context already owns the mutex fixes the root cause in one place.proposed fix
export function withStorageLock<T>(fn: () => Promise<T>): Promise<T> { + if (isStorageLockHeld()) { + return fn(); + } const previousMutex = storageMutex; let releaseLock: () => void = () => undefined; storageMutex = new Promise<void>((resolve) => { releaseLock = resolve; }); return previousMutex .then(() => storageLockHeldContext.run(true, fn)) .finally(() => releaseLock()); }as per coding guidelines,
lib/**: focus on auth rotation, windows filesystem io, and concurrency.🤖 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/storage/transactions.ts` around lines 39 - 47, The withStorageLock function deadlocks if called reentrantly; modify withStorageLock to short-circuit when the current async context already holds the lock by checking the existing storageLockHeldContext/isStorageLockHeld before creating a new promise, and if true simply run and return storageLockHeldContext.run(true, fn) without changing storageMutex or creating releaseLock; otherwise proceed with the existing logic that replaces storageMutex, sets releaseLock, awaits previousMutex, runs fn in the storageLockHeldContext, and finally calls releaseLock.
♻️ Duplicate comments (2)
lib/config.ts (1)
617-675:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeofflock release still lacks owner-check; stale takeover can orphan the new holder's lock.
lib/config.ts:670-673unconditionally unlinkslockPathin finally. if a save exceedsCONFIG_LOCK_TTL_MS(10s) and another process steals the lock viaisConfigLockStale, the original holder's finally will delete the new owner's lock — reopening the concurrent-save window. the prior review comment on this block requested an owner token + compare-before-unlink, which was not implemented.practical impact is low because 10s is generous for a config save, but slow/av-delayed filesystems or debugger-paused processes can hit this. a minimal mitigation: read the lock payload before unlink and only proceed if
pid === process.pid.🔒 proposed owner-safe release
async function unlinkConfigLockWithRetry(lockPath: string): Promise<void> { + // Only release if we still own the lock (pid matches). + try { + const content = await fs.readFile(lockPath, "utf-8"); + const parsed = JSON.parse(stripUtf8Bom(content)) as Partial<ConfigLockPayload> | undefined; + if (typeof parsed?.pid === "number" && parsed.pid !== process.pid) { + // Another process owns the lock now; do not unlink. + return; + } + } catch { + // Lock missing or unreadable — proceed to best-effort unlink. + } for (let attempt = 0; attempt < 5; attempt += 1) {🤖 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/config.ts` around lines 617 - 675, The finally block in withConfigFileLock currently unconditionally calls unlinkConfigLockWithRetry(lockPath) and can remove a lock stolen by another process; change it to read and parse the current lock file (the ConfigLockPayload written when acquiring the lock), and only call unlinkConfigLockWithRetry(lockPath) if the parsed payload.pid === process.pid (treat missing/unreadable file as safe/no-op). Use the existing symbols: withConfigFileLock, ConfigLockPayload, lockPath, isConfigLockStale, and unlinkConfigLockWithRetry to locate the code to modify; perform the read/JSON-parse check just before the unlink in the finally block and skip unlinking when the owner pid differs.test/mcodex-launcher.test.ts (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick windrop the explicit vitest globals import.
test/mcodex-launcher.test.ts:5importsdescribe,expect,iteven though vitest globals are enabled for test files. per coding guidelines: "Vitest globals (describe, it, expect) are enabled and should be used without explicit imports."this was flagged in past review comments (lines 5-5) with no addressed marker; still open.
proposed fix
import { dirname, join } from "node:path"; import { EventEmitter } from "node:events"; import { promises as fs } from "node:fs"; import { fileURLToPath, pathToFileURL } from "node:url"; -import { describe, expect, it, vi } from "vitest"; +import { vi } from "vitest"; import {🤖 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/mcodex-launcher.test.ts` at line 5, The test currently imports Vitest globals explicitly; remove describe, expect, and it from the import list and only import vi if you actually need the mocking utilities, or remove the import entirely if vi isn't used. Locate the import statement that currently reads something like import { describe, expect, it, vi } from "vitest" and change it to import { vi } from "vitest" (or delete the import) so the globals are used implicitly as intended.
🤖 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 `@docs/releases/v2.2.1.md`:
- Around line 21-29: The release note overstates tmux handling; update the
paragraph in docs/releases/v2.2.1.md to keep the Windows/bash fix, Node rewrite
(scripts/mcodex.js / scripts/mcodex), signal relay, and symlink canonicalization
details but remove or qualify any claim that tmux positional-args are fully
corrected—instead state that tmux/watch are invoked as argv arrays to avoid
shell interpolation where possible while noting a follow-up is required to fully
preserve tmux positional-argument fidelity.
In `@lib/routing-mutex.ts`:
- Around line 145-154: The current AsyncLocalStorage usage
(routingMutexHeldContext) stores a bare boolean so isRoutingMutexHeld() and the
withRoutingMutex -> runExclusive bypass allow sibling async tasks to inherit and
concurrently enter the critical section; change the storage to an owner-token +
depth model (e.g. { ownerId: string, depth: number }) and update
isRoutingMutexHeld() to only return true when the current store's ownerId
matches the mutex instance's owner token, increment depth on reentrant entry and
decrement on exit, and only bypass runExclusive when ownerId matches (preventing
sibling tasks from sharing the same owner token); update the withRoutingMutex
implementation (and any code that currently treats the store as a boolean) to
set a fresh unique owner token on first acquisition and propagate depth to
nested same-owner calls; finally add a vitest regression near the mentioned test
range that spawns sibling nested withRoutingMutex("enabled", ...) calls under
Promise.all from one held section to assert serialization (i.e., children must
not run concurrently).
In `@lib/storage/account-persistence.ts`:
- Around line 16-24: The current persistence copies pinnedAccountIndex and
affinityGeneration using typeof checks which allow NaN/Infinity and cause
JSON.stringify to emit null; change the guards in
lib/storage/account-persistence.ts to only copy when the values meet the
persisted numeric contract (finite, non-negative integers). Replace the typeof
checks on storage.pinnedAccountIndex and storage.affinityGeneration with
explicit runtime checks using Number.isFinite (and Number.isInteger if
appropriate) and >= 0 before assigning to cloned.pinnedAccountIndex and
cloned.affinityGeneration so only valid integers are persisted.
---
Outside diff comments:
In `@lib/forecast.ts`:
- Around line 34-49: Make the new exported field backward-compatible by changing
ForecastAccountResult.exhausted to optional (exhausted?: boolean) and update
places that read it (e.g., the code path around the enriched result usage in
lib/forecast.ts where you check exhausted at the logic around lines ~395-400) to
treat undefined as false (use a boolean coalescing check or explicit undefined
-> false) so existing consumers compiling against the type won’t break;
alternatively, if you prefer, create a separate internal enriched result type
for internal use and leave the exported ForecastAccountResult unchanged.
In `@lib/storage/transactions.ts`:
- Around line 39-47: The withStorageLock function deadlocks if called
reentrantly; modify withStorageLock to short-circuit when the current async
context already holds the lock by checking the existing
storageLockHeldContext/isStorageLockHeld before creating a new promise, and if
true simply run and return storageLockHeldContext.run(true, fn) without changing
storageMutex or creating releaseLock; otherwise proceed with the existing logic
that replaces storageMutex, sets releaseLock, awaits previousMutex, runs fn in
the storageLockHeldContext, and finally calls releaseLock.
In `@test/server.unit.test.ts`:
- Line 16: Remove the dead `_lastCode` test plumbing: delete the `_lastCode:
undefined as string | undefined,` mock field declared in the test file, remove
the corresponding `_lastCode` type declaration/usage (the test-local type that
references `_lastCode`), and remove the reset/assignment that clears `_lastCode`
in the test setup/teardown; update any tests that referenced `_lastCode` to use
the closure-captured value instead and run the unit tests to ensure nothing else
relies on the removed symbol.
---
Duplicate comments:
In `@lib/config.ts`:
- Around line 617-675: The finally block in withConfigFileLock currently
unconditionally calls unlinkConfigLockWithRetry(lockPath) and can remove a lock
stolen by another process; change it to read and parse the current lock file
(the ConfigLockPayload written when acquiring the lock), and only call
unlinkConfigLockWithRetry(lockPath) if the parsed payload.pid === process.pid
(treat missing/unreadable file as safe/no-op). Use the existing symbols:
withConfigFileLock, ConfigLockPayload, lockPath, isConfigLockStale, and
unlinkConfigLockWithRetry to locate the code to modify; perform the
read/JSON-parse check just before the unlink in the finally block and skip
unlinking when the owner pid differs.
In `@test/mcodex-launcher.test.ts`:
- Line 5: The test currently imports Vitest globals explicitly; remove describe,
expect, and it from the import list and only import vi if you actually need the
mocking utilities, or remove the import entirely if vi isn't used. Locate the
import statement that currently reads something like import { describe, expect,
it, vi } from "vitest" and change it to import { vi } from "vitest" (or delete
the import) so the globals are used implicitly as intended.
🪄 Autofix (Beta)
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
Run ID: ccb1dee6-c243-4f30-af08-73c9510e7628
📒 Files selected for processing (48)
.codex-plugin/plugin.jsonREADME.mddocs/README.mddocs/releases/v2.2.1.mdlib/auth/device-auth.tslib/auth/server.tslib/capability-policy.tslib/codex-cli/state.tslib/codex-manager.tslib/codex-manager/commands/workspace.tslib/config.tslib/forecast.tslib/local-client-tokens.tslib/model-capability-matrix.tslib/policy/runtime-policy.tslib/quota-cache.tslib/request/fetch-helpers.tslib/routing-mutex.tslib/runtime-rotation-proxy.tslib/storage.tslib/storage/account-persistence.tslib/storage/record-utils.tslib/storage/transactions.tspackage.jsonscripts/codex-bin-resolver.jsscripts/mcodexscripts/mcodex.jstest/account-persistence.test.tstest/capability-policy.test.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/codex-manager-detail-tone.test.tstest/codex-manager-workspace-command.test.tstest/config-save.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/forecast.test.tstest/issue-474-affinity-invalidation.test.tstest/local-client-tokens.test.tstest/mcodex-launcher.test.tstest/model-capability-matrix.test.tstest/quota-cache.test.tstest/record-utils.test.tstest/runtime-rotation-proxy.test.tstest/server.unit.test.tstest/storage-flagged.test.tstest/storage.test.tstest/transactions.test.ts
💤 Files with no reviewable changes (1)
- scripts/mcodex
📜 Review details
🧰 Additional context used
📓 Path-based instructions (29)
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/account-persistence.test.tstest/capability-policy.test.tstest/codex-manager-workspace-command.test.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/config-save.test.tstest/codex-manager-detail-tone.test.tstest/record-utils.test.tstest/quota-cache.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/transactions.test.tstest/storage.test.tstest/local-client-tokens.test.tstest/model-capability-matrix.test.tstest/runtime-rotation-proxy.test.tstest/storage-flagged.test.tstest/issue-474-affinity-invalidation.test.tstest/forecast.test.tstest/server.unit.test.tstest/mcodex-launcher.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
test/account-persistence.test.tslib/auth/server.tslib/storage/record-utils.tslib/policy/runtime-policy.tslib/storage/account-persistence.tstest/capability-policy.test.tslib/capability-policy.tstest/codex-manager-workspace-command.test.tslib/storage/transactions.tstest/codex-bin-wrapper.test.tslib/auth/device-auth.tslib/quota-cache.tstest/codex-cli-state.test.tslib/codex-cli/state.tstest/config-save.test.tstest/codex-manager-detail-tone.test.tslib/request/fetch-helpers.tslib/codex-manager.tstest/record-utils.test.tstest/quota-cache.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/transactions.test.tstest/storage.test.tstest/local-client-tokens.test.tslib/model-capability-matrix.tstest/model-capability-matrix.test.tslib/storage.tstest/runtime-rotation-proxy.test.tslib/forecast.tstest/storage-flagged.test.tslib/local-client-tokens.tslib/routing-mutex.tslib/codex-manager/commands/workspace.tstest/issue-474-affinity-invalidation.test.tslib/config.tstest/forecast.test.tstest/server.unit.test.tstest/mcodex-launcher.test.tslib/runtime-rotation-proxy.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax exclusively; the project is ESM-only with
"type": "module"
Files:
test/account-persistence.test.tslib/auth/server.tslib/storage/record-utils.tslib/policy/runtime-policy.tslib/storage/account-persistence.tstest/capability-policy.test.tslib/capability-policy.tstest/codex-manager-workspace-command.test.tslib/storage/transactions.tstest/codex-bin-wrapper.test.tslib/auth/device-auth.tslib/quota-cache.tstest/codex-cli-state.test.tslib/codex-cli/state.tsscripts/codex-bin-resolver.jstest/config-save.test.tstest/codex-manager-detail-tone.test.tslib/request/fetch-helpers.tslib/codex-manager.tstest/record-utils.test.tstest/quota-cache.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/transactions.test.tstest/storage.test.tstest/local-client-tokens.test.tslib/model-capability-matrix.tstest/model-capability-matrix.test.tslib/storage.tstest/runtime-rotation-proxy.test.tslib/forecast.tstest/storage-flagged.test.tslib/local-client-tokens.tslib/routing-mutex.tslib/codex-manager/commands/workspace.tsscripts/mcodex.jstest/issue-474-affinity-invalidation.test.tslib/config.tstest/forecast.test.tstest/server.unit.test.tstest/mcodex-launcher.test.tslib/runtime-rotation-proxy.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors where tests cover Windows locks
Files:
test/account-persistence.test.tstest/capability-policy.test.tstest/codex-manager-workspace-command.test.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/config-save.test.tstest/codex-manager-detail-tone.test.tstest/record-utils.test.tstest/quota-cache.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/transactions.test.tstest/storage.test.tstest/local-client-tokens.test.tstest/model-capability-matrix.test.tstest/runtime-rotation-proxy.test.tstest/storage-flagged.test.tstest/issue-474-affinity-invalidation.test.tstest/forecast.test.tstest/server.unit.test.tstest/mcodex-launcher.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (README.md)
Use the official
@openai/codexCLI as the primary Codex binary;codex-multi-authshould not take over thecodexcommandImplement three distinct global binaries:
codex-multi-auth(primary account manager),codex-multi-auth-codex(optional forwarding wrapper), andcodex-multi-auth-app-launcher(optional desktop launcher helper)Implement default-on loopback Responses proxy for live account rotation inside forwarded Codex CLI/app sessions; allow opt-out via
CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0Store credentials locally only; runtime rotation must be loopback-only with no external proxy forwarding
Implement health-aware account selection with quota forecasting, automatic failover, and flagged-account recovery mechanisms
Use bounded outbound request budgets to prevent a single prompt from exhausting the entire account pool during rotation
Disable whole-pool replay by default when every account is rate-limited; trigger short cooldown on repeated cross-account 5xx bursts instead of aggressive rotation
Implement staggered proactive refresh to reduce background refresh bursts across the account pool
Implement OAuth-based account credentials for personal development use only; document that this is not an official OpenAI product and users are responsible for policy compliance
Support optional hashed local client tokens for loopback-only local bridge access to
/health,/v1/models, and/v1/responsesendpointsImplement request-bearing forwarded Codex session affinity with live account sync, proactive refresh, and preemptive quota deferral controls
Implement strict Codex-oriented request/prompt compatibility with documented error contracts and runtime handling
Enable installed wrappers to perform optional daily npm version checks during normal forwarded Codex startup; only print manual notices on interactive TTY or when
CODEX_MULTI_AUTH_DEBUG=1; never automatically run npm install/updateSupport device authentication flow with `--device-au...
Files:
test/account-persistence.test.tslib/auth/server.tslib/storage/record-utils.tslib/policy/runtime-policy.tslib/storage/account-persistence.tstest/capability-policy.test.tslib/capability-policy.tstest/codex-manager-workspace-command.test.tslib/storage/transactions.tstest/codex-bin-wrapper.test.tslib/auth/device-auth.tslib/quota-cache.tstest/codex-cli-state.test.tslib/codex-cli/state.tsscripts/codex-bin-resolver.jstest/config-save.test.tstest/codex-manager-detail-tone.test.tslib/request/fetch-helpers.tslib/codex-manager.tstest/record-utils.test.tstest/quota-cache.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/transactions.test.tstest/storage.test.tstest/local-client-tokens.test.tslib/model-capability-matrix.tstest/model-capability-matrix.test.tslib/storage.tstest/runtime-rotation-proxy.test.tslib/forecast.tstest/storage-flagged.test.tslib/local-client-tokens.tslib/routing-mutex.tslib/codex-manager/commands/workspace.tsscripts/mcodex.jstest/issue-474-affinity-invalidation.test.tslib/config.tstest/forecast.test.tstest/server.unit.test.tstest/mcodex-launcher.test.tslib/runtime-rotation-proxy.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/account-persistence.test.tstest/capability-policy.test.tstest/codex-manager-workspace-command.test.tstest/codex-bin-wrapper.test.tstest/codex-cli-state.test.tstest/config-save.test.tstest/codex-manager-detail-tone.test.tstest/record-utils.test.tstest/quota-cache.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/transactions.test.tstest/storage.test.tstest/local-client-tokens.test.tstest/model-capability-matrix.test.tstest/runtime-rotation-proxy.test.tstest/storage-flagged.test.tstest/issue-474-affinity-invalidation.test.tstest/forecast.test.tstest/server.unit.test.tstest/mcodex-launcher.test.ts
docs/{README.md,docs/**/*.md}
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
docs/{README.md,docs/**/*.md}: Use canonical package namecodex-multi-authin all documentation
Verify internal links in documentation are valid before merge
Ensure no conflicting guidance exists between README, docs, and governance files before merge
Files:
docs/README.md
docs/{README.md,docs/getting-started.md}
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Update
README.mdanddocs/getting-started.mdfirst when runtime behavior changes
Files:
docs/README.md
docs/{README.md,docs/getting-started.md,docs/configuration.md,docs/troubleshooting.md,docs/reference/**/*.md}
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Verify every documented command is executable as written before merge
Files:
docs/README.md
docs/{docs/**/*.md,README.md}
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Verify paths in documentation match runtime modules (
lib/runtime-paths.ts,lib/storage.ts,lib/config.ts)
Files:
docs/README.md
docs/{README.md,docs/index.md,docs/README.md}
📄 CodeRabbit inference engine (docs/DOCUMENTATION.md)
Use accurate discoverability terms in public landing pages without keyword stuffing or ranking promises
Root README and docs landing pages should naturally include
Codex CLI,multi-account OAuth,account switching,health checks,runtime rotation,diagnostics, andrecoverywhen those topics are in scope
Files:
docs/README.md
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 documentationAll documentation files should follow the established hierarchy: Start Here (Getting Started, Daily-use landing page, FAQ, Troubleshooting), Daily Use (feature/configuration/architecture/privacy documentation), Release History (stable and archived release notes), Repair (troubleshooting and upgrade guides), and Reference sections (commands, settings, storage paths, public API, error contracts)
Files:
docs/README.mddocs/releases/v2.2.1.md
docs/{README.md,package.json,docs/development/GITHUB_DISCOVERABILITY.md}
📄 CodeRabbit inference engine (docs/STYLE_GUIDE.md)
Keep the repository description, package description, README lead, and
docs/development/GITHUB_DISCOVERABILITY.mdaligned
Files:
docs/README.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/README.mddocs/releases/v2.2.1.md
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/auth/server.tslib/storage/record-utils.tslib/policy/runtime-policy.tslib/storage/account-persistence.tslib/capability-policy.tslib/storage/transactions.tslib/auth/device-auth.tslib/quota-cache.tslib/codex-cli/state.tslib/request/fetch-helpers.tslib/codex-manager.tslib/model-capability-matrix.tslib/storage.tslib/forecast.tslib/local-client-tokens.tslib/routing-mutex.tslib/codex-manager/commands/workspace.tslib/config.tslib/runtime-rotation-proxy.ts
lib/auth/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Never hardcode OAuth ports; use the existing auth constants/helpers
Files:
lib/auth/server.tslib/auth/device-auth.ts
lib/auth/server.ts
📄 CodeRabbit inference engine (AGENTS.md)
OAuth callback port must remain 1455 and use canonical callback URL
http://127.0.0.1:1455/auth/callback
Files:
lib/auth/server.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/auth/server.tslib/storage/record-utils.tslib/policy/runtime-policy.tslib/storage/account-persistence.tslib/capability-policy.tslib/storage/transactions.tslib/auth/device-auth.tslib/quota-cache.tslib/codex-cli/state.tslib/request/fetch-helpers.tslib/codex-manager.tslib/model-capability-matrix.tslib/storage.tslib/forecast.tslib/local-client-tokens.tslib/routing-mutex.tslib/codex-manager/commands/workspace.tslib/config.tslib/runtime-rotation-proxy.ts
lib/storage/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/storage/**/*.ts: Worktree storage usesresolveProjectStorageIdentityRoot; never derive project pools from raw worktree paths
Never use bare recursive cleanup in Windows-sensitive paths without retry handling
Files:
lib/storage/record-utils.tslib/storage/account-persistence.tslib/storage/transactions.ts
package.json
📄 CodeRabbit inference engine (SECURITY.md)
package.json: Pinhonoto4.12.18or higher (but below4.12.0-4.12.1) to avoid GHSA-xh87-mx6m-69f3 authentication bypass vulnerability
Pinrollupto^4.59.0or higher to avoid vulnerable versions below4.59.0in Vite and Vitest transitive dependencies
Files:
package.json
**/*.json
📄 CodeRabbit inference engine (README.md)
Store configuration in
~/.codex/multi-auth/settings.jsonwith support for environment variable overrides likeCODEX_MULTI_AUTH_DIR,CODEX_MULTI_AUTH_CONFIG_PATH,CODEX_MODE, and various feature toggles
Files:
package.json
test/**/codex-bin-wrapper.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test bin wrapper lazy-loading, missing dist handling, and concurrent invocations
Files:
test/codex-bin-wrapper.test.ts
scripts/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors
Files:
scripts/codex-bin-resolver.jsscripts/mcodex.js
lib/request/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Node fetch returns decoded response bytes while preserving upstream
content-encoding; do not forward stale decoded encoding metadata to local clients
Files:
lib/request/fetch-helpers.ts
test/**/documentation.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test documentation parity including command flags, config precedence, changelog policy, and governance rules
Files:
test/documentation.test.ts
test/**/storage.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test concurrent access patterns, worktree migration, and forged pointer handling in storage tests
Files:
test/storage.test.ts
docs/releases/*.md
📄 CodeRabbit inference engine (docs/README.md)
Release notes should be organized by semantic version in individual markdown files, with stable releases (v2.x, v1.x) listed before pre-1.0 archive (v0.x) and legacy history. Current stable release should be at the top of the Release History table.
Files:
docs/releases/v2.2.1.md
lib/storage.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use case-insensitive email dedup via
normalizeEmailKey()with trim and lowercase normalization
Files:
lib/storage.ts
lib/runtime-rotation-proxy.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/runtime-rotation-proxy.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responsesDo not expose account emails or tokens in runtime proxy client response headers or logs
Files:
lib/runtime-rotation-proxy.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T19:09:40.588Z
Learning: Maintain project-scoped account storage under `~/.codex/multi-auth/projects/<project-key>/` for repo-specific workflows
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T19:09:40.588Z
Learning: Provide comprehensive documentation covering installation, getting started, features, configuration, troubleshooting, command reference, public API contract, error contracts, settings reference, storage paths, upgrade guides, privacy, and release notes
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T19:09:40.588Z
Learning: Implement a terminal dashboard with interactive hotkeys (Up/Down for navigation, Enter to select, 1-9 for quick switch, / for search, ? for help, Q to cancel) for account management and settings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T19:09:54.607Z
Learning: Documentation governance is defined by a formal contract (DOCUMENTATION.md) that governs how all documentation must be maintained, updated, and structured across the project.
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T19:09:54.607Z
Learning: All public-facing documentation should prioritize user workflows and common adoption questions, with links to architectural details, API contracts, and error semantics available in dedicated reference sections rather than embedded in daily-use guides.
🔇 Additional comments (60)
lib/codex-cli/state.ts (3)
41-45: LGTM!Also applies to: 410-417, 527-531
401-408: LGTM!
426-428: LGTM!Also applies to: 454-456, 495-496, 514-515, 524-525
lib/config.ts (6)
453-483: LGTM!
485-529: LGTM!
547-559: LGTM!Also applies to: 561-565
567-588: LGTM!
590-615: LGTM!
888-927: LGTM!test/codex-cli-state.test.ts (1)
1092-1175: LGTM!test/config-save.test.ts (3)
127-187: LGTM!
189-228: LGTM!
230-293: LGTM!lib/auth/server.ts (1)
105-127: capturedState check is purely defensive — fine, just noting.the handler already rejects any non-matching url state at
lib/auth/server.ts:56-60, socapturedStatecan only ever equal the server'sstate. that means the mismatch branch atlib/auth/server.ts:115-120only fires when a caller passes a differentexpectedStatethan the server was started with. harmless as a defensive guard, no change needed.lib/auth/device-auth.ts (1)
170-191: LGTM!test/server.unit.test.ts (1)
226-246: solid regression test for the cross-bind fix.
test/server.unit.test.ts:226-246correctly exercises isolation: under the oldserver._lastCodeapproach the shared mock object would letserverBclobberserverA's code, so this would fail returningcode-bforserverA. with closures each handler writes its own captured value. good coverage.lib/runtime-rotation-proxy.ts (1)
1128-1145: drop the stale l4 gap comment.
lib/runtime-rotation-proxy.ts:1128-1145now contradicts the enabled-path implementation atlib/runtime-rotation-proxy.ts:1740-1788and the regression coverage intest/runtime-rotation-proxy.test.ts:2226-2350. leaving it in place will send the next concurrency change in the wrong direction.lib/model-capability-matrix.ts (2)
85-90: LGTM!
100-109: LGTM!lib/capability-policy.ts (3)
76-88: LGTM!
90-104: LGTM!
106-120: LGTM!lib/request/fetch-helpers.ts (4)
69-70: LGTM!
192-200: LGTM!
219-230: LGTM!
243-257: LGTM!test/model-capability-matrix.test.ts (4)
54-87: LGTM!
89-114: LGTM!
116-137: LGTM!
139-158: LGTM!test/capability-policy.test.ts (1)
107-124: LGTM!test/fetch-helpers.test.ts (3)
825-839: LGTM!
841-851: LGTM!
1190-1210: LGTM!lib/local-client-tokens.ts (4)
30-58: LGTM!
162-206: LGTM!
231-277: LGTM!
279-306: LGTM!test/local-client-tokens.test.ts (3)
84-128: LGTM!
130-163: LGTM!
165-193: LGTM!test/account-persistence.test.ts (2)
28-44: LGTM!
46-55: LGTM!lib/quota-cache.ts (1)
238-252: LGTM!test/quota-cache.test.ts (1)
66-79: LGTM!test/record-utils.test.ts (1)
1-53: LGTM!package.json (1)
3-3: LGTM!Also applies to: 117-117, 132-132
scripts/codex-bin-resolver.js (1)
4-4: LGTM!Also applies to: 28-41, 110-115, 178-183
test/codex-bin-wrapper.test.ts (1)
4312-4355: LGTM!docs/README.md (1)
35-36: LGTM!scripts/mcodex.js (3)
1-34: LGTM!
36-103: LGTM!
315-367: LGTM!test/documentation.test.ts (1)
582-582: LGTM!test/mcodex-launcher.test.ts (4)
15-52: LGTM!
54-148: LGTM!
150-185: LGTM!
187-251: LGTM!.codex-plugin/plugin.json (1)
3-3: LGTM!README.md (1)
386-387: LGTM!
| - **mcodex is now a Node launcher (Windows-fatal fix).** `scripts/mcodex` was | ||
| `#!/usr/bin/env bash`; npm's generated `.cmd`/`.ps1` shim invoked bare `bash`, so | ||
| when the WSL stub (`System32\bash.exe`) or the WindowsApps app-execution alias | ||
| resolved before git-bash, `mcodex` died with `HCS_E_SERVICE_NOT_AVAILABLE`. The | ||
| launcher is rewritten in Node (`scripts/mcodex.js`): zero bash dependency on the | ||
| default forward path, `tmux`/`watch` invoked as argv arrays (no shell string | ||
| interpolation), and graceful degradation with the same friendly messages when | ||
| those POSIX tools are absent. The direct-run gate canonicalizes symlinks so the | ||
| launcher still runs when invoked through an npm-created symlink bin. |
There was a problem hiding this comment.
dial back the tmux claim in these release notes.
the node rewrite is the real fix here, but this paragraph reads like tmux forwarding is fully corrected too. the pr notes still carry a tmux positional-args gap as follow-up, so saying tmux is handled cleanly overstates the shipped behavior. keep the windows/bash fix, signal relay, and symlink-gate details, but avoid implying full tmux argv fidelity until that lands. as per coding guidelines, "keep README, SECURITY, and docs consistent with actual CLI flags and workflows."
🤖 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 `@docs/releases/v2.2.1.md` around lines 21 - 29, The release note overstates
tmux handling; update the paragraph in docs/releases/v2.2.1.md to keep the
Windows/bash fix, Node rewrite (scripts/mcodex.js / scripts/mcodex), signal
relay, and symlink canonicalization details but remove or qualify any claim that
tmux positional-args are fully corrected—instead state that tmux/watch are
invoked as argv arrays to avoid shell interpolation where possible while noting
a follow-up is required to fully preserve tmux positional-argument fidelity.
| const routingMutexHeldContext = new AsyncLocalStorage<true>(); | ||
|
|
||
| /** | ||
| * Reports whether the caller is already running inside a `withRoutingMutex` | ||
| * critical section (in `"enabled"` mode). Callers that may run both standalone | ||
| * and nested under a held mutex use this to avoid re-acquiring and deadlocking. | ||
| */ | ||
| export function isRoutingMutexHeld(): boolean { | ||
| return routingMutexHeldContext.getStore() === true; | ||
| } |
There was a problem hiding this comment.
the reentrancy shortcut drops mutual exclusion for sibling async work.
lib/routing-mutex.ts:145-154 stores a bare boolean in AsyncLocalStorage, and lib/routing-mutex.ts:193-200 treats that as full mutex ownership. if a caller fan-outs nested withRoutingMutex("enabled", ...) work under promise.all, every child inherits the same store, skips runExclusive(), and runs concurrently inside what should still be one serialized region. that breaks the mutex contract in lib/routing-mutex.ts:69-79 and can reopen overlapping cursor mutation. switch this to an owner-token/depth model, or otherwise limit the bypass to true same-stack reentrancy. please also add a vitest regression near test/runtime-rotation-proxy.test.ts:2235-2291 that spawns sibling nested calls from one held section.
Also applies to: 193-200
🤖 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/routing-mutex.ts` around lines 145 - 154, The current AsyncLocalStorage
usage (routingMutexHeldContext) stores a bare boolean so isRoutingMutexHeld()
and the withRoutingMutex -> runExclusive bypass allow sibling async tasks to
inherit and concurrently enter the critical section; change the storage to an
owner-token + depth model (e.g. { ownerId: string, depth: number }) and update
isRoutingMutexHeld() to only return true when the current store's ownerId
matches the mutex instance's owner token, increment depth on reentrant entry and
decrement on exit, and only bypass runExclusive when ownerId matches (preventing
sibling tasks from sharing the same owner token); update the withRoutingMutex
implementation (and any code that currently treats the store as a boolean) to
set a fresh unique owner token on first acquisition and propagate depth to
nested same-owner calls; finally add a vitest regression near the mentioned test
range that spawns sibling nested withRoutingMutex("enabled", ...) calls under
Promise.all from one held section to assert serialization (i.e., children must
not run concurrently).
| // Preserve the user's manual pin (issue #474) and affinity generation across | ||
| // the combined account+flagged transaction (incl. doctor restore). Dropping | ||
| // these erased a manual `switch <n>` pin on persistence. | ||
| if (typeof storage?.pinnedAccountIndex === "number") { | ||
| cloned.pinnedAccountIndex = storage.pinnedAccountIndex; | ||
| } | ||
| if (typeof storage?.affinityGeneration === "number") { | ||
| cloned.affinityGeneration = storage.affinityGeneration; | ||
| } |
There was a problem hiding this comment.
match the persisted numeric contract for pin and generation.
lib/storage/account-persistence.ts:19-24 accepts nan/infinity because typeof x === "number". json.stringify will emit those as null, so the combined transaction can silently clear these fields even though lib/storage.ts:1250-1283 only accepts finite non-negative integers on read.
proposed fix
- if (typeof storage?.pinnedAccountIndex === "number") {
- cloned.pinnedAccountIndex = storage.pinnedAccountIndex;
+ if (
+ typeof storage?.pinnedAccountIndex === "number" &&
+ Number.isInteger(storage.pinnedAccountIndex) &&
+ storage.pinnedAccountIndex >= 0
+ ) {
+ cloned.pinnedAccountIndex = storage.pinnedAccountIndex;
}
- if (typeof storage?.affinityGeneration === "number") {
- cloned.affinityGeneration = storage.affinityGeneration;
+ if (
+ typeof storage?.affinityGeneration === "number" &&
+ Number.isInteger(storage.affinityGeneration) &&
+ storage.affinityGeneration >= 0
+ ) {
+ cloned.affinityGeneration = storage.affinityGeneration;
}📝 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.
| // Preserve the user's manual pin (issue #474) and affinity generation across | |
| // the combined account+flagged transaction (incl. doctor restore). Dropping | |
| // these erased a manual `switch <n>` pin on persistence. | |
| if (typeof storage?.pinnedAccountIndex === "number") { | |
| cloned.pinnedAccountIndex = storage.pinnedAccountIndex; | |
| } | |
| if (typeof storage?.affinityGeneration === "number") { | |
| cloned.affinityGeneration = storage.affinityGeneration; | |
| } | |
| // Preserve the user's manual pin (issue `#474`) and affinity generation across | |
| // the combined account+flagged transaction (incl. doctor restore). Dropping | |
| // these erased a manual `switch <n>` pin on persistence. | |
| if ( | |
| typeof storage?.pinnedAccountIndex === "number" && | |
| Number.isInteger(storage.pinnedAccountIndex) && | |
| storage.pinnedAccountIndex >= 0 | |
| ) { | |
| cloned.pinnedAccountIndex = storage.pinnedAccountIndex; | |
| } | |
| if ( | |
| typeof storage?.affinityGeneration === "number" && | |
| Number.isInteger(storage.affinityGeneration) && | |
| storage.affinityGeneration >= 0 | |
| ) { | |
| cloned.affinityGeneration = storage.affinityGeneration; | |
| } |
🤖 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/storage/account-persistence.ts` around lines 16 - 24, The current
persistence copies pinnedAccountIndex and affinityGeneration using typeof checks
which allow NaN/Infinity and cause JSON.stringify to emit null; change the
guards in lib/storage/account-persistence.ts to only copy when the values meet
the persisted numeric contract (finite, non-negative integers). Replace the
typeof checks on storage.pinnedAccountIndex and storage.affinityGeneration with
explicit runtime checks using Number.isFinite (and Number.isInteger if
appropriate) and >= 0 before assigning to cloned.pinnedAccountIndex and
cloned.affinityGeneration so only valid integers are persisted.
…nchors
Deep re-audit workflow (6 subsystems, adversarial verification) surfaced a
CRITICAL I verified by hand against the wiring.
CRITICAL:
- storage: loadFlaggedAccountsState's legacy-migration save callback was wired to
the LOCKING saveFlaggedAccounts at the loadFlaggedAccounts call site, while
loadFlaggedAccounts runs inside the already-held global mutex during
withAccountAndFlaggedStorageTransaction. The non-reentrant lock would deadlock
on a reachable path (legacy blocked-accounts file present + flagged primary
absent, e.g. doctor restore). Guarded the callback exactly like its sibling
persistRecoveredBackup: saveFlaggedAccountsUnlocked when isStorageLockHeld(),
else the locking save. Added a regression that migrates inside a held
transaction and fails on hang.
MEDIUM:
- codex-manager styleAccountDetailText: the success-tone regex
/ok|working|succeeded|valid/ was unanchored, so soft-failure details with no
failed/error keyword ("token is invalid", "refresh token revoked") matched
via substrings (valid in invalid, ok in revoked/token) and rendered green.
Anchored to /\b(ok|working|succeeded|valid)\b/ on both the prefix and compact
paths; added a regression.
Deferred (documented, pre-existing, not introduced by this release; need design,
not release-blocking): the runtime-app-helper orphan-reaper cluster in
scripts/codex.js (dead-owner reaping, PID-reuse-safe liveness, wrapper-path
signal handlers), the config lock lease-renewal, and the unified default-path
cross-process save lock (narrow same-mtime-tick window behind a module cycle).
Full suite: 4345 passed, 3 skipped, 0 failed; typecheck + lint clean.
1 participant
Summary
Post-release deep audit of the v2.2.0 tree (6 parallel auditors across auth, proxy/rotation, scripts/bins, quota/policy, storage, and the codex-manager CLI, plus a live PowerShell repro). Fixes 4 HIGH and 6 MEDIUM bugs that escaped pre-release review. Every fix has a test; full suite green.
HIGH
scripts/mcodex→scripts/mcodex.js): the flagship feat: add mcodex launcher statusline #500 launcher was a bash script shipped as a Windows bin. npm's.cmd/.ps1shim invoked barebash; when the WSL stub (System32\bash.exe/ WindowsApps alias) shadowed git-bash on PATH,mcodexdied withHCS_E_SERVICE_NOT_AVAILABLE. Reproduced live in the user's PowerShell. Rewritten as a Node bin (zero bash dependency; tmux/watch built as argv arrays — no shell interpolation; graceful degrade when absent). Live-verified default-forward +--monitorin PowerShell.lib/auth/server.ts):_lastCode/_lastStatestored on the sharedhttp.Server; parallel logins could cross-bind callback state. Now per-call closures.lib/model-capability-matrix.ts): read with the sha256 account key but the store is written under the entitlement key → matrix reported every account supports every model. Now reads the entitlement key (mirrors the existingruntime-policy.tsfix).lib/storage/transactions.ts,lib/storage.ts):withStorageLock(global mutex, no reentrancy) re-acquired bypersistRecoveredBackupinside a held flagged transaction → permanent deadlock wedging all future saves (doctor restore + flagged.bakwithout primary). Now ownership-tracked via AsyncLocalStorage; recovery persists unlocked when already locked.MEDIUM
lib/codex-manager.ts): aworkingprefix rendered green whenfailedwas trapped inside the(NN%)quota segment (same precedence class as the just-shipped suffix fix). Now scans the whole detail for failure keywords.lib/codex-manager/commands/workspace.ts):parseIntsilently truncated1.9→1 /2abc→2. Now/^\d+$/guarded, matchingswitch.lib/request/fetch-helpers.ts): classifier omittedNORMALIZED_UNSUPPORTED_MODEL_PATTERN→ real entitlement block treated as transient outage. Added to both branches.lib/storage/account-persistence.ts): combined-transaction clone droppedpinnedAccountIndex/affinityGeneration→ doctor restore erased the user's manual pin. Now carried through.lib/storage.ts,lib/quota-cache.ts): secret dirs created without0o700. Nowmode: 0o700+ best-effort chmod on POSIX.lib/forecast.ts): policy-blocked/exhausted accounts survived the candidate filter and were recommended with "pick shortest wait". Now excluded; returns null + reason when none ready.LOW (tracked as follow-ups, not in this PR)
12 lower-severity items (proxy hot-path readFileSync, backpressure, local-bridge body cap, device-auth 403→pending, budget TOCTOU, etc.) documented internally for a separate pass — they need more careful proxy-path work.
Verification
codex-cli 0.135.0;--monitorprints friendly message, no WSL error)Targets a v2.2.1 patch release.
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
post-release deep audit remediation for v2.2.0, closing 4 high and 6 medium bugs across auth, storage, proxy, quota, and the mcodex launcher. all fixes are backed by new tests and the suite is green at 4303 passed.
withStorageLockandwithRoutingMutexnow carry reentrancy state viaAsyncLocalStorage, allowing recovery callbacks to persist without re-acquiring the global mutex;startLocalOAuthServermovescapturedCode/capturedStateinto per-call closures, eliminating the shared-server cross-bindresolveEntitlementAccountKey(matching the write sites) instead ofgetAccountPolicyKey; the forecast candidate filter explicitly excludesexhaustedandunavailableaccounts so quota-drained accounts are no longer recommended as "pick shortest wait" fallbacksrelaySignalsToChild) restores SIGTERM forwarding; however, the--tmux --live-accountspane and the new-session codex pane are built by spreading argv tokens as separate positionals to tmux — whether tmux joins them into a single shell-command string is version-dependent and untestedConfidence Score: 4/5
the core lib fixes (deadlock, oauth, capability matrix, forecast filter, clone) are clean and well-tested; the mcodex tmux pane construction is the one area that needs verification before the Windows/tmux happy path can be trusted
all library fixes are backed by tests and the logic is sound. the mcodex node launcher is a complete rewrite of a user-facing entry point: the default forward and --monitor paths were live-verified, but the --tmux --live-accounts pane relies on tmux consuming multiple separate argv positionals as a joined shell-command string — behavior that varies across tmux versions and is not covered by the test suite. if tmux on a user's machine only consumes the first positional, the live-accounts pane runs a bare watch and the codex pane runs a bare node repl
scripts/mcodex.js — specifically the runTmuxMode tmux split-window and new-session argv construction; all lib/* fixes are safe
Important Files Changed
Comments Outside Diff (1)
scripts/mcodex.js, line 637-655 (link)the old bash
exec codex-multi-auth-codex "$@"replaced the mcodex process, so SIGTERM sent to the launcher PID was received by codex directly. here, mcodex stays alive as a parent; node's default SIGTERM disposition exits the parent immediately without sending anything to the child, leaving codex running as an orphan. this is directly relevant to the primary use-case: when tmux kills a pane it sends SIGTERM to the foreground process (the mcodex node wrapper), so codex survives the pane close with no way to reach it.adding
process.once("SIGTERM", () => { child.kill("SIGTERM"); process.exit(143); })right after spawn would restore the bash-equivalent behaviour. the same gap exists inrunMonitorfor the watch child, though that's lower-stakes.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (7): Last reviewed commit: "fix(audit): CRITICAL legacy-flagged migr..." | Re-trigger Greptile