Repository navigation
Conversation
📊 Automated PR Analysis
SummaryFixes incorrect weekly aggregation between RTK and ccusage data by joining on the same Monday key instead of applying an erroneous two-day Saturday-to-Monday shift, and aggregates duplicate project-week rows (summing instead of overwriting) with checked counters and finite-cost validation. Adds extensive new unit tests covering merge boundaries, missing-side handling, zero-token cases, and invalid date handling. Review Checklist
Linked issues: #3418 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
KuSh
left a comment
There was a problem hiding this comment.
Round 1 — CHANGES REQUESTED
Claim: rtk cc-economics weekly rows join ccusage spend and rtk savings on the same Monday, and per-project ccusage rows for a week are summed instead of the last one overwriting the others.
Scope: accept. Monday is the right key: rtk gain --weekly already buckets Monday to Sunday on develop (this PR only corrects the doc comment), and ccusage ≥ 19 emits only Monday weeks (v20 rejects --start-of-week).
Ran: real ccusage 18.0.11 / 19.0.3 / 20.0.36 on 3.3 GB of real Claude Code logs × a real tracking DB (705k rows), each on develop ecff3e0, this head, and this head merged onto develop. ccusage configs: none, {}, startOfWeek: sunday, per-project rows (with and without startOfWeek: monday). Modes: --weekly, --monthly, --all × text/json/csv. Invariants: every decision (no retry / retry / retry still non-Monday), accounting (per-week and per-month cost against ccusage's own sums), stdout and stderr captured separately. Savings floor: not applicable (a report, not an output filter).
Pins: 15 decisions, 15 mutated, 1 survived (retry also applied to --daily)
Matrix: executed this round; pin check: executed this round
CI @781643fd2: all green (15)
The weekly join works. With no ccusage config, develop joins 0 of 22 weekly rows on all three ccusage versions; this head joins 10 of 12. The 2 unjoined rows are the UTC vs local-time edge you left out of scope. With per-project rows, develop shows the wrong cost on 9 of 11 weeks (week of 2026-10-12: $47.42 instead of $1,388.12); this head matches ccusage's sums on all 11.
Blocks merge (2)
-
src/analytics/ccusage.rs:175— the retry assumes v18 honours--start-of-week; with any ccusage config file it does not, and the report now fails where it used to print. ccusage 18 drops the CLI flag whenever it loads a config file (.ccusage/ccusage.jsonin the cwd, orccusage.jsonin the Claude config dir), even an empty{}:-w mondayand--start-of-week=mondayboth still return Sunday weeks. The second response is then non-Monday and theensure!fails:--weekly: develop exits 0 and prints 28 lines; head exits 1 with empty stdout andccusage returned non-Monday weeks after requesting --start-of-week monday.--all --format json: develop exits 0 with 70 daily, 22 weekly and 4 monthly rows; head exits 1 with nothing.--all --format csv: head exits 1 after printing the header and the 70 daily rows, leaving a truncated CSV.--alltext: prints the daily section, then aborts before monthly.
One of the failing configs is
"startOfWeek": "sunday", i.e. a user who explicitly chose Sunday weeks. A failed retry subprocess already degrades to rtk-only rows with a warning (failed_retry_discards_initial_spending_and_preserves_rtk_only_reporting), andfetch's doc promisesOk(None)for graceful degradation; a retry that answers with the wrong weekday should not be the one case that takes down--all. A correct fix passes these rows: ccusage 18 +{}config and ccusage 18 +startOfWeek: sunday→--weeklyexits 0 and shows the rtk rows;--all --format jsonexits 0 with the daily and monthly sections intact.ignored_monday_flag_errors_after_exactly_two_callsthen has to change, since it currently pins the failure. Which shape do you prefer?- (a) On a non-Monday retry, warn and return
Ok(None): rtk-only weekly rows, like the failed-retry case. - (b) Instead of re-running ccusage, bucket the rtk side on the weekday ccusage returned (all rows share one). This keeps the v18 user's own week start, joins with or without a config file, and removes the second full pricing pass (about 40 s per call on this data set).
-
src/analytics/ccusage.rs:173— unpinned behaviour (the retry is weekly-only). Evidence: mutatingmatches!(granularity, Granularity::Weekly)intomatches!(granularity, Granularity::Weekly | Granularity::Daily)passes every unit test and all ofcc_economics_weekly_test. Under that mutation, daily keys (every weekday) would trigger a--start-of-week mondayre-run, which v20 rejects with exit 2. The test must: run--daily --format jsonagainst a stub returning a non-Monday daily key (e.g.2026-09-30) and assert exactly one ccusage call plus the joined daily row. The mutation must turn it red.
Same file, same class — please fold in, or say so and it becomes an issue
merge_monthly (and the default summary view, which uses monthly) has the overwrite this PR fixes for weekly. ccusage 18 with per-project grouping emits 27 rows across 3 months; this head shows $27.58 / $34.40 / $26.33 for 2026-08/09/10 where ccusage's own sums are $918.67 / $7,719.78 / $4,804.08. aggregate_weekly_periods would apply as-is. Daily is not affected: ccusage 18 emits one row per day there.
Optional — will not hold merge
merge_weeklyre-parsesweek_start, which SQLiteDATE()always returns asYYYY-MM-DD, so its warn-and-skip branch cannot trigger;entry.week_startcould be used as the key directly, asmerge_dailydoes.
Checked and correct — no need to re-verify
Tracker::get_by_weekbuckets Monday to Sunday, including the 2020-W53 year boundary and the 2024 leap day (your tests, and the SQLDATE(ts, 'weekday 0', '-6 days')).- ccusage 19 and 20 output: Monday keys, no retry, one call (call log).
- ccusage 18 without config: Sunday keys, one retry with
--start-of-week monday, then a Monday join. - Per-project weekly aggregation: totals match ccusage's sums on all 11 weeks of real data, and
totalTokensis kept as reported. - Mutations killed: removing the shift fix, removing the retry, soft-failing the retry, checking only the first row, skipping the key canonicalization, skipping invalid dates, keeping the last row, recomputing
totalTokens, removing each overflow and finiteness check, flipping the retry flag value, skipping the aggregation. - fmt, clippy and the full suite on this head merged onto develop ecff3e0.
Next
Make a non-Monday retry degrade instead of failing (option a or b, rows above), and add the daily single-call test. Monthly aggregation too if you take it; otherwise I'll file it.
|
|
||
| if matches!(granularity, Granularity::Weekly) { | ||
| if !canonicalize_weekly_keys(&mut periods)? { | ||
| anyhow::ensure!( |
There was a problem hiding this comment.
Blocks merge 1. ccusage 18 ignores --start-of-week whenever it loads a config file (even {}), so this retry returns Sunday weeks again and the ensure! fails. With ccusage 18 + {} or "startOfWeek": "sunday": --weekly and --all --format json exit 1 with empty stdout (develop: exit 0), and --all --format csv exits 1 after the daily rows. A correct fix keeps --weekly at exit 0 with the rtk rows, and --all with daily and monthly intact. Options (a)/(b) in the review body.
| let mut periods = parse_json(&result.stdout, granularity) | ||
| .context("Failed to parse ccusage JSON output")?; | ||
|
|
||
| if matches!(granularity, Granularity::Weekly) { |
There was a problem hiding this comment.
Blocks merge 2 — unpinned. Changing this guard to matches!(granularity, Granularity::Weekly | Granularity::Daily) passes every unit test and all of cc_economics_weekly_test. A test running --daily --format json against a non-Monday daily key (e.g. 2026-09-30), asserting exactly one ccusage call and the joined row, must turn that mutation red.
2cc56e0 to
ca86311
Compare
Summary
--start-of-week monday, replacing the entire initial report instead of shifting labels.totalTokensindependently. Legacy project grouping can otherwise silently keep only the last row: costs 11 + 29 become 29.Partially addresses #3418: weekly spending joins and aggregation only. Monthly savings, session adoption, and timezone behavior remain unchanged. This PR does not close the whole issue.
Test plan
cargo fmt --all -- --checkcargo clippy --locked --all-targets: no warningscargo test --locked --all: 4,130 passed, 8 ignoredcargo test --locked --all -- --include-ignored: 4,138 passed, including a full rerun under conflicting locale, Git, and RTK environment settingsgit diff --checkRuntime verification was on Linux with Rust/Cargo 1.91.0. ccusage/npx tests use isolated mocks; no live ccusage, package execution through npx, or real spending was tested. macOS and Windows were not run locally.
The fixture set includes an actual upstream focused Zcode renderer snapshot containing synthetic records and an explicitly source-derived synthetic root-weekly fixture. Neither is a captured live root-CLI billing report. Legacy retries can require a second usage-loading/pricing pass; a retry that still returns non-Monday weeks is rejected.