Keep full-session estimate visible while idle#2336
Conversation
![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: c9079925ff
ℹ️ 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".
| guard group.resetsAt < currentSessionResetsAt.addingTimeInterval(-Self.resetEquivalenceTolerance), | ||
| let precedesCurrentSession = currentSessionResetsAt.map { | ||
| group.resetsAt < $0.addingTimeInterval(-Self.resetEquivalenceTolerance) | ||
| } ?? true |
There was a problem hiding this comment.
When currentSessionResetsAt is nil, this branch makes the eligible history groups depend on now via the later group.resetsAt <= now check, but UsageStore still caches the estimator result under the same nil-reset key across menu openings. If the first idle computation happens before a recorded partial session's reset, it can cache nil or an older median; after that reset passes with no history revision, the now-completed group is still skipped because the stale cached estimate is reused.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 20, 2026, 11:44 AM ET / 15:44 UTC. Summary Reproducibility: no. high-confidence live reproduction is included. The source path and focused tests make the missing-reset condition reproducible from code, but no observed current-app run demonstrates the before-and-after behavior. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land a narrowly tested optional-reset forecast path only after a redacted runtime capture demonstrates the idle estimate remaining visible and a minute-boundary refresh admitting newly completed history. Do we have a high-confidence way to reproduce the issue? No high-confidence live reproduction is included. The source path and focused tests make the missing-reset condition reproducible from code, but no observed current-app run demonstrates the before-and-after behavior. Is this the best way to solve the issue? Yes, conditionally: making the session reset optional while retaining reset validation when present is a narrow solution, and the minute-bucket cache refresh addresses the time-dependent historical eligibility path. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f8636cb37eb0. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (3 earlier review cycles) |
0c3021f to
8f63218
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
1 participant
Summary
Why
An idle Claude five-hour window can have no
resetsAtvalue. The forecast pipeline treated that missing reset as a hard failure even when enough completed history already existed to calculate the weekly full-session estimate. As a result, a learned estimate disappeared while idle and returned as soon as the user consumed any session quota.The current session reset is not needed for the estimate itself. Weekly remaining quota and the learned weekly burn per completed full session are sufficient, so this change makes the reset optional without weakening the weekly-window or historical-evidence guards.
Because historical eligibility depends on the current time, idle cache keys also include a minute bucket. This avoids indefinitely reusing a stale
nilor older estimate after a recorded session reset passes.User impact
Once CodexBar has learned a full-session estimate, it remains visible during idle periods and picks up newly completed historical sessions within the menu's minute-level refresh cadence.
Validation
swift test --filter SessionEquivalentForecastTests— 40 tests passed.make check— passed with no formatting or lint violations.make testwas attempted earlier in development; it stopped in unrelated Claude OAuth temporary-keychain-cache tests (ClaudeOAuthCredentialsStoreTemporaryKeychainCacheTests).