fix(wrapper): support codex-multi-auth version flags#140
fix(wrapper): support codex-multi-auth version flags#140
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. |
📝 Walkthroughwalkthroughthe pr adds explicit changes
sequence diagram(s)(skipped) estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes review flags
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.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@scripts/codex-multi-auth.js`:
- Around line 36-40: The stray line setting process.exitCode is incorrectly
unindented outside the else block; move the statement that assigns
process.exitCode (the ternary using Number.isInteger(exitCode) ? exitCode : 1)
so it is inside the else block immediately after the await
runCodexMultiAuthCli(args) call (near the runCodexMultiAuthCli symbol), matching
the indentation of the surrounding block.
🪄 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: 2ece97a8-f70a-42d9-8ecd-78b8ae1964ec
📒 Files selected for processing (8)
README.mddocs/README.mddocs/getting-started.mddocs/reference/commands.mddocs/troubleshooting.mddocs/upgrade.mdscripts/codex-multi-auth.jstest/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 (2)
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/upgrade.mddocs/getting-started.mddocs/troubleshooting.mddocs/reference/commands.mddocs/README.md
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.ts
🔇 Additional comments (13)
docs/getting-started.md (1)
29-37: lgtm — clear distinction between the two version surfaces.the verification step now correctly documents both
codex --version(official cli) andcodex-multi-auth --version(wrapper package), matching the implementation inscripts/codex-multi-auth.js:28-35.docs/upgrade.md (1)
35-41: lgtm — migration checklist now covers both version surfaces.consistent with
docs/getting-started.mdanddocs/troubleshooting.mdchanges.docs/reference/commands.md (1)
72-73: lgtm — command reference accurately documents version flag behavior.correctly documents both
--versionand-vfor the wrapper, matchingscripts/codex-multi-auth.js:5whereversionFlags = new Set(["--version", "-v"]).docs/troubleshooting.md (2)
30-34: lgtm — troubleshooting verification now includes wrapper version check.helps users diagnose routing issues by confirming both cli surfaces are functional.
122-122: lgtm — issue report checklist updated.including
codex-multi-auth --versionoutput in bug reports will help maintainers quickly identify version mismatches.README.md (2)
65-71: lgtm — readme verification section accurately describes both version surfaces.consistent with
docs/getting-started.mdanddocs/reference/commands.md.
297-299: release note links match docs/readme.md updates.same v1.2.0 → v1.1.10 shift applied here.
test/codex-multi-auth-bin-wrapper.test.ts (3)
45-49: fixture correctly replicates the expected directory structure.
package.jsonatfixtureRoot/and script atfixtureRoot/scripts/codex-multi-auth.jsmeanscreateRequire(import.meta.url).require("../package.json")resolves correctly inscripts/codex-multi-auth.js:10.
77-93: lgtm — tests for --version and -v flags are deterministic and cover both short/long forms.assertions check exit code, stdout content, and empty stderr. good coverage.
95-108: lgtm — error path test confirms user-facing error message and exit code 1.this covers the case where
package.jsonhas noversionfield, matchingscripts/codex-multi-auth.js:32-34.scripts/codex-multi-auth.js (2)
28-35: version flag handling only matches single-arg invocations — intentional?
args.length === 1 && versionFlags.has(args[0])meanscodex-multi-auth --version auth statuswould pass through to the runtime rather than printing the version. this is probably fine (matches typical cli behavior), but worth confirming it's intended.
7-19: resolveCliVersion() is solid — best-effort with proper fallback.catches require errors and validates
pkg.versionis a non-empty string. the empty catch block is appropriate for "best effort only" semantics.docs/README.md (1)
29-31: release note links are validall referenced release files exist:
docs/releases/v1.1.10.md,docs/releases/v0.1.9.md,docs/releases/v0.1.8.md.
1 participant
Summary
Make the standalone
codex-multi-authwrapper answer--versionand-vdirectly, without changing the existingcodex --versionpassthrough behavior.What Changed
scripts/codex-multi-auth.jsso wrapper version flags resolve frompackage.jsonbefore loading the runtimerunCodexMultiAuthCli(...)path--version,-v, and the missing-version error pathdocs/releases/v1.2.0.mdso documentation validation passes cleanlyValidation
npm test -- test/codex-multi-auth-bin-wrapper.test.ts test/codex-bin-wrapper.test.tsnpm test -- test/documentation.test.tsnpm run lintnpm run typechecknpm testnpm run buildnpm run clean:repo:checkDocs and Governance Checklist
codex auth ...as the documented command standardRisk and Rollback
960ca3eAdditional Notes
codex --versionis intentionally unchanged and still forwards to the official@openai/codexCLIcodex-multi-auth --versionandcodex-multi-auth -vnow report the installed wrapper package versionnote: 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 adds a clean short-circuit to
scripts/codex-multi-auth.jssocodex-multi-auth --versionandcodex-multi-auth -vresolve the wrapper package version frompackage.jsonwithout loading thedist/runtime — keepingcodex --versionunchanged as a passthrough to the official@openai/codexcli.await import("../dist/lib/codex-manager.js")is correctly placed in the else branch, so version-only invocations skip the runtime entirelyCODEX_MULTI_AUTH_CLI_VERSIONis still set eagerly before the branch, preserving existing behavior for runtime invocationsENOTEMPTYadded toisRetriableFsError— good windows filesystem safety improvement["--version", "extra"]) added in response to prior review commentcreateChildEnv()improves test isolation but stripsNODE_NO_WARNINGS; on older node targets or ci environments that set this, child stderr warnings could break thestderr.toBe("")assertions in the new version testsdocs/releases/v1.2.0.mdlinks corrected across readme and docs index — unblocks documentation validationConfidence Score: 4/5
Important Files Changed
runCodexMultiAuthClicorrectly skips runtime load for solo--version/-v,CODEX_MULTI_AUTH_CLI_VERSIONis still set eagerly for all paths, error path exits with code 1 without touching the runtime.--version,-v, missing-version error, and multi-arg passthrough;ENOTEMPTYadded to retriable codes for Windows safety;createChildEnv()improves isolation but silently stripsNODE_NO_WARNINGS, risking stderr noise on older Node targets.v1.2.0release link replaced;codex-multi-auth --versionadded to verify wiring section; accurate and consistent with the implementation.v1.2.0links replaced withv1.1.10andv0.1.8; both table entries updated consistently.codex --version(official CLI) andcodex-multi-auth --version(wrapper) clearly documented.codex-multi-auth --versionadded to both the diagnostic section and bug report attachment list.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A([codex-multi-auth invoked]) --> B[resolveCliVersion\nrequire ../package.json] B --> C{version resolved?} C -- yes --> D[set CODEX_MULTI_AUTH_CLI_VERSION] C -- no --> E[version = empty string] D --> F{args.length === 1\nAND flag in versionFlags?} E --> F F -- no\nmulti-arg or non-version cmd --> G[lazy import\nrunCodexMultiAuthCli] G --> H[await runCodexMultiAuthCli args] H --> I[exitCode = integer ? exitCode : 1] F -- yes --> J{version resolved?} J -- yes --> K[stdout: version string\nexitCode = 0] J -- no --> L[stderr: version unavailable\nexitCode = 1]Comments Outside Diff (1)
test/codex-multi-auth-bin-wrapper.test.ts, line 18-19 (link)ENOTEMPTYmissing from retriable fs error codesisRetriableFsErroronly retries onEBUSYandEPERM, omittingENOTEMPTY. on windows, antivirus or the indexer can hold a directory open long enough forrmSync({ recursive: true })to fail withENOTEMPTYon the directory itself even though node handles the tree walk internally. every other cleanup helper in this repo (e.g.repo-hygiene.test.ts) includesENOTEMPTYin the retriable set perAGENTS.md. this test's cleanup could silently leave temp dirs on a windows ci agent and pollute subsequent runs.Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: "test: harden wrapper..."