fix(quota): keep multi-workspace fallbacks distinct#122
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughchanges add an email-based quota fallback and thread an Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Concerns
Suggested labels
🚥 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
📝 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}
ndycode
changed the title
ndycode
force-pushed
the
git-plan/21-issue-121-multi-workspace-quota
branch
from
23ad139 to
67e41fc
Compare
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 the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 1628-1631: The snapshotting of quotaEmailFallbackState leads to
stale decisions when identities change during a single refresh pass; update code
so every place that mutates account identity (e.g., inside the refresh flow that
updates email/accountId and any functions that call
buildQuotaEmailFallbackState) recomputes quotaEmailFallbackState immediately
after the mutation instead of using the earlier-captured variable; replace uses
of the top-level quotaEmailFallbackState with a freshly computed const (call
buildQuotaEmailFallbackState(storage.accounts)) right after any identity update
in the refresh/token-rotation paths (refer to the refresh logic and the code
locations using quotaEmailFallbackState), and ensure all branches that call
byEmail updates/deletes use that recomputed value. Add a vitest regression that
reproduces the refresh race: create two accounts that converge to the same email
with different accountIds during one run, mirror the Windows-targeted fixture
structure from test/storage.test.ts (around the Windows IO fixtures), and assert
that byEmail updates/deletes follow the recomputed fallback state; include
handling for EBUSY/429 retry semantics per lib/** guidelines.
In `@test/codex-manager-cli.test.ts`:
- Around line 2420-2424: Replace persistent mocks with single-call mocks: for
each test that currently calls
vi.mocked(authModule.createAuthorizationFlow).mockResolvedValue(...) (and the
other similar mocks in this file), change to mockResolvedValueOnce(...) so the
mock is only applied for the single expected invocation; update the calls that
set the resolved value for createAuthorizationFlow (and the other mocked
functions at the nearby occurrences) to use mockResolvedValueOnce to document
single-call intent and avoid leaking behavior between assertions.
🪄 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: 1796fb81-eb49-49e7-b6f5-c5e7d114dba5
📒 Files selected for processing (4)
lib/codex-manager.tstest/codex-manager-cli.test.tstest/index.test.tstest/storage.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/storage.test.tstest/codex-manager-cli.test.tstest/index.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/codex-manager.ts
🔇 Additional comments (7)
test/storage.test.ts (1)
229-257: good regression for shared refresh-token collisions across account ids.this case is deterministic and it validates both matching and dedup behavior for same-email/different-accountid flows (
test/storage.test.ts:229-257).lib/codex-manager.ts (2)
443-490: fallback safety and lookup precedence are solid.the new state model and read path correctly prioritize unique
accountIdand only allow email fallback when safe (lib/codex-manager.ts:443-490,lib/codex-manager.ts:563-584).Also applies to: 563-584
629-634: cloning cache before menu refresh is a good reliability improvement.using
cloneQuotaCacheDatakeeps refresh writes isolated until persistence, which lowers accidental in-place mutation risk in the menu flow (lib/codex-manager.ts:629-634,lib/codex-manager.ts:741-783,lib/codex-manager.ts:922-930).Also applies to: 741-783, 922-930
test/index.test.ts (1)
2012-2080: solid regression coverage for same-email workspace preservation on manual callback.
test/index.test.tslines 2012-2080 correctly pins the shared-refresh-token path and asserts both workspace rows remain distinct with the expected identity fields.test/codex-manager-cli.test.ts (3)
2801-2915: good accountid-scoped cache write regression for same-email multi-workspace accounts.
test/codex-manager-cli.test.tslines 2801-2915 cleanly verifies per-workspacebyAccountIdwrites and prevents fallback pollution intobyEmail.
2916-3028: good stale email-cache pruning regression for no-stored-accountid rows.
test/codex-manager-cli.test.tslines 2916-3028 explicitly proves stale email-scoped cache entries are removed in the same-email multi-workspace path.
3300-3410: good precedence regression: accountid cache beats email fallback when one email spans workspaces.
test/codex-manager-cli.test.tslines 3300-3410 validates row ordering and quota percentages frombyAccountId, which is exactly the expected multi-workspace behavior.
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 (1)
lib/codex-manager.ts (1)
2297-2309:⚠️ Potential issue | 🟠 Majornon-json forecast/fix paths still persist the stale cache object.
you now write updates into
workingQuotaCache, but non-json tails still savequotaCache(lib/codex-manager.ts:2473-2475,lib/codex-manager.ts:3413-3415). this drops live probe updates in default cli output mode. please save the working copy in both paths.also add vitest regressions for non-json
forecast --liveandfix --livepersistence paths (e.g.test/codex-manager-cli.test.ts:1).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."proposed fix
@@ - if (quotaCache && quotaCacheChanged) { - await saveQuotaCache(quotaCache); - } + if (workingQuotaCache && quotaCacheChanged) { + await saveQuotaCache(workingQuotaCache); + } @@ - if (quotaCache && quotaCacheChanged) { - await saveQuotaCache(quotaCache); - } + if (workingQuotaCache && quotaCacheChanged) { + await saveQuotaCache(workingQuotaCache); + }Also applies to: 2384-2386, 3088-3100, 3336-3338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 2297 - 2309, The non-JSON "forecast" and "fix" live paths still persist the original stale quotaCache instead of the modified workingQuotaCache; update the persistence calls in the CLI paths that handle --live (the code that currently writes quotaCache at the non-JSON branches around the referenced areas) to write workingQuotaCache (the object produced by cloneQuotaCacheData and mutated with live probe updates), ensuring quotaCacheChanged logic still controls writes; add vitest regression tests in test/codex-manager-cli.test.ts to cover "forecast --live" and "fix --live" persistence (verifying the saved cache equals workingQuotaCache), and ensure changes touch the same code paths referenced (symbols: workingQuotaCache, quotaCache, cloneQuotaCacheData, buildQuotaEmailFallbackState, and the forecast/fix CLI handlers) without leaking secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 791-793: Add a new Vitest that reproduces concurrent-writer races
for saveQuotaCache by reusing the Windows fixture pattern from storage tests:
mock the filesystem so that some parallel writes throw EBUSY/EPERM (to trigger
the exponential backoff in lib/quota-cache.ts) while launching multiple
simultaneous calls to saveQuotaCache(...) (the call sites in codex-manager.ts
are the callers to target). The test should run several parallel saveQuotaCache
invocations, allow retries/backoff to occur, then read the cache file and assert
it contains the expected, uncorrupted JSON; ensure the mock simulates
intermittent EBUSY only for some attempts so retries are exercised and final
content is deterministic.
In `@test/codex-manager-cli.test.ts`:
- Around line 570-626: The failing tests use generic Error("save failed") so
they do not cover Windows EBUSY filesystem behavior; update the three
failure-case tests (the one using runCodexMultiAuthCli and saveQuotaCacheMock /
loadQuotaCacheMock shown here and the other two groups referenced) to add at
least one mockRejectedValueOnce that returns a Node-style errno error with code
"EBUSY" (use your test helper makeErrnoError or Error with .code="EBUSY")
instead of or in addition to new Error("save failed"), so
saveQuotaCacheMock.mockRejectedValueOnce(makeErrnoError(..., "EBUSY")) is
exercised and the assertions still expect the same unchanged originalQuotaCache
and call counts for saveQuotaCacheMock and the same payload shape for the saved
cache.
- Around line 3040-3145: The test currently asserts saveQuotaCacheMock was not
called, but the scenario intends to prune stale email-scoped entries loaded via
loadQuotaCacheMock; update the assertions after calling runCodexMultiAuthCli to
expect saveQuotaCacheMock was called once and validate the saved cache argument
no longer contains the "owner@example.com" key (inspect
saveQuotaCacheMock.mock.calls[0][0] or use
vi.mocked(saveQuotaCacheMock).mock.calls to assert saved.byEmail does not have
"owner@example.com"); keep other mocks (fetchCodexQuotaSnapshotMock,
runCodexMultiAuthCli, loadQuotaCacheMock) unchanged so the test stays
deterministic.
---
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 2297-2309: The non-JSON "forecast" and "fix" live paths still
persist the original stale quotaCache instead of the modified workingQuotaCache;
update the persistence calls in the CLI paths that handle --live (the code that
currently writes quotaCache at the non-JSON branches around the referenced
areas) to write workingQuotaCache (the object produced by cloneQuotaCacheData
and mutated with live probe updates), ensuring quotaCacheChanged logic still
controls writes; add vitest regression tests in test/codex-manager-cli.test.ts
to cover "forecast --live" and "fix --live" persistence (verifying the saved
cache equals workingQuotaCache), and ensure changes touch the same code paths
referenced (symbols: workingQuotaCache, quotaCache, cloneQuotaCacheData,
buildQuotaEmailFallbackState, and the forecast/fix CLI handlers) without leaking
secrets.
🪄 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: fc1cc326-9ffd-4778-ae0b-2a05eacc0e85
📒 Files selected for processing (2)
lib/codex-manager.tstest/codex-manager-cli.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/codex-manager-cli.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/codex-manager.ts
🔇 Additional comments (6)
lib/codex-manager.ts (2)
443-489: safe fallback gating looks correct and materially reduces cross-workspace cache collisions.the split between
buildQuotaEmailFallbackStateandhasSafeQuotaEmailFallback, plus thecanStoreguard, is a good fix for ambiguous same-email ownership inlib/codex-manager.ts:443-489andlib/codex-manager.ts:652-683.Also applies to: 562-632, 652-683
1752-1754: good auth-rotation handling: fallback state is recomputed after identity mutation.recomputing fallback state after email/accountid changes closes the stale-key race in refresh flows at
lib/codex-manager.ts:1752-1754andlib/codex-manager.ts:3215-3217.Also applies to: 3215-3217
test/codex-manager-cli.test.ts (4)
2503-2605: strong regression coverage for multi-workspace fallback and refresh-race behavior.these cases are solid and directly cover mixed same-email/accountId behavior plus token-refresh fallback recomputation at test/codex-manager-cli.test.ts:2503 (Line 2503), test/codex-manager-cli.test.ts:2925 (Line 2925), test/codex-manager-cli.test.ts:3146 (Line 3146), test/codex-manager-cli.test.ts:3543 (Line 3543), and test/codex-manager-cli.test.ts:4884 (Line 4884). this meaningfully lowers concurrency-risk regressions around fallback-state transitions.
Also applies to: 2925-3038, 3146-3271, 3543-3652, 4884-5014
2647-2661: good move to one-shot oauth mocks for determinism.using
mockResolvedValueOncehere at test/codex-manager-cli.test.ts:2647 (Line 2647) and test/codex-manager-cli.test.ts:2661 (Line 2661) keeps these tests isolated and explicit.
2718-2718: updated probe-count and cache-shape assertions look correct.the adjusted expectations at test/codex-manager-cli.test.ts:2718 (Line 2718), test/codex-manager-cli.test.ts:2753 (Line 2753), and test/codex-manager-cli.test.ts:2796 (Line 2796) align with the new quota snapshot/cache flow and tighten behavior checks.
Also applies to: 2753-2753, 2796-2807
3273-3302: menu-loop and source-index mapping coverage is clean.these login/settings assertions at test/codex-manager-cli.test.ts:3273 (Line 3273) and test/codex-manager-cli.test.ts:3303 (Line 3303) are deterministic and validate sort-order-to-source-index mapping clearly.
Also applies to: 3303-3434
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/codex-manager-cli.test.ts`:
- Around line 4897-5027: Add a new deterministic vitest case mirroring the
existing "recomputes live quota fallback state after refresh changes a
shared-workspace email" test but exercise the branch where the refreshed token
changes accountId while the email remains the same: set loadAccountsMock to
include two entries with the same email but different initial accountId for the
stale access token, mock queuedRefreshMock to return an access token that maps
(via mocked accountsModule.extractAccountId) to a different accountId while
extractAccountEmail still returns the same email, ensure
fetchCodexQuotaSnapshotMock and loadQuotaCacheMock setup triggers the live
fallback recompute path, then call
runCodexMultiAuthCli(["auth","fix","--live","--json"]) and assert exit code 0,
queuedRefreshMock called once, fetchCodexQuotaSnapshotMock called expected
times, saveQuotaCacheMock written with cleared byEmail/byAccountId, and
saveAccountsMock reflects the updated accountId for the refreshed token;
reference mocks/exports: runCodexMultiAuthCli, queuedRefreshMock,
loadAccountsMock, loadQuotaCacheMock, fetchCodexQuotaSnapshotMock,
saveQuotaCacheMock, saveAccountsMock, and
accountsModule.extractAccountId/extractAccountEmail to locate where to
change/add the test.
🪄 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: 3e164a23-4a3e-40a7-98b0-192efa5e962c
📒 Files selected for processing (1)
test/codex-manager-cli.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 (1)
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/codex-manager-cli.test.ts
🔇 Additional comments (2)
test/codex-manager-cli.test.ts (2)
570-631: good windows ebusy + immutability regression coverage.
test/codex-manager-cli.test.ts:570,test/codex-manager-cli.test.ts:1319, andtest/codex-manager-cli.test.ts:4849now validateebusyfailures while asserting the originally loaded quota cache object is unchanged. this is the right guardrail for working-copy isolation.Also applies to: 1319-1378, 4849-4895
2933-3046: good multi-workspace fallback safety coverage.the new cases at
test/codex-manager-cli.test.ts:2933,test/codex-manager-cli.test.ts:3048,test/codex-manager-cli.test.ts:3154, andtest/codex-manager-cli.test.ts:3551cover accountid/email routing boundaries well and reduce risk of collapsing same-email workspaces.Also applies to: 3048-3153, 3154-3279, 3551-3660
|
@greptile rerun |
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 (2)
test/codex-manager-cli.test.ts (2)
5052-5345:⚠️ Potential issue | 🟠 Majormirror these refresh-drift regressions on
auth check.
test/codex-manager-cli.test.ts:5052andtest/codex-manager-cli.test.ts:5183lock down theauth fix --live --jsonfallback rebuilds, but the refreshedauth checkpath still only has generic coverage attest/codex-manager-cli.test.ts:1300andtest/codex-manager-cli.test.ts:1396. that leaves one of the two refreshed fallback branches from the pr objective unguarded.targeted test to add
+ it("recomputes live quota fallback state during auth check when refresh changes accountId for the same email", async () => { + // mirror test/codex-manager-cli.test.ts:5183 with runCodexMultiAuthCli(["auth", "check"]) + // assert stale byEmail fallback is cleared or re-keyed after refresh + });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. reject changes that mock real secrets or skip assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 5052 - 5345, Add two new vitest tests in test/codex-manager-cli.test.ts that mirror the existing "recomputes live quota fallback state after refresh changes a shared-workspace email" and "recomputes live quota fallback state after refresh changes accountId for the same email" cases but call runCodexMultiAuthCli(["auth","check","--live","--json"]) instead of "fix"; reproduce the same mock setups for loadAccountsMock, loadQuotaCacheMock, queuedRefreshMock, accountsModule.extractAccountId, accountsModule.extractAccountEmail, fetchCodexQuotaSnapshotMock, and spies (console.log) and assert the same outcomes (exit code 0, queuedRefreshMock called once, fetchCodexQuotaSnapshotMock twice, saveQuotaCacheMock and saveAccountsMock called with the expected byAccountId/byEmail shape and account/email changes, and that the JSON reports contain two "healthy" reports) to ensure deterministic coverage of both refreshed fallback branches for the "auth check" path.
3164-3229:⚠️ Potential issue | 🟠 Majorprove the stale email fallback never reaches the menu.
test/codex-manager-cli.test.ts:3164seeds an unsafebyEmail["owner@example.com"]entry, buttest/codex-manager-cli.test.ts:3226only asserts that probing and saving were skipped. if the login menu still renders that stale fallback for both rows, this regression still passes while the original same-email display bug survives.suggested assertion
expect(exitCode).toBe(0); expect(fetchCodexQuotaSnapshotMock).not.toHaveBeenCalled(); expect(saveQuotaCacheMock).not.toHaveBeenCalled(); + const menuAccounts = promptLoginModeMock.mock.calls[0]?.[0] as Array<{ + quota5hLeftPercent?: number; + }>; + expect( + menuAccounts.every((account) => account.quota5hLeftPercent == null), + ).toBe(true); });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. reject changes that mock real secrets or skip assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 3164 - 3229, The test seeds a stale byEmail entry but never asserts the login menu didn't show that stale email; update the test around runCodexMultiAuthCli to explicitly assert that the mocked menu (promptLoginModeMock) was not passed any choices containing "owner@example.com" (i.e., inspect promptLoginModeMock.mock.calls or .mock.lastCall and ensure no displayed labels/choices include that email), while keeping the existing assertions that fetchCodexQuotaSnapshotMock and saveQuotaCacheMock were not called; reference promptLoginModeMock and runCodexMultiAuthCli to locate where to add the new assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 2473-2475: Add vitest regression tests that exercise the non-JSON
code paths by running the CLI handlers for "codex auth forecast --live" and
"codex auth fix --live" (the same CLI entrypoints covered by
test/codex-manager-cli.test.ts) and assert they succeed under intermittent
Windows-style rename failures and concurrent save pressure; specifically, mock
the filesystem rename/write used by saveQuotaCache to throw EBUSY/EPERM on some
attempts and succeed on retries, spawn concurrent callers to trigger concurrent
save attempts, and verify the code uses workingQuotaCache and completes without
crashing. Place these new tests alongside test/quota-cache.test.ts and
test/storage.test.ts, reference the saveQuotaCache function and the CLI handlers
for auth forecast/fix in assertions, and ensure the tests simulate both
single-threaded retry and concurrent contention scenarios (including 429-like
retry behaviour) to prove the branches under Windows rename contention.
In `@test/codex-manager-cli.test.ts`:
- Around line 3051-3123: The test relies on ordered mock responses from
fetchCodexQuotaSnapshotMock but never asserts which workspace/token each call
used, allowing a regression where both probes target the same workspace; update
the test around runCodexMultiAuthCli to include explicit toHaveBeenNthCalledWith
assertions on fetchCodexQuotaSnapshotMock for call 1 and call 2 verifying the
expected probe inputs (e.g., that the first call was invoked for
"workspace-alpha" with its token and the second for "workspace-beta" with its
token), keeping the existing saveQuotaCacheMock and promptLoginModeMock
assertions intact and ensuring the checks use Vitest matchers so the test
deterministically proves the two live probes remain distinct.
In `@test/quota-cache.test.ts`:
- Around line 178-241: The test currently only checks final file validity but
must also assert that no save hit the warning path in lib/quota-cache.ts (the
warning logged around the retry/failure handling at the saveQuotaCache path).
Update the test to mock the logger module (../lib/logger.js) with vitest
(vi.mock) and spy on the logWarn export before importing
saveQuotaCache/getQuotaCachePath/loadQuotaCache, then after the concurrent
Promise.all call assert that the spy was not called (logWarn stayed cold);
restore/mock cleanup in the finally block to keep the test deterministic.
---
Outside diff comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 5052-5345: Add two new vitest tests in
test/codex-manager-cli.test.ts that mirror the existing "recomputes live quota
fallback state after refresh changes a shared-workspace email" and "recomputes
live quota fallback state after refresh changes accountId for the same email"
cases but call runCodexMultiAuthCli(["auth","check","--live","--json"]) instead
of "fix"; reproduce the same mock setups for loadAccountsMock,
loadQuotaCacheMock, queuedRefreshMock, accountsModule.extractAccountId,
accountsModule.extractAccountEmail, fetchCodexQuotaSnapshotMock, and spies
(console.log) and assert the same outcomes (exit code 0, queuedRefreshMock
called once, fetchCodexQuotaSnapshotMock twice, saveQuotaCacheMock and
saveAccountsMock called with the expected byAccountId/byEmail shape and
account/email changes, and that the JSON reports contain two "healthy" reports)
to ensure deterministic coverage of both refreshed fallback branches for the
"auth check" path.
- Around line 3164-3229: The test seeds a stale byEmail entry but never asserts
the login menu didn't show that stale email; update the test around
runCodexMultiAuthCli to explicitly assert that the mocked menu
(promptLoginModeMock) was not passed any choices containing "owner@example.com"
(i.e., inspect promptLoginModeMock.mock.calls or .mock.lastCall and ensure no
displayed labels/choices include that email), while keeping the existing
assertions that fetchCodexQuotaSnapshotMock and saveQuotaCacheMock were not
called; reference promptLoginModeMock and runCodexMultiAuthCli to locate where
to add the new assertion.
🪄 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: f55b8ffb-57b4-445c-873f-01cf6ceb0ab2
📒 Files selected for processing (3)
lib/codex-manager.tstest/codex-manager-cli.test.tstest/quota-cache.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/codex-manager-cli.test.tstest/quota-cache.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/codex-manager.ts
🔇 Additional comments (3)
lib/codex-manager.ts (3)
448-489: good fallback-state guardrails for shared emails.this correctly prevents unsafe by-email reuse when multiple workspace identities share one email, while still allowing safe fallback for a single-account email path. nice direction for the
#121objective inlib/codex-manager.ts:448-489.
1752-1754: good recompute on identity mutation during auth rotation.rebuilding fallback state immediately after email/accountid mutations avoids stale key decisions in the same pass. this is the right fix shape for refresh-time identity drift in
lib/codex-manager.ts:1752-1754andlib/codex-manager.ts:3213-3215.Also applies to: 3213-3215
1634-1635: working cache isolation is correctly threaded.cloning into
workingQuotaCachebefore live updates is a solid isolation boundary and avoids mutating the loaded cache object on failed saves across health/forecast/fix flows inlib/codex-manager.ts:1634-1635,lib/codex-manager.ts:2297-2298, andlib/codex-manager.ts:3088-3089.Also applies to: 2297-2298, 3088-3089
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/codex-manager-cli.test.ts (1)
5833-5938:⚠️ Potential issue | 🟡 Minorrestore mocks and consider adding probe input binding assertions.
test/codex-manager-cli.test.ts:5834-5846- missing mock restoration forextractAccountIdandextractAccountEmail.additionally, consider adding
toHaveBeenNthCalledWithassertions forfetchCodexQuotaSnapshotMockto explicitly bind each probe to its workspace-alpha/workspace-beta input, similar totest/codex-manager-cli.test.ts:3434-3443. this guards against regressions where both probes accidentally hit the same workspace.as per coding guidelines
test/**: tests must stay deterministic and demand regression cases that reproduce concurrency bugs.proposed fix for mock restoration
expect( payload.reports.filter((report) => report.outcome === "healthy"), ).toHaveLength(2); + vi.mocked(accountsModule.extractAccountId).mockImplementation(() => "acc_test"); + vi.mocked(accountsModule.extractAccountEmail).mockImplementation(() => undefined); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 5833 - 5938, The test leaks mocked implementations for accountsModule.extractAccountId and accountsModule.extractAccountEmail; restore them after the scenario and add explicit probe-input binding assertions: call vi.mocked(accountsModule.extractAccountId).mockRestore() and vi.mocked(accountsModule.extractAccountEmail).mockRestore() at the end of the test (or in a finally/afterEach), and add toHaveBeenNthCalledWith checks on fetchCodexQuotaSnapshotMock (e.g., assert the 1st call used workspace-alpha probe/input and the 2nd used workspace-beta) to ensure each probe targeted the intended account; reference the mocked symbols extractAccountId, extractAccountEmail and fetchCodexQuotaSnapshotMock when locating where to add restoration and toHaveBeenNthCalledWith assertions.
🤖 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/codex-manager-cli.test.ts`:
- Around line 3648-3713: The test replaces accountsModule.extractAccountId but
never restores it, leaving global test state mutated; after the assertions add a
cleanup to restore the original implementation — either call
vi.mocked(accountsModule.extractAccountId).mockRestore() (or .mockReset() if
mockRestore isn't available) or run vi.resetAllMocks()/vi.restoreAllMocks() at
the end of the test so extractAccountId from the imported accountsModule is
returned to its original behavior.
- Around line 3549-3586: The test contains dead setup for
fetchCodexQuotaSnapshotMock (the .mockResolvedValueOnce(...) chain) but later
asserts fetchCodexQuotaSnapshotMock was not called, so remove the unnecessary
mockResolvedValueOnce setup to clarify intent; also restore the previously
mocked extractAccountId (call extractAccountIdMock.mockRestore() or
jest.restoreMockFor('extractAccountId') depending on how it was mocked) before
importing runCodexMultiAuthCli so the real extractAccountId behavior is used;
keep existing references to promptLoginModeMock and saveQuotaCacheMock
assertions intact.
- Around line 1494-1561: The test sets custom implementations for
accountsModule.extractAccountId and accountsModule.extractAccountEmail but never
restores them, which can leak state to later tests; after the assertions at the
end of this test, call vi.mocked(accountsModule.extractAccountId).mockRestore()
and vi.mocked(accountsModule.extractAccountEmail).mockRestore() (or the
equivalent restore/reset used elsewhere in this file) to revert the mocks to
their original behavior so other tests aren’t affected.
- Around line 1619-1706: The test sets custom mocks for
accountsModule.extractAccountId and accountsModule.extractAccountEmail but
doesn’t restore them; after the assertions at the end of this test, call
vi.mocked(accountsModule.extractAccountId).mockRestore() and
vi.mocked(accountsModule.extractAccountEmail).mockRestore() (or
vi.restoreAllMocks() if you prefer) to restore original implementations so
subsequent tests aren’t affected.
- Around line 5675-5774: The test stubs for accountsModule.extractAccountId and
accountsModule.extractAccountEmail are not being restored, so add mock
restoration at the end of the test (after the assertions) by calling the
appropriate restore on the vi-mocked functions (restore the mocks created via
vi.mocked(accountsModule.extractAccountId) and
vi.mocked(accountsModule.extractAccountEmail)) so the mocks do not leak into
other tests.
---
Outside diff comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 5833-5938: The test leaks mocked implementations for
accountsModule.extractAccountId and accountsModule.extractAccountEmail; restore
them after the scenario and add explicit probe-input binding assertions: call
vi.mocked(accountsModule.extractAccountId).mockRestore() and
vi.mocked(accountsModule.extractAccountEmail).mockRestore() at the end of the
test (or in a finally/afterEach), and add toHaveBeenNthCalledWith checks on
fetchCodexQuotaSnapshotMock (e.g., assert the 1st call used workspace-alpha
probe/input and the 2nd used workspace-beta) to ensure each probe targeted the
intended account; reference the mocked symbols extractAccountId,
extractAccountEmail and fetchCodexQuotaSnapshotMock when locating where to add
restoration and toHaveBeenNthCalledWith assertions.
🪄 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: fbbfcbe7-d0fa-431c-8016-6f5fa159fff6
📒 Files selected for processing (3)
lib/codex-manager.tstest/codex-manager-cli.test.tstest/quota-cache.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/codex-manager-cli.test.tstest/quota-cache.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/codex-manager.ts
🔇 Additional comments (21)
lib/codex-manager.ts (12)
443-475: lgtm on the fallback state builder.the
buildQuotaEmailFallbackStatefunction correctly handles undefined/empty emails by skipping them vianormalizeQuotaEmail. thedistinctAccountIdsset properly tracks unique accountIds per email to detect multi-workspace collisions.
477-489: core logic for issue#121looks solid.
hasSafeQuotaEmailFallbackcorrectly returns false whendistinctAccountIds.size > 1, which is the key guard preventing multiple workspaces sharing an email from collapsing into one cached quota entry. thematchingCount === 1check ensures we only use email fallback when there's exactly one account with that email.
562-585: email fallback guard is correct.the
hasSafeQuotaEmailFallbackcheck at line 579 prevents returning stale email-keyed entries when multiple workspaces share the same email. callers intoExistingAccountInfo,runHealthCheck, andrunFixproperly pass pre-computedemailFallbackStateto avoid repeated rebuilds.
611-632: pruning logic handles stale byEmail entries correctly.lines 627-630 delete
cache.byEmail[email]when either:
- the account now has a unique accountId (prefer byAccountId), or
- the email fallback is no longer safe (multi-workspace conflict)
this ensures stale email-keyed cache entries don't persist after identity resolution improves.
634-641: shallow clone is sufficient here.the comment correctly notes that entries are replaced not mutated. this isolates
workingQuotaCachefrom the originalquotaCacheso failed saves don't corrupt loaded state. see usage atlib/codex-manager.ts:1658,lib/codex-manager.ts:2329,lib/codex-manager.ts:3120.
643-663: prune function correctly cleans up after identity mutations.
pruneUnsafeQuotaEmailCacheEntryis called atlib/codex-manager.ts:1780andlib/codex-manager.ts:3249afteraccountIdentityChangedis detected. theaccounts.some()loop ensures we only delete when no account still qualifies for safe email fallback.
700-706: canStore guard avoids wasted probes.the guard at lines 702-706 correctly skips probing when neither
hasUniqueQuotaAccountIdnorhasSafeQuotaEmailFallbackis true. this prevents fetching quota that can't be cached, aligning with the comment at lines 700-701.
768-820: isolation pattern in refreshQuotaCacheForMenu is correct.the function clones at line 779, mutates
nextCache, and saves at line 816. the originalcacheparam is never mutated. returningnextCacheat line 819 ensures callers get the updated state.
1777-1786: fallback state is rebuilt after identity mutation.this addresses the past review concern about stale
quotaEmailFallbackState. whenaccountIdentityChangedis true, line 1778 rebuilds the state before callingpruneUnsafeQuotaEmailCacheEntry. subsequentupdateQuotaCacheForAccountcalls at line 1806-1813 receive the fresh state viaquotaEmailFallbackState ?? undefined.
2338-2341: single fallback state build is correct for runForecast.unlike
runHealthCheckandrunFix,runForecastdoesn't persist identity changes back tostorage.accounts. the refresh results at lines 2357-2367 update only localprobeAccessToken/probeAccountId, not the account objects in storage. so the fallback state computed once at line 2338 remains valid throughout.
3246-3255: fallback state rebuild in runFix matches runHealthCheck pattern.line 3247 rebuilds
quotaEmailFallbackStateafter identity mutation, and line 3249-3254 prunes stale email entries. this is consistent with therunHealthCheckpattern atlib/codex-manager.ts:1777-1786.
959-967: single fallback state build is correct for read-only iteration.
toExistingAccountInfodoesn't mutate accounts, so buildingemailFallbackStateonce at line 959 and passing it to allgetQuotaCacheEntryForAccountcalls is efficient and correct.test/quota-cache.test.ts (3)
6-13: helper is clean and type-safe.
makeErrnoErrorcorrectly constructs errno-style exceptions matching theRETRYABLE_FS_CODESset inlib/quota-cache.ts:35. the type constraint ensures only valid retry codes are used in tests.
178-250: concurrent save test addresses past review concern.the logger mock at lines 181-183 and assertion at line 242 address the previous review comment about verifying no saves hit the warning path. the test correctly exercises:
- concurrent writes (4 parallel saves at line 230)
- windows-style errors (EBUSY/EPERM alternating at lines 214-219)
- atomic file integrity (JSON parse assertion at lines 234-239)
- temp file cleanup (line 245)
the fixed error schedule via
retryableAttemptsmap makes the test deterministic.
229-241: assertionattempts > 4correctly validates retry behavior.with 4 concurrent saves and errors on attempts 1, 2, 4, 5, each failing save must retry. the
toBeGreaterThan(4)assertion confirms retry logic executed. the interleaved execution order varies, but the assertion accounts for this by checking minimum expected attempts rather than exact count.test/codex-manager-cli.test.ts (6)
570-631: lgtm - ebusy regression coverage added.test covers
lib/codex-manager.tslive forecast save failure withEBUSYerrno, verifying original quota cache object stays unmutated. assertions are complete for call count and payload shape.
3357-3480: lgtm - proper probe binding assertions present.
test/codex-manager-cli.test.ts:3434-3443usestoHaveBeenNthCalledWithto explicitly verify which workspace/token each live probe targeted. this addresses the past concern about ensuring probes stay distinct per workspace.
2935-3036: lgtm - good coverage for multi-workspace same-email oauth flows.test correctly verifies that distinct same-email workspaces (workspace-alpha, workspace-beta) are preserved during oauth login when refresh token is reused. uses
mockImplementationOncepattern for test-scoped mocks.
5159-5196: lgtm - lockout prevention regression added.
test/codex-manager-cli.test.ts:5159-5196covers the edge case where the last enabled account fails refresh but must stay enabled to prevent lockout. assertion at line 5187-5188 explicitly checks enabled state and line 5194-5195 verifies the warning message.
5283-5329: lgtm - ebusy regression for live fix account save failure.test verifies original quota cache object stays unmutated when live fix account save fails with
EBUSY. assertion at line 5324-5327 checks reference equality. note thatsaveQuotaCacheMockis expected NOT to be called since the failure happens before quota cache save.
5543-5616:⚠️ Potential issue | 🟡 Minorrestore extractaccountid and extractaccountemail mocks at test end.
test/codex-manager-cli.test.ts:5544-5557- same pattern as other tests. add mock restoration after assertions.proposed fix
expect( payload.reports.filter((report) => report.outcome === "healthy"), ).toHaveLength(2); + vi.mocked(accountsModule.extractAccountId).mockImplementation(() => "acc_test"); + vi.mocked(accountsModule.extractAccountEmail).mockImplementation(() => undefined); });> Likely an incorrect or invalid review comment.
1 participant
Summary
auth checkandauth fixrefreshes change shared-workspace identitiesaccountIdrows, workspace-specific probe inputs, stale-email pruning, WindowsEBUSYsave failures, and concurrent quota-cache retriesValidation
npm run typechecknpm test -- test/codex-manager-cli.test.ts test/quota-cache.test.tsnpm test -- test/storage.test.ts test/index.test.tsnpm test -- test/codex-manager-cli.test.ts