fix: preserve zero-usage days in Codex cost chart - #3232
Conversation
改动内容与目的: - 为 token 成本 provider 增加补齐缺失日历日的展示能力,并仅在 Codex 启用。 - 将缺少记录的日期建模为零值点,避免连续日期被压缩;保留已有但无法定价记录的 nil 语义。 - 补充零用量日期、账户切换和架构门禁回归覆盖。
|
🦞👀 Pull request received. I will update this pull request when review starts. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72714ea2c7
ℹ️ 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".
steipete
left a comment
There was a problem hiding this comment.
Thanks @findwangdi — preserving calendar spacing in the inline Codex chart is a useful fix. I reviewed all six changed files at 72714ea2c7d2f103acc3834a077247103f00a370 and traced the scanner, publication, settings, and production renderer. Three issues need addressing before this can land:
- Incomplete history is not zero usage. The new helper fills dates based only on the provider capability. Initial bounded Codex scans can publish nonempty snapshots with
historyCoverageIsEstablished == false, so missing dates may still have unscanned sessions. This confirms the existing automated finding. Only synthesize zero costs when coverage is established; unknown dates must not announce$0.00. - Use the scanner's bucket time zone.
UsageStore+TokenCostpassessettings.costUsageBucketCalendarto the fetcher, while the new helper defaults toCalendar.currentand its caller supplies no calendar. With Los Angeles pinned and the system in Shanghai at August 24, 00:30, a one-day snapshot correctly contains August 23 usage. The helper instead generates August 24, drops the real row, and substitutes zero. Pass the authoritative bucket calendar through presentation and cover midnight/DST boundaries. - Keep long histories within the renderer's width. The supported 365-day setting now produces 365 points even for one activity day.
MiniUsageBarsstill uses an HStack with two-point spacing: the gaps alone require 728 points, exceeding the normal 310-point menu. This existed for dense histories, but the new fill introduces it for sparse histories too. Adapt the layout and verify the actual production view at 1, 30, and 365 days; the precise clipping/appearance has not yet been captured.
The ownership boundary should remain small: calendar position and whether a day's cost is known are separate facts. Existing nil-cost rows still disappear through compactMap; keeping their date slots without inventing a cost would also avoid leaving part of the original compression problem behind. I found no account-isolation change, and this review does not establish a connection to #3209's refresh issue.
This is source review, not independent execution of the PR's claimed checks. Before landing, please add deterministic temporary-log → scanner → snapshot → production-chart coverage for partial/completed scans, zero/unknown costs, pinned calendars, and the long-history layout, plus before/after pictures of the real view. No live account credentials are needed for these fixtures. Maintainer changelog/credit can be added during landing.
|
Codex review: needs maintainer review before merge. Reviewed August 28, 2026, 11:36 PM ET / August 29, 2026, 03:36 UTC. ClawSweeper reviewWhat this changesThe PR makes Codex inline cost charts preserve each calendar day while showing confirmed zero usage separately from unknown or unpriced history. Regression provenancePossible regression — probable (reviewed change; reproduction). No predecessor PR is attributed. Merge readinessKeep open for normal merge review: current main does not contain this chart behavior, and the PR’s introduced implementation addresses the prior coverage, time-zone, and width concerns without a definite new defect. Priority: P2 Review scores
Verification
How this fits togetherCodexBar scans local Codex usage into daily cost snapshots and renders those snapshots in the menu card and provider settings. The change carries the scanner’s bucket time zone into both chart-model entry points and changes how missing daily slots are displayed. flowchart LR
A[Local Codex usage history] --> B[Daily cost snapshot]
B --> C[Coverage and bucket time zone]
C --> D[Menu card chart model]
C --> E[Provider settings chart model]
D --> F[Inline cost bars]
E --> F
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk options
Maintainer options:
Technical reviewBest possible solution: Land the focused Codex-only presentation change once required checks finish, retaining unknown values until scan coverage is established and the shared provider capability boundary. Do we have a high-confidence way to reproduce the issue? Yes. The added focused tests and supplied production-renderer scenario cover a four-day snapshot with priced, missing, and unpriced days plus bounded-scan coverage transitions. Is this the best way to solve the issue? Yes. A Codex-only provider capability, with the shared renderer handling optional values, preserves other providers’ sparse-history behavior and matches the existing ownership boundary. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 69df3415adf9. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Keep unknown costs distinct from zero, thread the bucket calendar through both card surfaces, and bound daily bar geometry. Verify native scan catch-up, accessibility, and production-renderer output. Co-authored-by: wangdi <di.wang@ipinyou.com>
steipete
left a comment
There was a problem hiding this comment.
The maintainer repair addresses all three concerns from my original review: coverage-aware unknown/zero semantics, the scanner's pinned bucket calendar in both presentation entry points, and bounded layout through 365 days. Actual production rendering also exposed and now verifies the accessibility containment fix.
Reviewed the complete repaired delta, adjacent call sites, native-log regressions, and before/after renderer proof. Focused tests and make check passed; independent source review and Codex autoreview found no actionable defect. The latest automated review covers the repaired head and has no code finding. Its attachment-access limitation does not block this review: I inspected the generated production PNGs locally before uploading them.
The local full matrix remains red in the unchanged RPC fixture suite; the same failure predates this PR. Captured fixture processes stalled at dynamic-loader entry before the Python interpreter started, and the tests also consult the real login shell. This is not counted as a local pass. Both hosted macOS test shards passed on this candidate. No RPC code or timeout has been changed to mask the failure.
Exact-head CI is now fully green in run 33231620211, including the unchanged-candidate musl retry. GitGuardian passed too. This supersedes my original changes-requested review.
Thanks @findwangdi for the original calendar-gap fix. Contributor credit is preserved.
|
Landed as 41c53c3. Thanks @findwangdi — your original calendar-gap fix is preserved with co-author credit and a changelog entry. The maintainer repair keeps each calendar date separate from whether its cost is known. Unscanned and unpriced days stay Unknown, confirmed missing days become zero only after coverage is established, both presentation entry points use the scanner's bucket time zone, and up to 365 bars fit the menu. Actual rendering also caught and fixed the enclosing accessibility label masking individual days. Scanner, persistence, credentials, and provider account ownership are unchanged. Verification on the exact PR head, whose tree matches the merged commit:
Important local caveat: The checkout is back on |
Summary
Preserve calendar spacing in the Codex inline cost chart without treating unknown costs as zero. The repair keeps one slot per requested day, uses the same pinned bucket time zone as the scanner, and fits 1–365 days within the existing menu width. Other providers retain their existing sparse-history presentation.
Missing dates become zero only once history coverage is established. Existing unpriced rows remain unknown even after scanning completes. Accessibility labels explicitly say Unknown, and unknown values do not affect the chart scale. The range stays anchored to the snapshot date rather than moving when a stale snapshot is reopened.
The provider owns the calendar-slot capability; the shared renderer only lays out points and optional values. Both the menu and Provider settings pass the bucket calendar. No scanning, stored-history, cache, account ownership, credential, or network behavior changes.
Thanks @findwangdi for the original calendar-gap fix. This maintainer follow-up addresses the coverage, time-zone, and long-history issues from review, with contributor credit preserved.
Verification
$0.00.swift test --jobs 8 --filter 'InlineCostHistoryCalendarTests|InlineCostHistoryScreenshotTests|InlineCostHistoryDashboardLabelTests|CodexProfileHomeAccountTests|ProviderArchitectureGatekeeperTests': 67 tests passed, plus the opt-in production renderer. The renderer emitted light/dark proof for constructed and native-derived histories and verified actual accessibility labels. Run it withCODEXBAR_INLINE_COST_PROOF_DIR=<output-directory>; ordinary runs skip screenshot generation.make check: passed, including 73 cleanup tests (one platform skip), sharding checks, and strict lint with zero violations in 2,039 files.5cfcfb8cdefd3fa7c9f3e214ee3165c14e78de36in run 33231620211, including both macOS test shards, plugin-engine checks, Linux x64/ARM64/musl builds, lint, and the aggregate gate. The musl SDK installer initially crashed before CodexBar compilation; the unchanged-candidate retry passed installation and the complete build. GitGuardian also passed.make test: failed, not counted as green. Groups 1–28 passed; group 29 failed in the unchangedCodexUsageFetcherFallbackTestssuite on initialization and elapsed-time assertions, including its automatic retry. A standalone rerun and a checkpoint-validated continuation reproduced those failures, so the remaining local groups were not executed. The same RPC failure was recorded before this PR and then passed unchanged in the prior triage pass. Two captured temporary Python fixture processes were still at dynamic-loader entry, with valid pipes but no interpreter runtime loaded; another sample showed real login-shell PATH capture during fixture setup. This establishes a local startup limitation, not a chart assertion failure. The existingCODEXBAR_TEST_SUITE_TIMEOUT=600outer watchdog was used; no individual assertion or production timeout was relaxed. Hosted macOS tests provide the completed full-suite proof.--jobs 8and a task-localTMPDIR. The first actual renderer execution exposed the accessibility-label bug; the final rerun passed. None of those earlier attempts is counted as green.Production-renderer proof
The before images use the original PR code. Both sides use the same synthetic four-day history: priced August 21, absent August 22, unpriced August 23, and priced August 24. The repair retains the unpriced day's slot instead of compressing the chart. At 365 days the original renderer pushes activity outside the visible width; the repaired renderer retains all slots within 310 points.
The following images come from temporary native Codex JSONL files through the real scanner, fetcher, card model, and renderer. Prices are fixture/bundled list-price estimates, not live billing verification. The unknown model's day stays blank and accessible as Unknown after catch-up.
Final accessible four-day labels from the actual production view: