Preserve spend totals while local history catches up - #3051
Preserve spend totals while local history catches up#3051mauriciopolvora wants to merge 3 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
a597df3 to
fa7c088
Compare
|
Codex review: needs changes before merge. Reviewed August 19, 2026, 6:10 AM ET / 10:10 UTC. ClawSweeper reviewWhat this changesThis PR retains established Codex spend totals in the menu and Spend Dashboard while a bounded local-history refresh is incomplete, with account, window, and time-zone scope checks. Merge readinessNeeds one compatibility repair before merge: the regenerated parser hash rebuilds existing local cost caches, temporarily removing the totals this PR aims to preserve. Likely related people: steipete (recent catch-up work) and NickGuAI (scan-completeness contract). Priority: P1 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCodexBar scans local Codex session history into cached cost snapshots, then publishes them to the menu and Spend Dashboard. The changed paths decide whether an incomplete refresh replaces a prior established snapshot or keeps it visible until convergence. flowchart LR
A[Local Codex session history] --> B[Bounded cost scanner]
B --> C{Coverage established?}
C -->|Yes| D[Store and publish new totals]
C -->|No| E[Validate retained snapshot scope]
E --> F[Menu and Spend Dashboard]
D --> F
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Adopt the prior parsed cache rows while clearing only the stale retained-report payload, then preserve scope-matched established totals during catch-up. Do we have a high-confidence way to reproduce the issue? Yes—the supplied isolated real-history trace exercises established, pending, and converged states, and current-head tests cover the scope boundaries; this review did not execute tests because the checkout is read-only. Is this the best way to solve the issue? No—the retained-total implementation is sound, but it must preserve prior parsed cache data while invalidating only stale retained-report metadata on upgrade. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 453174fe13ee. LabelsLabel changes:
Label 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
History |
fa7c088 to
d3cf7c8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3cf7c8ab1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let sourceCache: CostUsageCache? = if plan.shouldRefresh, | ||
| !currentScanIsPending, | ||
| !cache.days.isEmpty |
There was a problem hiding this comment.
Record the requested window in retained reports
When a shared Codex cache already spans 365 days but a routine 30-day refresh becomes bounded and pending, buildCodexReportFromCache creates a 30-day report while CostUsageCodexPreviousReport(report:cache:) labels it with the cache's wider scanSinceKey/scanUntilKey. A later 365-day dashboard or hydration request therefore passes matches, receives only the 30-day payload, and can treat it as established, hiding older spend until catch-up completes. Persist the actual requested report window rather than the cache's retained scan window.
Useful? React with 👍 / 👎.
| if !incompleteCodexSourceIDs.isEmpty { | ||
| let retainedInputs = self.loadedInputs.filter { | ||
| incompleteCodexSourceIDs.contains($0.id) && | ||
| !unsafeSourceIDs.contains($0.id) && | ||
| $0.provider == .codex && | ||
| $0.snapshot.historyCoverageIsEstablished |
There was a problem hiding this comment.
Avoid retaining snapshots across bucket-time-zone changes
When the user changes bucketTimeZoneIdentifier while the replacement Codex scan reports incomplete coverage, this filter retains the prior established input solely by source ID. Bucket time zone is not part of sameSourceOwnership or unsafeSourceIDs, so the old snapshot's day keys are then rebuilt using the new calendar, misassigning boundary usage and selected-day totals until catch-up finishes. Retention should also require the previous and requested bucket-calendar scopes to match.
Useful? React with 👍 / 👎.
Summary
Root cause
An incomplete bounded refresh is a successful scan result, not a hard failure. It correctly carries
historyCoverageIsEstablished == false, and the Spend Dashboard correctly excludes such rows from totals. However, that successful incomplete result replaced the previous established snapshot, so Codex disappeared from Usage & Spend while Cursor remained visible.The existing retained-history path covered scanner failures, but not successful exact-to-pending transitions during routine refreshes.
Safety invariants
historyDaysas the incomplete replacement.After-fix behavior proof
The runtime trace was captured on base fix commit
d3cf7c8ab19c5ffe66658525bdfdf36493d8b3a3against a copy-on-write, isolated mirror of the real local Codex session corpus. No synthetic session rows were used. A temporary uncommitted CLI harness only selected the isolated cache root and invoked the app's existing two-second bounded-refresh mode; it was removed before validation.Amounts, token counts, account identity, paths, session IDs, and prompt contents are redacted. The trace records only state and equality checks:
This demonstrates that established spend remains present while the new scan is explicitly pending/stale, and that the newly converged report replaces it and clears the retained marker. The subsequent review commits only tighten request-scope matching and retained-report metadata; their current-head regressions are listed below.
Review follow-up
Validation
main; regenerated parser hasha8843ee5c69a90fcXCODE_DEFAULT_TOOLCHAIN_OVERRIDE=/Library/Developer/CommandLineTools make check— passed, 0 violationsswift build --product CodexBarCLI— passed on current head4a60d7f4d9674e2e02e17002ea6387160f118af0CostUsageFetcherCacheSnapshotTests|SpendDashboardCachedPresentationTests|UsageStoreCodexCostCatchUpTests— 37 tests passed in 3 suitesmake test— attempted, but this host has Command Line Tools only and the unmodified runner cannot discover tests without Xcode'sTestingmodule; the focused suites were run with local framework paths and pull-request CI uses the repository's Xcode matrixThe regression fixture proves an exact scan, an appended active tail, a bounded pending refresh, an unchanged retained total, and an eventual complete replacement. Additional tests cover dashboard presentation, cross-scope rejection, and retained-payload bounds.
Refs #2815, #2628, #2520, #2918.