add manual login mode for headless auth flows#132
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 environment- and flag-driven non-browser oauth paths. it introduces Changes
Sequence Diagram(s)sequenceDiagram
actor user
participant cli as cli (index.ts)
participant authmgr as auth manager (lib/codex-manager.ts)
participant browser as browser helper (lib/auth/browser.ts)
participant oauthsrv as local oauth server
participant prompt as manual prompt
user->>cli: codex auth login [--manual]
cli->>authmgr: runCodexMultiAuthCli(args)
authmgr->>authmgr: parseAuthLoginArgs(args)
authmgr->>browser: isBrowserLaunchSuppressed()
browser-->>authmgr: true|false
alt prefer manual (flag or suppressed)
authmgr->>prompt: print oauth url, prompt/manual input
prompt->>user: show url / accept pasted redirect
user-->>prompt: paste redirect url
prompt-->>authmgr: callback data
else browser-first
authmgr->>oauthsrv: startLocalOAuthServer()
oauthsrv-->>authmgr: started | failed
alt server started
authmgr->>browser: openBrowserUrl(authUrl)
browser-->>authmgr: opened|suppressed
oauthsrv->>authmgr: callback received
else server failed
authmgr->>prompt: fallback to manual prompt
prompt->>user: show url / accept pasted redirect
end
end
authmgr->>authmgr: exchangeAuthorizationCode()
authmgr-->>user: login complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes notes and flagged concerns:
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/commands.md (1)
48-65:⚠️ Potential issue | 🟠 Majoradd upgrade-note and cross-doc parity for the new login flags.
docs/reference/commands.md:48-65documents new user-visible login behavior, but this change set still leaves parity updates unchecked for README/getting-started/features/upgrade docs. ship these together so users get one consistent migration path.As per coding guidelines, "docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/commands.md` around lines 48 - 65, Update the docs to add an upgrade note and ensure cross-document parity for the new login flags: add a brief "Upgrade notes" section explaining the new codex auth login flags (e.g., --manual/--no-browser behavior and the deterministic non-TTY fallback driven by CODEX_TUI/CODEX_DESKTOP/TERM_PROGRAM/ELECTRON_RUN_AS_NODE), and update README, getting-started, features, and upgrade docs to match docs/reference/commands.md (including mention of the new flags and their effects on codex auth login and non-TTY behavior); also add a line in the upgrade note that callers should be aware of any new npm scripts or migration steps introduced for auth flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/auth/browser.ts`:
- Around line 24-32: The function isBrowserLaunchSuppressed incorrectly reuses
BROWSER_DISABLED_VALUES for CODEX_AUTH_NO_BROWSER, causing values like
"0"/"false"/"off" to be treated as suppression; change the explicitNoBrowser
check in isBrowserLaunchSuppressed to only treat explicit truthy values as
suppression (e.g., "1", "true", "yes", "on") instead of using
BROWSER_DISABLED_VALUES, or introduce a dedicated TRUE-like set (e.g.,
CODEX_AUTH_NO_BROWSER_TRUE_VALUES) and compare explicitNoBrowser against that;
keep the rest of the function (browserSetting check against
BROWSER_DISABLED_VALUES) unchanged and reference isBrowserLaunchSuppressed,
CODEX_AUTH_NO_BROWSER, and BROWSER_DISABLED_VALUES when making the change.
In `@lib/codex-manager.ts`:
- Around line 1471-1476: The catch around startLocalOAuthServer currently
swallows all errors; update the catch to log the caught error (including message
and stack) at debug/warn level before setting oauthServer = null so operators
can see why the local listener failed (e.g., port in use); reference
startLocalOAuthServer and the oauthServer variable and log with the existing
logger (e.g., processLogger or similar) so downstream
UI_COPY.oauth.callbackUnavailable still occurs but the root cause is recorded.
- Around line 1129-1144: The non-tty path in promptManualCallback (when
options.allowNonTty is true) calls rl.question("") which can wait indefinitely
on closed/empty stdin; update promptManualCallback to detect non-interactive
stdin and either (a) write a short stderr hint/notice before waiting (e.g.,
using console.error or output.write) describing expected piped input format, or
(b) immediately return null if stdin is closed/empty; also add a brief note to
docs/reference/commands.md describing the required stdin format when using
allowNonTty. Ensure changes reference promptManualCallback, options.allowNonTty
and the rl.question call so reviewers can find the fix.
- Line 12: Add a unit test that simulates Windows permission failures (EACCES or
EPERM) when startLocalOAuthServer attempts to bind the port and assert the code
handles it same as other bind failures: stub/mock startLocalOAuthServer to throw
an error with code "EACCES" or "EPERM" and OS set to "win32", run the auth flow
with isBrowserLaunchSuppressed behavior tested (and with CODEX_AUTH_NO_BROWSER
variations) and verify the CLI falls back or surfaces the expected permission
error path; reference the startLocalOAuthServer function and the
isBrowserLaunchSuppressed check in your test so the failure scenario is
explicitly covered.
In `@test/browser.test.ts`:
- Around line 76-82: Add deterministic vitest cases around
isBrowserLaunchSuppressed() and openBrowserUrl() to cover
CODEX_AUTH_NO_BROWSER="0" and "false" (these should NOT suppress; expect
isBrowserLaunchSuppressed() toBe(false) and openBrowserUrl(...) to call
mockedSpawn), BROWSER="none" (should suppress; expect toBe(true) and
openBrowserUrl(...) to be false and mockedSpawn not called), and both envs set
together to assert the actual precedence used by the implementation (write two
sub-cases: CODEX_AUTH_NO_BROWSER="1" + BROWSER="none" and
CODEX_AUTH_NO_BROWSER="0" + BROWSER="none")—each test should set/clear
process.env deterministically, call isBrowserLaunchSuppressed() and
openBrowserUrl("https://example.com"), and assert mockedSpawn call expectations
against the implementation behavior.
In `@test/codex-manager-cli.test.ts`:
- Around line 55-59: Add a deterministic test that flips the browser suppression
mock to true and asserts the auto-manual OAuth path: change the vi.mock for
isBrowserLaunchSuppressed to return true, stub openBrowserUrl and
startLocalOAuthServer (or its caller) to ensure they are not invoked, simulate
the user-pasted callback flow (use the same helper used elsewhere for pasted
callback verification) and assert token acquisition and clipboard behavior
(copyTextToClipboard) without relying on listeners or local server startup;
reference isBrowserLaunchSuppressed, openBrowserUrl, startLocalOAuthServer, and
copyTextToClipboard to locate the code to modify and the helper used for
pasted-callback assertions.
- Around line 2960-3005: Save and restore real TTY flags and assert the local
OAuth server was never used: before calling runCodexMultiAuthCli, capture and
set process.stdin.isTTY = true and process.stdout.isTTY = true (restore them
after the test) so the CLI behaves as a TTY, and after running, add an assertion
that vi.mocked(startLocalOAuthServer) (from serverModule / function
startLocalOAuthServer) was not called; keep the existing checks for
openBrowserUrl and storageState.accounts. Ensure you reference
runCodexMultiAuthCli, startLocalOAuthServer, and openBrowserUrl when making
these changes.
In `@test/index.test.ts`:
- Around line 564-580: The test for manual auth flow currently forces browser
suppression via browserModule.isBrowserLaunchSuppressed but doesn't assert the
browser open path wasn't used; update the test that calls autoMethod.authorize()
(from plugin.auth.methods[0]) to also mock and spy on
browserModule.openBrowserUrl (or the exported openBrowserUrl) and add an
assertion that openBrowserUrl was not called (e.g.,
expect(spy).not.toHaveBeenCalled()) to lock in the "no-browser" guarantee for
this path. Ensure you use vi.mocked(...) or a vitest spy for openBrowserUrl to
keep the test deterministic.
---
Outside diff comments:
In `@docs/reference/commands.md`:
- Around line 48-65: Update the docs to add an upgrade note and ensure
cross-document parity for the new login flags: add a brief "Upgrade notes"
section explaining the new codex auth login flags (e.g., --manual/--no-browser
behavior and the deterministic non-TTY fallback driven by
CODEX_TUI/CODEX_DESKTOP/TERM_PROGRAM/ELECTRON_RUN_AS_NODE), and update README,
getting-started, features, and upgrade docs to match docs/reference/commands.md
(including mention of the new flags and their effects on codex auth login and
non-TTY behavior); also add a line in the upgrade note that callers should be
aware of any new npm scripts or migration steps introduced for auth flows.
🪄 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: 2e58a340-f62f-4196-9a8c-fc3b3e173b36
📒 Files selected for processing (8)
docs/reference/commands.mdindex.tslib/auth/browser.tslib/codex-manager.tslib/ui/copy.tstest/browser.test.tstest/codex-manager-cli.test.tstest/index.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 (3)
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/reference/commands.md
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/ui/copy.tslib/codex-manager.tslib/auth/browser.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/browser.test.tstest/index.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (8)
lib/codex-manager.ts (5)
314-343: parseAuthLoginArgs looks correctargument parsing handles
--manual,--no-browser, and--help/-h. returns early on unknown flags with a clear error message. the early return pattern for help ({ ok: false, message: "" }) is a bit unusual but handled correctly inrunAuthLoginat lines 4169-4175.
1465-1470: manual mode routing logic is sound
preferManualModecorrectly combines explicitoptions.manualwith environment-basedisBrowserLaunchSuppressed(). this covers both cli flag and headless detection paths. the downstream skip ofpromptOAuthSignInMode()at line 1479 is appropriate.
1514-1530: oauth server null-safety and close cleanupthe optional chaining on
oauthServer?.readyandoauthServer?.close()correctly handles the null case from server startup failure. the fallback message viaUI_COPY.oauth.callbackUnavailableprovides user feedback when local listener is unavailable.
4166-4177: runAuthLogin parses and forwards options correctlyargs are parsed via
parseAuthLoginArgs, error handling is clean, andloginOptionsis passed through torunOAuthFlowat line 4296. the empty-message check at line 4169 distinguishes help output from actual errors.Also applies to: 4296-4296
4153-4153: explicit manual: false on refresh flowpassing
{ manual: false }explicitly inhandleManageActionensures refresh operations always use interactive mode regardless of environment flags. this is the correct behavior for dashboard-initiated refreshes.lib/ui/copy.ts (1)
45-45: copy update is clear and safe.
lib/ui/copy.ts:45adds the fallback message without exposing sensitive values or changing behavior.lib/auth/browser.ts (1)
107-109: good early return before spawn.
lib/auth/browser.ts:107-109cleanly blocks launcher side effects when browser usage is suppressed.index.ts (1)
2389-2392: manual/no-browser mode gating is wired correctly.
index.ts:2389-2392now routes--manual,--no-browser, and env suppression through one decision path, which keeps headless auth behavior deterministic.
| function isReadlineClosedError(error: unknown): boolean { | ||
| if (!(error instanceof Error)) { | ||
| return false; | ||
| } | ||
| const errorCode = | ||
| typeof error === "object" && error !== null && "code" in error | ||
| ? String((error as { code?: unknown }).code) | ||
| : ""; | ||
| return errorCode === "ERR_USE_AFTER_CLOSE" || /readline was closed/i.test(error.message); |
There was a problem hiding this comment.
instanceof Error check
error instanceof Error already narrows error to Error, which is guaranteed to be a non-null object. the typeof error === "object" && error !== null in the ternary is therefore unreachable dead code — it will always evaluate to true at that point.
| function isReadlineClosedError(error: unknown): boolean { | |
| if (!(error instanceof Error)) { | |
| return false; | |
| } | |
| const errorCode = | |
| typeof error === "object" && error !== null && "code" in error | |
| ? String((error as { code?: unknown }).code) | |
| : ""; | |
| return errorCode === "ERR_USE_AFTER_CLOSE" || /readline was closed/i.test(error.message); | |
| function isReadlineClosedError(error: unknown): boolean { | |
| if (!(error instanceof Error)) { | |
| return false; | |
| } | |
| const errorCode = "code" in error ? String((error as { code?: unknown }).code) : ""; | |
| return errorCode === "ERR_USE_AFTER_CLOSE" || /readline was closed/i.test(error.message); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 1218-1226
Comment:
**Redundant type guard after `instanceof Error` check**
`error instanceof Error` already narrows `error` to `Error`, which is guaranteed to be a non-null object. the `typeof error === "object" && error !== null` in the ternary is therefore unreachable dead code — it will always evaluate to `true` at that point.
```suggestion
function isReadlineClosedError(error: unknown): boolean {
if (!(error instanceof Error)) {
return false;
}
const errorCode = "code" in error ? String((error as { code?: unknown }).code) : "";
return errorCode === "ERR_USE_AFTER_CLOSE" || /readline was closed/i.test(error.message);
}
```
How can I resolve this? If you propose a fix, please make it concise.
| function setStoragePathState(state: StoragePathState): void { | ||
| currentStorageState = state; | ||
| storagePathStateContext.enterWith(state); | ||
| } |
There was a problem hiding this comment.
AsyncLocalStorage.enterWith called outside a run() scope — leaks into the root async context
enterWith is designed to be called from within an existing AsyncLocalStorage.run() callback. when called at the module level (i.e. outside any run() scope), it mutates the root async context permanently and propagates the new state to every subsequent async operation spawned in the current event loop tick and beyond.
for a single-command cli process this is functionally safe — there is only one logical flow. but in the vitest test process, multiple tests share the same node.js async root. a setStoragePathState call in one test (e.g. setStoragePath(null)) will bleed its context into subsequent tests that happen to inherit the same async root, potentially causing subtle path-mismatch reads in getStoragePath() when storagePathStateContext.getStore() returns the wrong test's state instead of falling back to currentStorageState.
a safe alternative is to restructure callers to use storagePathStateContext.run(state, callback) so the context is explicitly scoped. if that restructuring is out of scope here, at minimum document the enterWith risk in a comment and ensure every test that mutates storage state explicitly resets it via setStoragePath(null) in an afterEach.
windows filesystem note: on windows, path comparison uses case-insensitive ntfs paths; if the leaked context carries a differently-cased path variant, transactionState.storagePath === currentStoragePath in exportAccounts could silently mismatch and trigger a redundant loadAccountsInternal read against the token file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 287-290
Comment:
**`AsyncLocalStorage.enterWith` called outside a `run()` scope — leaks into the root async context**
`enterWith` is designed to be called from within an existing `AsyncLocalStorage.run()` callback. when called at the module level (i.e. outside any `run()` scope), it mutates the root async context permanently and propagates the new state to every subsequent async operation spawned in the current event loop tick and beyond.
for a single-command cli process this is functionally safe — there is only one logical flow. but in the vitest test process, multiple tests share the same node.js async root. a `setStoragePathState` call in one test (e.g. `setStoragePath(null)`) will bleed its context into subsequent tests that happen to inherit the same async root, potentially causing subtle path-mismatch reads in `getStoragePath()` when `storagePathStateContext.getStore()` returns the wrong test's state instead of falling back to `currentStorageState`.
a safe alternative is to restructure callers to use `storagePathStateContext.run(state, callback)` so the context is explicitly scoped. if that restructuring is out of scope here, at minimum document the `enterWith` risk in a comment and ensure every test that mutates storage state explicitly resets it via `setStoragePath(null)` in an `afterEach`.
windows filesystem note: on windows, path comparison uses case-insensitive ntfs paths; if the leaked context carries a differently-cased path variant, `transactionState.storagePath === currentStoragePath` in `exportAccounts` could silently mismatch and trigger a redundant `loadAccountsInternal` read against the token file.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage.ts (1)
1950-1963:⚠️ Potential issue | 🟠 Majorpin transaction writes to the captured storage path.
lib/storage.ts:1950-1963pins the read path in transaction state, butpersist()still resolves the write target lazily throughsaveAccountsUnlocked()atlib/storage.ts:1801; the flagged variant has the same problem throughlib/storage.ts:1977-2017andlib/storage.ts:2251-2289. if a handler callssetStoragePath()orsetStoragePathDirect()mid-transaction, this becomes a path-switch race: read path a, then write or roll back path b.exportAccounts()already guards the read side atlib/storage.ts:2356-2365; the transaction helpers should pin the write target too. i also do not see a storage regression alongside the auth coverage intest/codex-manager-cli.test.ts:3007-3043,test/browser.test.ts:76-82, andtest/index.test.ts:564-580.as per coding guidelines,
lib/**: focus on auth rotation, windows filesystem io, and concurrency.Also applies to: 1977-1980
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1950 - 1963, The transaction helpers currently capture the read path (storagePath = getStoragePath()) but persist() still resolves the write target lazily via saveAccountsUnlocked(), allowing a mid-transaction setStoragePath()/setStoragePathDirect() to change the write target; fix by pinning the write path into the transaction state and making persist() use that pinned path (i.e. call the save routine with the captured storagePath or invoke an internal write helper that accepts the pinned path) instead of calling saveAccountsUnlocked() with its own lookup; update the persist closures in the transaction helpers (the persist function returned alongside state.snapshot in transactionSnapshotContext.run and its flagged variant) so all writes/rollbacks use state.storagePath, matching exportAccounts()’s read-side guard.
♻️ Duplicate comments (1)
test/browser.test.ts (1)
123-136:⚠️ Potential issue | 🔴 Criticalsame issue — missing
mockedExistsSyncsetup.this test also expects
openBrowserUrlto returntrue(line 130) but doesn't override the defaultmockedExistsSync.mockReturnValue(false)frombeforeEach.proposed fix
it("does not treat CODEX_AUTH_NO_BROWSER=false as suppression when BROWSER is disabled", () => { Object.defineProperty(process, "platform", { value: "darwin" }); process.env.PATH = "/usr/bin"; process.env.CODEX_AUTH_NO_BROWSER = "false"; process.env.BROWSER = "none"; + mockedExistsSync.mockImplementation( + (candidate) => typeof candidate === "string" && candidate.endsWith("open"), + ); + mockedStatSync.mockReturnValue({ + isFile: () => true, + mode: 0o755, + } as unknown as ReturnType<typeof fs.statSync>); expect(isBrowserLaunchSuppressed()).toBe(false); expect(openBrowserUrl("https://example.com")).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/browser.test.ts` around lines 123 - 136, The test "does not treat CODEX_AUTH_NO_BROWSER=false as suppression when BROWSER is disabled" fails to override the default mockedExistsSync behavior from beforeEach; before calling isBrowserLaunchSuppressed() and openBrowserUrl() set mockedExistsSync.mockReturnValue(true) so the path check passes and openBrowserUrl can return true and invoke mockedSpawn; update the test to explicitly set mockedExistsSync for this case (referencing mockedExistsSync, isBrowserLaunchSuppressed, openBrowserUrl, and mockedSpawn).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features.md`:
- Around line 57-59: The docs row about "Browser-first OAuth with manual
fallback" omits localhost callback-listener bind failures; update
docs/features.md to mention that manual/non-TTY login also covers local
callback-listener bind failures (when startLocalOAuthServer() is skipped in
manual mode in lib/codex-manager.ts and the bind-failure fallback exercised by
test/codex-manager-cli.test.ts:3043), and add/update corresponding upgrade notes
and README/SECURITY/docs entries to reflect the changed CLI workflows, flags
(`--manual`, `--no-browser`, CODEX_AUTH_NO_BROWSER) and any new npm scripts
introduced by this PR.
In `@docs/reference/commands.md`:
- Around line 59-67: Update the phrasing in the Upgrade Notes to state that
setting CODEX_AUTH_NO_BROWSER to any non-empty value (including "0" or "false")
overrides the BROWSER environment check and short-circuits the logic in
lib/auth/browser.ts (the early return that uses explicitNoBrowser and
NO_BROWSER_TRUTHY_VALUES), so operators know CODEX_AUTH_NO_BROWSER takes
precedence (see related behavior covered by test/browser.test.ts).
In `@lib/codex-manager.ts`:
- Line 4233: The dashboard re-login path currently calls runOAuthFlow(true, {
manual: false }) and ignores the session's loginOptions; change the call sites
inside handleManageAction (and the other path at the similar call near 4376) to
pass through the existing loginOptions object (e.g., runOAuthFlow(true,
loginOptions)) so the manual flag is honored; update handleManageAction
signature/ callers if needed to accept loginOptions so the same options are
threaded, and add a regression test beside the manual-flow coverage in
test/codex-manager-cli.test.ts (around 3007-3043) that verifies codex auth login
--manual keeps manual=true on refresh/re-login.
In `@README.md`:
- Around line 119-123: The README example instructs users to echo the callback
URL on the command line (which leaks secrets via shell history and process
listings); update the example to show feeding the callback URL on stdin from a
secure source (e.g., pipe the URL from a protected file or clipboard helper into
the CLI) instead of using echo with the URL literal. Change the line in
README.md and the duplicate in docs/getting-started.md to a secure stdin pattern
and note that lib/codex-manager.ts (accepting stdin at the handler around lines
1132-1207) will read the callback from stdin when using `codex auth login
--manual`.
- Around line 115-117: Add explicit Windows examples for setting the
CODEX_AUTH_NO_BROWSER env var to README.md and mirror them in
docs/getting-started.md: show both PowerShell and cmd ways to set
CODEX_AUTH_NO_BROWSER (matching the cross-platform toggle implemented in
lib/auth/browser.ts and the suppression behavior verified in
test/browser.test.ts) and ensure the CLI examples after setting the var reflect
the same `codex auth login` flows so README, docs, and tests remain consistent.
In `@test/browser.test.ts`:
- Around line 108-121: The tests calling openBrowserUrl() fail because
commandExists() uses fs.existsSync and the suite's beforeEach sets
mockedExistsSync.mockReturnValue(false), so override mockedExistsSync to return
true in the two tests that expect opener discovery; specifically, in the tests
that call isBrowserLaunchSuppressed() and openBrowserUrl() (the blocks at lines
around the open tests) add mockedExistsSync.mockReturnValue(true) before
invoking openBrowserUrl() so commandExists() finds the platform opener; apply
the same mockedExistsSync.mockReturnValue(true) fix to both test blocks that
expect open to return true (the test using CODEX_AUTH_NO_BROWSER="0" and the
test in the following block).
In `@test/codex-manager-cli.test.ts`:
- Around line 3220-3265: The test name says it should simulate a Windows-style
callback bind failure but currently doesn't; modify the test to mock the server
bind path (startLocalOAuthServer in the auth server module or whatever function
runCodexMultiAuthCli calls to start the local OAuth server) to reject/throw an
OSError-like error (e.g., with code "EACCES" or "EADDRINUSE") so the flow falls
back to manual input; keep the existing assertions (expect(promptQuestionMock)
called, openBrowserUrl not called, startLocalOAuthServer was attempted and
failed, and storageState.accounts length is 1) and reference
startLocalOAuthServer, runCodexMultiAuthCli, promptQuestionMock and
openBrowserUrlMock when adding the mock and verifying the fallback behavior.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 1950-1963: The transaction helpers currently capture the read path
(storagePath = getStoragePath()) but persist() still resolves the write target
lazily via saveAccountsUnlocked(), allowing a mid-transaction
setStoragePath()/setStoragePathDirect() to change the write target; fix by
pinning the write path into the transaction state and making persist() use that
pinned path (i.e. call the save routine with the captured storagePath or invoke
an internal write helper that accepts the pinned path) instead of calling
saveAccountsUnlocked() with its own lookup; update the persist closures in the
transaction helpers (the persist function returned alongside state.snapshot in
transactionSnapshotContext.run and its flagged variant) so all writes/rollbacks
use state.storagePath, matching exportAccounts()’s read-side guard.
---
Duplicate comments:
In `@test/browser.test.ts`:
- Around line 123-136: The test "does not treat CODEX_AUTH_NO_BROWSER=false as
suppression when BROWSER is disabled" fails to override the default
mockedExistsSync behavior from beforeEach; before calling
isBrowserLaunchSuppressed() and openBrowserUrl() set
mockedExistsSync.mockReturnValue(true) so the path check passes and
openBrowserUrl can return true and invoke mockedSpawn; update the test to
explicitly set mockedExistsSync for this case (referencing mockedExistsSync,
isBrowserLaunchSuppressed, openBrowserUrl, and mockedSpawn).
🪄 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: c51ee175-024c-4814-a17e-46b08993128d
📒 Files selected for processing (12)
README.mddocs/features.mddocs/getting-started.mddocs/reference/commands.mddocs/upgrade.mdlib/auth/browser.tslib/codex-manager.tslib/storage.tslib/ui/copy.tstest/browser.test.tstest/codex-manager-cli.test.tstest/index.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
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/ui/copy.tslib/auth/browser.tslib/storage.tslib/codex-manager.ts
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/features.mddocs/getting-started.mddocs/reference/commands.mddocs/upgrade.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/index.test.tstest/codex-manager-cli.test.tstest/browser.test.ts
🔇 Additional comments (13)
test/index.test.ts (2)
56-58: lgtm — mock setup forisBrowserLaunchSuppressedis correct.default mock returns
false, matching production behavior when no env suppression is set.
564-582: lgtm — past feedback addressed.this test now correctly:
- mocks
isBrowserLaunchSuppressedto returntrue- asserts
openBrowserUrlwas not called (line 581)- validates manual flow selection via
method: "code"and instruction textdeterministic and covers the suppressed-browser path as requested.
docs/upgrade.md (1)
52-62: lgtm — upgrade notes align with implementation.documentation correctly describes:
- browser-first default behavior
--manual/--no-browserflagsCODEX_AUTH_NO_BROWSER=1suppression- stdin callback url for non-tty shells
cross-checked against
lib/auth/browser.ts:11-33— false-like values (0,false) are not in the truthy set, so they don't suppress. docs accurately reflect this.test/browser.test.ts (2)
45-67: lgtm — env var save/restore prevents test pollution.properly preserving and restoring
CODEX_AUTH_NO_BROWSERandBROWSERinafterEachkeeps tests deterministic across different ci environments.
79-85: lgtm — suppression regression cases address past feedback.tests now cover:
CODEX_AUTH_NO_BROWSER="1"suppression (lines 79-85)BROWSER="none"suppression (lines 138-144)- both truthy vars set (lines 146-153)
this locks in the no-browser guarantee for headless/automation paths.
Also applies to: 138-153
test/codex-manager-cli.test.ts (8)
44-51: mock aligns with test usage but is simpler than production.the regex-based mock for
parseAuthorizationInputhandles URL-stylecode=...&state=...inputs, which is sufficient for the callback strings used in these tests. the real implementation atlib/auth/auth.ts:69-105also handles hash fragments and plain code values, but those paths aren't exercised here.
56-59: default mock forisBrowserLaunchSuppressedlooks correct.returns
falseby default, allowing individual tests to override withmockReturnValueOnce(true)when testing the suppression path (e.g., lines 3119).
2960-3018: test covers--manuallogin without browser or local server usage.verifies
openBrowserUrl,startLocalOAuthServer, andwaitForCodeare all not called, and that the pasted callback flow persists an account. the tty state is explicitly pinned viasetInteractiveTTY(true). addresses prior feedback about asserting the server isn't touched.
3020-3080: interactive manual selection path is covered.ensures that when
selectMockreturns"manual", the flow skips browser launch and local server, then uses the pasted callback. good coverage for the interactive menu → manual flow.
3082-3131: browser-suppression fallback test addresses prior review gap.flips
isBrowserLaunchSuppressedtotrueand verifies pasted-callback handling without browser launch or listener dependence. this closes the coverage hole flagged in past comments.
3133-3178: non-tty manual mode test is deterministic and thorough.sets
isTTYtofalse, passes--manual, and assertspromptQuestionMockis called with an empty string (non-interactive prompt). verifies no browser or server usage, and account persistence.
3180-3218: state mismatch rejection test is solid.verifies that a callback with
state=wrong-statedoes not triggerexchangeAuthorizationCodeand leaves storage unchanged. good edge-case coverage.
3267-3303: stdin close handling test is correct.simulates
promptQuestionMockrejecting with "readline was closed" and verifies graceful exit: no exchange, no persisted accounts, exit code 0. deterministic and useful for piped-stdin edge cases.
| | Browser-first OAuth with manual fallback | `codex auth login` stays browser-first, while `--manual`, `--no-browser`, and `CODEX_AUTH_NO_BROWSER=1` keep login usable in browser-restricted shells | | ||
|
|
||
| Manual/non-TTY login accepts the full callback URL on stdin, so automation and host-managed shells can complete auth without relying on a local browser handoff. |
There was a problem hiding this comment.
mention callback-listener failures here too.
lib/codex-manager.ts:1540-1561 skips startLocalOAuthServer() in manual mode, and the bind-failure fallback is already exercised in test/codex-manager-cli.test.ts:3043. this row should mention localhost callback-listener failures as well, not only browser-restricted shells, otherwise the feature map under-documents the flow this pr adds.
as per coding guidelines, docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/features.md` around lines 57 - 59, The docs row about "Browser-first
OAuth with manual fallback" omits localhost callback-listener bind failures;
update docs/features.md to mention that manual/non-TTY login also covers local
callback-listener bind failures (when startLocalOAuthServer() is skipped in
manual mode in lib/codex-manager.ts and the bind-failure fallback exercised by
test/codex-manager-cli.test.ts:3043), and add/update corresponding upgrade notes
and README/SECURITY/docs entries to reflect the changed CLI workflows, flags
(`--manual`, `--no-browser`, CODEX_AUTH_NO_BROWSER) and any new npm scripts
introduced by this PR.
| ## Upgrade Notes | ||
|
|
||
| - `codex auth login` remains browser-first by default. | ||
| - `codex auth login --manual` and `codex auth login --no-browser` force the manual callback flow instead of launching a browser. | ||
| - `CODEX_AUTH_NO_BROWSER=1` suppresses browser launch for automation/headless sessions. False-like values such as `0` and `false` do not disable browser launch by themselves. | ||
| - In non-TTY/manual shells, pass the full redirect URL on stdin, for example: `echo "http://127.0.0.1:1455/auth/callback?code=..." | codex auth login --manual`. | ||
| - No new npm scripts or storage migration steps were introduced for this auth-flow update. | ||
|
|
||
| --- |
There was a problem hiding this comment.
documentation phrasing may mislead operators on env precedence.
line 63 says "False-like values such as 0 and false do not disable browser launch by themselves."
per lib/auth/browser.ts:18-21, when CODEX_AUTH_NO_BROWSER is set to any non-empty value (including "0"), the function returns early:
if (explicitNoBrowser.length > 0) {
return NO_BROWSER_TRUTHY_VALUES.has(explicitNoBrowser);
}this means CODEX_AUTH_NO_BROWSER="0" returns false (browser allowed) and skips the BROWSER check entirely.
result: CODEX_AUTH_NO_BROWSER="0" + BROWSER="none" → browser launches (confirmed by test/browser.test.ts:108-136).
consider clarifying: "Setting CODEX_AUTH_NO_BROWSER to any value (including 0 or false) overrides the BROWSER environment variable check."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reference/commands.md` around lines 59 - 67, Update the phrasing in the
Upgrade Notes to state that setting CODEX_AUTH_NO_BROWSER to any non-empty value
(including "0" or "false") overrides the BROWSER environment check and
short-circuits the logic in lib/auth/browser.ts (the early return that uses
explicitNoBrowser and NO_BROWSER_TRUTHY_VALUES), so operators know
CODEX_AUTH_NO_BROWSER takes precedence (see related behavior covered by
test/browser.test.ts).
| if (!existing) return; | ||
|
|
||
| const tokenResult = await runOAuthFlow(true); | ||
| const tokenResult = await runOAuthFlow(true, { manual: false }); |
There was a problem hiding this comment.
carry the session's manual preference into account refresh.
lib/codex-manager.ts:4376 honors loginOptions, but lib/codex-manager.ts:4233 hardcodes { manual: false } for the dashboard re-login path. in a codex auth login --manual session, refresh/re-login account drops back to browser-first behavior instead of honoring the explicit flag. please thread the same loginOptions through handleManageAction(), and add a regression beside the existing manual-flow coverage in test/codex-manager-cli.test.ts:3007-3043.
suggested fix
-async function handleManageAction(
+async function handleManageAction(
storage: AccountStorageV3,
menuResult: Awaited<ReturnType<typeof promptLoginMode>>,
+ loginOptions: AuthLoginOptions,
): Promise<void> {
@@
- const tokenResult = await runOAuthFlow(true, { manual: false });
+ const tokenResult = await runOAuthFlow(true, loginOptions);- await handleManageAction(currentStorage, menuResult);
+ await handleManageAction(currentStorage, menuResult, loginOptions);
@@
- await handleManageAction(currentStorage, menuResult);
+ await handleManageAction(currentStorage, menuResult, loginOptions);Also applies to: 4376-4376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` at line 4233, The dashboard re-login path currently
calls runOAuthFlow(true, { manual: false }) and ignores the session's
loginOptions; change the call sites inside handleManageAction (and the other
path at the similar call near 4376) to pass through the existing loginOptions
object (e.g., runOAuthFlow(true, loginOptions)) so the manual flag is honored;
update handleManageAction signature/ callers if needed to accept loginOptions so
the same options are threaded, and add a regression test beside the manual-flow
coverage in test/codex-manager-cli.test.ts (around 3007-3043) that verifies
codex auth login --manual keeps manual=true on refresh/re-login.
| codex auth login --manual | ||
| CODEX_AUTH_NO_BROWSER=1 codex auth login | ||
| ``` |
There was a problem hiding this comment.
document the windows syntax for CODEX_AUTH_NO_BROWSER.
lib/auth/browser.ts:25-32 makes CODEX_AUTH_NO_BROWSER a cross-platform toggle, and the suppression behavior is covered in test/browser.test.ts:76-82. the example here is posix-only, so windows users copying it from powershell or cmd will not actually set the variable. please add a powershell example here, and mirror it in docs/getting-started.md:61-64.
as per coding guidelines, docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
suggested fix
codex auth login --manual
CODEX_AUTH_NO_BROWSER=1 codex auth login
+$env:CODEX_AUTH_NO_BROWSER="1"; codex auth login🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 115 - 117, Add explicit Windows examples for setting
the CODEX_AUTH_NO_BROWSER env var to README.md and mirror them in
docs/getting-started.md: show both PowerShell and cmd ways to set
CODEX_AUTH_NO_BROWSER (matching the cross-platform toggle implemented in
lib/auth/browser.ts and the suppression behavior verified in
test/browser.test.ts) and ensure the CLI examples after setting the var reflect
the same `codex auth login` flows so README, docs, and tests remain consistent.
| In non-TTY/manual shells, provide the full redirect URL on stdin instead of waiting for a browser callback: | ||
|
|
||
| ```bash | ||
| echo "http://127.0.0.1:1455/auth/callback?code=..." | codex auth login --manual | ||
| ``` |
There was a problem hiding this comment.
do not teach users to put the callback url on the command line.
echo "http://127.0.0.1:1455/auth/callback?code=..." | codex auth login --manual puts the live oauth code into shell history and makes it visible in process listings. lib/codex-manager.ts:1132-1207 already accepts stdin input, so the safer doc pattern is “feed the callback url on stdin from a secure source” rather than showing it as a command-line argument. the same example appears in docs/getting-started.md:66-70.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 119 - 123, The README example instructs users to echo
the callback URL on the command line (which leaks secrets via shell history and
process listings); update the example to show feeding the callback URL on stdin
from a secure source (e.g., pipe the URL from a protected file or clipboard
helper into the CLI) instead of using echo with the URL literal. Change the line
in README.md and the duplicate in docs/getting-started.md to a secure stdin
pattern and note that lib/codex-manager.ts (accepting stdin at the handler
around lines 1132-1207) will read the callback from stdin when using `codex auth
login --manual`.
| it("lets explicit false-like CODEX_AUTH_NO_BROWSER override a disabling BROWSER value", () => { | ||
| Object.defineProperty(process, "platform", { value: "darwin" }); | ||
| process.env.PATH = "/usr/bin"; | ||
| process.env.CODEX_AUTH_NO_BROWSER = "0"; | ||
| process.env.BROWSER = "none"; | ||
|
|
||
| expect(isBrowserLaunchSuppressed()).toBe(false); | ||
| expect(openBrowserUrl("https://example.com")).toBe(true); | ||
| expect(mockedSpawn).toHaveBeenCalledWith( | ||
| "open", | ||
| ["https://example.com"], | ||
| { stdio: "ignore", shell: false }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# run the specific test to verify it passes
cd "$(git rev-parse --show-toplevel)"
npm test -- test/browser.test.ts --reporter=verbose 2>&1 | head -100Repository: ndycode/codex-multi-auth
Length of output: 178
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && wc -l test/browser.test.tsRepository: ndycode/codex-multi-auth
Length of output: 92
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && sed -n '45,55p' test/browser.test.ts && echo "---" && sed -n '87,121p' test/browser.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1668
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && sed -n '1,20p' test/browser.test.tsRepository: ndycode/codex-multi-auth
Length of output: 552
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && sed -n '108,155p' test/browser.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1837
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && sed -n '138,160p' test/browser.test.tsRepository: ndycode/codex-multi-auth
Length of output: 961
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && sed -n '1,100p' lib/auth/browser.js | head -60Repository: ndycode/codex-multi-auth
Length of output: 130
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && grep -n "existsSync\|openBrowserUrl\|isBrowserLaunchSuppressed" lib/auth/browser.js | head -20Repository: ndycode/codex-multi-auth
Length of output: 120
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && find . -name "browser.*" -type f | grep -v node_modules | head -10Repository: ndycode/codex-multi-auth
Length of output: 112
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && git ls-files | grep browserRepository: ndycode/codex-multi-auth
Length of output: 108
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && wc -l lib/auth/browser.tsRepository: ndycode/codex-multi-auth
Length of output: 91
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && cat -n lib/auth/browser.ts | head -100Repository: ndycode/codex-multi-auth
Length of output: 4417
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && sed -n '100,150p' lib/auth/browser.tsRepository: ndycode/codex-multi-auth
Length of output: 1559
tests at 108-121 and 123-136 will fail: missing mockedExistsSync mock prevents opener discovery.
commandExists() at lib/auth/browser.ts:45-92 calls fs.existsSync() to check if the opener binary ("open" on darwin, "powershell.exe" on win32) exists on PATH. the beforeEach at test/browser.test.ts:50 sets mockedExistsSync.mockReturnValue(false) by default, so commandExists() always returns false unless overridden.
tests at lines 108-121 and 123-136 both call openBrowserUrl() expecting true but don't override the mock. they'll return false instead. the test at 87-106 correctly sets up the mock at lines 91-93—these two need the same pattern.
it("lets explicit false-like CODEX_AUTH_NO_BROWSER override a disabling BROWSER value", () => {
Object.defineProperty(process, "platform", { value: "darwin" });
process.env.PATH = "/usr/bin";
process.env.CODEX_AUTH_NO_BROWSER = "0";
process.env.BROWSER = "none";
+ mockedExistsSync.mockImplementation(
+ (candidate) => typeof candidate === "string" && candidate.endsWith("open"),
+ );
+ mockedStatSync.mockReturnValue({
+ isFile: () => true,
+ mode: 0o755,
+ } as unknown as ReturnType<typeof fs.statSync>);
expect(isBrowserLaunchSuppressed()).toBe(false);
expect(openBrowserUrl("https://example.com")).toBe(true);also apply the same fix to test/browser.test.ts:123-136.
📝 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("lets explicit false-like CODEX_AUTH_NO_BROWSER override a disabling BROWSER value", () => { | |
| Object.defineProperty(process, "platform", { value: "darwin" }); | |
| process.env.PATH = "/usr/bin"; | |
| process.env.CODEX_AUTH_NO_BROWSER = "0"; | |
| process.env.BROWSER = "none"; | |
| expect(isBrowserLaunchSuppressed()).toBe(false); | |
| expect(openBrowserUrl("https://example.com")).toBe(true); | |
| expect(mockedSpawn).toHaveBeenCalledWith( | |
| "open", | |
| ["https://example.com"], | |
| { stdio: "ignore", shell: false }, | |
| ); | |
| }); | |
| it("lets explicit false-like CODEX_AUTH_NO_BROWSER override a disabling BROWSER value", () => { | |
| Object.defineProperty(process, "platform", { value: "darwin" }); | |
| process.env.PATH = "/usr/bin"; | |
| process.env.CODEX_AUTH_NO_BROWSER = "0"; | |
| process.env.BROWSER = "none"; | |
| mockedExistsSync.mockImplementation( | |
| (candidate) => typeof candidate === "string" && candidate.endsWith("open"), | |
| ); | |
| mockedStatSync.mockReturnValue({ | |
| isFile: () => true, | |
| mode: 0o755, | |
| } as unknown as ReturnType<typeof fs.statSync>); | |
| expect(isBrowserLaunchSuppressed()).toBe(false); | |
| expect(openBrowserUrl("https://example.com")).toBe(true); | |
| expect(mockedSpawn).toHaveBeenCalledWith( | |
| "open", | |
| ["https://example.com"], | |
| { stdio: "ignore", shell: false }, | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/browser.test.ts` around lines 108 - 121, The tests calling
openBrowserUrl() fail because commandExists() uses fs.existsSync and the suite's
beforeEach sets mockedExistsSync.mockReturnValue(false), so override
mockedExistsSync to return true in the two tests that expect opener discovery;
specifically, in the tests that call isBrowserLaunchSuppressed() and
openBrowserUrl() (the blocks at lines around the open tests) add
mockedExistsSync.mockReturnValue(true) before invoking openBrowserUrl() so
commandExists() finds the platform opener; apply the same
mockedExistsSync.mockReturnValue(true) fix to both test blocks that expect open
to return true (the test using CODEX_AUTH_NO_BROWSER="0" and the test in the
following block).
| it("falls back to pasted manual input when Windows-style callback bind fails", async () => { | ||
| setInteractiveTTY(false); | ||
| const now = Date.now(); | ||
| let storageState = { | ||
| version: 3 as const, | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 0 }, | ||
| accounts: [] as Array<Record<string, unknown>>, | ||
| }; | ||
| loadAccountsMock.mockImplementation(async () => structuredClone(storageState)); | ||
| saveAccountsMock.mockImplementation(async (nextStorage) => { | ||
| storageState = structuredClone(nextStorage); | ||
| }); | ||
| promptLoginModeMock.mockResolvedValueOnce({ mode: "cancel" }); | ||
| promptQuestionMock.mockResolvedValueOnce( | ||
| "http://127.0.0.1:1455/auth/callback?code=oauth-code&state=oauth-state", | ||
| ); | ||
|
|
||
| const authModule = await import("../lib/auth/auth.js"); | ||
| vi.mocked(authModule.createAuthorizationFlow).mockResolvedValueOnce({ | ||
| pkce: { challenge: "pkce-challenge", verifier: "pkce-verifier" }, | ||
| state: "oauth-state", | ||
| url: "https://auth.openai.com/mock", | ||
| }); | ||
| vi.mocked(authModule.exchangeAuthorizationCode).mockResolvedValueOnce({ | ||
| type: "success", | ||
| access: "access-eacces", | ||
| refresh: "refresh-eacces", | ||
| expires: now + 7_200_000, | ||
| idToken: "id-token-eacces", | ||
| multiAccount: true, | ||
| }); | ||
|
|
||
| const browserModule = await import("../lib/auth/browser.js"); | ||
| const openBrowserUrlMock = vi.mocked(browserModule.openBrowserUrl); | ||
| const serverModule = await import("../lib/auth/server.js"); | ||
|
|
||
| const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); | ||
| const exitCode = await runCodexMultiAuthCli(["auth", "login", "--manual"]); | ||
|
|
||
| expect(exitCode).toBe(0); | ||
| expect(promptQuestionMock).toHaveBeenCalledWith(""); | ||
| expect(openBrowserUrlMock).not.toHaveBeenCalled(); | ||
| expect(vi.mocked(serverModule.startLocalOAuthServer)).not.toHaveBeenCalled(); | ||
| expect(storageState.accounts).toHaveLength(1); | ||
| }); |
There was a problem hiding this comment.
test name claims windows bind failure but no failure is simulated.
the test "falls back to pasted manual input when Windows-style callback bind fails" doesn't mock any bind error (e.g., EACCES, EADDRINUSE). it's functionally identical to the non-tty manual mode test above and doesn't exercise the bind-failure recovery path.
if the intent is to cover windows bind failures, simulate the error and verify the fallback:
suggested change to actually test bind failure
it("falls back to pasted manual input when Windows-style callback bind fails", async () => {
- setInteractiveTTY(false);
+ setInteractiveTTY(true);
const now = Date.now();
// ... storage setup ...
+ const serverModule = await import("../lib/auth/server.js");
+ vi.mocked(serverModule.startLocalOAuthServer).mockRejectedValueOnce(
+ makeErrnoError("bind EACCES 127.0.0.1:1455", "EACCES"),
+ );
+
// ... rest of test setup ...
const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js");
- const exitCode = await runCodexMultiAuthCli(["auth", "login", "--manual"]);
+ const exitCode = await runCodexMultiAuthCli(["auth", "login"]);
expect(exitCode).toBe(0);
expect(promptQuestionMock).toHaveBeenCalled();
+ expect(vi.mocked(serverModule.startLocalOAuthServer)).toHaveBeenCalled();
// ... existing assertions ...
});as per coding guidelines, test/**: demand regression cases that reproduce windows filesystem behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-cli.test.ts` around lines 3220 - 3265, The test name says
it should simulate a Windows-style callback bind failure but currently doesn't;
modify the test to mock the server bind path (startLocalOAuthServer in the auth
server module or whatever function runCodexMultiAuthCli calls to start the local
OAuth server) to reject/throw an OSError-like error (e.g., with code "EACCES" or
"EADDRINUSE") so the flow falls back to manual input; keep the existing
assertions (expect(promptQuestionMock) called, openBrowserUrl not called,
startLocalOAuthServer was attempted and failed, and storageState.accounts length
is 1) and reference startLocalOAuthServer, runCodexMultiAuthCli,
promptQuestionMock and openBrowserUrlMock when adding the mock and verifying the
fallback behavior.
1 participant
Summary
What Changed
Validation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
f82a748Additional 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 introduces a
--manual/--no-browserflag tocodex auth loginand a newisBrowserLaunchSuppressed()helper that readsCODEX_AUTH_NO_BROWSER/BROWSERenv vars, enabling headless and browser-restricted auth flows. the core fix — gatingstartLocalOAuthServeronsignInMode === "browser"only — correctly resolves the priorwaitForCodeindefinite-block in manual mode.promptManualCallbackis extended with anallowNonTtypath for stdin-pipe use. storage path state is refactored from module-level vars into aStoragePathStatestruct backed byAsyncLocalStorage.runOAuthFlownow receivesAuthLoginOptionsand computespreferManualMode = options.manual || isBrowserLaunchSuppressed(), skipping server startup and the mode-picker prompt when trueisBrowserLaunchSuppresseduses a two-tier precedence: explicitCODEX_AUTH_NO_BROWSER(truthy values suppress, falsy values override even a disablingBROWSERsetting) thenBROWSERdisabled-value checksetStoragePathStateusesAsyncLocalStorage.enterWith()outside arun()scope, which permanently mutates the root async context — safe for single-command cli execution but worth watching in concurrent test workers or future server-mode usage--manualskip-server, interactive picker manual choice, env-var suppressed fallback, non-tty stdin paste, state-mismatch rejection, eacces bind failure, and stdin-close graceful exitfeatures.md,getting-started.md,commands.md, andupgrade.mdare all updated consistentlyConfidence Score: 4/5
enterWithcontext leakagewaitForCodein manual mode) is properly fixed by gating server startup onsignInMode === "browser". theisBrowserLaunchSuppressedlogic is clean and the env-var precedence is documented. the main concern isAsyncLocalStorage.enterWithinsetStoragePathStatebeing called outside arun()scope, which leaks the context into the root async tree — this is fine for single-command cli use but is a fragile pattern that could cause subtle path mismatches in concurrent vitest workers or a hypothetical future server mode. no token safety or windows filesystem regressions introduced.setStoragePathStatefunction's use ofenterWithoutside arun()scope deserves a second look before this pattern is extended furtherImportant Files Changed
--manual/--no-browserflag parsing viaparseAuthLoginArgs, passesAuthLoginOptionsthroughrunOAuthFlow; correctly gatesstartLocalOAuthServeronsignInMode === "browser"only, fixing the priorwaitForCodeblocking bug;isReadlineClosedErrorcontains a redundant post-instanceofguardisBrowserLaunchSuppressed()with two-tier env-var logic (CODEX_AUTH_NO_BROWSERtakes precedence overBROWSER); asymmetry between the two sets is intentional and documented; windows powershell escaping and clipboard handling unchanged and correctStoragePathStatestruct backed byAsyncLocalStorage;setStoragePathStateusesenterWithoutside arun()scope, which leaks the context into the root async tree — safe for single-command cli use but risky in concurrent test/worker scenarios;exportAccountscorrectly guards transaction reuse by comparingstoragePath--manualskip-server, interactive mode-picker manual path, suppressed-browser fallback, non-tty stdin paste, state-mismatch rejection, windows eacces fallback, stdin-close graceful exit; one test stages aselectMockvalue that is never consumed due topreferManualModebypassingpromptOAuthSignInModeisBrowserLaunchSuppressedcovering truthy/falsyCODEX_AUTH_NO_BROWSER,BROWSER=nonesuppression, explicit false overriding disablingBROWSER, and combined-flag cases; env teardown inafterEachis correctcallbackBypassedandcallbackUnavailableUI strings for manual-mode messaging; straightforward, no issuesisBrowserLaunchSuppressedto the browser mock and covers the globally-suppressed auto-method path; assertions onflow.method,flow.instructions, andvalidateare correctSequence Diagram
sequenceDiagram participant CLI as codex auth login participant Parser as parseAuthLoginArgs participant OAuthFlow as runOAuthFlow participant BrowserUtil as isBrowserLaunchSuppressed participant Server as startLocalOAuthServer participant Browser as openBrowserUrl participant Prompt as promptManualCallback CLI->>Parser: args (e.g. ["--manual"]) Parser-->>CLI: { ok: true, options: { manual: true } } CLI->>OAuthFlow: runOAuthFlow(forceNew, { manual: true }) OAuthFlow->>BrowserUtil: isBrowserLaunchSuppressed() BrowserUtil-->>OAuthFlow: false (env not set) Note over OAuthFlow: preferManualMode = options.manual || suppressed = true OAuthFlow-->>OAuthFlow: signInMode = "manual" (skip promptOAuthSignInMode) Note over Server: startLocalOAuthServer NOT called OAuthFlow->>Browser: openBrowserUrl(url) — skipped (else branch) OAuthFlow-->>OAuthFlow: print URL + copy to clipboard Note over OAuthFlow: waitingForCallback = false (oauthServer is null) OAuthFlow->>Prompt: promptManualCallback(state, { allowNonTty: true }) Prompt-->>OAuthFlow: code (from stdin or TTY paste) OAuthFlow-->>CLI: TokenResult { type: "success" }Prompt To Fix All With AI
Last reviewed commit: "test(auth): consume ..."