fix: harden wrapper exits and matrix regressions#30
Conversation
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
|
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:
📝 WalkthroughWalkthroughadds windows shim guards and cli-version hydration to the wrapper, normalizes propagated exit codes, refactors model-matrix arg construction, and centralizes codex server cleanup state. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant user as User
participant wrapper as Codex Multi-Auth Wrapper
participant shim as Windows Shim Installer
participant fs as Filesystem
participant codex as Real Codex Binary
user->>wrapper: invoke cli
wrapper->>wrapper: hydrate cli version (read package.json)
wrapper->>shim: ensureWindowsShellShimGuards()
shim->>fs: write shims / update powershell profile
fs-->>shim: ack
shim-->>wrapper: shim ready
wrapper->>codex: resolve real codex executable
wrapper->>codex: forward args
codex-->>wrapper: exitCode (number|string)
wrapper->>wrapper: normalize exit code -> process.exitCode
wrapper-->>user: process exits with propagated code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes flagged concerns:
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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-multi-auth.js`:
- Around line 17-18: The wrapper currently treats only integer-typed values as
valid (using Number.isInteger on exitCode), so numeric strings like "7" are
downgraded to 1; fix this in the codex-multi-auth wrapper by coercing the
returned exitCode from runCodexMultiAuthCli into a Number first (e.g., const
numeric = Number(exitCode)) and then setting process.exitCode to numeric if
Number.isInteger(numeric) otherwise fallback to 1; update the logic around the
exitCode variable and the process.exitCode assignment to mirror the
normalization used in codex.js while referencing runCodexMultiAuthCli and the
exitCode variable.
In `@test/codex-bin-wrapper.test.ts`:
- Around line 188-289: These two Windows-only tests ("installs Windows codex
shell guards to survive shim takeover" and "prefers invocation-derived shim
directory over PATH-decoy shim entries") currently return early on non-win32 and
can pass with zero assertions; add a guard to force a failure (or explicit skip)
when not running on Windows by inserting expect.hasAssertions() at the top of
each test (before the current if (process.platform !== "win32") return;) so a
non-Windows run cannot silently succeed, or alternatively replace the
early-return with an explicit test skip (vitest.test.skip for the whole case) —
update the test bodies that call runWrapper and runWrapperScript accordingly so
the unique test names and calls to runWrapper/runWrapperScript remain covered.
In `@test/codex-multi-auth-bin-wrapper.test.ts`:
- Around line 14-20: The isRetriableFsError function currently returns true only
for "EBUSY" and "EPERM" and should also treat "EACCES" as retriable to handle
transient Windows FS locks; update the function (isRetriableFsError) to include
code === "EACCES" in the return condition (or add it to the list of retriable
codes) so cleanup tests become deterministic on Windows while keeping the same
type-guard logic and error shape check.
- Around line 91-107: Add a regression test that covers the numeric-string exit
path: create a new test in test/codex-multi-auth-bin-wrapper.test.ts that
mirrors the existing non-integer test but writes codex-manager.js to export
runCodexMultiAuthCli returning the string "7" (instead of a number or
non-integer), then call runWrapper(fixtureRoot, ["auth","status"]) and assert
result.status is 7; this ensures the behavior referenced at
lib/scripts/codex-multi-auth.js:18 correctly propagates numeric-string exits.
Ensure you use createWrapperFixture and runWrapper just like the existing tests
so it remains deterministic under vitest.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
scripts/codex-multi-auth.jsscripts/codex.jsscripts/test-model-matrix.jstest/codex-bin-wrapper.test.tstest/codex-multi-auth-bin-wrapper.test.tstest/test-model-matrix-script.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/test-model-matrix-script.test.tstest/codex-multi-auth-bin-wrapper.test.tstest/codex-bin-wrapper.test.ts
🧬 Code graph analysis (4)
scripts/codex-multi-auth.js (1)
scripts/codex.js (4)
require(13-13)require(78-78)pkg(14-14)version(15-15)
test/codex-multi-auth-bin-wrapper.test.ts (2)
scripts/codex.js (3)
code(209-209)code(222-222)attempt(218-218)lib/utils.ts (1)
sleep(65-67)
scripts/codex.js (2)
scripts/codex-multi-auth.js (3)
require(7-7)pkg(8-8)version(9-9)lib/utils.ts (1)
sleep(65-67)
test/codex-bin-wrapper.test.ts (1)
scripts/codex.js (4)
shimDir(271-271)codexBatchPath(446-446)scriptDir(86-86)scriptDir(265-265)
🔇 Additional comments (7)
scripts/codex-multi-auth.js (1)
6-15: good best-effort version hydration.the startup env hydration is safe and non-blocking in
lib/scripts/codex-multi-auth.js:6.scripts/test-model-matrix.js (2)
133-182: cleanup serialization looks solid.the queueing in
lib/scripts/test-model-matrix.js:176prevents overlapping pid cleanup and reduces race risk for concurrent runs.
247-269: good extraction for model-case arg construction.
lib/scripts/test-model-matrix.js:247cleanly centralizes args/token building and supports deterministic verification through the test hook.test/test-model-matrix-script.test.ts (1)
66-119: nice regression coverage expansion here.the new assertions in
test/test-model-matrix-script.test.ts:66andtest/test-model-matrix-script.test.ts:91validate fallback behavior and command-arg construction directly.test/codex-bin-wrapper.test.ts (1)
348-367: good regression for numeric-string exit propagation.the assertion in
test/codex-bin-wrapper.test.ts:348closes an important wrapper-exit edge case.scripts/codex.js (2)
194-243: windows fs retry hardening is well done.the retry wrapper in
lib/scripts/codex.js:213covers transient lock codes and uses bounded backoff, which is the right guardrail for shim/profile writes.
516-553: startup sequencing for guard install is strong.running guard installation before command routing in
lib/scripts/codex.js:550reduces post-update shim drift and keeps wrapper behavior stable.
Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-bin-wrapper.test.ts`:
- Around line 82-94: Two identical spawn helper functions are duplicated in this
test file; consolidate them by creating a single shared helper (e.g.,
runWrapperScript) and replace the duplicated implementation (the other spawn
helper at lines 64-80) with calls to that single function. Ensure the single
helper accepts (scriptPath: string, args: string[], extraEnv?:
NodeJS.ProcessEnv) and returns SpawnSyncReturns<string>, preserves the same
spawnSync options (encoding: "utf8", env merged from process.env and extraEnv),
and update all tests that referenced the duplicated helper to import or call
this unified runWrapperScript function so behavior stays aligned as tests
evolve.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff894183-7127-41a6-827d-c7589c129a89
📒 Files selected for processing (3)
scripts/codex-multi-auth.jstest/codex-bin-wrapper.test.tstest/codex-multi-auth-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.tstest/codex-multi-auth-bin-wrapper.test.ts
🧬 Code graph analysis (3)
test/codex-bin-wrapper.test.ts (1)
scripts/codex.js (4)
shimDir(271-271)codexBatchPath(446-446)scriptDir(86-86)scriptDir(265-265)
test/codex-multi-auth-bin-wrapper.test.ts (3)
lib/logger.ts (1)
error(389-393)scripts/codex.js (3)
code(209-209)code(222-222)attempt(218-218)lib/utils.ts (1)
sleep(65-67)
scripts/codex-multi-auth.js (1)
scripts/codex.js (4)
require(13-13)require(78-78)pkg(14-14)version(15-15)
🔇 Additional comments (4)
scripts/codex-multi-auth.js (1)
6-15: good hardening of wrapper bootstrap and exit normalization.the best-effort version hydration and numeric-string propagation are both in place and consistent with expected wrapper behavior in
lib/scripts/codex-multi-auth.js:6-20.Also applies to: 18-20
test/codex-bin-wrapper.test.ts (2)
188-281: solid windows shim regression coverage.the assertions on
.bat/.cmd/.ps1guard content and invocation-path precedence give good protection for windows-specific shim behavior intest/codex-bin-wrapper.test.ts:188-281.as per coding guidelines "test/**: 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."
340-359: good regression for numeric-string exit propagation.this closes the exit-code gap by explicitly asserting
"7"propagates as status7intest/codex-bin-wrapper.test.ts:340-359.test/codex-multi-auth-bin-wrapper.test.ts (1)
71-125: exit-behavior regression set is complete for this wrapper path.the integer, non-integer, and numeric-string cases in
test/codex-multi-auth-bin-wrapper.test.ts:71-125are targeted and deterministic.as per coding guidelines "test/**: 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."
- extract shared runNodeScript helper for wrapper test process spawning - route runWrapper and runWrapperScript through the shared helper to avoid drift Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-bin-wrapper.test.ts`:
- Around line 186-280: Add a deterministic regression test in
test/codex-bin-wrapper.test.ts that simulates a transient Windows fs write
failure (EBUSY/EPERM/EACCES) on the first attempted shim write and verifies the
wrapper's retry logic eventually creates the guard; to do this, use vitest's
vi.spyOn or vi.stub to intercept fs.writeFileSync (or the wrapper-internal write
helper) used by runWrapper/runWrapperScript and make it throw a single error
with code "EBUSY" for the first matching path (e.g., the
codex.bat/codex.cmd/codex.ps1 target) then behave normally, invoke runWrapper or
runWrapperScript with the same environment variables as the existing Windows
tests (CODEX_MULTI_AUTH_WINDOWS_BATCH_SHIM_GUARD etc.), and assert result.status
=== 0 plus the same expect(readFileSync(...)).toContain("codex-multi-auth
windows shim guardian v1") checks; ensure the spy is restored/cleared after the
test so other tests remain deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 774186a4-31a2-42f5-8d9e-85e08b896af5
📒 Files selected for processing (1)
test/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
🧬 Code graph analysis (1)
test/codex-bin-wrapper.test.ts (1)
scripts/codex.js (4)
shimDir(271-271)codexBatchPath(446-446)scriptDir(86-86)scriptDir(265-265)
🔇 Additional comments (2)
test/codex-bin-wrapper.test.ts (2)
69-92: good helper consolidation for spawn behavior.test/codex-bin-wrapper.test.ts:69-92 cleanly centralizes process spawning, so env merge and encoding stay consistent across wrapper tests.
338-357: good regression for numeric-string exit propagation.test/codex-bin-wrapper.test.ts:338-357 directly validates
"7"exit propagation through the wrapper and closes the matrix exit normalization gap.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- prevent extra leading separators when rewriting existing PowerShell guard blocks - add deterministic Windows retry regression for transient shim write failure - add repeated-startup idempotency regression for PowerShell profile guard content Co-authored-by: Codex <noreply@openai.com>
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 292-336: The two functions buildWindowsBatchShimContent and
buildWindowsCmdShimContent are identical; remove the duplicate by creating a
single shared builder (e.g., buildWindowsShimContent) that uses
WINDOWS_SHIM_MARKER and returns the joined CRLF string, then update any callers
or exports that referenced buildWindowsBatchShimContent and
buildWindowsCmdShimContent to call the single buildWindowsShimContent (or
re-export it under both names if backward compatibility is needed) so the
bootstrap logic is maintained in one place.
In `@test/codex-bin-wrapper.test.ts`:
- Around line 460-479: Add a second deterministic test alongside the existing
numeric-string case that verifies clean-exit propagation for the explicit
numeric-string "0": in test/codex-bin-wrapper.test.ts create a fixture similar
to the existing it("propagates numeric-string multi-auth exit codes") block but
have runCodexMultiAuthCli return "0" and then call runWrapper(fixtureRoot,
["auth","status"], { CODEX_MULTI_AUTH_BYPASS: "",
CODEX_MULTI_AUTH_REAL_CODEX_BIN: "" }) and assert expect(result.status).toBe(0);
this pins the zero-exit path and prevents normalization regressions while using
the same helper symbols (createWrapperFixture, runCodexMultiAuthCli,
runWrapper).
- Around line 247-316: The test injects only EBUSY but the retry logic in
scripts/codex.js also treats EPERM and EACCES as retriable; update the test
"retries transient shim write failures and still installs guards" to
parameterize over the three error codes (EBUSY, EPERM, EACCES). For each code,
create the preload hook (hookPath) that sets error.code to the current code, set
NODE_OPTIONS to require that hook, call runWrapper(fixtureRoot, ["--version"],
{...}) and assert status 0 and that codex.bat/cmd/ps1 contain the guardian
string; reuse existing helpers createWrapperFixture, createFakeCodexBin and
shimDir creation, and use test.each or a small loop so the same expectations are
executed for each error code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e36ce10-f5d3-4483-84ca-1d515b1a7e1c
📒 Files selected for processing (2)
scripts/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
🧬 Code graph analysis (2)
scripts/codex.js (2)
scripts/codex-multi-auth.js (1)
require(7-7)lib/utils.ts (1)
sleep(65-67)
test/codex-bin-wrapper.test.ts (1)
scripts/codex.js (4)
shimDir(271-271)codexBatchPath(446-446)scriptDir(86-86)scriptDir(265-265)
- treat only integer numbers or non-empty numeric strings as valid exit codes - prevent null/empty-string coercion to success via Number() - add regressions for null and empty-string exit code normalization Co-authored-by: Codex <noreply@openai.com>
Deduplicate identical Windows cmd/batch shim content builders, expand transient shim-write retry regression coverage to EBUSY/EPERM/EACCES, and add numeric-string "0" clean-exit coverage. Co-authored-by: Codex <noreply@openai.com>
Prevent codex-multi-auth wrapper from accepting hex/octal-like string exit codes via Number coercion. Parse only signed decimal digit strings and keep non-decimal strings fail-closed. Add regressions for 0xff and 0o7 inputs. Co-authored-by: Codex <noreply@openai.com>
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)
scripts/codex.js (1)
183-192:⚠️ Potential issue | 🟡 Minor
scripts/codex.js:183should restrict exit code parsing to decimal integers, matchingscripts/codex-multi-auth.js:17.
normalizeExitCodeacceptsNumber("0xff")= 255, butparseCliExitCoderejects"0xff"via regex/^[+-]?\d+$/. this violates the stated intent to "restrict wrapper string exit codes to decimal integers". test/codex-bin-wrapper.test.ts also lacks the regression test case present in test/codex-multi-auth-bin-wrapper.test.ts:127 that validates hex/octal strings are treated as failures.alignment required
function normalizeExitCode(value) { if (typeof value === "number" && Number.isInteger(value)) { return value; } - const parsed = Number(value); - if (Number.isInteger(parsed)) { - return parsed; + if (typeof value === "string") { + const trimmed = value.trim(); + if (/^[+-]?\d+$/.test(trimmed)) { + const parsed = Number.parseInt(trimmed, 10); + if (Number.isInteger(parsed)) { + return parsed; + } + } } return 1; }add test case to test/codex-bin-wrapper.test.ts matching test/codex-multi-auth-bin-wrapper.test.ts:127 for "0xff" and "0o7" inputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/codex.js` around lines 183 - 192, normalizeExitCode currently coerces non-decimal numeric strings (e.g., "0xff") via Number(...) which allows hex/octal; change normalizeExitCode to only accept decimal integer strings by first checking the string against the same regex used by parseCliExitCode (/^[+-]?\d+$/) or by using a decimal-only parse path, and only then converting to Number; update logic in function normalizeExitCode to mirror parseCliExitCode behavior so "0xff" and "0o7" are rejected and fall through to returning 1. Also add a regression test to test/codex-bin-wrapper.test.ts (matching the case in test/codex-multi-auth-bin-wrapper.test.ts:127) verifying that "0xff" and "0o7" inputs are treated as failures.
🤖 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 213-231: The function runWithWindowsFsRetry contains an
unreachable return (return undefined) after the for-loop because any failure on
the final attempt throws from the catch, so remove the trailing "return
undefined" to clean up dead code; locate the runWithWindowsFsRetry function and
delete the final return statement (no other logic changes needed).
In `@test/codex-multi-auth-bin-wrapper.test.ts`:
- Around line 71-181: Add two regression tests to the codex-multi-auth bin
wrapper suite: one where runCodexMultiAuthCli returns a whitespace-only string
(" ") and assert runWrapper(...).status === 1, and one where it returns the
numeric-string "0" and assert runWrapper(...).status === 0; mirror the existing
test pattern (use createWrapperFixture(), mkdirSync(join(fixtureRoot, "dist",
"lib"), { recursive: true }), writeFileSync(join(distLibDir,
"codex-manager.js"), ...) exporting async function runCodexMultiAuthCli() that
returns the desired value, then call runWrapper(fixtureRoot, ["auth","status"])
and add the two expects to the describe block.
---
Outside diff comments:
In `@scripts/codex.js`:
- Around line 183-192: normalizeExitCode currently coerces non-decimal numeric
strings (e.g., "0xff") via Number(...) which allows hex/octal; change
normalizeExitCode to only accept decimal integer strings by first checking the
string against the same regex used by parseCliExitCode (/^[+-]?\d+$/) or by
using a decimal-only parse path, and only then converting to Number; update
logic in function normalizeExitCode to mirror parseCliExitCode behavior so
"0xff" and "0o7" are rejected and fall through to returning 1. Also add a
regression test to test/codex-bin-wrapper.test.ts (matching the case in
test/codex-multi-auth-bin-wrapper.test.ts:127) verifying that "0xff" and "0o7"
inputs are treated as failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 065f8f65-0243-4167-9083-852d83007e81
📒 Files selected for processing (4)
scripts/codex-multi-auth.jsscripts/codex.jstest/codex-bin-wrapper.test.tstest/codex-multi-auth-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-multi-auth-bin-wrapper.test.tstest/codex-bin-wrapper.test.ts
🧬 Code graph analysis (4)
scripts/codex.js (2)
scripts/codex-multi-auth.js (3)
require(7-7)pkg(8-8)version(9-9)lib/utils.ts (1)
sleep(65-67)
scripts/codex-multi-auth.js (1)
scripts/codex.js (8)
require(13-13)require(78-78)pkg(14-14)version(15-15)parsed(187-187)exitCode(551-551)exitCode(578-578)runCodexMultiAuthCli(547-547)
test/codex-multi-auth-bin-wrapper.test.ts (3)
lib/logger.ts (1)
error(389-393)scripts/codex.js (2)
code(209-209)code(222-222)lib/utils.ts (1)
sleep(65-67)
test/codex-bin-wrapper.test.ts (1)
scripts/codex.js (4)
shimDir(271-271)codexBatchPath(431-431)scriptDir(86-86)scriptDir(265-265)
🔇 Additional comments (19)
scripts/codex-multi-auth.js (3)
6-15: version hydration looks good.mirrors the pattern in
scripts/codex.js:11-22with best-effort error suppression. the defensive try/catch ensures startup isn't blocked if package.json is unreadable.
17-31: parseCliExitCode correctly restricts to decimal integers.the
/^[+-]?\d+$/regex rejects hex/octal-like strings ("0xff", "0o7") per commit message, andNumber.parseInt(trimmed, 10)ensures decimal parsing. the fallback to1on invalid input implements fail-closed semantics.
33-34: exit code propagation correctly normalized.the past review comment about
"7"being downgraded to1is addressed -parseCliExitCodenow handles numeric strings properly.test/codex-bin-wrapper.test.ts (6)
64-92: spawn helper consolidation looks correct.
runNodeScriptis now the single source for spawn/env wiring, with bothrunWrapperandrunWrapperScriptdelegating cleanly. this addresses the past nitpick about avoiding drift.
186-244: windows shim guard test covers the critical paths.
it.skipIfprevents silent no-assert passes on non-win32. assertions verify all three shim files (codex.bat,codex.cmd,codex.ps1) and the powershell profile guard. deterministic fixture isolation with cleanup inafterEach.
246-318: retry regression test correctly parameterizes all retriable codes.the loop over
["EBUSY", "EPERM", "EACCES"]exercises each code path inscripts/codex.js:197(RETRYABLE_WINDOWS_FS_CODES). the preload hook withsyncBuiltinESMExports()ensures the esm binding sees the injected fault.
320-368: profile guard idempotency test is solid.the double invocation with content comparison at
test/codex-bin-wrapper.test.ts:362locks the idempotency behavior. thestartsWith("\r\n") === falsecheck at line 363 catches the separator regression noted in pr objectives.
370-403: shim directory precedence test correctly validates invocation-derived path.directly exercises
resolveWindowsShimDirectoryFromInvocationinscripts/codex.js:261-276. the assertion that decoy dir has nocodex.bat(line 402) confirms precedence logic.
462-502: numeric-string exit code regressions are complete.covers both
"7"(non-zero propagation) and"0"(clean-exit propagation). these lock the behavior ofparseCliExitCodeinscripts/codex-multi-auth.js:17-31.scripts/codex.js (6)
11-22: version hydration pattern is consistent with codex-multi-auth.js.both wrappers now propagate
CODEX_MULTI_AUTH_CLI_VERSIONfrom package.json at startup. the try/catch ensures startup isn't blocked on read failures.
199-203: local sleep helper duplicates lib/utils.ts:64-66.minor duplication, but acceptable here to avoid adding an import dependency in the bin wrapper script. no action needed.
292-321: shim builder consolidation correctly deduplicates batch/cmd content.
buildWindowsCmdLikeShimContentis the shared base, and bothbuildWindowsBatchShimContentandbuildWindowsCmdShimContentdelegate to it. this addresses the past review nitpick.
355-410: ensureWindowsShellShim handles idempotency and stock shim detection correctly.the marker check at line 368 prevents unnecessary rewrites. stock openai shim detection at lines 382-395 allows takeover without
overwriteCustomShim.
444-485: profile guard upsert handles separator idempotency correctly.the whitespace normalization at lines 460-464 and the
trimEnd()prevent accumulation of extra separators on repeated runs. this addresses the pr note about "corrected separator handling."
537-539: startup order is correct: hydration then guards before cli processing.
hydrateCliVersionEnv()andensureWindowsShellShimGuards()run before argument processing, ensuring version metadata and shim protection are established early.test/codex-multi-auth-bin-wrapper.test.ts (4)
14-38: retriable fs error handling includes EACCES.line 19 now includes
code === "EACCES"alongsideEBUSYandEPERM, addressing the past review about cleanup flakiness on windows.
71-125: exit code propagation tests cover the main paths.integer (line 88), non-integer (line 106), and decimal numeric-string (line 124) all have assertions. these lock the behavior of
scripts/codex-multi-auth.js:17-31.
127-145: hex/octal regression test locks decimal-only restriction.
"0xff"and"0o7"both normalize to exit 1, as expected per the/^[+-]?\d+$/regex inscripts/codex-multi-auth.js:23. addresses the greptile flag in pr objectives.
147-181: null and empty-string regressions ensure fail-closed behavior.both
null(line 162) and""(line 180) normalize to exit 1, implementing the fail-closed semantics from the pr commit message.
Restrict codex-multi-auth string exit parsing to unsigned decimal values, add regressions for signed/hex/octal/whitespace edge cases, and keep zero-string success coverage. Also ensure test fixture cleanup attempts all directories even when one removal fails, and remove unreachable retry helper dead code. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
|
Merged via integration PR to safely combine multiple approved changes. |
1 participant
Summary - update
scripts/codex.jsto use structuredmain()flow andprocess.exitCodenormalization for multi-auth paths - updatescripts/codex-multi-auth.jsto normalize non-integer exit codes to1while preserving integer exit propagation - remove stale matrix runner plumbing and align matrix execution with current CLI args inscripts/test-model-matrix.js- expand regression coverage for wrapper exit behavior, Windows shim guard stability, matrix command args, and.cmdfallback paths ## Risk Notes - low runtime risk: wrapper change is limited to exit handling and startup guard behavior - matrix harness changes are scoped to script utilities and tests ## Validation Evidence -npm testfromD:\backup\projects\projects-nobuild\repo-projects\codex-multi-authpassed (100files,2325tests) -npx vitest run test/test-model-matrix-script.test.ts test/codex-bin-wrapper.test.ts test/codex-multi-auth-bin-wrapper.test.tspassed (3files,20tests) ## Worktree - worktree path:D:\backup\projects\projects-nobuild\repo-projects\codex-multi-auth-wt-wrapper-matrix-review- base:origin/mainat46510fed4d25945f43c61226976ac59698d24a2a- branch:fix/wrapper-matrix-review-followups## PR Resolution Update (2026-03-05, follow-up) ### What changed - Deduplicated wrapper test process-spawn helpers intest/codex-bin-wrapper.test.tsby introducing sharedrunNodeScript(...)and routingrunWrapper(...)andrunWrapperScript(...)through it. ### How to test -npm test -- test/codex-bin-wrapper.test.ts-npm run typecheck-npm run lint### Risk / rollout notes - Test-only refactor; no runtime behavior change. ## PR Resolution Update (2026-03-05, follow-up 2) ### What changed - Fixed PowerShell profile guard idempotency inscripts/codex.jsby avoiding unconditional leading/trailing separators when replacing an existing guard block. - Added deterministic retry-path coverage intest/codex-bin-wrapper.test.tsfor transient shim write failure (EBUSY) using a preload hook +syncBuiltinESMExports(). - Added repeated-startup idempotency regression intest/codex-bin-wrapper.test.tsto ensure profile content is stable on subsequent runs. ### How to test -npm test -- test/codex-bin-wrapper.test.ts-npm run typecheck-npm run lint### Risk / rollout notes - Runtime change is scoped to Windows PowerShell profile guard rewrite formatting/idempotency. - Main risk is profile-content normalization edge cases; covered by repeated-startup regression.PR Resolution Update (2026-03-05, follow-up 3)
What changed
scripts/codex-multi-auth.jsexit normalization to fail closed fornull/ empty-string returns fromrunCodexMultiAuthCli.test/codex-multi-auth-bin-wrapper.test.tsfor null/empty-string exit paths.How to test
npm test -- test/codex-multi-auth-bin-wrapper.test.ts test/codex-bin-wrapper.test.tsnpm run typechecknpm run lintRisk / rollout notes
Follow-up Review Resolution (2026-03-05)
How to test
Risk / rollout 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 hardens wrapper exit normalization, adds windows shim/powershell profile guard installation with retry-backed writes, and expands regression coverage across three test files. the runtime changes are low-risk and well-scoped; the main gaps are in
test/codex-bin-wrapper.test.tscleanup hygiene.scripts/codex-multi-auth.js:parseCliExitCodereplaces the old single-line guard; regex/^\d+$/correctly rejects signed, hex, and octal strings — all prior review iterations fully addressedscripts/codex.js: windows shim install logic usesrunWithWindowsFsRetry(EBUSY/EPERM/EACCES, exponential backoff) for all file writes;upsertPowerShellProfileGuardidempotency fix prevents redundant profile rewrites on second startup;buildWindowsBatchShimContent/buildWindowsCmdShimContentconsolidated behind shared helperscripts/test-model-matrix.js:buildModelCaseArgsextraction andportStartremoval are clean; no behavioral changetest/codex-multi-auth-bin-wrapper.test.ts: new file with correct per-dir try-catch inafterEachandEACCES-aware retry — the right patterntest/codex-bin-wrapper.test.ts: two cleanup issues relative to the companion file:afterEachlacks per-dir try-catch (first failure leaks remaining dirs) andisRetriableFsErroromitsEACCES, inconsistent withscripts/codex.jsand the companion test file added in this same PRConfidence Score: 4/5
test/codex-bin-wrapper.test.tsare the only remaining gapsafterEachmissing try-catch andisRetriableFsErrormissingEACCES) are test-only and can cause leaked temp dirs / flaky cleanup on windows, but do not affect production behaviorafterEachcleanup andisRetriableFsErrorneed alignment withtest/codex-multi-auth-bin-wrapper.test.tsImportant Files Changed
parseCliExitCode; regex/^\d+$/correctly rejects signed, hex, and octal strings, addressing all prior review iterationsbuildWindowsCmdLikeShimContenthelper; async chain is correct and all writes are routed through retry-backed helpersrunQuiet, extractsbuildModelCaseArgs/__buildModelCaseArgsForTests, and removes unusedportStart; clean refactor with no behavioral changeafterEachlacks per-dir try-catch (leaks dirs on first failure) andisRetriableFsErroromitsEACCES, inconsistent with the companion file added in the same PRafterEachwith per-dir try-catch andEACCES-aware retry; comprehensive exit-code coverage including null, empty, signed, hex/octal, and whitespace-only cases.cmdfallback test and matrix arg shape test against the new__buildModelCaseArgsForTestsexport; straightforward regression coverageFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[codex.js main] --> B[hydrateCliVersionEnv] B --> C{win32?} C -->|no| E[normalizeAuthAlias / route args] C -->|yes| D[ensureWindowsShellShimGuards] D --> D1{CODEX_MULTI_AUTH_WINDOWS_BATCH_SHIM_GUARD != 0?} D1 -->|no| E D1 -->|yes| D2[resolveWindowsShimDirectoryFromPath] D2 --> D3[ensureWindowsShellShim codex.bat / .cmd / .ps1] D3 --> D4[runWithWindowsFsRetry\nEBUSY/EPERM/EACCES\nexponential backoff] D4 --> D5[ensurePowerShellProfileGuard] D5 --> D6[upsertPowerShellProfileGuard\nidempotent - skips if content unchanged] D6 --> E E --> F{shouldHandleMultiAuthAuth?} F -->|yes| G[runCodexMultiAuthCli] G --> H[parseCliExitCode\nregex /^\\d+$/ - rejects signed/hex/octal] H --> I[process.exitCode] F -->|no| J[spawn real codex bin] J --> ILast reviewed commit: cee89c7
Comments Outside Diff (2)
test/codex-bin-wrapper.test.ts, line 154-158 (link)afterEachleaks dirs if first removal throwsthe loop calls
removeDirectoryWithRetrywithout a try-catch, so a single failure (after exhausting retries) throws, exits the loop, and abandons every remaining dir increatedDirs. on windows this leaves temp dirs with open handles that poison subsequent test runs.codex-multi-auth-bin-wrapper.test.ts(the companion file added in this same PR) uses the correct pattern: wrap each removal in try-catch, accumulate the first error, rethrow after all dirs are attempted.test/codex-bin-wrapper.test.ts, line undefined (link)isRetriableFsErrormissingEACCESthis helper only retries on
EBUSYandEPERM, butscripts/codex.jsretries on all three codes (EBUSY,EPERM,EACCES) and the companioncodex-multi-auth-bin-wrapper.test.ts(added in this PR) also handlesEACCES. on windows, antivirus or acl-based locks emitEACCESon directory deletion — missing the case means cleanup silently fails after one attempt instead of backing off.Additional Follow-up Resolution (2026-03-05)
How to test
Risk / rollout notes