fix(codex): classify quota windows by duration instead of position - #468
Conversation
There was a problem hiding this comment.
Thanks — the exact free-account payload from #356 is mapped correctly, paid 5h/7d behavior is preserved, and the regression coverage is useful. I found one standards issue and one scope/correctness concern before this is merge-ready:
-
The new
codex-monthlydisplay value is hardcoded as"Monthly". This is user-facing text, and the repository requires the existing localization path.quota.metric.monthlyalready exists and is already used byzai-monthly, so please use"quota.metric.monthly".localizedStatic()here and adjust the test accordingly. -
Monthly classification is not part of #356 and the
>= 20 daysthreshold is not backed by an upstream payload in the PR. It changes the existing rule where long windows are weekly, while the issue specifically establishes a 7-day weekly window. Please either add real API evidence for this separate bucket/threshold or remove the monthly branch from this fix. Thereset_after_secondsfallback should also be documented as a heuristic: it is remaining time, not window length, so it cannot reliably distinguish weekly from monthly and cannot identify a weekly window that is less than a day from reset.
The effective merge with current master builds successfully and all 78 tests pass. The issue payload itself is handled correctly; these requested changes keep the fix localized and evidence-based.
Address review feedback on nguyenphutrong#468: - Remove the monthly window bucket and its `>= 20 days` threshold. Issue nguyenphutrong#356 establishes a 7-day weekly window; a separate monthly bucket is out of scope for this fix and belongs in its own change. This also drops the `codex-monthly` kind and its hardcoded "Monthly" display name, so no unlocalized user-facing string is introduced. - Document the `reset_after_seconds` fallback as a heuristic. It reports the time remaining in a window, not the window's length, so it is only a lower bound: it rules out the 5h session window but cannot determine the actual window length, and a weekly window less than a day from resetting falls through to the positional fallback. The issue nguyenphutrong#356 payload regression test is unchanged and still green.
|
Thanks for the review. Both points are addressed in 2. Monthly classification — removed from this fix. I did go looking for upstream evidence first, and I want to report honestly that I found some, because it affects how you may want to handle the follow-up.
I still removed the branch, because your scope objection stands on its own and I think it is the right call regardless of the evidence: issue #356 is specifically about a 7-day window arriving in Happy to open a separate issue and PR for the monthly lane citing the above, with its own regression test using the real 1. Localization. Resolved by the removal: the
Rewrote the comment to state the limitation accurately rather than assert it as a fact: // Heuristic, used only when the authoritative `limit_window_seconds` is
// absent. `reset_after_seconds` is the time REMAINING in the window, not
// the window's length, so it is only ever a lower bound: a horizon of
// more than a day rules out the 5h session window, but it cannot tell how
// long the window actually is, and a weekly window that is less than a day
// from resetting is indistinguishable from a session one and falls through
// to the positional fallback below.
Verification. |
nguyenphutrong
left a comment
There was a problem hiding this comment.
The requested changes are resolved. The speculative monthly classification and hardcoded label are gone, the exact #356 payload is classified from limit_window_seconds, and reset_after_seconds is now used only as a documented conservative lower-bound heuristic with positional fallback for ambiguous cases. The synthetic merge builds and the targeted Codex mapper tests pass.
Summary
Codex free accounts can return only a weekly window in
primary_window(limit_window_seconds: 604800) withsecondary_window: null. Quotio's original mapping assumedprimary_window == Sessionandsecondary_window == Weekly, so the weekly quota was rendered as a "Session" bucket with a multi-day reset time.#434 introduced duration-based classification via
limit_window_seconds, which fixes the reported payload, but two gaps remained:limit_window_secondsis missing from a window, classification silently falls back to the positional session/weekly labels, reproducing the exact mislabel from this issue. Thereset_after_secondsfield that disambiguates this (a 5h session window can never reset days out) was not decoded at all.There was also no test coverage locking any of this behavior in.
Changes
CodexUsageMapper.standardWindowKindnow classifies in this order:limit_window_seconds:<= 1 day-> Session,>= 20 days-> Monthly,>= 6 days-> Weeklyreset_after_secondshorizon beyond one day identifies a Weekly windowreset_after_secondsonWindowSnapshotcodex-monthlybucket id and its "Monthly" display nameQuotioTests/CodexUsageMapperTestscovering:secondary_window: nullwith a genuine 5h primary -> single Session bucketlimit_window_secondsclassified viareset_after_secondsBuckets are only emitted for windows the API actually returned, so a weekly-only free account shows exactly one Weekly row.
Validation
xcodebuild -project Quotio.xcodeproj -scheme Quotio -configuration Debug buildsucceedsxcodebuild -project Quotio.xcodeproj -scheme Quotio -configuration Debug test -destination 'platform=macOS': 73 tests passed, 0 failed (7 new)git diff --checkcleanFixes #356