fix(accounts): persist account workspaces and surface active workspace in labels (#491 foundation)#492
fix(accounts): persist account workspaces and surface active workspace in labels (#491 foundation)#492ndycode wants to merge 1 commit 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.
|
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 schema definitions to ChangesWorkspace-aware account labeling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // strips workspace tracking on every load, so login-captured workspaces | ||
| // silently vanish after one read/write round-trip. | ||
| workspaces: z.array(WorkspaceSchema).optional(), | ||
| currentWorkspaceIndex: z.number().optional(), |
There was a problem hiding this comment.
currentWorkspaceIndex is declared as z.number(), which accepts any floating-point or negative value. JavaScript array indexing with a float (e.g. 1.5) returns undefined, and activeWorkspaceName then silently falls back to workspaces[0] — so a corrupt persisted value would display the wrong workspace in every label without any validation error. z.number().int().min(0) rejects these at parse time and aligns with the existing disableCurrentWorkspace bounds guard.
| currentWorkspaceIndex: z.number().optional(), | |
| currentWorkspaceIndex: z.number().int().min(0).optional() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/schemas.ts
Line: 187
Comment:
`currentWorkspaceIndex` is declared as `z.number()`, which accepts any floating-point or negative value. JavaScript array indexing with a float (e.g. `1.5`) returns `undefined`, and `activeWorkspaceName` then silently falls back to `workspaces[0]` — so a corrupt persisted value would display the wrong workspace in every label without any validation error. `z.number().int().min(0)` rejects these at parse time and aligns with the existing `disableCurrentWorkspace` bounds guard.
```suggestion
currentWorkspaceIndex: z.number().int().min(0).optional()
```
How can I resolve this? If you propose a fix, please make it concise.
| const idx = account?.currentWorkspaceIndex ?? 0; | ||
| const workspace = workspaces[idx] ?? workspaces[0]; | ||
| return workspace?.name?.trim() || undefined; |
There was a problem hiding this comment.
when currentWorkspaceIndex is stale or out of range, activeWorkspaceName silently falls back to workspaces[0], while getCurrentWorkspace (line 1676) returns null for the same input. this means the account label can show a workspace tag for an account that the manager itself would treat as having no active workspace — misleading in list/status output. suggest aligning: workspaces[idx] ?? null and returning undefined on null, or calling getCurrentWorkspace directly (requires a ManagedAccount reference, but even a bounds check here would avoid the silent mis-label).
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/accounts.ts
Line: 1756-1758
Comment:
**out-of-bounds index fallback inconsistency**
when `currentWorkspaceIndex` is stale or out of range, `activeWorkspaceName` silently falls back to `workspaces[0]`, while `getCurrentWorkspace` (line 1676) returns `null` for the same input. this means the account label can show a workspace tag for an account that the manager itself would treat as having no active workspace — misleading in `list`/`status` output. suggest aligning: `workspaces[idx] ?? null` and returning `undefined` on null, or calling `getCurrentWorkspace` directly (requires a `ManagedAccount` reference, but even a bounds check here would avoid the silent mis-label).
How can I resolve this? If you propose a fix, please make it concise.| /** | ||
| * Name of the account's currently-selected workspace, if any. Lets same-email | ||
| * accounts that live in different workspaces (personal Plus vs business/team) | ||
| * stay distinguishable in `list`/`status` output. See issue #491. | ||
| */ | ||
| function activeWorkspaceName( | ||
| account: | ||
| | { workspaces?: Workspace[]; currentWorkspaceIndex?: number } | ||
| | undefined, | ||
| ): string | undefined { | ||
| const workspaces = account?.workspaces; | ||
| if (!workspaces || workspaces.length === 0) return undefined; | ||
| const idx = account?.currentWorkspaceIndex ?? 0; | ||
| const workspace = workspaces[idx] ?? workspaces[0]; | ||
| return workspace?.name?.trim() || undefined; | ||
| } |
There was a problem hiding this comment.
currentWorkspaceIndex
the new tests cover valid indices (0, 1) and the empty/unnamed workspace paths, but there's no case for an out-of-range index (e.g. currentWorkspaceIndex: 99 with one workspace). the silent fallback to workspaces[0] is undocumented behaviour — a test would pin it intentionally and prevent a future breakage from going unnoticed. without it, the 80% coverage threshold won't catch a regression on this branch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/accounts.ts
Line: 1744-1759
Comment:
**missing vitest coverage for out-of-range `currentWorkspaceIndex`**
the new tests cover valid indices (0, 1) and the empty/unnamed workspace paths, but there's no case for an out-of-range index (e.g. `currentWorkspaceIndex: 99` with one workspace). the silent fallback to `workspaces[0]` is undocumented behaviour — a test would pin it intentionally and prevent a future breakage from going unnoticed. without it, the 80% coverage threshold won't catch a regression on this branch.
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!
| /** | ||
| * Workspace entry within an account. Supports a single OpenAI/Google account | ||
| * that belongs to multiple ChatGPT workspaces (e.g. personal Plus + a | ||
| * business/team workspace), each a distinct quota pool keyed by its org_id | ||
| * (`id`). Mirrors the `Workspace` TS interface in `lib/accounts.ts`. See #491. | ||
| */ | ||
| export const WorkspaceSchema = z.object({ | ||
| id: z.string(), | ||
| name: z.string().optional(), | ||
| enabled: z.boolean(), | ||
| disabledAt: z.number().optional(), | ||
| isDefault: z.boolean().optional(), | ||
| }); | ||
|
|
||
| export type WorkspaceFromSchema = z.infer<typeof WorkspaceSchema>; |
There was a problem hiding this comment.
WorkspaceFromSchema and Workspace interface are parallel, drift-prone types
the comment says "mirrors the Workspace TS interface in lib/accounts.ts" — but there's no structural link between them. if a field is added to Workspace (e.g. a new role field) without updating WorkspaceSchema, Zod will silently strip it on every load, which is exactly the bug this PR fixes. consider exporting WorkspaceFromSchema and using it as (or in place of) the Workspace interface, or at minimum adding a compile-time assignability check (const _: Workspace = {} as WorkspaceFromSchema) to catch drift at build time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/schemas.ts
Line: 149-163
Comment:
**`WorkspaceFromSchema` and `Workspace` interface are parallel, drift-prone types**
the comment says "mirrors the `Workspace` TS interface in `lib/accounts.ts`" — but there's no structural link between them. if a field is added to `Workspace` (e.g. a new `role` field) without updating `WorkspaceSchema`, Zod will silently strip it on every load, which is exactly the bug this PR fixes. consider exporting `WorkspaceFromSchema` and using it as (or in place of) the `Workspace` interface, or at minimum adding a compile-time assignability check (`const _: Workspace = {} as WorkspaceFromSchema`) to catch drift at build time.
How can I resolve this? If you propose a fix, please make it concise.
ndycode
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/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 `@lib/schemas.ts`:
- Around line 155-160: Workspace schema and persisted index validation are too
permissive: change WorkspaceSchema.id to a nonempty string (e.g.,
z.string().min(1)) and tighten the persisted currentWorkspaceIndex schema to
require an integer >= 0 (e.g., z.number().int().nonnegative()) so empty IDs,
negative or fractional indexes are rejected at load time; update the tests to
add Vitest regressions that assert storage load rejects workspace.id === "" and
rejects negative/non-integer currentWorkspaceIndex, and add the suggested
Windows and concurrency regression tests to ensure tightening doesn’t break
multi-process read/write flows.
In `@test/schemas.test.ts`:
- Around line 236-271: Add a negative regression test that asserts
AccountMetadataV3Schema rejects malformed currentWorkspaceIndex values (e.g., -1
and 1.5) to lock the boundary; create a vitest case similar to the existing
"rejects a workspace missing its id" test that calls
AccountMetadataV3Schema.safeParse with validAccount plus workspaces and
currentWorkspaceIndex set to -1 and another case set to 1.5 (or loop both), and
assert result.success is false so invalid indices are rejected.
🪄 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: 205b054a-0584-4d96-8251-fecdef3f1377
📒 Files selected for processing (4)
lib/accounts.tslib/schemas.tstest/accounts.test.tstest/schemas.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/accounts.tstest/schemas.test.tslib/schemas.tstest/accounts.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errorTypeScript assertions
Files:
lib/accounts.tstest/schemas.test.tslib/schemas.tstest/accounts.test.ts
**/lib/accounts.ts
📄 CodeRabbit inference engine (AGENTS.md)
Email dedup is case-insensitive via
normalizeEmailKey()(trim + lowercase)
Files:
lib/accounts.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/accounts.tslib/schemas.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
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/accounts.tslib/schemas.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/schemas.test.tstest/accounts.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/schemas.test.tstest/accounts.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/schemas.test.tstest/accounts.test.ts
| export const WorkspaceSchema = z.object({ | ||
| id: z.string(), | ||
| name: z.string().optional(), | ||
| enabled: z.boolean(), | ||
| disabledAt: z.number().optional(), | ||
| isDefault: z.boolean().optional(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# locate relevant schema + normalization
rg -n "WorkspaceSchema|WorkspaceFromSchema|currentWorkspaceIndex|workspaces" lib -S || true
rg -n "normalizeAccountStorage" lib -S || true
# inspect normalizeAccountStorage implementation + surrounding code
FILE="$(rg -l "function normalizeAccountStorage|normalizeAccountStorage" lib -S | head -n 1 || true)"
if [ -n "${FILE}" ]; then
echo "=== normalizeAccountStorage file: $FILE ==="
sed -n '1,260p' "$FILE" | cat -n
# also print any section mentioning workspaces/currentWorkspaceIndex
rg -n "workspaces|currentWorkspaceIndex" "$FILE" -n -S || true
rg -n "return|accounts" "$FILE" -n -S | head -n 80 || true
fi
# find tests referencing workspaces/currentWorkspaceIndex
rg -n "currentWorkspaceIndex|workspace.*index|workspaces" test -S --glob='**/*.{ts,tsx,js,mjs}' || true
rg -n "schemas\.ts|WorkspaceSchema" test -S || true
# inspect lib/schemas.ts around the reported lines
SCHEMAS="lib/schemas.ts"
if [ -f "$SCHEMAS" ]; then
echo "=== lib/schemas.ts excerpt ==="
sed -n '120,230p' "$SCHEMAS" | cat -n
fiRepository: ndycode/codex-multi-auth
Length of output: 33115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# inspect normalizeAccountStorage in lib/storage.ts
rg -n "function normalizeAccountStorage|normalizeAccountStorage\s*\(" lib/storage.ts
rg -n "workspaces|currentWorkspaceIndex" lib/storage.ts
# show normalizeAccountStorage body (targeted ranges around the definition)
sed -n '1160,1320p' lib/storage.ts | cat -n
sed -n '1320,1500p' lib/storage.ts | cat -n
# show any helper that parses V3 workspace fields (if present)
rg -n "AccountMetadataV3Schema|WorkspaceSchema|currentWorkspaceIndex|workspaces" lib/schemas.ts
# check for tests covering invalid workspace id / index values
rg -n "min\\(1\\)|empty|''|currentWorkspaceIndex: -|currentWorkspaceIndex.*-1|currentWorkspaceIndex.*0\\.|workspaces.*id.*''" test -S || true
rg -n "currentWorkspaceIndex.*(negative|fraction|float|non-?int|int\\(\\))" test -S || true
rg -n "WorkspaceSchema|WorkspaceSchema.*error|zod|safeParse|AnyAccountStorageSchema" test/schemas.test.ts -S
# show relevant test blocks around `#491` and clamp test
sed -n '200,310p' test/schemas.test.ts | cat -n
sed -n '690,880p' test/schemas.test.ts | cat -n
rg -n "clamps currentWorkspaceIndex" test/index.test.ts -n
sed -n '4320,4415p' test/index.test.ts | cat -n
# check label/index selection behavior for out-of-range indices (runtime guardrails)
sed -n '1620,1760p' lib/accounts.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 1970
tighten workspace schema constraints to prevent malformed persisted state
lib/schemas.ts:156usesz.string()forWorkspaceSchema.id, so""passes.lib/schemas.ts:186-187usesz.number()forcurrentWorkspaceIndex, so negative/fractional values can persist and later affect workspace selection (runtime guards only handle out-of-range, not non-integer indexes, e.g.lib/accounts.ts:1686-1690).- tests cover preserving workspaces/index across load (
test/schemas.test.ts:236-268,test/schemas.test.ts:717-848) and clamping when refreshed metadata removes the active workspace (test/index.test.ts:4335-4410), but not rejecting malformed persisted values.
proposed fix
export const WorkspaceSchema = z.object({
- id: z.string(),
+ id: z.string().min(1),
name: z.string().optional(),
enabled: z.boolean(),
disabledAt: z.number().optional(),
isDefault: z.boolean().optional(),
});
@@
- workspaces: z.array(WorkspaceSchema).optional(),
- currentWorkspaceIndex: z.number().optional(),
+ workspaces: z.array(WorkspaceSchema).optional(),
+ currentWorkspaceIndex: z.number().int().min(0).optional(),
});- add vitest regression asserting storage load rejects
workspace.id === ""andcurrentWorkspaceIndexthat is negative or non-integer. - add windows regression for reading/writing persisted storage with invalid workspace fields and a concurrency regression for partial persist/load so schema tightening doesn’t destabilize multi-process flows.
🤖 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/schemas.ts` around lines 155 - 160, Workspace schema and persisted index
validation are too permissive: change WorkspaceSchema.id to a nonempty string
(e.g., z.string().min(1)) and tighten the persisted currentWorkspaceIndex schema
to require an integer >= 0 (e.g., z.number().int().nonnegative()) so empty IDs,
negative or fractional indexes are rejected at load time; update the tests to
add Vitest regressions that assert storage load rejects workspace.id === "" and
rejects negative/non-integer currentWorkspaceIndex, and add the suggested
Windows and concurrency regression tests to ensure tightening doesn’t break
multi-process read/write flows.
| it("preserves workspaces and currentWorkspaceIndex (#491)", () => { | ||
| const account = { | ||
| ...validAccount, | ||
| workspaces: [ | ||
| { | ||
| id: "org-AAAA", | ||
| name: "Personal Plus", | ||
| enabled: true, | ||
| isDefault: true, | ||
| }, | ||
| { | ||
| id: "org-BBBB", | ||
| name: "GkTech Business", | ||
| enabled: false, | ||
| disabledAt: 123, | ||
| }, | ||
| ], | ||
| currentWorkspaceIndex: 1, | ||
| }; | ||
| const result = AccountMetadataV3Schema.safeParse(account); | ||
| expect(result.success).toBe(true); | ||
| if (result.success) { | ||
| expect(result.data.workspaces).toHaveLength(2); | ||
| expect(result.data.workspaces?.[0]?.name).toBe("Personal Plus"); | ||
| expect(result.data.workspaces?.[1]?.enabled).toBe(false); | ||
| expect(result.data.currentWorkspaceIndex).toBe(1); | ||
| } | ||
| }); | ||
|
|
||
| it("rejects a workspace missing its id (#491)", () => { | ||
| const result = AccountMetadataV3Schema.safeParse({ | ||
| ...validAccount, | ||
| workspaces: [{ name: "Personal Plus", enabled: true }], | ||
| }); | ||
| expect(result.success).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
add a regression case for invalid currentWorkspaceIndex values
these new tests cover missing workspace.id, but they do not assert rejection of malformed currentWorkspaceIndex (for example -1 or 1.5). please add one negative test so this boundary stays locked.
As per coding guidelines "test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
Also applies to: 820-849
🤖 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 `@test/schemas.test.ts` around lines 236 - 271, Add a negative regression test
that asserts AccountMetadataV3Schema rejects malformed currentWorkspaceIndex
values (e.g., -1 and 1.5) to lock the boundary; create a vitest case similar to
the existing "rejects a workspace missing its id" test that calls
AccountMetadataV3Schema.safeParse with validAccount plus workspaces and
currentWorkspaceIndex set to -1 and another case set to 1.5 (or loop both), and
assert result.success is false so invalid indices are rejected.
1 participant
Summary
Foundation for #491 (does not fully close it - see "Remaining" below).
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. The login path already captures every workspace from the token(
getAccountIdCandidates) into one account'sworkspaces[]array, andpromptAccountSelectionlets the user pick which one binds. The breakage wasthat this data did not survive and was not visible:
AccountMetadataV3Schema(z.object) did not declareworkspaces/currentWorkspaceIndex, so Zod stripped them on every load.Login wrote them to disk; the next read wiped them. This is the root cause of
the reported "workspaceName always empty even after login".
formatAccountLabelonly rendered email and id-suffix, so two same-emailaccounts were hard to tell apart in
list/status.Changes
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. The id is bare when alone and
prefixed with
id:once another segment precedes it.label disambiguation coverage.
Remaining for full #491 (tracked for a follow-up PR)
list/status(show all workspaces under anaccount with an active marker), so both workspaces are visible at once.
on rate-limit / disabled errors).
login --org <id>flag, only if a real two-workspace login is found toreturn a single org at a time (current code path captures all orgs in one
login, so this may be unnecessary).
Risk notes
formatAccountLabeloutputs are unchanged (existing tests still pass).id(matches theWorkspaceinterface);malformed entries are rejected by validation, and the raw-normalize fallback
still surfaces such files via
schemaErrors.Verification
npm run typecheck- cleannpm run lint- cleannpm run build- cleannpm test- 4027 passed (268 files)