feat: support multiple workspaces for the same email (#491)#493
feat: support multiple workspaces for the same email (#491)#493ndycode wants to merge 5 commits into
Conversation
Support a single OpenAI/Google account that belongs to multiple ChatGPT workspaces (personal Plus vs business/team), each a distinct quota pool keyed by its org_id. The composite accountId+email matching already keeps such entries separate, but the workspace metadata captured at login did not survive a reload and was invisible in list/status output. - schemas: add WorkspaceSchema and declare workspaces and currentWorkspaceIndex on AccountMetadataV3Schema. The strict z.object stripped these fields on every load, so login-captured workspaces vanished after one read/write round-trip. This is the root cause of the reported "workspaceName always empty even after login". - accounts: surface the active workspace name in formatAccountLabel so two same-email accounts in different workspaces stay distinguishable, while preserving every existing label format. - tests: schema round-trip preservation plus label disambiguation coverage.
Build on the workspace-persistence fix by making an email's multiple workspaces visible and selectable, so personal Plus and business/team under one Google account can be tracked and rotated between independently. - accounts: add formatWorkspaceLines, one display line per workspace with the active one marked and disabled ones flagged. - status/list: list every workspace beneath an account when it tracks more than one, so both stay visible at once. - new "workspace <account> [workspace]" command: with an account index it lists that account's workspaces; with a workspace index it sets the active workspace and persists it. Wired into the command registry, dispatch, and help. - tests: formatWorkspaceLines coverage and nine workspace-command cases.
|
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 workspace listing and switching. includes a formatter ( ChangesWorkspace Selection Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes review notes
possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@lib/codex-manager.ts`:
- Around line 3563-3569: Add a regression vitest that verifies the new
"workspace" dispatcher path calls runWorkspaceCommand with the forwarded args
and dependency bundle: mock or spy on runWorkspaceCommand and invoke the
dispatcher code path that sets command === "workspace" (simulate input like
"auth workspace ..."), then assert runWorkspaceCommand was called once with the
expected rest argument array and the deps object containing setStoragePath,
loadAccounts, saveAccounts; place the test alongside existing dispatcher tests
and reference lib/codex-manager.ts symbols runWorkspaceCommand and the
dispatcher function used to parse "auth workspace".
In `@lib/codex-manager/commands/status.ts`:
- Around line 246-253: The change to status output modifies workspace rendering;
add/update a vitest that verifies formatWorkspaceLines output is stable by
asserting the active marker, disabled annotation, and indentation are present in
the lines emitted to logInfo for an account with multiple workspaces;
specifically write a test that calls formatWorkspaceLines (or invokes the status
command path that calls it) with sample workspaces including an active and a
disabled workspace and asserts the returned lines contain the expected leading
indentation, active marker string, and "disabled" annotation so future changes
to formatWorkspaceLines or how status.ts logs workspaces will fail the test.
In `@lib/codex-manager/commands/workspace.ts`:
- Around line 125-128: The code accesses target.id.length and
target.id.slice(-6) directly which can throw if id is undefined; update the
idSuffix computation to use optional chaining and a safe default (e.g. const
idSuffix = target.id?.length ? target.id.slice(-6) : "") and adjust the logInfo
call to rely on that idSuffix (referencing target.id, idSuffix, logInfo and the
surrounding workspace formatting like formatWorkspaceLines if present) so
runtime undefined ids are handled without exceptions.
In `@test/codex-manager-workspace-command.test.ts`:
- Around line 98-148: Add a unit test that covers non-numeric workspace indices:
call runWorkspaceCommand with args ["1","xyz"], assert it returns 1, assert
deps.logError was called with "Invalid workspace index. Valid range: 1-2", and
ensure deps.saveAccounts was not called; this mirrors the existing out-of-range
test and verifies the Number.parseInt/validation logic in the workspace
selection code (see runWorkspaceCommand and the parseInt-based validation in
workspace.ts).
- Around line 50-73: Add a unit test that asserts non-numeric account indices
are handled with an explicit error: create a test in
test/codex-manager-workspace-command.test.ts that calls
runWorkspaceCommand(["abc"], deps) (use createDeps()), expects a return code of
1, and expects deps.logError to have been called with "Invalid account index:
abc"; this covers the edge case where Number.parseInt returns NaN and verifies
behavior implemented in workspaces logic (see Number.parseInt check in
lib/codex-manager/commands/workspace.ts and the runWorkspaceCommand function).
🪄 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: 0566d4fb-37ae-4abe-9c52-47b644f04dc4
📒 Files selected for processing (7)
lib/accounts.tslib/codex-manager.tslib/codex-manager/commands/status.tslib/codex-manager/commands/workspace.tslib/codex-manager/help.tstest/accounts.test.tstest/codex-manager-workspace-command.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)
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only (
"type": "module"), Node >= 18
Files:
lib/codex-manager/help.tslib/codex-manager/commands/status.tslib/accounts.tslib/codex-manager.tstest/accounts.test.tstest/codex-manager-workspace-command.test.tslib/codex-manager/commands/workspace.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errorTypeScript assertions
Files:
lib/codex-manager/help.tslib/codex-manager/commands/status.tslib/accounts.tslib/codex-manager.tstest/accounts.test.tstest/codex-manager-workspace-command.test.tslib/codex-manager/commands/workspace.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/codex-manager/help.tslib/codex-manager/commands/status.tslib/accounts.tslib/codex-manager.tslib/codex-manager/commands/workspace.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/help.tslib/codex-manager/commands/status.tslib/accounts.tslib/codex-manager.tslib/codex-manager/commands/workspace.ts
**/lib/accounts.ts
📄 CodeRabbit inference engine (AGENTS.md)
Email dedup is case-insensitive via
normalizeEmailKey()(trim + lowercase)
Files:
lib/accounts.ts
lib/accounts*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Account health is 0-100 and should be updated through the account manager APIs
Files:
lib/accounts.ts
{**/scripts/**/*.js,**/test/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling for transient
EBUSY/EPERM/ENOTEMPTYerrors
Files:
test/accounts.test.tstest/codex-manager-workspace-command.test.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/accounts.test.tstest/codex-manager-workspace-command.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/accounts.test.tstest/codex-manager-workspace-command.test.ts
🔇 Additional comments (10)
lib/accounts.ts (1)
1809-1830: LGTM!test/accounts.test.ts (1)
852-887: LGTM!lib/codex-manager/commands/workspace.ts (4)
1-66: LGTM!
67-86: LGTM!
88-113: LGTM!
115-120: LGTM!test/codex-manager-workspace-command.test.ts (1)
75-96: LGTM!lib/codex-manager.ts (1)
91-91: LGTM!Also applies to: 206-206
lib/codex-manager/commands/status.ts (1)
5-5: LGTM!lib/codex-manager/help.ts (1)
15-15: LGTM!
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| describe("runWorkspaceCommand", () => { | ||
| it("errors when no accounts are configured", async () => { | ||
| const deps = createDeps({ loadAccounts: vi.fn(async () => null) }); | ||
| const result = await runWorkspaceCommand(["1"], deps); | ||
| expect(result).toBe(1); | ||
| expect(deps.logError).toHaveBeenCalledWith("No accounts configured."); | ||
| }); | ||
|
|
There was a problem hiding this comment.
saveAccountsWithRetry handles EBUSY/EPERM internally, but there's no test that verifies runWorkspaceCommand surfaces an unretryable error correctly. project convention (see unified-settings.test.ts) is to cover the EBUSY retry path for every new save path. worth adding a case that makes saveAccounts throw an EBUSY-coded error and asserts the rejection propagates (or is caught) as expected.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/codex-manager-workspace-command.test.ts
Line: 48-56
Comment:
**missing ebusy/eperm save-retry coverage**
`saveAccountsWithRetry` handles `EBUSY`/`EPERM` internally, but there's no test that verifies `runWorkspaceCommand` surfaces an unretryable error correctly. project convention (see `unified-settings.test.ts`) is to cover the EBUSY retry path for every new save path. worth adding a case that makes `saveAccounts` throw an `EBUSY`-coded error and asserts the rejection propagates (or is caught) as expected.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| account.currentWorkspaceIndex = workspaceIndex; | ||
| await saveAccountsWithRetry(storage, deps.saveAccounts); |
There was a problem hiding this comment.
this follows the same load-mutate-save pattern as switch/unpin, but worth calling out explicitly: the runtime-rotation-proxy writes to the same accounts file (e.g. disableCurrentWorkspace, lastUsed updates). if the proxy writes between loadAccounts() and saveAccountsWithRetry() here, this save silently overwrites those changes — most critically, a workspace enabled: false written by the proxy could be restored to true. no file-level lock or re-read-before-write guards against this. low probability in practice, but concurrency tests for this path are absent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager/commands/workspace.ts
Line: 122-123
Comment:
**concurrent write race (TOCTOU)**
this follows the same load-mutate-save pattern as `switch`/`unpin`, but worth calling out explicitly: the runtime-rotation-proxy writes to the same accounts file (e.g. `disableCurrentWorkspace`, `lastUsed` updates). if the proxy writes between `loadAccounts()` and `saveAccountsWithRetry()` here, this save silently overwrites those changes — most critically, a workspace `enabled: false` written by the proxy could be restored to `true`. no file-level lock or re-read-before-write guards against this. low probability in practice, but concurrency tests for this path are absent.
How can I resolve this? If you propose a fix, please make it concise.Let a user register a specific workspace for an email that belongs to more than one (personal Plus vs business/team). `--org <org_id>` reuses the CODEX_AUTH_ACCOUNT_ID override that every login resolver already honors, so the chosen org is bound for that login and a second workspace can be added on demand instead of always resolving to the default one. - help: parse --org <id> and --org=<id> (rejects a missing value), document it in the login usage line. - codex-manager: runAuthLogin sets the override env var from --org for the invocation, taking precedence over any inherited value. - tests: --org parsing and missing-value cases.
ndycode
changed the title
ndycode
changed the base branch from
feat/issue-491-persist-surface-workspaces
to
main
| if (loginOptions.org) { | ||
| process.env.CODEX_AUTH_ACCOUNT_ID = loginOptions.org; | ||
| console.log(`Binding this login to workspace org id: ${loginOptions.org}`); | ||
| } |
There was a problem hiding this comment.
CODEX_AUTH_ACCOUNT_ID never restored after --org login
process.env.CODEX_AUTH_ACCOUNT_ID is set here but never deleted or restored, so it persists for the entire process lifetime. resolveAccountSelection reads it on every subsequent call — if runAuthLogin is invoked again in the same process (e.g. from an existing vitest worker that exercises the login path, or any future interactive/menu flow), that second login silently binds to the org from the first call even without --org, causing a token-to-wrong-org mismatch. on Windows, env var mutation can also behave unexpectedly across async boundaries. save the original value before setting it and restore (or delete) it once runAuthLogin returns.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 2792-2795
Comment:
**`CODEX_AUTH_ACCOUNT_ID` never restored after `--org` login**
`process.env.CODEX_AUTH_ACCOUNT_ID` is set here but never deleted or restored, so it persists for the entire process lifetime. `resolveAccountSelection` reads it on every subsequent call — if `runAuthLogin` is invoked again in the same process (e.g. from an existing vitest worker that exercises the login path, or any future interactive/menu flow), that second login silently binds to the `org` from the first call even without `--org`, causing a token-to-wrong-org mismatch. on Windows, env var mutation can also behave unexpectedly across async boundaries. save the original value before setting it and restore (or delete) it once `runAuthLogin` returns.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 7be686a. runAuthLogin is now a thin wrapper that saves the prior CODEX_AUTH_ACCOUNT_ID, sets it only for the org-bound login, and restores or deletes it in a finally, delegating the unchanged flow to runAuthLoginFlow. A later login in the same process or a reused test worker no longer inherits a stale org.
…dices (#491) Address review feedback on the workspace command. - guard `target.id` with optional chaining when building the id suffix, matching formatWorkspaceLines, so non-conforming on-disk data cannot throw. - add tests for non-numeric account and workspace indices.
|
Addressed review feedback in 6fff40b: Fixed
Intentionally not changed, with reasons
Gates after the change: typecheck, lint, build clean; |
The --org flag set process.env.CODEX_AUTH_ACCOUNT_ID without restoring it, so it leaked for the whole process: a later login in the same invocation (menu re-entry) or a reused test worker would silently bind to the stale org. Split runAuthLogin into a thin wrapper that saves the prior override, sets it for the org-bound login, and restores (or deletes) it in a finally, delegating the unchanged flow to runAuthLoginFlow.
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
1 participant
Summary
Closes #491.
A single OpenAI/Google account can belong to multiple ChatGPT workspaces
(personal Plus vs business/team), each a distinct quota pool keyed by its
org_id. This PR makes such workspaces persist, be visible, be switchable, andbe registrable on demand.
Root cause: the login path already captures every workspace from the token
(
getAccountIdCandidates) into one account'sworkspaces[], andpromptAccountSelectionlets the user pick which binds. But the strictAccountMetadataV3Schema(z.object) did not declareworkspaces/currentWorkspaceIndex, so Zod stripped them on every load. Login wrote them todisk; the next read wiped them. That is the reported "workspaceName always empty
even after login", and it also left
listunable to distinguish two same-emailaccounts.
Changes
Persistence + labels
lib/schemas.ts: addWorkspaceSchemaand declareworkspaces/currentWorkspaceIndexonAccountMetadataV3Schemaso workspace trackingsurvives the load round-trip.
lib/accounts.ts: surface the active workspace name informatAccountLabel(for example
Account 1 ([Personal Plus], user@gmail.com, id:g-AAAA)),preserving every existing label format.
Visibility + switching
lib/accounts.ts: addformatWorkspaceLines(one line per workspace, activemarked with
*, disabled flagged).lib/codex-manager/commands/status.ts: understatus/list, list everyworkspace beneath an account that tracks more than one.
lib/codex-manager/commands/workspace.ts(new)workspace <account> [workspace]:list an account's workspaces, or set the active one and persist it. Wired into
the command registry, dispatch, and help.
On-demand registration
lib/codex-manager/help.ts+lib/codex-manager.ts: addlogin --org <org_id>(and
--org=<id>). It reuses theCODEX_AUTH_ACCOUNT_IDoverride that everylogin resolver already honors, so a specific workspace can be bound at login
even when selection would otherwise pick the default org.
Tests
formatWorkspaceLines, nineworkspace-command cases, and--orgparsing.Example
Risk notes
formatAccountLabelis behavior-preserving when no workspaces are tracked;all prior outputs are unchanged.
--orgdoes not change the auth resolvers; it sets the existing override envvar for the invocation, so all three login paths honor it without new wiring.
Verification
npm run typecheck- cleannpm run lint- cleannpm run build- cleannpm test- 4041 passed (269 files)Follow-up to confirm
Whether a real two-workspace login returns both orgs in one token (then the
workspacecommand alone covers switching) or one at a time (then--orgisthe path to add the second). Both are now supported; a sanitized
accounts.jsonfrom a real two-workspace account would confirm which path users hit.
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
fixes the root cause of workspace data loss (strict
z.objectstrippingworkspaces/currentWorkspaceIndexon every load) and builds the full workspace ux on top: persistent schema fields, label disambiguation for same-email accounts,status/listsub-display, a newworkspacecommand for listing and switching, andlogin --orgfor on-demand workspace registration.lib/schemas.ts: addsWorkspaceSchemaand declaresworkspaces/currentWorkspaceIndexonAccountMetadataV3Schema— additive, no migration needed;currentWorkspaceIndexusesz.number()without int/min(0) constraints, which can cause split-brain display if a float or negative lands on disk.lib/codex-manager.ts:--orguses atry/finallyto scopeCODEX_AUTH_ACCOUNT_IDto the invocation and restore it afterward — correct, but the finally-path restore has no vitest coverage.lib/codex-manager/commands/workspace.ts: new command with full validation, disabled-workspace guard, and already-active no-op; inherits the TOCTOU/EBUSY concerns already flagged in the previous review round.Confidence Score: 5/5
safe to merge; changes are additive and behavior-preserving for accounts without workspaces
the schema fix is a straightforward additive field declaration; the workspace command and --org flag are well-guarded; the try/finally env-restore pattern in codex-manager.ts is correct; remaining gaps are test coverage and a minor schema constraint
lib/schemas.ts (currentWorkspaceIndex constraint) and lib/codex-manager.ts (--org env-restore test coverage)
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[codex-multi-auth login --org org-X] --> B[parseAuthLoginArgs] B --> C{org present?} C -- yes --> D[save prev CODEX_AUTH_ACCOUNT_ID] D --> E[set env var to org-X] E --> F[runAuthLoginFlow] F --> G{success / throw} G --> H[finally: restore/delete env var] C -- no --> I[runAuthLoginFlow directly] J[codex-multi-auth workspace N] --> K[loadAccounts] K --> L{workspaces.length > 0?} L -- no --> M[log: no tracked workspaces] L -- yes --> N{workspace arg?} N -- no --> O[formatWorkspaceLines list] N -- yes --> P{valid index?} P -- no --> Q[error: out of range] P -- yes --> R{enabled?} R -- no --> S[error: disabled] R -- yes --> T{already active?} T -- yes --> U[no-op log] T -- no --> V[set currentWorkspaceIndex saveAccountsWithRetry] W[status / list] --> X[formatAccountLabel with workspaceName] X --> Y{workspaces.length > 1?} Y -- yes --> Z[formatWorkspaceLines sub-display]Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "fix(login): scope --org override and res..." | Re-trigger Greptile