fix: harden wrapper exits and matrix regressions#30
Conversation
Co-authored-by: Codex <noreply@openai.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: Codex <noreply@openai.com>
📝 WalkthroughThis PR hardens wrapper exit handling and matrix test regressions through refactored scripts, Windows shell guard infrastructure, and expanded test coverage. Severity: Medium (maintenance/hardening). Risk is low per the author's assessment; all 2,325 existing tests pass locally, and 20 new regression tests were added across 3 test files covering exit code normalization, Windows shim behavior, and matrix command args. A follow-up commit explicitly addresses a reviewable concern—adding filesystem retry guards (EBUSY/EPERM/EACCES) for Windows shim file writes to align with project conventions. Key architectural changes:
Test additions verify integer exit code propagation, non-integer normalization, Windows shim precedence over PATH decoys, and .cmd fallback to shell mode. No public API changes or data-loss concerns identified. Walkthroughadds Windows-specific filesystem resilience and shell-guard infrastructure to the Codex multi-auth CLI wrapper. CLI version is hydrated into environment at startup, guards are installed before command processing, and exit codes are properly propagated through the wrapper layers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes the Windows shell guard infrastructure in flagged concerns:
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
| const exitCode = await runCodexMultiAuthCli(process.argv.slice(2)); | ||
| process.exitCode = Number.isInteger(exitCode) ? exitCode : 1; |
There was a problem hiding this comment.
fix numeric-string exit propagation in this wrapper.
lib/scripts/codex-multi-auth.js:18 treats only integer-typed numbers as valid, so "7" is downgraded to 1. this breaks wrapper exit semantics and diverges from the normalization pattern used in lib/scripts/codex.js:183.
proposed fix
-const exitCode = await runCodexMultiAuthCli(process.argv.slice(2));
-process.exitCode = Number.isInteger(exitCode) ? exitCode : 1;
+const exitCode = await runCodexMultiAuthCli(process.argv.slice(2));
+const parsedExitCode =
+ typeof exitCode === "number" && Number.isInteger(exitCode)
+ ? exitCode
+ : Number(exitCode);
+process.exitCode = Number.isInteger(parsedExitCode) ? parsedExitCode : 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const exitCode = await runCodexMultiAuthCli(process.argv.slice(2)); | |
| process.exitCode = Number.isInteger(exitCode) ? exitCode : 1; | |
| const exitCode = await runCodexMultiAuthCli(process.argv.slice(2)); | |
| const parsedExitCode = | |
| typeof exitCode === "number" && Number.isInteger(exitCode) | |
| ? exitCode | |
| : Number(exitCode); | |
| process.exitCode = Number.isInteger(parsedExitCode) ? parsedExitCode : 1; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/codex-multi-auth.js` around lines 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.
| it("installs Windows codex shell guards to survive shim takeover", () => { | ||
| if (process.platform !== "win32") { | ||
| return; | ||
| } | ||
|
|
||
| const fixtureRoot = createWrapperFixture(); | ||
| const fakeBin = createFakeCodexBin(fixtureRoot); | ||
| const shimDir = join(fixtureRoot, "shim-bin"); | ||
| mkdirSync(shimDir, { recursive: true }); | ||
| writeFileSync( | ||
| join(shimDir, "codex-multi-auth.cmd"), | ||
| "@ECHO OFF\r\nREM fixture codex-multi-auth shim\r\n", | ||
| "utf8", | ||
| ); | ||
| writeFileSync( | ||
| join(shimDir, "codex.cmd"), | ||
| '@ECHO OFF\r\necho "%dp0%\\node_modules\\@openai\\codex\\bin\\codex.js"\r\n', | ||
| "utf8", | ||
| ); | ||
| writeFileSync( | ||
| join(shimDir, "codex.ps1"), | ||
| 'Write-Output "$basedir/node_modules/@openai/codex/bin/codex.js"' + "\r\n", | ||
| "utf8", | ||
| ); | ||
|
|
||
| const result = runWrapper(fixtureRoot, ["--version"], { | ||
| CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, | ||
| CODEX_MULTI_AUTH_WINDOWS_BATCH_SHIM_GUARD: "1", | ||
| PATH: `${shimDir}${delimiter}${process.env.PATH ?? ""}`, | ||
| USERPROFILE: fixtureRoot, | ||
| HOME: fixtureRoot, | ||
| }); | ||
| expect(result.status).toBe(0); | ||
|
|
||
| const codexBatchPath = join(shimDir, "codex.bat"); | ||
| expect(readFileSync(codexBatchPath, "utf8")).toContain( | ||
| "codex-multi-auth windows shim guardian v1", | ||
| ); | ||
| const codexCmdPath = join(shimDir, "codex.cmd"); | ||
| expect(readFileSync(codexCmdPath, "utf8")).toContain( | ||
| "codex-multi-auth windows shim guardian v1", | ||
| ); | ||
| expect(readFileSync(codexCmdPath, "utf8")).toContain( | ||
| "node_modules\\codex-multi-auth\\scripts\\codex.js", | ||
| ); | ||
| const codexPs1Path = join(shimDir, "codex.ps1"); | ||
| expect(readFileSync(codexPs1Path, "utf8")).toContain( | ||
| "codex-multi-auth windows shim guardian v1", | ||
| ); | ||
| expect(readFileSync(codexPs1Path, "utf8")).toContain( | ||
| "node_modules/codex-multi-auth/scripts/codex.js", | ||
| ); | ||
| const pwshProfilePath = join( | ||
| fixtureRoot, | ||
| "Documents", | ||
| "PowerShell", | ||
| "Microsoft.PowerShell_profile.ps1", | ||
| ); | ||
| expect(readFileSync(pwshProfilePath, "utf8")).toContain( | ||
| "# >>> codex-multi-auth shell guard >>>", | ||
| ); | ||
| expect(readFileSync(pwshProfilePath, "utf8")).toContain("CodexMultiAuthShim"); | ||
| }); | ||
|
|
||
| it("prefers invocation-derived shim directory over PATH-decoy shim entries", () => { | ||
| if (process.platform !== "win32") { | ||
| return; | ||
| } | ||
|
|
||
| const fixtureRoot = mkdtempSync(join(tmpdir(), "codex-wrapper-invoke-fixture-")); | ||
| createdDirs.push(fixtureRoot); | ||
| const globalShimDir = join(fixtureRoot, "global-bin"); | ||
| const scriptDir = join(globalShimDir, "node_modules", "codex-multi-auth", "scripts"); | ||
| mkdirSync(scriptDir, { recursive: true }); | ||
| copyFileSync(join(repoRootDir, "scripts", "codex.js"), join(scriptDir, "codex.js")); | ||
| copyFileSync(join(repoRootDir, "scripts", "codex-routing.js"), join(scriptDir, "codex-routing.js")); | ||
| writeFileSync( | ||
| join(globalShimDir, "codex-multi-auth.cmd"), | ||
| "@ECHO OFF\r\nREM real shim\r\n", | ||
| "utf8", | ||
| ); | ||
| const decoyShimDir = join(fixtureRoot, "decoy-bin"); | ||
| mkdirSync(decoyShimDir, { recursive: true }); | ||
| writeFileSync( | ||
| join(decoyShimDir, "codex-multi-auth.cmd"), | ||
| "@ECHO OFF\r\nREM decoy shim\r\n", | ||
| "utf8", | ||
| ); | ||
| const fakeBin = createFakeCodexBin(fixtureRoot); | ||
| const scriptPath = join(scriptDir, "codex.js"); | ||
| const result = runWrapperScript(scriptPath, ["--version"], { | ||
| CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, | ||
| PATH: `${decoyShimDir}${delimiter}${globalShimDir}${delimiter}${process.env.PATH ?? ""}`, | ||
| USERPROFILE: fixtureRoot, | ||
| HOME: fixtureRoot, | ||
| }); | ||
| expect(result.status).toBe(0); | ||
| expect(readFileSync(join(globalShimDir, "codex.bat"), "utf8")).toContain( | ||
| "codex-multi-auth windows shim guardian v1", | ||
| ); | ||
| expect(() => readFileSync(join(decoyShimDir, "codex.bat"), "utf8")).toThrow(); | ||
| }); |
There was a problem hiding this comment.
avoid silent no-assert passes for windows-only tests.
test/codex-bin-wrapper.test.ts:189 and test/codex-bin-wrapper.test.ts:253 return early on non-windows, so these tests can pass with zero assertions. that leaves lib/scripts/codex.js:516 windows guard behavior unprotected on common runners.
suggested test-shape update
-it("installs Windows codex shell guards to survive shim takeover", () => {
- if (process.platform !== "win32") {
- return;
- }
+it.skipIf(process.platform !== "win32")("installs Windows codex shell guards to survive shim takeover", () => {
// ...
});
-it("prefers invocation-derived shim directory over PATH-decoy shim entries", () => {
- if (process.platform !== "win32") {
- return;
- }
+it.skipIf(process.platform !== "win32")("prefers invocation-derived shim directory over PATH-decoy shim entries", () => {
// ...
});also make a required windows ci lane for this suite so these regressions are always exercised.
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."
🤖 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 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.
| function isRetriableFsError(error: unknown): boolean { | ||
| if (!error || typeof error !== "object" || !("code" in error)) { | ||
| return false; | ||
| } | ||
| const { code } = error as { code?: unknown }; | ||
| return code === "EBUSY" || code === "EPERM"; | ||
| } |
There was a problem hiding this comment.
include eacces in retriable cleanup errors.
test/codex-multi-auth-bin-wrapper.test.ts:19 omits EACCES, which is a common transient windows fs lock code and can make cleanup flaky.
proposed fix
- return code === "EBUSY" || code === "EPERM";
+ return code === "EBUSY" || code === "EPERM" || code === "EACCES";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."
📝 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.
| function isRetriableFsError(error: unknown): boolean { | |
| if (!error || typeof error !== "object" || !("code" in error)) { | |
| return false; | |
| } | |
| const { code } = error as { code?: unknown }; | |
| return code === "EBUSY" || code === "EPERM"; | |
| } | |
| function isRetriableFsError(error: unknown): boolean { | |
| if (!error || typeof error !== "object" || !("code" in error)) { | |
| return false; | |
| } | |
| const { code } = error as { code?: unknown }; | |
| return code === "EBUSY" || code === "EPERM" || code === "EACCES"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-multi-auth-bin-wrapper.test.ts` around lines 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.
| it("normalizes non-integer exit codes to 1", () => { | ||
| const fixtureRoot = createWrapperFixture(); | ||
| const distLibDir = join(fixtureRoot, "dist", "lib"); | ||
| mkdirSync(distLibDir, { recursive: true }); | ||
| writeFileSync( | ||
| join(distLibDir, "codex-manager.js"), | ||
| [ | ||
| "export async function runCodexMultiAuthCli() {", | ||
| '\treturn "ok";', | ||
| "}", | ||
| ].join("\n"), | ||
| "utf8", | ||
| ); | ||
|
|
||
| const result = runWrapper(fixtureRoot, ["auth", "status"]); | ||
| expect(result.status).toBe(1); | ||
| }); |
There was a problem hiding this comment.
add a numeric-string exit regression test here.
this suite validates integer and non-integer exits, but it misses the numeric-string path that should propagate (e.g. "7"). this gap leaves the behavior in lib/scripts/codex-multi-auth.js:18 unguarded.
proposed test addition
+ it("propagates numeric-string exit codes", () => {
+ const fixtureRoot = createWrapperFixture();
+ const distLibDir = join(fixtureRoot, "dist", "lib");
+ mkdirSync(distLibDir, { recursive: true });
+ writeFileSync(
+ join(distLibDir, "codex-manager.js"),
+ [
+ "export async function runCodexMultiAuthCli() {",
+ '\treturn "7";',
+ "}",
+ ].join("\n"),
+ "utf8",
+ );
+
+ const result = runWrapper(fixtureRoot, ["auth", "status"]);
+ expect(result.status).toBe(7);
+ });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."
📝 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.
| it("normalizes non-integer exit codes to 1", () => { | |
| const fixtureRoot = createWrapperFixture(); | |
| const distLibDir = join(fixtureRoot, "dist", "lib"); | |
| mkdirSync(distLibDir, { recursive: true }); | |
| writeFileSync( | |
| join(distLibDir, "codex-manager.js"), | |
| [ | |
| "export async function runCodexMultiAuthCli() {", | |
| '\treturn "ok";', | |
| "}", | |
| ].join("\n"), | |
| "utf8", | |
| ); | |
| const result = runWrapper(fixtureRoot, ["auth", "status"]); | |
| expect(result.status).toBe(1); | |
| }); | |
| it("normalizes non-integer exit codes to 1", () => { | |
| const fixtureRoot = createWrapperFixture(); | |
| const distLibDir = join(fixtureRoot, "dist", "lib"); | |
| mkdirSync(distLibDir, { recursive: true }); | |
| writeFileSync( | |
| join(distLibDir, "codex-manager.js"), | |
| [ | |
| "export async function runCodexMultiAuthCli() {", | |
| '\treturn "ok";', | |
| "}", | |
| ].join("\n"), | |
| "utf8", | |
| ); | |
| const result = runWrapper(fixtureRoot, ["auth", "status"]); | |
| expect(result.status).toBe(1); | |
| }); | |
| it("propagates numeric-string exit codes", () => { | |
| const fixtureRoot = createWrapperFixture(); | |
| const distLibDir = join(fixtureRoot, "dist", "lib"); | |
| mkdirSync(distLibDir, { recursive: true }); | |
| writeFileSync( | |
| join(distLibDir, "codex-manager.js"), | |
| [ | |
| "export async function runCodexMultiAuthCli() {", | |
| '\treturn "7";', | |
| "}", | |
| ].join("\n"), | |
| "utf8", | |
| ); | |
| const result = runWrapper(fixtureRoot, ["auth", "status"]); | |
| expect(result.status).toBe(7); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-multi-auth-bin-wrapper.test.ts` around lines 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.
1 participant
Summary
scripts/codex.jsto use structuredmain()flow andprocess.exitCodenormalization for multi-auth pathsscripts/codex-multi-auth.jsto normalize non-integer exit codes to1while preserving integer exit propagationscripts/test-model-matrix.js.cmdfallback pathsRisk Notes
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
D:\backup\projects\projects-nobuild\repo-projects\codex-multi-auth-wt-wrapper-matrix-revieworigin/mainat46510fed4d25945f43c61226976ac59698d24a2afix/wrapper-matrix-review-followupsnote: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
hardens wrapper exit handling and adds windows filesystem concurrency guards to survive antivirus locks and npm shim takeovers.
key changes:
runWithWindowsFsRetrywith exponential backoff for EBUSY/EPERM/EACCES errors (addresses previous review comment about missing retry logic)writeFileSyncandmkdirSynccalls in retry guards per project convention (scripts/repo-hygiene.js,scripts/copy-oauth-success.js)scripts/codex.jswithmain()flow andprocess.exitCodenormalizationscripts/codex-multi-auth.jswhile preserving integer propagationportStartplumbing, aligns with current cli argswindows fs safety:
retry logic defends against antivirus file locks during shim writes. powershell profile upsert has benign toctou race (read-modify-write without lock), but acceptable since both processes write identical guard blocks. profile corruption risk is minimal (millisecond window, idempotent content).
test coverage:
test/codex-bin-wrapper.test.ts: windows shim guard installation, invocation-derived shim preference over PATH decoys, numeric-string exit code propagationtest/codex-multi-auth-bin-wrapper.test.ts: integer exit preservation, non-integer normalizationtest/test-model-matrix-script.test.ts: .cmd parser fallback, matrix args with json/git-check-skip/variant configmissing coverage:
no tests for concurrent wrapper invocations or simulated EBUSY/EPERM failures (retry logic tested implicitly via CI windows runs).
Confidence Score: 4/5
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Start([codex wrapper invoked]) --> Hydrate[hydrate CLI version env] Hydrate --> CheckPlatform{windows?} CheckPlatform -->|yes| InstallGuards[ensureWindowsShellShimGuards] CheckPlatform -->|no| ParseArgs InstallGuards --> RetryBatch[writeFileSyncWithWindowsRetry<br/>codex.bat/cmd/ps1] RetryBatch -->|EBUSY/EPERM| Backoff[exponential backoff<br/>50ms, 100ms, 200ms] Backoff --> RetryBatch RetryBatch -->|success| ProfileGuard[upsertPowerShellProfileGuard] ProfileGuard --> ParseArgs[parse args] ParseArgs --> CheckBypass{bypass or<br/>!shouldHandle<br/>MultiAuth?} CheckBypass -->|no bypass,<br/>handle multi-auth| LoadMulti[loadRunCodexMultiAuthCli] CheckBypass -->|bypass or<br/>forward| FindReal[resolveRealCodexBin] LoadMulti --> RunMulti[runCodexMultiAuthCli] RunMulti --> NormalizeMulti[normalizeExitCode] NormalizeMulti --> TopNormalize FindReal --> AutoSync[autoSyncManagerActiveSelection] AutoSync --> Forward[forwardToRealCodex] Forward --> TopNormalize[normalizeExitCode at top level] TopNormalize --> Exit([process.exitCode set])Last reviewed commit: 8ae183f
Context used:
dashboard- What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... (source)