feat(codex): show per-account usage - #1488
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughCodex requests now receive stable non-PII labels. Usage summaries aggregate metrics by label, management API responses expose account breakdowns, and GUI account cards display 30-day tokens, estimated costs, and coverage. ChangesCodex account usage attribution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CodexRequest
participant RequestLog
participant UsageSummary
participant ManagementAPI
participant AccountCards
CodexRequest->>RequestLog: record validated accountLogLabel
RequestLog->>UsageSummary: persist labeled usage entry
UsageSummary->>ManagementAPI: return accounts breakdown
ManagementAPI->>AccountCards: provide 30-day account metrics
AccountCards->>AccountCards: join rows and render localized values
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Review against current dev@e8db4e03: two blocking attribution gaps remain.
-
Legacy pool accounts without a persisted
logLabelcannot display the new per-account usage data.core.tscorrectly derives an effective label viacodexAccountLogLabel(account), including the deterministic fallback, butpoolAccountDto()only exposesaccount.logLabelwhen it is physically present. For a legacy row, persisted usage is therefore keyed by the fallbackpXXXXXXwhile the GUI receives nologLabel, souseCodexAccountPoolcannot join the usage row to that account card. Return the same effective label from the account DTO (or migrate/persist it) and add a regression for an existing account with no storedlogLabel. -
Native
/responses/compactis still outside the new attribution path.compact.tsresolves the actual pool/main-poolauthCtxand records response usage throughinspectResponseLogJson(), but it never setslogCtx.accountLogLabelwhen that account is selected or when the bounded alternate-account retry switches accounts. Automatic pool compact requests therefore persist as bareopenaiwith no explicit account label, andbuildAccounts()classifies this newly generated usage aslegacy-ambiguousinstead of the account that actually served it. Carry the effective account label through native compact, including alternate-account retry, and cover main, added-pool, and retry attribution cases.
The branch is also 17 commits behind current dev, and the current Cross-platform CI / React Doctor workflow runs are action_required with no jobs, so CI has not actually exercised this head yet.
32765b9 to
0d65bc8
Compare
github-actions
Bot
added
the
intake: hygiene-blocked
|
@Wibias The requested attribution gaps are addressed in
Could you please re-review this head? Since the first fix necessarily touches |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d65bc8611
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function accountLabelForAttribution(provider: string, explicit: unknown): string | null { | ||
| if (baseProviderLabel(provider) !== "openai") return null; | ||
| return isCodexUsageAccountLogLabel(explicit) | ||
| ? explicit |
There was a problem hiding this comment.
When an existing exact-account selector uses a custom namespace such as side -> pool-a, the persisted attempt has provider openai-side and a valid explicit accountLogLabel, but this provider check rejects it because baseProviderLabel("openai-side") is not openai. Consequently, requests through custom selectors disappear entirely from that account's dashboard totals. Validate and return the explicit label first, using provider-name inference only for legacy rows without one.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| days: UsageDay[]; | ||
| models: UsageModel[]; | ||
| providers: UsageProvider[]; | ||
| accounts: UsageAccount[]; |
There was a problem hiding this comment.
If reading the usage log throws, /api/usage still returns its documented zeroed 200 response, but the catch-path object omits the newly required accounts array while retaining days, models, and providers. Clients consuming the additive response shape can therefore fail exactly when storage is temporarily unreadable; add accounts: [] to that fallback so the response remains shape-compatible.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs-site/src/content/docs/reference/management-api.md`:
- Around line 138-142: Update the pricing description for estimatedCostUsd in
docs-site/src/content/docs/reference/management-api.md lines 138-142 and
docs-site/src/content/docs/guides/web-dashboard.md lines 134-138 to describe
configured pricing data, including active user price overlays, rather than only
current API/list prices. Keep the surrounding API behavior and estimate
disclaimer unchanged.
In `@src/server/responses/core.ts`:
- Line 1679: Add route-level regression tests covering Responses account-label
propagation for main-pool and added-pool selection around handleResponses in
src/server/responses/core.ts:1679, and pool failover updating the final serving
account label around src/server/responses/core.ts:498-504. The compact-route
sites at src/server/responses/compact.ts:366 and
src/server/responses/compact.ts:595 require no direct changes because coverage
already exists in tests/responses-compaction-routing.test.ts:216-229 and
tests/responses-compaction-routing.test.ts:671-698.
In `@src/usage/summary.ts`:
- Around line 575-579: Update legacyCodexAccountLabel to recognize the
historical “-main” suffix and return the main-account label for openai-main
rows, while keeping only bare openai rows mapped to
LEGACY_AMBIGUOUS_ACCOUNT_LABEL and preserving legacy pool handling. Add a
regression case in tests/usage-summary.test.ts covering openai-main
classification.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 8a08ffb5-3e47-4b90-a093-59e9e1b28d45
⛔ Files ignored due to path filters (1)
docs-site/public/pr-screenshots/codex-account-usage.pngis excluded by!**/*.png
📒 Files selected for processing (29)
docs-site/src/content/docs/guides/web-dashboard.mddocs-site/src/content/docs/reference/management-api.mdgui/src/components/codex-account-pool-cards.tsxgui/src/components/codex-account-pool-main-card.tsxgui/src/hooks/useCodexAccountPool.tsgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/tr.tsgui/src/i18n/zh-TW.tsgui/src/i18n/zh.tsgui/tests/codex-account-pool-behaviour.test.tsxgui/tests/codex-account-pool-pinned-badge.test.tsxsrc/codex/account-label.tssrc/codex/auth-api.tssrc/server/request-log.tssrc/server/responses/compact.tssrc/server/responses/core.tssrc/usage/log.tssrc/usage/summary.tsstructure/05_gui-and-management-api.mdtests/api-usage.test.tstests/codex-auth-api.test.tstests/request-log.test.tstests/responses-compaction-routing.test.tstests/usage-log.test.tstests/usage-summary.test.ts
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-review on exact head 0d65bc86116e3814d9ad5dbde42ba3b7d7b68340.
The two attribution gaps from the previous review are fixed: legacy pool DTOs now expose the same effective fallback label used by persisted usage, and native compact records the selected main/pool account plus the bounded alternate account after a 429/402. The focused compact coverage demonstrates those paths.
I am still requesting changes because three current-head issues remain, and the branch is now 12 commits behind dev@37139818d2d3fe7b033e958ccf87080e4be2b4b7:
-
Historical
openai-mainrows are misclassified as ambiguous.legacyCodexAccountLabel()recognizes only a-pXXXXXXsuffix.baseProviderLabel("openai-main")isopenai, but the suffix regex missesmain, so the row falls intolegacy-ambiguous. This is not an unknown account: the historical provider label explicitly identifies the main account. Returnmainfor the exactopenai-mainlegacy shape and add a usage-summary regression; keep only bareopenaiambiguous. -
The cost wording contradicts the estimator used by this feature.
estimatedCostUsduses the existing cost lookup, where the active user-configuredproviders.<name>.modelCostsoverlay wins before built-in catalog/list-price fallbacks and historical entries are repriced from the current overlay. Update the Dashboard and management-API docs to describe configured display pricing and its fallback behavior rather than claiming the value is always based on current API list prices. Keep the existing non-billing/subscription disclaimer. -
The ordinary Responses attribution path lacks route-level regression coverage.
handleResponses()now setslogCtx.accountLogLabelafter initial auth and again during alternate-account retry, but the added tests cover compact only. Add focused main-pool, added-pool, and pool-failover tests through the ordinary Responses handler so a future auth/refactor cannot silently return those requests to the ambiguous bucket.
Please rebase onto the latest dev, address the three items above, rerun the focused backend/GUI checks, and request re-review. I am not applying maintainer-sponsored while the sensitive-path review still has correctness/test blockers, and I will not approve or merge this GUI-containing feature without the final owner review required by the repository's conservative UI policy.
0d65bc8 to
f4f65bb
Compare
|
@Ingwannu @Wibias Review blockers are addressed on the exact new head The five current-head findings are covered as follows:
Focused backend verification is 359 passed / 0 failed; focused GUI is 37 passed / 0 failed; typecheck, privacy scan, GUI lint/doctor/build, and the docs build pass. The full-suite limitation and exact prior counts remain disclosed in the PR description, so I have not attested that all local CI is green. Please re-review this head. Since |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Re-review on exact head f4f65bb12f196c9a1969f28c4bf49214316a9b28.
The earlier code blockers are fixed on this head:
- legacy pool account DTOs now expose the same effective fallback
logLabelused by persisted usage; - native
/responses/compactcarries the selected account label and updates it on bounded alternate-account retry; - historical
openai-mainrows map tomain, while bareopenairemains explicitly ambiguous; - explicit valid labels win before legacy provider-name inference, so custom account namespaces remain attributable;
- the read-failure
/api/usageshape keepsaccounts: []; - the pricing docs now match the estimator's configured
modelCostsoverlay + catalog/fallback behavior; - ordinary Responses now has focused main-pool, added-pool, and pre-stream failover attribution coverage.
I did not find another code-level blocker in this patch.
I am still requesting changes for integration/readiness only. Current dev is 70d2e1758c180188a729dd63812703b76cfeeba6; this head is 10 commits behind with merge base 49887650eb2c7efa5c90996ec5108c8a7cfd129b. The exact-head Cross-platform CI and React Doctor runs are action_required, and the Cross-platform run contains 0 jobs, so GitHub CI has not actually exercised this head. The PR is also still draft and its own readiness checklist leaves full local CI / ready-for-review unchecked; the body records that the repository-wide local test run is not green.
Please rebase onto current dev, run the maintained CI on the resulting exact head, and request re-review. If that integrated head is green and the patch remains equivalent, I see no remaining code blocker.
f4f65bb to
1fe7215
Compare
|
The patch is now rebased onto the current Exact-head local verification:
The Cross-platform CI and React Doctor pull-request workflows require maintainer approval for this external contributor and cannot be started by the author. Please approve those exact-head workflow runs, then re-review |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Re-review on exact head 1fe72150a3af6bd0a02a7b829d97ece0209e76e3.
The previously identified code blockers are fixed on this head, and I do not find a new code-level blocker in the feature patch:
- legacy pool DTOs expose the same effective fallback
logLabelused by persisted usage, and the main account exposesmain; - ordinary Responses assigns the selected account label, carries it into the physical attempt, and updates it when the bounded pool failover switches to the serving account;
- the new Responses regression covers main-pool, added-pool, and pre-stream 429 failover attribution;
- native compact retains the selected/failover label coverage from the earlier fix;
- explicit valid
accountLogLabelwins before legacy provider-name inference, so custom account namespaces remain attributable; - historical
openai-mainmaps tomain, pool suffixes remain attributable, and only bare legacyopenaiis ambiguous; - the
/api/usageread-failure shape includesaccounts: []; - the pricing documentation now matches the configured pricing-overlay/fallback behavior rather than claiming list-price-only estimates.
Several old bot threads are still shown as unresolved in GitHub, but the current code directly addresses their findings; I am not treating stale thread state as a blocker.
The remaining blocker is integration/readiness. Current dev is 4a299fa09352e9f3d5dd119f0d892f92d3d0bab1; this head is 1 ahead / 1 behind with merge base d667367528afa2c604d19af2fc8538b965b88cf1. The new dev commit is #1505 and overlaps src/server/responses/core.ts, so I want the exact integrated head rather than assuming the two changes remain semantically equivalent after merge. Exact-head Cross-platform CI (31566030365) and React Doctor (31566030308) are both action_required with 0 jobs, so GitHub CI has not executed this head. The PR also remains draft and its local full-suite/readiness boxes are still unchecked.
Please rebase onto current dev, run the maintained CI on that exact head, and request re-review. If the patch remains equivalent and CI executes cleanly, I see no remaining code blocker. Final owner review for the GUI change still applies separately.
Wibias
added
the
maintainer-sponsored
github-actions
Bot
removed
the
intake: hygiene-blocked
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Security-only deep review on exact head 1fe72150a3af6bd0a02a7b829d97ece0209e76e3.
I reviewed the changed trust boundaries separately from the earlier correctness review: account-label origin and generation, routing/failover propagation, request-log and usage.jsonl persistence/hydration, management API exposure, GUI rendering, malformed persisted history, collision behavior, and resource-exhaustion potential.
I found no reportable P0-P3 security finding in this patch.
accountLogLabelis not taken from the inbound data-plane request or upstream response. It comes from the resolved Codex pool/main-pool auth context plus configured account metadata.- New pool labels are random pseudonyms; legacy fallback labels are bounded pseudonyms. The wire/persistence validator accepts only
mainorp[0-9a-f]{6}and revalidates both top-level rows and attempts during hydration. Raw account ids, emails, access tokens, refresh tokens, ChatGPT account ids, and authorization headers are not added to the new persisted field. addFinalRequestLog,addRequestLog, andsealRequestAttemptIdentityall gate persistence on that label validator. Pool failover updates the label from the final resolved physical account rather than from caller-controlled namespace text./api/usageremains behind the existing management-auth gate. The new aggregation performs no outbound I/O and uses bounded, validated labels asMapkeys; the management usage reader already applies its configured byte ceiling (64 MiB default). A data-plane caller cannot manufacture unbounded distinct account labels.- The GUI joins usage by the bounded label and renders it through ordinary React text/
<code>nodes; there is no HTML/eval sink for a stored label. - A 24-bit label collision can at worst merge local admin-visible accounting buckets; the label is not used for credential selection, authorization, routing, or replay identity, so this is not an auth/isolation primitive.
One integration caveat remains: this head predates current dev's #1505 reasoning-replay hardening and overlaps src/server/responses/core.ts. The final rebased head still needs review/CI to ensure the merge preserves both changes, but I see no security blocker in #1488's current patch itself.
|
🧠 Learnings used
|
|
🧠 Learnings used
|
|
Thank you @Alvin0412 for the work on this PR and for addressing the review feedback. This is useful because Codex Pool usage is now attributable to the account that actually served each request, including failover, without exposing raw account identifiers or credentials. The stable non-PII labels plus per-account token, cost, and coverage data make it much easier to understand pool behavior, compare account utilization, and diagnose rotation/quota issues directly from the dashboard and management API. |
|
Tip For best results, initiate chat on the files or code changes.
CodeRabbit review was already triggered for this exact head. Maintainer review and workflow approval remain separate actions. 🧠 Learnings usedYou are interacting with an AI system. |
1 similar comment
|
Tip For best results, initiate chat on the files or code changes.
CodeRabbit review was already triggered for this exact head. Maintainer review and workflow approval remain separate actions. 🧠 Learnings usedYou are interacting with an AI system. |
3 participants
Summary
GET /api/usagewith an additiveaccountsbreakdown containing 30-day token totals, measured-usage coverage, and API-equivalent cost estimates. Historicalopenai-mainrows map tomain; only bare legacyopenairows remain ambiguous.accounts: []in read-failure responses, documenting configured pricing overlays, and covering ordinary Responses main/pool/failover attribution in addition to compact routing.Verification
bun test tests/codex-auth-api.test.ts tests/codex-account-label.test.ts tests/responses-account-label.test.ts tests/responses-compaction-routing.test.ts tests/usage-summary.test.ts tests/usage-log.test.ts tests/request-log.test.ts tests/api-usage.test.ts- 359 passed, 0 failed, 1,440 assertions.cd gui && bun test tests/codex-account-pool-behaviour.test.tsx tests/codex-account-pool-pinned-badge.test.tsx- 37 passed, 0 failed, 125 assertions.bun run typecheck- passed.bun run privacy:scan- passed.bun run lint:gui:if-changed- passed.bun run doctor:gui:if-changed- passed; React Doctor reported no issues.cd gui && bun run build- passed.cd docs-site && bun run build- passed.1fe72150is one feature commit based directly on currentdev@d6673675; its stable patch ID matches the previously reviewedf4f65bb1patch.bun run testis not green locally: 11,194 passed, 8 skipped, and 72 failed. The failures are confined to unmodified management-auth /0.0.0.0request paths, Compatibility Lab evidence and timing paths, and API-key attribution integration tests; no modified test file failed. The local-CI readiness box remains unchecked.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Documentation
Tests