fix: check quotas without model probe - #208
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 Codex OAuth plugin now probes ChangesCodex usage flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant HealthCheck
participant fetchCodexUsage
participant parseCodexUsagePayload
participant formatCodexQuotaLine
HealthCheck->>fetchCodexUsage: GET usage payload
fetchCodexUsage-->>HealthCheck: UsagePayload
HealthCheck->>parseCodexUsagePayload: Parse payload
parseCodexUsagePayload-->>HealthCheck: CodexUsageSummary
HealthCheck->>formatCodexQuotaLine: Format summary
formatCodexQuotaLine-->>HealthCheck: Quota log line
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
index.ts (1)
3059-3078: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving
formatCodexQuotaLineintolib/codex-usage.ts.It's a pure function of
CodexUsageSummarywith no closure dependency onauthorize's scope, and it sits alongsideformatUsageLimitTitle/formatUsageLimitSummary, which already live inlib/codex-usage.ts. Relocating it would let it be unit-tested directly (currently it's only exercised indirectly through the account-check flow) and keeps formatting logic consolidated with its siblings.♻️ Suggested relocation
- const formatCodexQuotaLine = (usage: CodexUsageSummary): string => { - const parts: string[] = []; - for (const window of [usage.primary, usage.secondary]) { - if (!hasUsageWindow(window)) continue; - parts.push( - `${formatUsageLimitTitle(window.windowMinutes)} ${formatUsageLimitSummary(window)}`, - ); - } - if (hasUsageWindow(usage.codeReview)) { - parts.push(`Code review ${formatUsageLimitSummary(usage.codeReview)}`); - } - for (const limit of usage.additionalLimits) { - if (hasUsageWindow(limit.window)) { - parts.push(`${limit.name} ${formatUsageLimitSummary(limit.window)}`); - } - } - if (usage.planType) parts.push(`plan:${usage.planType}`); - if (usage.credits) parts.push(`credits:${usage.credits}`); - return parts.length > 0 ? parts.join(", ") : "quota unavailable"; - };Move the body into
lib/codex-usage.tsasexport function formatCodexQuotaLine(usage: CodexUsageSummary): string { ... }and import it inindex.tsalongside the other usage helpers.🤖 Prompt for 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. In `@index.ts` around lines 3059 - 3078, Move the pure formatCodexQuotaLine function into lib/codex-usage.ts alongside formatUsageLimitTitle and formatUsageLimitSummary, exporting it with the same CodexUsageSummary signature. Remove the local definition from authorize’s scope and import the helper in index.ts with the other Codex usage utilities, preserving its existing formatting behavior.test/codex-usage.test.ts (1)
253-302: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood coverage for the new normalization path; consider covering the default (non-normalized) branch too.
Both new tests validate
normalizeAccountErrors: true. There's no regression test confirming that omittingnormalizeAccountErrors(the default for other existing callers) still falls through tosanitizeCodexApiErrorMessageon a 402/401 response, which is the behavior those callers rely on.✅ Suggested additional test
+ it("does not normalize account errors when normalizeAccountErrors is unset", async () => { + const fetchMock = vi.fn().mockResolvedValue( + new Response(JSON.stringify({ detail: { code: "deactivated_workspace" } }), { + status: 402, + }), + ); + vi.stubGlobal("fetch", fetchMock); + + await expect(fetchCodexUsage({ + accountId: "account-1", + accessToken: "access-1", + organizationId: undefined, + timeoutMs: 1_000, + })).rejects.toThrow(/^HTTP 402/); + });🤖 Prompt for 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. In `@test/codex-usage.test.ts` around lines 253 - 302, Add coverage in the “normalizes usage endpoint workspace and token failures” test for fetchCodexUsage when normalizeAccountErrors is omitted. Assert that 402 and 401 responses follow the existing sanitizeCodexApiErrorMessage behavior, while preserving the current normalized-error assertions.
🤖 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.
Nitpick comments:
In `@index.ts`:
- Around line 3059-3078: Move the pure formatCodexQuotaLine function into
lib/codex-usage.ts alongside formatUsageLimitTitle and formatUsageLimitSummary,
exporting it with the same CodexUsageSummary signature. Remove the local
definition from authorize’s scope and import the helper in index.ts with the
other Codex usage utilities, preserving its existing formatting behavior.
In `@test/codex-usage.test.ts`:
- Around line 253-302: Add coverage in the “normalizes usage endpoint workspace
and token failures” test for fetchCodexUsage when normalizeAccountErrors is
omitted. Assert that 402 and 401 responses follow the existing
sanitizeCodexApiErrorMessage behavior, while preserving the current
normalized-error assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ede8ad9e-4c34-41ab-83fa-859539bce074
📒 Files selected for processing (4)
index.tslib/codex-usage.tstest/codex-usage.test.tstest/index.test.ts
1 participant
Summary
/wham/usageendpointcodex-limitserrorsTesting
npm run typechecknpm run lintnpm test(113 files, 2,825 tests)Summary by CodeRabbit
New Features
Bug Fixes
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
replaces synthetic model quota probes with the model-independent usage endpoint.
Confidence Score: 5/5
the pr appears safe to merge.
the new usage endpoint preserves quota parsing, token-safe errors, workspace-scoped removal, and transaction-based concurrency protections without introducing an observable failure.
Important Files Changed
/wham/usagewhile retaining workspace-scoped cleanup and transactional persistence; no new concurrency defect was identified.Reviews (1): Last reviewed commit: "fix: check quotas without model probe" | Re-trigger Greptile
Context used: