feat(codex-limits): add codex-limits tool and deduplicate accounts by refreshToken#70
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. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📝 WalkthroughWalkthroughAdded a new public tool "codex-limits" that fetches Codex usage data per account (deduplicated by refresh token), refreshes tokens as needed, formats output for UI v2 or legacy text, persists updated storage, and includes error handling and tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Plugin as "codex-limits"
participant Storage
participant TokenSvc as "TokenManager"
participant CodexAPI as "Codex API"
Client->>Plugin: execute()
Plugin->>Storage: load accounts
Storage-->>Plugin: accounts list
Note over Plugin: deduplicate accounts by refreshToken
loop for each unique refreshToken
Plugin->>TokenSvc: ensure valid access token (refresh if needed)
TokenSvc-->>Plugin: access token
Plugin->>CodexAPI: fetch usage with token
CodexAPI-->>Plugin: usage payload / error
end
Plugin->>Plugin: format per-account output (UI v2 or legacy)
Plugin->>Storage: persist updated tokens/accounts
Storage-->>Plugin: persisted
Plugin-->>Client: return formatted string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (2)
test/index.test.ts (2)
765-787: Avoid coupling this test tosaveAccounts.Line 786 locks the test to one persistence helper, but the observable contract here is “refreshed auth gets persisted,” not “this exact function is called.” A switch to
withAccountStorageTransactionwould be a harmless refactor that breaks this test.Diff to keep the test behavior-focused
it("refreshes missing tokens before fetching usage", async () => { - const { saveAccounts } = await import("../lib/storage.js"); mockStorage.accounts = [ { refreshToken: "r1", accountId: "acc-1", email: "user@example.com" }, ]; @@ const result = await plugin.tool["codex-limits"].execute(); expect(result).toContain("100% left"); expect(mockStorage.accounts[0]?.accessToken).toBe("refreshed-access"); - expect(saveAccounts).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 765 - 787, The test currently couples to the persistence helper by asserting saveAccounts was called; instead, remove the expect(saveAccounts).Spy on or mock withAccountStorageTransaction (the higher-level transaction helper used by the storage layer) and assert that it was invoked around the plugin run, while keeping the behavioral assertions: mockStorage.accounts[0]?.accessToken becomes "refreshed-access" and the plugin invocation plugin.tool["codex-limits"].execute() still returns the usage string. In short: stop asserting saveAccounts; replace that assertion with a spy/assertion on withAccountStorageTransaction and retain the state-based check that the refreshed token was persisted in mockStorage.accounts.
825-832: Assert the duplicate row is gone, not just the count.Right now this mostly proves fetch deduplication plus a
2 accountheader. It would still miss a formatter regression that renders the shared credential twice from cached data. Add one assertion against the rendered per-account rows/labels to pin the PR’s main regression end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 825 - 832, The test currently asserts fetch deduplication and a "2 account" header but doesn't verify the duplicate row was removed; after calling plugin.tool["codex-limits"].execute() and getting result, add an assertion that the shared credential's rendered per-account label/row appears exactly once in result (and optionally that the two expected account labels are present) so the test catches formatter regressions that would render the cached credential twice; use the existing result variable to locate and count the specific account label(s) rather than relying solely on the header or fetch call count.
🤖 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 4301-4320: The fetchUsage function performs a fetch without a
timeout; update fetchUsage to use an AbortController and setTimeout using the
existing fetchTimeoutMs to abort the request if it exceeds the timeout, pass
controller.signal to fetch(${CODEX_BASE_URL}/wham/usage), clear the timeout on
success, and convert an AbortError into a clear timeout error (or throw a
descriptive Error) rather than letting the promise hang; keep the existing
headers creation via createCodexHeaders and preserved response handling.
In `@test/index.test.ts`:
- Around line 813-823: The test currently assigns a single Response instance to
globalThis.fetch via vi.fn().mockResolvedValue(new Response(...)), which causes
the Response body to be consumed on the first request and break the second;
update the mock so each fetch call returns a fresh Response (e.g., replace
mockResolvedValue(new Response(...)) with either mockImplementation(() =>
Promise.resolve(new Response(...))) or use mockResolvedValueOnce/new
Response(...) twice) so every invocation of globalThis.fetch yields a new
Response object that can be read independently by the code under test.
---
Nitpick comments:
In `@test/index.test.ts`:
- Around line 765-787: The test currently couples to the persistence helper by
asserting saveAccounts was called; instead, remove the expect(saveAccounts).Spy
on or mock withAccountStorageTransaction (the higher-level transaction helper
used by the storage layer) and assert that it was invoked around the plugin run,
while keeping the behavioral assertions: mockStorage.accounts[0]?.accessToken
becomes "refreshed-access" and the plugin invocation
plugin.tool["codex-limits"].execute() still returns the usage string. In short:
stop asserting saveAccounts; replace that assertion with a spy/assertion on
withAccountStorageTransaction and retain the state-based check that the
refreshed token was persisted in mockStorage.accounts.
- Around line 825-832: The test currently asserts fetch deduplication and a "2
account" header but doesn't verify the duplicate row was removed; after calling
plugin.tool["codex-limits"].execute() and getting result, add an assertion that
the shared credential's rendered per-account label/row appears exactly once in
result (and optionally that the two expected account labels are present) so the
test catches formatter regressions that would render the cached credential
twice; use the existing result variable to locate and count the specific account
label(s) rather than relying solely on the header or fetch call count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32d7cb2a-4284-4ed2-bd96-16563e2c992c
📒 Files selected for processing (2)
index.tstest/index.test.ts
soul667
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a new codex-limits tool to display live Codex usage windows per account while deduplicating output by refreshToken to avoid repeated credentials.
Changes:
- Introduce
codex-limitstool that fetches and formats/wham/usagerate-limit + plan/credits details. - Deduplicate displayed accounts by
refreshTokenso the same credential only appears once. - Add Vitest coverage for the new tool and deduplication behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
index.ts |
Adds codex-limits tool implementation, including usage fetch + per-account formatting + refreshToken-based dedup. |
test/index.test.ts |
Adds tests asserting tool registration, output formatting, token refresh behavior, and refreshToken dedup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
|
The PR description image shows a demonstration of the new
A summary line at the bottom states (in Chinese) that Account 1 is the active account with minor usage, while all others are essentially at full capacity. This visually validates the deduplication feature — only 4 unique accounts are shown despite the plugin storing duplicate org-scoped and token-scoped records.
✅ Actions performedFull review triggered. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!response.ok) { | ||
| const bodyText = await response.text().catch(() => ""); | ||
| throw new Error(bodyText || `HTTP ${response.status}`); |
There was a problem hiding this comment.
Raw API Error Body Surfaced to User Without Redaction
bodyText is the raw response body from the remote /wham/usage endpoint. it's thrown directly as the error message and then shown to the user via message.slice(0, 160) in the catch block. on windows this is also a token-leakage risk: if the api ever echoes back request headers or partial credential details in a 4xx body, they'd be written to screen output and potentially captured by logging middleware or AV scanners that hook stdout.
prefer discarding the raw body and surfacing only the status code, or at minimum strip anything that looks like a bearer token before throwing:
| if (!response.ok) { | |
| const bodyText = await response.text().catch(() => ""); | |
| throw new Error(bodyText || `HTTP ${response.status}`); | |
| if (!response.ok) { | |
| const bodyText = await response.text().catch(() => ""); | |
| const safe = bodyText.replace(/Bearer\s+\S+/gi, "[redacted]").slice(0, 200); | |
| throw new Error(safe || `HTTP ${response.status}`); | |
| } |
Rule Used: What: Every code change must explain how it defend... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: index.ts
Line: 4315-4317
Comment:
**Raw API Error Body Surfaced to User Without Redaction**
`bodyText` is the raw response body from the remote `/wham/usage` endpoint. it's thrown directly as the error message and then shown to the user via `message.slice(0, 160)` in the catch block. on windows this is also a token-leakage risk: if the api ever echoes back request headers or partial credential details in a 4xx body, they'd be written to screen output and potentially captured by logging middleware or AV scanners that hook stdout.
prefer discarding the raw body and surfacing only the status code, or at minimum strip anything that looks like a bearer token before throwing:
```suggestion
if (!response.ok) {
const bodyText = await response.text().catch(() => "");
const safe = bodyText.replace(/Bearer\s+\S+/gi, "[redacted]").slice(0, 200);
throw new Error(safe || `HTTP ${response.status}`);
}
```
**Rule Used:** What: Every code change must explain how it defend... ([source](https://app.greptile.com/review/custom-context?memory=637a42e6-7a78-40d6-9ef8-6a45e02e73b6))
How can I resolve this? If you propose a fix, please make it concise.
Summary
codex-limitstool to show per-account Codex usage for the 5-hour and weekly windowsrefreshTokenso the same credential is only displayed onceWhy
Some stored accounts appear twice because the plugin keeps both org-scoped and token-scoped records for the same underlying credential. This made the new limits output noisy and misleading even though the usage data was identical.
Verification
npm run typechecknpx vitest run test/index.test.tsnpm run buildopencode run "Reply with exactly OK" --model=openai/gpt-5.4 --variant=mediumSummary by CodeRabbit
New Features
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
this pr adds a new
codex-limitstool that fetches per-account codex usage windows from the/wham/usageendpoint, and deduplicates accounts sharing the samerefreshTokento avoid showing duplicate rows. it fits cleanly into the existing tool-dispatch pattern and the dedup logic correctly solves the noise problem described.key concerns:
invalidateAccountManagerCache()is not used after the storage write (manual reload instead), dedup collapses all accounts with a missing/emptyrefreshTokeninto one entry, and token rotation after a successful refresh leaves duplicate storage entries with the old (now-invalid) token. these are real concurrency and token-safety risks on windows where file writes are slow and stale tokens can sit on disk between saves./wham/usageis thrown and surfaced to users without redaction — if the upstream endpoint ever echoes back credential fragments, they'd be written to output and potentially captured by logging or AV processes on windows. a redaction pass is needed.describeblock functionally.fetchUsage— thecatchblock renderingError:rows is untested.Confidence Score: 2/5
Important Files Changed
codex-limitstool with dedup logic, token refresh, and usage formatting. three issues from prior review threads (cache invalidation, undefined-token dedup collapse, token rotation stale duplicates) remain open. new issue: raw api error body surfaced to user without redaction — windows token-leakage risk.codex-limits. dedup test is indented at wrong level (1-tab vs 2-tab) though it's still functionally inside the describe block. missing coverage for non-ok fetch responses.Sequence Diagram
sequenceDiagram participant C as Client participant T as codex-limits tool participant S as loadAccounts() participant D as Dedup (Set<refreshToken>) participant R as queuedRefresh() participant A as /wham/usage API participant Disk as saveAccounts() C->>T: execute() T->>S: loadAccounts() S-->>T: storage.accounts[] T->>D: build uniqueIndices via seenTokens Set D-->>T: uniqueIndices[] loop for each unique account index T->>T: check accessToken / expiresAt alt token missing or expired T->>R: queuedRefresh(account.refreshToken) R-->>T: {access, refresh, expires} T->>T: update account in-memory, storageChanged=true end T->>A: GET /wham/usage (with access token) A-->>T: UsagePayload T->>T: mapWindow / formatLimitSummary end alt storageChanged T->>Disk: saveAccounts(storage) T->>T: conditionally reload cachedAccountManager end T-->>C: formatted limits stringLast reviewed commit: f50d255
Context used: