-
Notifications
You must be signed in to change notification settings - Fork 32
fix(accounts): persist account workspaces and surface active workspace in labels (#491 foundation) #492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(accounts): persist account workspaces and surface active workspace in labels (#491 foundation) #492
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1741,13 +1741,37 @@ export class AccountManager { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * 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; | ||
| } | ||
|
Comment on lines
+1744
to
+1759
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Prompt To Fix With AIThis 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! |
||
|
|
||
| export function formatAccountLabel( | ||
| account: | ||
| | { email?: string; accountId?: string; accountLabel?: string } | ||
| | { | ||
| email?: string; | ||
| accountId?: string; | ||
| accountLabel?: string; | ||
| workspaces?: Workspace[]; | ||
| currentWorkspaceIndex?: number; | ||
| } | ||
| | undefined, | ||
| index: number, | ||
| ): string { | ||
| const accountLabel = account?.accountLabel?.trim(); | ||
| const workspaceName = activeWorkspaceName(account); | ||
| const email = account?.email?.trim(); | ||
| const accountId = account?.accountId?.trim(); | ||
| const idSuffix = accountId | ||
|
|
@@ -1756,19 +1780,23 @@ export function formatAccountLabel( | |
| : accountId | ||
| : null; | ||
|
|
||
| if (accountLabel && email && idSuffix) { | ||
| return `Account ${index + 1} (${accountLabel}, ${email}, id:${idSuffix})`; | ||
| } | ||
| if (accountLabel && email) | ||
| return `Account ${index + 1} (${accountLabel}, ${email})`; | ||
| if (accountLabel && idSuffix) | ||
| return `Account ${index + 1} (${accountLabel}, id:${idSuffix})`; | ||
| if (accountLabel) return `Account ${index + 1} (${accountLabel})`; | ||
| if (email && idSuffix) | ||
| return `Account ${index + 1} (${email}, id:${idSuffix})`; | ||
| if (email) return `Account ${index + 1} (${email})`; | ||
| if (idSuffix) return `Account ${index + 1} (${idSuffix})`; | ||
| return `Account ${index + 1}`; | ||
| const segments: string[] = []; | ||
| if (accountLabel) segments.push(accountLabel); | ||
| // Surface the active workspace so two same-email accounts in different | ||
| // workspaces remain distinguishable; skip it when it would just repeat the | ||
| // manual account label. | ||
| if (workspaceName && workspaceName !== accountLabel) { | ||
| segments.push(`[${workspaceName}]`); | ||
| } | ||
| if (email) segments.push(email); | ||
| // A bare id stands alone (e.g. "Account 1 (123456)"); once any other | ||
| // segment precedes it, prefix with "id:" for clarity. | ||
| if (idSuffix) { | ||
| segments.push(segments.length > 0 ? `id:${idSuffix}` : idSuffix); | ||
| } | ||
|
|
||
| if (segments.length === 0) return `Account ${index + 1}`; | ||
| return `Account ${index + 1} (${segments.join(", ")})`; | ||
| } | ||
|
|
||
| export function formatCooldown( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -146,6 +146,22 @@ export const RateLimitStateV3Schema = z.record( | |||||
|
|
||||||
| export type RateLimitStateV3FromSchema = z.infer<typeof RateLimitStateV3Schema>; | ||||||
|
|
||||||
| /** | ||||||
| * 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(), | ||||||
|
Comment on lines
+155
to
+160
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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
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(),
});
🤖 Prompt for AI Agents |
||||||
| }); | ||||||
|
|
||||||
| export type WorkspaceFromSchema = z.infer<typeof WorkspaceSchema>; | ||||||
|
Comment on lines
+149
to
+163
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
the comment says "mirrors the Prompt To Fix With AIThis 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. |
||||||
|
|
||||||
| /** | ||||||
| * Account metadata V3 - current storage format. | ||||||
| */ | ||||||
|
|
@@ -164,6 +180,11 @@ export const AccountMetadataV3Schema = z.object({ | |||||
| rateLimitResetTimes: RateLimitStateV3Schema.optional(), | ||||||
| coolingDownUntil: z.number().optional(), | ||||||
| cooldownReason: CooldownReasonSchema.optional(), | ||||||
| // Multi-workspace support (#491): without these here, the strict z.object | ||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis 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. |
||||||
| }); | ||||||
|
|
||||||
| export type AccountMetadataV3FromSchema = z.infer< | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -232,6 +232,43 @@ describe("AccountMetadataV3Schema", () => { | |
| }); | ||
| expect(result.success).toBe(false); | ||
| }); | ||
|
|
||
| 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); | ||
| }); | ||
|
Comment on lines
+236
to
+271
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win add a regression case for invalid these new tests cover missing 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 |
||
| }); | ||
|
|
||
| describe("AccountStorageV3Schema", () => { | ||
|
|
@@ -779,6 +816,37 @@ describe("safeParseJson", () => { | |
| expect(result).not.toBeNull(); | ||
| expect(result?.version).toBe(3); | ||
| }); | ||
|
|
||
| it("keeps workspaces through the load round-trip (#491)", () => { | ||
| // Regression: the strict z.object used to strip workspace tracking on | ||
| // every load, so login-captured workspaces vanished after one reload. | ||
| const raw = JSON.stringify({ | ||
| version: 3, | ||
| accounts: [ | ||
| { | ||
| refreshToken: "rt", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| accountId: "org-AAAA", | ||
| email: "user@gmail.com", | ||
| workspaces: [{ id: "org-AAAA", name: "Personal Plus", enabled: true }], | ||
| currentWorkspaceIndex: 0, | ||
| }, | ||
| ], | ||
| activeIndex: 0, | ||
| }); | ||
| const result = safeParseJson(raw, AnyAccountStorageSchema, "test.workspaces"); | ||
| expect(result).not.toBeNull(); | ||
| const account = result?.accounts?.[0] as | ||
| | { | ||
| workspaces?: Array<{ name?: string }>; | ||
| currentWorkspaceIndex?: number; | ||
| } | ||
| | undefined; | ||
| expect(account?.workspaces).toHaveLength(1); | ||
| expect(account?.workspaces?.[0]?.name).toBe("Personal Plus"); | ||
| expect(account?.currentWorkspaceIndex).toBe(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("safeParseFlaggedAccountStorageV1", () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when
currentWorkspaceIndexis stale or out of range,activeWorkspaceNamesilently falls back toworkspaces[0], whilegetCurrentWorkspace(line 1676) returnsnullfor 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 inlist/statusoutput. suggest aligning:workspaces[idx] ?? nulland returningundefinedon null, or callinggetCurrentWorkspacedirectly (requires aManagedAccountreference, but even a bounds check here would avoid the silent mis-label).Prompt To Fix With AI