fix(usage/codex): preserve reset-credit count and gate detail fetch - #757
Conversation
The Codex usage path derives reset_credits from the cheap inline summary (rate_limit_reset_credits.available_count), then unconditionally overwrote it with reset_credits_from_response(details). When the detail body's available_count is null that helper returns None, silently dropping a known non-zero summary count so the Reset button showed nothing. Additionally, should_fetch_reset_details defaulted to true whenever the summary was absent, so the TUI's ~30s refresh fired a second GET per Codex account every cycle, roughly doubling backend request volume and raising rate-limit risk. Root cause: the merge took the detail response verbatim instead of treating it as an optional enrichment, and the fetch gate was permissive on absence. Fix: - Extract merge_reset_credits(summary, details) = details.or(summary), so a null/None detail keeps the summary count. - Extract should_fetch_reset_details(summary) that only fires when the summary is present with available_count > 0; absent/zero summaries skip the call. Tests: added unit tests covering null-detail preservation, detail-preferred, summary-absent, and the fetch gate (absent/zero/positive summary). Confidence: high Scope-risk: narrow Directive: merge_reset_credits intentionally treats a null detail count as "no update" — do not revert to unconditional overwrite without preserving the summary fallback. Not-tested: live HTTP behavior of fetch_reset_credits (helpers are pure and tested in isolation).
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: d998468111
ℹ️ 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".
| /// would roughly double backend request volume per Codex account and raise | ||
| /// rate-limit risk. | ||
| fn should_fetch_reset_details(summary: Option<&UsageResetCredits>) -> bool { | ||
| summary.is_some_and(|credits| credits.available_count > 0) |
There was a problem hiding this comment.
When /wham/usage omits rate_limit_reset_credits, reset_credits_from_summary returns None, and this new gate now skips the detail endpoint entirely. That changes the previous “unknown, so fetch details” behavior into “unknown means no credits”, so accounts whose usage response lacks the inline summary but whose detail endpoint returns an available count will no longer show the Reset button or allow reset-credit consumption; the newly added merge_reset_credits_returns_detail_when_summary_absent test even covers a detail-only credit state that production can no longer reach.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in 376f581. The gate now uses is_none_or(|c| c.available_count > 0) instead of is_some_and(...), so we fetch the detail endpoint when the inline summary is absent (unknown) — restoring the original "unknown -> fetch details" behavior — and only skip when the summary is present and explicitly reports zero credits. The merge fix (summary preserved when the detail count is null) is unchanged, and merge_reset_credits_returns_detail_when_summary_absent is now reachable in production again. Updated the gate test (renamed to should_fetch_reset_details_unless_summary_is_explicitly_zero) to assert None -> fetch. cargo test -p tokscale-cli (122 passing) and cargo clippy -p tokscale-cli --tests are green.
The reset-credit gate added in this PR used `is_some_and(... > 0)`, which skipped the detail GET whenever `/wham/usage` omitted `rate_limit_reset_credits`. That turned the prior "unknown -> fetch details" behavior into "unknown -> no credits", hiding reset credits for accounts whose usage payload lacks the inline summary but whose detail endpoint returns an available count. Switch the gate to `is_none_or(... > 0)`: fetch on an absent (unknown) summary, and only skip when the summary is present and explicitly zero. The merge fix (summary preserved when detail count is null) is unchanged. Constraint: Detail call is the only credit source when usage omits the summary Rejected: Skip on absent summary | regresses production reset-credit display Confidence: high Scope-risk: narrow Addresses review feedback on PR #757 (chatgpt-codex-connector P2).
ported from upstream junhoyeo#728 ported from upstream junhoyeo#757 ported from upstream junhoyeo#759 ported from upstream junhoyeo#760
…unhoyeo#757) * fix(usage/codex): preserve reset-credit count and gate detail fetch The Codex usage path derives reset_credits from the cheap inline summary (rate_limit_reset_credits.available_count), then unconditionally overwrote it with reset_credits_from_response(details). When the detail body's available_count is null that helper returns None, silently dropping a known non-zero summary count so the Reset button showed nothing. Additionally, should_fetch_reset_details defaulted to true whenever the summary was absent, so the TUI's ~30s refresh fired a second GET per Codex account every cycle, roughly doubling backend request volume and raising rate-limit risk. Root cause: the merge took the detail response verbatim instead of treating it as an optional enrichment, and the fetch gate was permissive on absence. Fix: - Extract merge_reset_credits(summary, details) = details.or(summary), so a null/None detail keeps the summary count. - Extract should_fetch_reset_details(summary) that only fires when the summary is present with available_count > 0; absent/zero summaries skip the call. Tests: added unit tests covering null-detail preservation, detail-preferred, summary-absent, and the fetch gate (absent/zero/positive summary). Confidence: high Scope-risk: narrow Directive: merge_reset_credits intentionally treats a null detail count as "no update" — do not revert to unconditional overwrite without preserving the summary fallback. Not-tested: live HTTP behavior of fetch_reset_credits (helpers are pure and tested in isolation). * fix(codex): fetch reset-credit detail when usage summary is absent The reset-credit gate added in this PR used `is_some_and(... > 0)`, which skipped the detail GET whenever `/wham/usage` omitted `rate_limit_reset_credits`. That turned the prior "unknown -> fetch details" behavior into "unknown -> no credits", hiding reset credits for accounts whose usage payload lacks the inline summary but whose detail endpoint returns an available count. Switch the gate to `is_none_or(... > 0)`: fetch on an absent (unknown) summary, and only skip when the summary is present and explicitly zero. The merge fix (summary preserved when detail count is null) is unchanged. Constraint: Detail call is the only credit source when usage omits the summary Rejected: Skip on absent summary | regresses production reset-credit display Confidence: high Scope-risk: narrow Addresses review feedback on PR junhoyeo#757 (chatgpt-codex-connector P2).
1 participant
Problem
In the Codex usage path (
crates/tokscale-cli/src/commands/usage/codex.rs), two issues affected reset-credit handling:Silent drop of a known count (correctness).
reset_creditsis first derived from the cheap inline summary (rate_limit_reset_credits.available_count). When a detail fetch was issued, anOkdetail response unconditionally overwrotereset_creditsviareset_credits_from_response(details), which returnsNonewhen the detail body'savailable_countis null. A non-zero summary count was silently dropped and the Reset button then showed nothing.Extra round-trip per refresh (efficiency / rate-limit risk).
should_fetch_reset_detailsdefaulted totrue(unwrap_or(true)) whenever the summary was absent, so the TUI's ~30s periodic refresh fired a second GET per Codex account every cycle, roughly doubling backend request volume.Root cause
The merge took the detail response verbatim instead of treating it as an optional enrichment, and the fetch gate was permissive when the summary was absent.
Fix
merge_reset_credits(summary, details)(=details.or(summary)): aNone/null detail count keeps the known summary count instead of dropping it.should_fetch_reset_details(summary): only fires when the summary is present withavailable_count > 0; absent or zero-count summaries skip the call entirely.Both helpers are pure, so the merge/gate logic is now unit-testable without network I/O.
Tests
Added unit tests:
merge_reset_credits_preserves_summary_when_detail_count_is_null(regression for finding a)merge_reset_credits_prefers_detail_when_presentmerge_reset_credits_returns_detail_when_summary_absentshould_fetch_reset_details_only_when_summary_has_credits(regression for finding b)cargo test -p tokscale-clipasses (122 + new unit tests).cargo clippy -p tokscale-cli --testsis clean forcodex.rs(3 unrelated pre-existing warnings remain inreport.rs).Residual concerns
fetch_reset_creditsis not unit-tested; the extracted helpers are pure and tested in isolation.available_count > 0means a future "0 -> N" transition is only picked up on the next refresh that has a non-zero summary, which is the intended conservative behavior.🤖 Generated with Claude Code
Summary by cubic
Preserves reset-credit counts in Codex usage and gates detail fetches to fix empty Reset states and cut extra requests. Fetches detail when the summary is unknown or positive; skips when it’s explicitly zero.
Bug Fixes
available_countwhen detailavailable_countis null viamerge_reset_credits; prefer detail when present.should_fetch_reset_details: fetch when summary is absent oravailable_count > 0; skip on explicit zero.Tests
Written for commit 376f581. Summary will update on new commits.