fix: preserve workspace-specific usage quotas + harden test isolation#161
Conversation
… store The suite redirects storage via setStoragePathDirect(TEST_STORAGE_PATH) and resets it to null in afterAll. A pending saveToDiskDebounced() timer could fire after the reset and write fixture accounts into the real default store (~/.opencode/oc-codex-multi-auth-accounts.json). - Drain registered shutdown-flush handlers with runCleanup() before resetting the storage path in afterAll. - Flush the pending debounced save and dispose the shutdown handler in the debounce-specific tests while the test path is still active. No production code changes; test isolation only.
Add JSDoc explaining workspace-identity dedupe and the active-account marker rule. Docs only; no behavior change.
resolveCodexUsageActiveAccount's every() guard accessed account.enabled directly, which throws when storage contains an undefined slot. Treat null/undefined slots as disabled, matching the selection loop below. Adds regression tests for sparse slots and all-empty/disabled storage. Addresses CodeRabbit review on fork PR #1.
Document getUsageAccountDedupeKey, deduplicateUsageAccountIndices, resolveCodexUsageActiveAccount, and normalizeUsageIdentityPart to explain the workspace-identity-first dedupe strategy and collision-safe keys. Docs only; no behavior change.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughThis PR changes account deduplication to prefer workspace identity ( ChangesWorkspace-identity deduplication and account selection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-usage.ts`:
- Around line 590-609: The active account selection treats a missing
activeAccount.lastUsed as -1 while other enabled accounts default to 0, causing
enabled active accounts with undefined lastUsed to lose to other enabled
accounts; update the activeLastUsed initialization in lib/codex-usage.ts
(symbol: activeLastUsed) to use the same fallback as other accounts (0) when the
active account is enabled so newestLastUsed/newestIndex logic (symbols:
newestLastUsed, newestIndex, storage.accounts, activeAccount, index) compares
like-for-like and preserves the active marker correctly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cdea7d64-69be-4e85-a464-fc98db47fe79
📒 Files selected for processing (5)
lib/codex-usage.tslib/tools/codex-limits.tstest/codex-usage.test.tstest/index.test.tstest/rotation-integration.test.ts
- resolveCodexUsageActiveAccount: an enabled active account with a missing/invalid lastUsed now falls back to 0 (not -1), so a lower-index enabled account with lastUsed 0 can no longer steal the active marker (CodeRabbit). - codex-limits: match the active account by workspace identity first, so the [active] marker survives when the active entry is a same-workspace duplicate carrying a re-issued refresh token; warn when the active index is deduped out (Greptile). - deduplicateUsageAccountIndices: document that accounts with no workspace identity and no refresh token are dropped (Greptile). - rotation-integration test: drop the global runCleanup() in afterAll, which could drain other files' shutdown-flush handlers under non-default vitest pools; debounce tests already flush/dispose their own managers (Greptile). - Add regression tests for each case.
deduplicateUsageAccountIndices kept the first occurrence of each workspace key. After a token re-issue (re-add account), the surviving first occurrence carried the invalidated refresh token, so the active workspace showed an error right after re-authenticating. Keep the last (most recently added) occurrence instead; the [active] marker already matches by workspace identity, so this is safe. Display order still follows first appearance. Addresses Greptile P1 follow-up on PR ndycode#161.
- resolveCodexUsageActiveAccount: an enabled active account with a missing/invalid lastUsed now falls back to 0 (not -1), so a lower-index enabled account with lastUsed 0 can no longer steal the active marker (CodeRabbit). - codex-limits: match the active account by workspace identity first, so the [active] marker survives when the active entry is a same-workspace duplicate carrying a re-issued refresh token; warn when the active index is deduped out (Greptile). - deduplicateUsageAccountIndices: document that accounts with no workspace identity and no refresh token are dropped (Greptile). - rotation-integration test: drop the global runCleanup() in afterAll, which could drain other files' shutdown-flush handlers under non-default vitest pools; debounce tests already flush/dispose their own managers (Greptile). - Add regression tests for each case.
deduplicateUsageAccountIndices kept the first occurrence of each workspace key. After a token re-issue (re-add account), the surviving first occurrence carried the invalidated refresh token, so the active workspace showed an error right after re-authenticating. Keep the last (most recently added) occurrence instead; the [active] marker already matches by workspace identity, so this is safe. Display order still follows first appearance. Addresses Greptile P1 follow-up on PR ndycode#161.
Summary
What changed?
codex-limitsand TUI usage discovery deduplicated accounts purely by refresh token. Multiple ChatGPT workspaces under one login share a single refresh token while exposing distinctaccountId/organizationId, so same-token workspaces collapsed into one row. Usage accounts are now deduplicated by workspace identity (accountId+organizationId) when available, falling back to refresh token only when no workspace identity exists. Dedupe keys areJSON.stringifyarrays so delimiter characters can't cause collisions. Disabled accounts are skipped, and the active-account marker tracks the active workspace identity rather than the shared refresh token alone.resolveCodexUsageActiveAccountno longer throws on an undefined account slot; empty slots are treated as disabled, matching the selection loop.test/rotation-integration.test.tscould leak fixture accounts into the real account store (~/.opencode/oc-codex-multi-auth-accounts.json): it redirects storage withsetStoragePathDirect(TEST_STORAGE_PATH)and resets tonullinafterAll, but a pendingsaveToDiskDebounced()timer could fire after the reset. The suite now drains shutdown-flush handlers viarunCleanup()before resetting, and flushes/disposes managers in the debounce tests while the test path is still active.codex-limitstool factory.Why is this needed? Users running multiple workspaces under one login saw only one workspace's quota in
codex-limits/TUI. This restores correct per-workspace visibility. The test leak is a safety issue: running the suite could overwrite a developer's real local account store.Testing
npm run lintnpm run buildnpm testLocal results on this branch: lint clean, typecheck clean,
npm test85 files / 2386 tests passing. Added regression tests for distinct same-token workspaces, skipped disabled accounts, delimiter-collision safety, sparse/undefined slots, and all-empty/disabled storage returningnull. Verified the live account store checksum/mtime is unchanged before/after the full suite.Summary by CodeRabbit
Bug Fixes
Tests
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 workspace-aware dedupe so multiple chatgpt workspaces sharing one refresh token are shown as separate rows in
codex-limits/tui, and hardens test isolation so the debounced-save tests can't leak fixture data into the developer's real account store.deduplicateUsageAccountIndicesnow keys onaccountId+organizationIdfirst, falling back to refresh token, using json-serialised tagged arrays to prevent delimiter collisions; per-key it keeps the last occurrence so a re-issued refresh token wins over the stale original.resolveCodexUsageActiveAccountno longer throws on sparse account slots; disabled accounts are skipped throughout both helpers.rotation-integration.test.tsdebounced-save tests now flush and dispose their ownAccountManagerinstances beforeafterAllresets the storage path, removing therunCleanup()call that could have drained shutdown handlers from other workers.Confidence Score: 5/5
the dedup and active-marker changes are correct and well-tested; no data loss or token-leak paths introduced
core logic in
deduplicateUsageAccountIndicesandresolveCodexUsageActiveAccountis sound — last-occurrence preference correctly surfaces the freshest credential, the workspace-identity key prevents delimiter collisions, and the sparse-slot guard behaves correctly. the two stale inline comments are documentation noise and do not affect runtime behaviour. test isolation fix is clean.the
logWarncomment inlib/tools/codex-limits.tsand the test comment intest/index.test.tsboth describe the old first-occurrence dedup direction; worth correcting before this ships to avoid future confusionImportant Files Changed
getUsageAccountDedupeKeyand rewritesdeduplicateUsageAccountIndicesto key on workspace identity first, keep the last (freshest) occurrence per key, and skip disabled accounts;resolveCodexUsageActiveAccounthardened against sparse slots and disabled active accounts — logic is sound and well-testedlogWarnon deduped-out active index added, but the prose explanation above the warn describes the inverted (old first-occurrence) scenariorunCleanup()correctly removed to avoid draining the global shutdown queue across workersFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[storage.accounts] --> B[deduplicateUsageAccountIndices] B --> C{account exists?} C -- no --> D[skip sparse slot] C -- yes --> E{enabled === false?} E -- yes --> F[skip disabled] E -- no --> G[getUsageAccountDedupeKey] G --> H{accountId or organizationId?} H -- yes --> I["key = JSON.stringify(['workspace', accountId, orgId])"] H -- no --> J{refreshToken?} J -- yes --> K["key = JSON.stringify(['refresh', token])"] J -- no --> L[skip: no identity] I --> M["indexByIdentity.set(key, i) - overwrites → last occurrence wins"] K --> M M --> N["[...indexByIdentity.values()] - first-appearance order"] N --> O[uniqueIndices for codex-limits display] O --> P{i === activeIndex or accountUsageKey === activeUsageKey?} P -- yes --> Q[show active marker] P -- no --> R[no marker]Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix: prefer freshest occurrence in usage..." | Re-trigger Greptile