Fix established-empty Codex history - #2978
Conversation
Co-authored-by: Yuxin Qiao <104957188+Yuxin-Qiao@users.noreply.github.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![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: 5ca592ee18
ℹ️ 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".
| ? CostUsageTokenSnapshot.entry(in: daily.data, forLocalDayContaining: now, calendar: calendar) | ||
| : CostUsageTokenSnapshot.latestEntry(in: daily.data) | ||
| let hasHistoricalRows = !daily.data.isEmpty | ||
| let establishedEmptyHistory = historyCoverageIsEstablished && daily.data.isEmpty |
There was a problem hiding this comment.
When Claude or Vertex returns an empty transcript report, loadLocalTokenScanResult unconditionally marks coverage established for every non-Codex provider; Bedrock likewise calls this helper with the default true. This shared predicate therefore converts those providers' existing “no data available” state into fabricated zero token and spend totals. Pass a Codex-specific established-empty flag instead of deriving zeroes from the generic coverage value.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs changes before merge. Reviewed August 16, 2026, 9:10 AM ET / 13:10 UTC. ClawSweeper reviewWhat this changesPreserve a completed empty Codex local-history scan as known-zero usage and spend across refreshes and cache hydration. Merge readinessKeep this PR open: the previous P2 finding remains on the current head, where an empty Claude or Vertex local scan is converted into known-zero usage and spend instead of remaining unavailable. Priority: P2 Review scores
Verification
How this fits togetherCodexBar's cost-usage fetcher scans provider-local history and turns it into token and spend snapshots for the menu and dashboard. The snapshot layer must distinguish a completed Codex scan with no activity from an unavailable empty result from other providers. flowchart LR
A[Provider local history] --> B[Usage scanner]
B --> C[Coverage assessment]
C --> D[Token and spend snapshot]
D --> E[Menu and spend dashboard]
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: Keep the established-empty invariant explicit and Codex-only, while all other providers retain unavailable results for empty local transcript scans. Do we have a high-confidence way to reproduce the issue? Yes—source inspection provides a high-confidence path: an empty Claude or Vertex scan is marked established at the local-scan boundary and then enters the new empty-history zero conversion. Is this the best way to solve the issue? No—the generic coverage flag is too broad; a Codex-specific established-empty flag is the narrower solution and preserves existing non-Codex unavailable semantics. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 361c9d679c99. 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)
|
…empty-history # Conflicts: # CHANGELOG.md
…empty-history # Conflicts: # CHANGELOG.md
2 participants
Summary
This supersedes #2932, which became conflicting after #2961 landed.
Reconciliation and review
The two contracts compose without weakening either invariant. The fetcher now preserves an established empty report through cache hydration, while the dashboard continues to gate zeroes and partial totals on established coverage and aggregate consistency. The empty report carries no synthetic summary values; zeroes are derived only at the coverage-gated token snapshot boundary.
Review found no blocking correctness issues. Added coverage proves that an LA-local midnight rollover invalidates a narrower empty cache, and direct dashboard tests distinguish established empty history from unestablished empty history. Cache hydration retains the native scan timestamp. Gatekeeper anchors were updated for the reconciled source layout.
Credit to @Yuxin-Qiao for the original fix and regression coverage.
Validation
make checkCODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test --disable-sandbox --filter 'CostUsageFetcherTests|CostUsageFetcherCacheSnapshotTests|CostUsageTokenSnapshotDaySelectionTests'— 46 tests in 3 suites passedCODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test --disable-sandbox --filter 'SpendDashboardPartialCost|SpendDashboard'— 140 tests in 13 suites passedHOME,CFFIXED_USER_HOME, andCODEX_HOME: completed empty refresh and a separate no-refresh process returned established zeroes; widening from 1 to 7 days produced a new scan timestampNo Keychain, browser, web, or real-account tests were run.