feat: auto-rotate on disabled/expired workspace errors#136
Conversation
…witching Adds a new CLI command that automatically switches to the best standing account based on the forecast algorithm. Supports --live, --json, and --model flags. - Add 'best' to AUTH_SUBCOMMANDS in codex-routing.js - Implement runBest() function in codex-manager.ts - Add 'best' switch reason to schemas and types - Update help text and feature list Usage: codex auth best [--live] [--json] [--model <model>]
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughthis pr introduces multi-workspace support to oauth token refresh and request retry logic. accounts now track workspace metadata with enabled/disabled states and current workspace index. on detecting workspace-disabled 403 errors, the request handler rotates to alternative workspaces within the same account before retrying, falling back to other accounts if exhausted. Changes
Sequence DiagramsequenceDiagram
participant Client as Request Handler
participant AM as AccountManager
participant FH as Fetch Helpers
rect rgba(200, 150, 100, 0.5)
Note over Client,FH: Workspace Rotation Flow (403 Disabled)
Client->>FH: handleErrorResponse(response)
FH->>FH: isWorkspaceDisabledError(403, code, body)
FH-->>Client: { response, errorBody }
Client->>AM: rotateToNextWorkspace(account)
AM-->>Client: Workspace | null
alt Workspace available
Client->>Client: Rebuild headers with new workspaceId
Client->>Client: Retry request
else Exhausted
Client->>AM: disableCurrentWorkspace(account)
Client->>AM: setAccountEnabled(account, false)
Client->>Client: Continue to next account
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Senior Review Notescritical blocker: state management risks:
logic review gaps:
missing regression coverage:
test quality observations:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage/migrations.ts (1)
28-53:⚠️ Potential issue | 🟡 Minorupdate the disk reader before persisting
"best".
lib/storage/migrations.ts:28-53andlib/schemas.ts:85now accept"best", butlib/storage.ts:1829-1862still strips anything outside"rate-limit" | "initial" | "rotation"on load. that means a migrated or newly saved"best"value round-trips back toundefinedafter the next reload. please update that validator and add a small round-trip vitest for the storage path.As per coding guidelines,
lib/**: 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.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 1968-1970: The warning currently emits raw account.email in
logWarn (and passes the same raw text in the structured payload); remove any
direct use of account.email in that log call and instead log only the account
index (account.index + 1) or a masked identifier (e.g., last N chars or a masked
suffix produced by a helper like maskEmailSuffix) and ensure the structured
payload does not contain the unmasked email or full upstream text; update the
logWarn invocation that references account.email so it only includes the safe
identifier and keep errorCode/errorMessage but scrub any embedded PII before
passing them.
In `@lib/codex-manager.ts`:
- Around line 4123-4148: When queuedRefresh(bestAccount.refreshToken) returns a
non-"success" result, add a warning log similar to runSwitch's behavior instead
of silently continuing: detect refreshResult.type !== "success" after the
queuedRefresh call in the block guarding hasUsableAccessToken, and call
console.warn with a message containing bestAccount (or parsed identifier) and
normalizeFailureDetail(refreshResult.message, refreshResult.reason) so callers
are informed the silent refresh failed before sync; keep existing success-path
assignments to syncAccessToken/syncRefreshToken/syncExpiresAt/syncIdToken
unchanged.
- Around line 4105-4162: There is duplicated account-switching logic between the
block starting at the duplicated section and runSwitch; extract a shared helper
(e.g., performAccountSwitch(storage, targetIndex, reason)) that encapsulates:
setting storage.activeIndex and storage.activeIndexByFamily (MODEL_FAMILIES),
enabling the account if disabled, refreshing tokens via hasUsableAccessToken +
queuedRefresh, syncing fields on bestAccount (accessToken, refreshToken,
expiresAt, email, accountId, accountIdSource, lastUsed, lastSwitchReason),
calling saveAccounts(storage), and invoking setCodexCliActiveSelection with the
final tokens and idToken; replace both the current block and the runSwitch logic
(the code around runSwitch) to call performAccountSwitch with appropriate
arguments (targetIndex, "best" or other reason) to ensure a single source of
truth.
- Around line 4024-4055: The for-loop iterating storage.accounts is indented one
level too deep creating inconsistent formatting; re-indent the entire loop block
starting at the for (let i = 0; i < storage.accounts.length; i += 1) line so its
body aligns with surrounding code, preserving all logic (checks using
account.enabled, hasUsableAccessToken(account, now),
queuedRefresh(account.refreshToken), extractAccountId, fetchCodexQuotaSnapshot,
and updates to liveQuotaByIndex and refreshFailures) but removing the extra
leading indentation introduced by the copy-paste.
- Around line 3981-4180: The runBest CLI flow (function runBest) lacks
integration test coverage and callers of queuedRefresh need verification for
429/EBUSY transient handling; add a new integration test file
test/codex-manager-cli.test.ts that exercises runBest end-to-end (live probing
via fetchCodexQuotaSnapshot, handling refresh failures, re-enabling disabled
accounts, switching/sync behavior via setCodexCliActiveSelection, JSON and human
outputs, and cases where no recommendation is available) and assert proper
outputs and account state updates; separately, update
refresh-queue.ts/queuedRefresh to explicitly handle 429 and EBUSY transient
errors with retry/backoff and ensure callers (runBest and other queuedRefresh
callers) correctly interpret transient vs permanent failures (TokenFailure), and
add unit tests that simulate 429/EBUSY to validate deduplication, lease
coordination, and backoff behavior.
In `@lib/request/fetch-helpers.ts`:
- Around line 268-309: The isWorkspaceDisabledError function currently treats
numeric/non-string codes and overly-broad billing/payment messages as
terminal—fix by first normalizing the code safely (e.g., const normCode = typeof
code === "string" ? code.toLowerCase() : "") and use exact-match or full-token
checks (not substring includes) against a tightened workspaceErrorCodes list
(e.g., match "workspace_disabled", "workspace_expired", "workspace_terminated",
"account_disabled", "account_expired", "organization_disabled" only); remove or
narrow generic billing/payment entries (or require they co-occur with
"workspace" or "account" in haystack) and tighten regexes in disabledPatterns to
avoid matching plan/billing notices; update tests in test/fetch-helpers.test.ts
to cover numeric codes and billing 403 scenarios and ensure no regressions.
In `@test/fetch-helpers.test.ts`:
- Around line 316-359: Add a deterministic vitest that exercises the actual
auto-rotate path (not just isWorkspaceDisabledError): simulate at least two
accounts, trigger a 403+workspace-disabled response so the code path in index.ts
(the auto-rotate/rotation loop around lines ~1958-1986) marks the current
account enabled === false, goes through the debounced persistence path (the
save/persist debounced function used by the rotation logic), clears session
affinity, and then attempts the next account; assert that enabled === false was
persisted and that the rotation moved to the next account. Use vitest
timers/promises to control debounce and make the test deterministic (do not mock
real secrets, ensure assertions are present).
🪄 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: 8de9776b-930e-428d-8c93-315ed87c20a2
📒 Files selected for processing (10)
index.tslib/accounts.tslib/codex-manager.tslib/request/fetch-helpers.tslib/schemas.tslib/storage/migrations.tsscripts/codex-multi-auth.jsscripts/codex-routing.jsscripts/codex.jstest/fetch-helpers.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 (2)
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.tslib/storage/migrations.tslib/request/fetch-helpers.tslib/codex-manager.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/fetch-helpers.test.ts
🔇 Additional comments (4)
lib/codex-manager.ts (4)
295-295: lgtm - help text accurately reflects the exposed flags.the command always uses live probing internally, so not exposing
--liveis a reasonable simplification.
357-357: lgtm - feature entry looks good.
4301-4303: lgtm - dispatch wiring follows existing patterns.
4152-4152: "best" is a validlastSwitchReasonvalue — confirmed inlib/schemas.ts:85andlib/accounts.ts:94. the value is explicitly included in the union type and zod schema alongside "rate-limit", "initial", and "rotation".
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 2000-2001: The call to
accountManager.disableCurrentWorkspace(account) can redundantly disable a
workspace if another request already did so; update disableCurrentWorkspace to
first check the current workspace's enabled flag and return early (or return a
boolean) when it's already false, perform the state change and persistence only
if enabled was true, and ensure rotateToNextWorkspace() is invoked only when
disableCurrentWorkspace actually changed state (or make rotateToNextWorkspace
resilient to a no-op); reference the disableCurrentWorkspace and
rotateToNextWorkspace methods and the account/accountManager interaction when
applying the change.
- Around line 1982-2039: Concurrent requests can race when mutating workspace
state via disableCurrentWorkspace(account) and rotateToNextWorkspace(account) on
a shared ManagedAccount; wrap the whole disable+rotate+save sequence in a
per-account lock (e.g., add and use an
accountManager.acquireWorkspaceLock(account.index)/releaseWorkspaceLock) or at
minimum re-check the workspace state after acquiring a short mutex and before
calling disableCurrentWorkspace to ensure it hasn't already been disabled, then
call rotateToNextWorkspace and saveToDiskDebounced under the lock; update the
code path that handles isWorkspaceDisabledError to acquire the lock, perform the
conditional check + disable + rotate + save, release the lock, and only then
continue/disable account/sessionAffinityStore as needed so in-memory mutations
are serialized.
In `@lib/accounts.ts`:
- Around line 876-881: Add a brief doc comment above the hasEnabledWorkspaces
method explaining that when account.workspaces is undefined or an empty array
the method returns true for backwards compatibility by assuming a single
implicit workspace, and that the method otherwise checks for any workspace where
enabled !== false; reference the function name hasEnabledWorkspaces and keep the
comment concise (one or two sentences) to clarify the non-obvious default
behavior.
- Around line 841-854: In disableCurrentWorkspace, replace the direct Date.now()
call with the utility nowMs() so the function uses the same time source as the
rest of the module; update the assignment workspace.disabledAt = nowMs() in the
disableCurrentWorkspace method (ensure the module already imports nowMs from
utils.js or add that import if missing).
- Around line 822-890: Add vitest unit tests in test/accounts.test.ts that cover
the seven new workspace methods: write tests for setWorkspaces and
getCurrentWorkspace including undefined currentWorkspaceIndex, getWorkspaceCount
and getEnabledWorkspaceCount for empty and populated lists,
disableCurrentWorkspace behavior and that disabledAt is set,
rotateToNextWorkspace with mixed enabled/disabled workspaces and when no enabled
workspace exists, and hasEnabledWorkspaces for tracked vs untracked accounts;
include edge-case tests for out-of-bounds currentWorkspaceIndex (negative and >=
length) to ensure methods return null/false as expected; and add a test
simulating concurrent updates to account.currentWorkspaceIndex (e.g., rapid
sequential updates or Promise.all modifications to mimic Windows filesystem
race) to assert deterministic rotation behavior—use the functions setWorkspaces,
rotateToNextWorkspace, disableCurrentWorkspace, getCurrentWorkspace,
hasEnabledWorkspaces, getWorkspaceCount, and getEnabledWorkspaceCount to locate
the code under test.
In `@lib/storage/migrations.ts`:
- Around line 11-17: The Workspace interface is duplicated; export the single
canonical Workspace interface from the persistence-focused module where it
currently exists and remove the duplicate declaration in the other module,
replacing it with an import of that exported Workspace type; update any local
references to use the imported Workspace so only one definition is maintained
and exported for reuse.
In `@test/index-retry.test.ts`:
- Around line 110-117: The test suite is missing regression tests for the
workspace-disabled (403) retry path and the mocks are contradictory
(getCurrentWorkspace() returns null while hasEnabledWorkspaces() returns true);
add tests in test/index-retry.test.ts that (1) simulate a 403 with error code
"workspace-disabled" and assert disableCurrentWorkspace() and
rotateToNextWorkspace() are called, (2) simulate repeated workspace-disabled
responses until no workspaces remain and assert setAccountEnabled(false) is
called, and (3) simulate concurrent requests receiving workspace-disabled to
surface race conditions between rotateToNextWorkspace() and
disableCurrentWorkspace(); also fix the mock state by making
getCurrentWorkspace() return a valid workspace object (with id/name) when
hasEnabledWorkspaces() is true, or set hasEnabledWorkspaces() to false when
getCurrentWorkspace() is null so the mocked state is consistent. Ensure tests
spy/await the async retry logic in index.ts so the calls are observed.
🪄 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: 326ca737-54ca-4e49-901c-2200ed53acaa
📒 Files selected for processing (4)
index.tslib/accounts.tslib/storage/migrations.tstest/index-retry.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 (2)
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/index-retry.test.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/storage/migrations.tslib/accounts.ts
🔇 Additional comments (7)
index.ts (3)
1995-1997: pii emission in log - flagged in previous review.
account.emailis still emitted in the logWarn call. this was flagged in a past review comment. the suggested fix was to use masked account reference instead.
1576-1585: lgtm - workspace id resolution fallback.the code correctly falls back to
account.accountIdwhen no workspace is configured. this handles migrated accounts that don't have workspaces populated yet.
625-640: workspace merge preserves enabled flag correctly.the merge logic at lines 626-629 preserves the existing workspace's
enabledstatus while updating other fields from the new workspace data. this prevents re-enabling a disabled workspace during token refresh.lib/accounts.ts (2)
82-88: duplicate workspace interface.this interface is duplicated at
lib/storage/migrations.ts:11-17. see comment there for consolidation suggestion.
856-874: lgtm - rotation logic is correct.
rotateToNextWorkspace()correctly searches for the next enabled workspace starting fromcurrentIdx + 1, wrapping around. returns null when all workspaces are disabled.lib/storage/migrations.ts (2)
36-36: lgtm - switch reason extended.adding
"best"tolastSwitchReasonaligns with the cliauth bestsubcommand inscripts/codex-routing.js.
65-66: workspace fields added to v3 schema but not initialized in migration.
migrateV1ToV3at lines 80-113 doesn't initializeworkspacesorcurrentWorkspaceIndex. this is acceptable since the workspace methods inlib/accounts.ts:833-890handle undefined gracefully (e.g.,hasEnabledWorkspacesreturnstruewhen no workspaces tracked). just noting for clarity.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
index.ts (2)
1577-1585:⚠️ Potential issue | 🟠 Majorbuild the entitlement key from the current request workspace.
after a rotate in
index.ts:1987-2011,account.accountIdstill holds the previous workspace whenindex.ts:1579-1584runs. that makes the first request on the new workspace record entitlement blocks, capability failures, and quota state under the old workspace id. useaccountIdfor the key, or assignaccount.accountId = accountIdbefore deriving it.suggested fix
- const entitlementAccountKey = resolveEntitlementAccountKey({ - accountId: account.accountId ?? accountId, + account.accountId = accountId; + const entitlementAccountKey = resolveEntitlementAccountKey({ + accountId, email: resolvedEmail, refreshToken: account.refreshToken, index: account.index, }); - account.accountId = accountId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 1577 - 1585, The entitlement key is being built using stale account.accountId (which may still be the previous workspace after rotation) causing entitlement/quota to be recorded under the wrong workspace; update the code that computes entitlementAccountKey (the call to resolveEntitlementAccountKey) to use the current request workspace id (accountId) instead of account.accountId, or assign account.accountId = accountId before calling resolveEntitlementAccountKey; ensure you continue to pass resolvedEmail (from extractAccountEmail(accountAuth.access) ?? account.email), account.refreshToken and account.index unchanged.
559-571:⚠️ Potential issue | 🟠 Majorpersist the selected workspace index with new multi-workspace accounts.
selectBestAccountCandidate()can choose a non-zero slot inindex.ts:424-435, butindex.ts:559-571only savesworkspaces.lib/accounts.ts:872-878then falls back to slot0, so the first request after login or restore can send the wrong workspace id and rewriteaccount.accountIdto the wrong workspace. seedcurrentWorkspaceIndexfrom the chosenaccountIdwhen you persist the row.suggested fix
accounts.push({ accountId, accountIdSource, accountLabel, email: accountEmail, refreshToken: result.refresh, accessToken: result.access, expiresAt: result.expires, addedAt: now, lastUsed: now, workspaces: result.workspaces, + currentWorkspaceIndex: + result.workspaces && result.workspaces.length > 0 + ? Math.max( + 0, + result.workspaces.findIndex((workspace) => workspace.id === accountId), + ) + : undefined, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 559 - 571, selectBestAccountCandidate can pick a non-zero workspace slot but the new account row you push into accounts (in the accounts.push block in index.ts) only stores workspaces, so lib/accounts.ts falls back to slot 0; fix by persisting the chosen workspace index into the new account object: when creating the account entry in the accounts.push (index.ts), add a currentWorkspaceIndex (or existing field used by lib/accounts.ts) seeded from the index returned/used by selectBestAccountCandidate so subsequent requests use the correct workspace and do not rewrite accountId to the wrong workspace.lib/accounts.ts (1)
763-772:⚠️ Potential issue | 🟠 Majorthe workspace reset invariant is still bypassed on manual re-enable.
lib/accounts.ts:763-772only runs when callers usesetAccountEnabled(), butindex.ts:3261-3266still flipstarget.enableddirectly in the manage flow. re-enabling an exhausted account from that path leaves every workspace disabled on disk, so the account comes back already exhausted. route that path through this helper, or move the reset into the persistence/update path that all enable toggles share.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/accounts.ts` around lines 763 - 772, The bug: re-enabling an account via the manage flow flips target.enabled directly and bypasses setAccountEnabled(), so resetWorkspaces(account) is never called and workspaces remain disabled; fix by ensuring all enable/disable toggles go through the single helper or a common persistence/update hook. Concretely, replace the direct assignment of target.enabled in the manage flow with a call to setAccountEnabled(index, true/false) (or call resetWorkspaces(target) immediately after any direct set), or alternatively move the resetWorkspaces logic into the shared persistence/update routine used by all enable toggles so that any path that changes account.enabled invokes resetWorkspaces(account). Ensure you reference setAccountEnabled and resetWorkspaces when updating the manage flow to avoid skipping the workspace reset.
♻️ Duplicate comments (1)
test/index-retry.test.ts (1)
357-535: 🛠️ Refactor suggestion | 🟠 Majoradd the concurrent workspace-disabled regression.
the new cases in
test/index-retry.test.ts:357-535close the single-request paths, but they still only drive one request at a time. the changed branch inindex.ts:1987-2028mutates shared workspace state on a cachedManagedAccount, so we still need a deterministicPromise.allor barrier-based case that sends two 403 workspace-disabled responses through the same account and proves we do not double-disable or disable the account prematurely. 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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index-retry.test.ts` around lines 357 - 535, Add a deterministic concurrent regression test to test/index-retry.test.ts that simulates two simultaneous workspace-disabled 403 responses against the same ManagedAccount to ensure shared workspace state isn't mutated twice: create an account with a single workspace, stub globalThis.fetch (or fetchMock) to return two 403 workspace_disabled Responses in sequence, import OpenAIAuthPlugin and loader as in existing tests, then call Promise.all([sdk.fetch(url, {}), sdk.fetch(url, {})]) to issue the two requests concurrently and await both; assert response statuses, that accountManagerState.disableCurrentWorkspaceCalls === 1, accountManagerState.setAccountEnabledCalls contains a single { index: X, enabled: false } entry (no double-disable), and that rotateToNextWorkspaceCalls is correct — follow existing test patterns (e.g., reusing createMockAccount, OpenAIAuthPlugin, plugin.auth.loader, accountManagerState) and keep the test deterministic using vitest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/accounts.test.ts`:
- Around line 225-257: After the first disableCurrentWorkspace call, assert the
parent account remains enabled and only one workspace is still enabled to catch
regressions where a single workspace failure flips account.enabled;
specifically, after expect(manager.disableCurrentWorkspace(account,
"workspace-1")).toBe(true) add assertions checking account.enabled (or
manager.getAccountByIndex(0).enabled) is true and that
account.workspaces?.filter(w => w.enabled).length === 1 (and the enabled flags
equal [false, true]) so the test fails if disableCurrentWorkspace incorrectly
disables the whole account.
In `@test/fetch-helpers.test.ts`:
- Around line 316-374: Add a deterministic vitest case exercising the
split/token filter path for isWorkspaceDisabledError by asserting that wrapped
codes like "error.workspace_disabled" and "workspace_expired:error" (and a
negative wrapped example like "something_else:error") are classified the same as
the canonical codes; place the new it() inside the existing
describe('isWorkspaceDisabledError') block so it calls
isWorkspaceDisabledError(403, 'error.workspace_disabled', '') and
isWorkspaceDisabledError(403, 'workspace_expired:error', '') expecting true, and
a wrapped non-disable token expecting false, ensuring the test covers the
tokenized-code matching branch rather than only exact codes or body-text
regexes.
In `@test/index.test.ts`:
- Around line 230-237: The test mocks are shallow-copying nested workspaces so
mockStorage and the transaction snapshot share the same workspace objects;
update both mock copy sites to deep-clone the workspaces array and each
workspace object (clone workspaces and each item) when creating the mockStorage
snapshot and when creating the transaction snapshot so mutations don't alias;
alternatively change assertions to check saveAccountsMock.mock.calls instead of
in-memory state. Ensure you modify the code paths that reference workspaces,
mockStorage and the transaction snapshot creation so they produce independent
copies.
---
Outside diff comments:
In `@index.ts`:
- Around line 1577-1585: The entitlement key is being built using stale
account.accountId (which may still be the previous workspace after rotation)
causing entitlement/quota to be recorded under the wrong workspace; update the
code that computes entitlementAccountKey (the call to
resolveEntitlementAccountKey) to use the current request workspace id
(accountId) instead of account.accountId, or assign account.accountId =
accountId before calling resolveEntitlementAccountKey; ensure you continue to
pass resolvedEmail (from extractAccountEmail(accountAuth.access) ??
account.email), account.refreshToken and account.index unchanged.
- Around line 559-571: selectBestAccountCandidate can pick a non-zero workspace
slot but the new account row you push into accounts (in the accounts.push block
in index.ts) only stores workspaces, so lib/accounts.ts falls back to slot 0;
fix by persisting the chosen workspace index into the new account object: when
creating the account entry in the accounts.push (index.ts), add a
currentWorkspaceIndex (or existing field used by lib/accounts.ts) seeded from
the index returned/used by selectBestAccountCandidate so subsequent requests use
the correct workspace and do not rewrite accountId to the wrong workspace.
In `@lib/accounts.ts`:
- Around line 763-772: The bug: re-enabling an account via the manage flow flips
target.enabled directly and bypasses setAccountEnabled(), so
resetWorkspaces(account) is never called and workspaces remain disabled; fix by
ensuring all enable/disable toggles go through the single helper or a common
persistence/update hook. Concretely, replace the direct assignment of
target.enabled in the manage flow with a call to setAccountEnabled(index,
true/false) (or call resetWorkspaces(target) immediately after any direct set),
or alternatively move the resetWorkspaces logic into the shared
persistence/update routine used by all enable toggles so that any path that
changes account.enabled invokes resetWorkspaces(account). Ensure you reference
setAccountEnabled and resetWorkspaces when updating the manage flow to avoid
skipping the workspace reset.
---
Duplicate comments:
In `@test/index-retry.test.ts`:
- Around line 357-535: Add a deterministic concurrent regression test to
test/index-retry.test.ts that simulates two simultaneous workspace-disabled 403
responses against the same ManagedAccount to ensure shared workspace state isn't
mutated twice: create an account with a single workspace, stub globalThis.fetch
(or fetchMock) to return two 403 workspace_disabled Responses in sequence,
import OpenAIAuthPlugin and loader as in existing tests, then call
Promise.all([sdk.fetch(url, {}), sdk.fetch(url, {})]) to issue the two requests
concurrently and await both; assert response statuses, that
accountManagerState.disableCurrentWorkspaceCalls === 1,
accountManagerState.setAccountEnabledCalls contains a single { index: X,
enabled: false } entry (no double-disable), and that rotateToNextWorkspaceCalls
is correct — follow existing test patterns (e.g., reusing createMockAccount,
OpenAIAuthPlugin, plugin.auth.loader, accountManagerState) and keep the test
deterministic using vitest.
🪄 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: a287a92e-bc3d-42c7-93f5-ac77a2b03144
📒 Files selected for processing (8)
index.tslib/accounts.tslib/request/fetch-helpers.tslib/storage/migrations.tstest/accounts.test.tstest/fetch-helpers.test.tstest/index-retry.test.tstest/index.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 (2)
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/storage/migrations.tslib/request/fetch-helpers.tslib/accounts.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/index.test.tstest/fetch-helpers.test.tstest/index-retry.test.ts
2 participants
Summary
Fixes the remaining workspace-disabled review follow-ups.
What changed
Validation
npx vitest run test/fetch-helpers.test.ts test/index-retry.test.ts test/accounts.test.tsnpm run lintnpm run typechecknpm run buildnpm testnote: 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 lands auto-rotation on workspace-disabled/expired 403s, correctly re-issues the request with rebuilt headers against the next workspace within the same account before falling back to another account. all five issues flagged in the previous review round are addressed: the entitlement cache is guarded with
!isDisabledWorkspaceError,attempted.delete(account.index)precedes the rotationcontinue, the exhausted-account path usescontinue accountAttemptLoopinstead ofbreak,disableCurrentWorkspaceguards against double-disable with anexpectedWorkspaceIdcheck plus anenabled === falseearly-return, and re-enabling an account now resets all workspace flags to prevent an infinite-disable cycle.key findings:
rotateToNextWorkspace(test/index-retry.test.ts:230): the mock iteratesoffset <= workspaces.length(inclusive) vs the reali < totalWorkspaces(exclusive), meaning it revisits the current workspace index on the final pass; harmless today because the disabled workspace is filtered byenabled !== false, but it diverges from the real implementation and could mask concurrency edge cases whererotateToNextWorkspaceis called beforedisableCurrentWorkspacecompleteslib/request/fetch-helpers.ts:278):normalizedCodeandbodyTextare joined with a space before pattern matching, so a short code like"workspace"plus a body containing an unrelated"disabled"word could produce a false positive; the explicitworkspaceErrorCodesset is the correct gate for string codes and should be applied before pattern matchingreturn errorResponsedirectly (line 1998 ofindex.ts); worth adding to ensure the fallback contract is locked downcurrentWorkspaceIndexinitialisation are all covered by well-structured tests; thecloneMockAccounthelper correctly prevents deep-state leakage across test casesConfidence Score: 4/5
Important Files Changed
attempted.deletebeforecontinue,!isDisabledWorkspaceErrorguard on entitlement cache, andaccountAttemptLooplabel all address prior review findings; minor coverage gap in single-account workspace-less exhaustion pathdisableCurrentWorkspaceguards against double-disable viaexpectedWorkspaceIdcheck andenabled === falseearly-return;resetWorkspacescalled on re-enable correctly fixes the infinite-disable cyclehaystackconstruction mergesnormalizedCodeandbodyTextso regex patterns can match across the boundary, risking false positives for generic short codes paired with unrelated body textworkspacesandcurrentWorkspaceIndexfields toAccountMetadataV3, no migration logic needed since both fields are optional and backwards-compatible"workspace"is combined with an unrelated"disabled"body word to validate the cross-boundary regex behaviourrotateToNextWorkspaceusesoffset <= workspaces.length(inclusive) diverging from the reali < totalWorkspaces(exclusive), which can mask edge cases where rotation is called without a prior disabledisabledAtmerge, workspace removal index clamping, and reorder tracking;cloneMockAccountcorrectly deep-clones workspace arrays to prevent test state leakageFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Request → accountAttemptLoop] --> B[getAccount] B --> C[Fetch with workspaceId or accountId] C --> D{Response} D -->|2xx| E[Return success] D -->|403| F[isWorkspaceDisabledError?] F -->|No| G[entitlementCache.markBlocked\ncapabilityPolicyStore.recordFailure] G --> H[Other error handling] F -->|Yes - no tracked workspaces| I{hasRemainingAccounts?} I -->|Yes| J[continue accountAttemptLoop\naccount stays enabled] I -->|No| K[return errorResponse] F -->|Yes - has workspaces| L[disableCurrentWorkspace\nexpectedId guard + enabled===false guard] L --> M[rotateToNextWorkspace\nlinear scan i=1..totalWorkspaces-1] M -->|nextWorkspace found| N[saveToDiskDebounced\nshowToast\nattempted.delete\ncontinue accountAttemptLoop] M -->|null - all exhausted| O[setAccountEnabled false\nsaveToDiskDebounced\nshowToast\nforgetSession\ncontinue accountAttemptLoop] N --> B J --> B O --> B B -->|attempted.size >= accountCount| P[return errorResponse]Comments Outside Diff (1)
test/index-retry.test.ts, line 230 (link)mock
rotateToNextWorkspaceiterates one slot too manythe mock uses
offset <= workspaces.lengthwhile the real implementation atlib/accounts.ts:903usesi < totalWorkspaces. the mock's final iteration computesnextIndex = (currentIdx + workspaces.length) % workspaces.length = currentIdx, revisiting the workspace that was just disabled. the results are identical in every current test because the disabled workspace hasenabled === false, but the divergence means a test whererotateToNextWorkspaceis invoked without a priordisableCurrentWorkspace(e.g. the fallback path on line 2015 of index.ts) would silently allow the mock to return the current workspace while the real impl would not. concurrency note: if two requests race on the same account, one may callrotateToNextWorkspacebefore the other'sdisableCurrentWorkspaceruns; the mock would return the stale workspace, masking that bug in tests.Prompt To Fix All With AI
Last reviewed commit: "Tighten workspace-di..."