Fix Codex weekly reset publication - #3177
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs real behavior proof before merge. Reviewed August 25, 2026, 12:28 AM ET / 04:28 UTC. ClawSweeper reviewWhat this changesThe PR delays publication of suspicious low Codex weekly readings until a later matching OAuth refresh, persisting per-account evidence and reset-celebration safeguards. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 4 items remain The PR is still necessary, but a persisted candidate can outlive its promised 30-minute limit when provider timestamps stall; it also lacks real after-fix refresh proof. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCodexBar fetches Codex usage and stores a published snapshot for the menu-bar UI. This change decides whether a low weekly reading is safe to publish and whether a reset event is emitted. flowchart LR
A[OAuth usage refresh] --> B[Weekly reset admission]
C[Prior account snapshot] --> B
D[Reset-credit evidence] --> B
B --> E[Persisted reset candidate]
E --> F[Later exact refresh]
F --> G[Published usage snapshot]
G --> H[Menu bar and reset celebration]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Make expiry depend on local persistence time, add a relaunch/stalled-timestamp regression, and attach redacted real refresh proof. Do we have a high-confidence way to reproduce the issue? Yes, from source: persist a candidate older than 30 minutes, then return a valid but non-advancing provider timestamp; the branch retains it without evaluating expiry. Is this the best way to solve the issue? No; delayed confirmation is reasonable, but expiry must be independent of provider timestamp progress before persisted relaunch recovery is safe. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b6a4ce968c16. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
steipete
left a comment
There was a problem hiding this comment.
Thanks for taking this on — the confirmation-state-machine structure is a reasonable shape, all 46 new tests pass, and make check is clean. But a deep review found the core scenario from #3168/#3179 still isn't fixed, and the workaround path introduces two new failure modes. Details:
P0 — the reported 1 → 1 early-reset case remains stuck. CodexWeeklyResetConfirmation.swift:170 still rejects an early reset unless a credit was consumed, the previous inventory was explicitly zero, or the boundary is unchanged within one second. A real backend reset advances the boundary, and unchanged positive credit inventory satisfies none of those arms — which is exactly the residual case both issues describe. The existing test at CodexWeeklyResetPublicationTests.swift:467 actively asserts the scenario stays frozen at the previous reading, and none of the new tests feeds unchanged positive inventories across all three snapshots. Please add a test that reproduces #3179's table (1 available → 1 available, boundary advanced, fresh 0%) and make it publish.
P1 — the claimed relaunch migration doesn't repair the persisted baseline. UsageStore+PlanUtilization.swift:956 discards legacy confetti-detector state, but the stale usage snapshot is persisted separately and rehydrated at UsageStore.swift:534. The migration test only checks detector-state deletion, so upgrading and relaunching won't recover an already-stuck install.
P1 — false confetti on mundane corrections. UsageStore+LimitResetCelebration.swift:429 treats any crossing from >5% to ≤5% as a reset candidate regardless of magnitude — a 6% → 5% → 5% drift with unchanged plan and boundary celebrates a reset after 60 s. Gate on boundary advancement, credit consumption, or a substantial drop.
P1 — transient zero glitches publish immediately. UsageStore+CodexWeeklyResetConfirmation.swift:81 fires the confirmation fetch immediately and CodexWeeklyResetConfirmation.swift:185 accepts two matching zeros on account/plan/boundary match alone, so a brief backend cache glitch hitting both requests persists a wrong reading; the 60 s / 30 min windows only guard confetti, not publication.
Happy to re-review once the 1 → 1 case actually publishes and the persisted-baseline recovery is proven by a test.
8b4d413 to
653628c
Compare
|
Addressed the requested changes in
|
Co-authored-by: Trim <womayeyonggugele@gmail.com>
steipete
left a comment
There was a problem hiding this comment.
All requested weekly-reset cases now pass, including unchanged positive reset-credit inventory, advanced boundaries, persisted recovery across relaunch, and false-confetti suppression. Maintainer hardening additionally separates persisted local observation time from provider timestamps, expires stalled and regressing candidates reliably, rejects wrong-account or wrong-source stale observations, preserves sibling-account caches, and decodes preexisting snapshots compatibly. Focused account-ownership, confirmation, publication, celebration, and provider-architecture regressions plus make check are green.
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: Fix Codex weekly reset publication This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
|
Landed in 0a1aa53 after addressing every maintainer review finding and additional account-safety defects. Exact OAuth weekly resets with unchanged positive reset-credit inventory now publish only after an independent later refresh; private account-scoped evidence survives relaunch without publishing transient zero readings or false reset confetti. Provider timestamps establish evidence ordering, while a separately persisted local observation clock enforces the 60-second minimum and 30-minute maximum even when provider timestamps stall, regress, or jump forward. Expired evidence is removed from persisted snapshots; stale observations from another account, plan, or source fail closed; and creating an active-account candidate preserves every sibling account snapshot. Legacy persisted candidates decode compatibly without cache migration. Verification: 198 focused account-ownership, weekly-reset confirmation, publication, celebration, persistence, and provider-architecture tests passed; make check passed with zero violations; the full local suite passed all 930 selections across 78 groups with zero failures, retries, or timeouts; and exact-head GitHub Actions run 32873319421 passed both macOS shards, Linux x64, arm64 and musl builds, lint, and security checks. Contributor credit and changelog included. |
Summary
1 available -> 1 availablecase with an advanced reset boundary6% -> 5% -> 5%from triggering reset confettiWhy
Codex can report a fresh post-reset weekly value while the reset credit remains available. The old guard treated that as unproven indefinitely, so both the usage value and its “Updated” timestamp could remain frozen. An immediate second request is not independent evidence, so this change carries a private candidate into a later refresh cycle instead.
Safety
Testing
swift test --filter CodexWeeklyResetConfirmationTests— 26 passedswift test --filter CodexWeeklyResetPublicationTests— 16 passedswift test --filter UsageStoreCodexWeeklyResetCelebrationTests— 9 passedswift test --filter '^CodexBarTests\.CodexAccountScopedRefreshTests/'— 120 passedmake check— formatting and lint cleanmake test— 929 selections in 78 groups passed with zero failures, retries, or timeoutsNo screenshot is included because this changes provider publication state rather than menu layout. The rollover cases are verified at the deterministic state/model seams, including persisted relaunch recovery.
Scope: this fixes the frozen Codex weekly snapshot in #3179. It does not claim to fix the separate menu-presentation issue in #3168.
Closes #3179