feat(claude): add opt-in cost breakdown with accurate periods - #3244
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
![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: dd3b8472f9
ℹ️ 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
steipete
left a comment
There was a problem hiding this comment.
Thanks @Yuxin-Qiao. I reviewed dd3b8472f95258eb880f3068e10dd4614ca659d6. This is now the intended two-file change, without the unrelated provider additions or lint relaxations from #3238. The independent unknown-cost/token state, partial-ranking qualification, stable tie-breaker, and cold/debounced journal-cache pruning are substantive improvements. I am not repeating those resolved findings.
The remaining rendering issues need focused regressions:
-
Use the configured bucket calendar throughout.
runCostcreates the fetcher withbucketCalendar, butclaudeDailyBreakdownLinescreatesCalendar.currentand the text-rendering call receives no bucket calendar. At August 27 00:30 UTC, a UTC-bucketed snapshot rendered on a Los Angeles host can omit its August 27 row while still matching six older days, so the recorded-day fallback never runs. Pass the existing calendar through; cover a pinned timezone differing from the host, midnight, and DST. -
A populated-row count is still not an elapsed-day interval. Inside the calendar-filtered branch, rows for August 21 and August 27 produce “last 2 days.” The fallback fixes only the case where there are no recent rows. Label the actual seven-day interval, or accurately describe recorded dates in both branches. Test sparse recent history as well as entirely old history.
-
Checked aggregation must reach the new model renderer.
cur.tokens = (cur.tokens ?? 0) + tcan overflow across individually representable days. For the same model,Int.max - 1followed by2traps before the unknown-state flag can help. Use checked accumulation and keep an invalid total unknown on later additions, with an isolated multi-day renderer test. These helpers run across providers, including local readers handling untrusted counters. -
An empty breakdown is also missing attribution when the day has usage. The
guard let breakdownscatches nil but not[]. A positive-usage day with an empty model list, alongside another model-attributed day, still yields an unqualified ranking of the latter subset. Preserve the partial distinction for both representations; cover positive unattributed usage, truly zero days, and mixed model coverage.
The diff still adds no tests for the new renderer or the conditional journal-cache reconciliation. Please add the isolated empty/unknown/partial/explicit-zero/metered/cross-provider/tied-ranking cases and temporary-root journal tests covering cold cache, debounce/memo reuse, force refresh, and valid nested transcripts. The journal filter remains defensive unless a versioned producer sample establishes a real counting error; the pinned upstream comments say current journal records already parse to zero.
The output-scope point from the previous review also remains: this changes ordinary text output for every provider with daily data, not just Claude. Keep that decision explicit and use shared rendering or descriptor-owned policy, rather than a misleading Claude-specific helper or an arbitrary provider conditional. The existing local Claude reader is not newly introduced by this PR; please keep the description precise.
This is source review, not execution of this head. Formatting and parser-hash checks do not prove these cases. The focused tests, unchanged make check, and full suite are still required before landing.
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs maintainer review before merge. Reviewed September 8, 2026, 12:42 AM ET / 04:42 UTC. ClawSweeper reviewWhat this changesAdds an optional Claude CLI cost breakdown showing daily usage and ranked models with matching calendar or recorded-day periods and partial-attribution warnings. Merge readiness✅ Ready for maintainer review Keep open as a useful, maintainer-supported addition absent from current main and the latest release. The revised patch resolves the applicable earlier findings, and exact-head CLI evidence supports the changed behavior. No blocking defect remains. Priority: P2 Review scores
Verification
How this fits togetherCodexBar’s cost command reads local usage logs into shared daily reports. This change formats those existing reports into optional Claude text details without changing scanning, stored data, or JSON output. flowchart TD
A[Local Claude and mirror logs] --> B[Existing cost scanner]
B --> C[Daily usage snapshot]
C --> D{Claude text with breakdown enabled}
D -->|Yes| E[Select shared calendar or recorded days]
E --> F[Daily rows and ranked models]
D -->|No| G[Existing output]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Retain the opt-in presentation layer over existing cost reports, with shared periods and explicit partial attribution. Do we have a high-confidence way to reproduce the issue? Not applicable as a new optional view; the supplied built-CLI observations directly exercise its recent-day selection, recorded fallback, and output compatibility. Is this the best way to solve the issue? Yes. The patch reuses existing scanner reports, confines the addition to explicitly requested Claude text output, and follows the owner-approved scope. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 2a71b479a5d4. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (13 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
7fd3cc9 to
192ce5d
Compare
Yuxin-Qiao
changed the title
|
@clawsweeper re-review\n\nRebased onto current main and replaced the previous journal-filtering scope with a Claude-only CLI detail change. Added focused calendar, provider-isolation, overflow, attribution, and fallback tests. Local verification: make check passed; make test passed all 80 groups on the first attempt with no retries or timeouts. |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
192ce5d to
a20e584
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a20e584732
ℹ️ 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".
clawsweeper
Bot
removed
the
merge-risk: 🚨 compatibility
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6961d27489
ℹ️ 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".
clawsweeper
Bot
added
the
merge-risk: 🚨 other
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e503f5284
ℹ️ 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".
|
Follow-up on current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a4c258cb9
ℹ️ 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".
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Addressed the current JSON-total correctness finding on
Verification:
No live account, browser-cookie, or Keychain reads were used; the regression is covered with synthetic snapshots. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d4dcfd3fd
ℹ️ 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".
9d4dcfd to
aff5476
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aff54769e0
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ea9f1d058
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4d296c092
ℹ️ 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".
| title = "Daily breakdown (last \(intervalDays) calendar day\(intervalDays == 1 ? "" : "s")):" | ||
| } else { | ||
| let sortedDaily = snapshot.daily.sorted { $0.date > $1.date } | ||
| recent = Array(sortedDaily.prefix(7)) |
There was a problem hiding this comment.
When no rows match the current calendar window, such as when rendering a stale snapshot, this fallback always selects up to seven entries even if the user requested --days 1 or another shorter interval. That makes the new breakdown expose more history than requested; limit the fallback with intervalDays rather than a fixed seven.
Useful? React with 👍 / 👎.
| private static func claudeTopModelsLines(snapshot: CostUsageTokenSnapshot, useColor: Bool) -> [String] { | ||
| var hasUnattributedDay = false | ||
| var modelAgg: [String: ClaudeModelAggregation] = [:] | ||
| for entry in snapshot.daily { |
There was a problem hiding this comment.
When the daily section takes its recorded-day fallback because every row falls outside the current calendar window, this loop still aggregates every row in snapshot.daily, and the result is labeled Top models (Last N days). A stale snapshot with attributed historical rows can therefore present old models as if they belonged to the requested current window; aggregate the same selected entries as the daily section or label the ranking as recorded history.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
Keep only the Claude --breakdown daily and top-model detail. Drop the cross-provider JSON fail-closed totals and Cursor calendar plumbing; daily and model sections share one calendar-window selection with no stale recorded-day fallback.
c4d296c to
8420cc3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8420cc30c1
ℹ️ 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".
| let baseLabel = snapshot.historyLabel ?? "Last \(snapshot.historyDays) days" | ||
| let title = isPartial | ||
| ? "Top models (\(baseLabel) \u{2014} partial):" | ||
| : "Top models (\(baseLabel)):" |
There was a problem hiding this comment.
When --breakdown uses the default --days 30, claudeRecentEntries restricts the aggregation to seven calendar days, but this label still renders Top models (Last 30 days). Users may therefore interpret a seven-day ranking as covering the full requested history; derive the label from the same capped interval used to select entries.
Useful? React with 👍 / 👎.
clawsweeper
Bot
removed
the
merge-risk: 🚨 compatibility
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
Integrate current main, share the selected calendar or recorded interval between daily and model headings, and preserve partial attribution for merged unpriced usage. Keep ordinary text and JSON output unchanged. Co-authored-by: Yuxin Qiao <104957188+Yuxin-Qiao@users.noreply.github.com>
steipete
changed the title
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Maintainer verification complete on
The August 27 changes-requested review remains attached to its old head. This rewrite addresses its calendar/interval/overflow/attribution concerns; the former journal-filter implementation was removed. No review was dismissed. Changelog is consolidated in #3491. Land-ready; no merge performed here. Contributor credit remains preserved for @Yuxin-Qiao. |
clawsweeper
Bot
added
rating: 🦞 diamond lobster
2 participants
Add
codexbar cost --provider claude --breakdownfor opt-in daily token/cost rows and a top-five model ranking. Both sections use one selection and one accurate period label: the last seven calendar days (or a shorter requested interval), with an explicitly labeled recorded-day fallback when that interval has no rows.Maintainer repair of @Yuxin-Qiao's contribution, integrated with current main. The ranking no longer calls a seven-day subset “Last 30 days,” and merged unpriced usage retains its partial qualifier. Default text, other providers, and JSON remain unchanged. The implementation keeps the configured bucket calendar, checked token accumulation, stable ranking order, and existing source boundaries.
Regression coverage includes sparse history with custom summary labels, singular intervals, recorded-day fallback, DST, exclusion of older models, integer overflow, mixed priced/unpriced merged reports, and other-provider isolation with the flag enabled.
Built CLI proof on
d168aa8e628d593df6982605227384ee037e2fae, with compact synthetic Claude logs, isolated child-process home/config/cache, and a seeded pricing catalog:--breakdownshows only today's 1,100-token model under matching “last 7 calendar days” headings.--breakdownis equal after excluding the per-callupdatedAttimestamp; ordinary text contains neither added section.make checkpassed. Local and committed-branch autoreview are clean through P2; all 41 focused architecture gatekeeper tests passed. Fullmake testpassed all 1,034 selections across 87 groups without retries or timeouts. Exact-head CI succeeded; all nine checks are green.Release-note context is collected in the batch's separate notes PR: add opt-in Claude CLI cost breakdowns with accurate calendar/recorded periods and partial-attribution labels; thanks @Yuxin-Qiao.