Skip to content

Learn the plan before the zero-delta exit, and stop implying account count - #98

Merged
btsouth merged 1 commit into
mainfrom
fix/plan-attribution-review
Jul 21, 2026
Merged

Learn the plan before the zero-delta exit, and stop implying account count#98
btsouth merged 1 commit into
mainfrom
fix/plan-attribution-review

Conversation

@btsouth

@btsouth btsouth commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Addresses both CodeRabbit findings on #97. Both were legitimate.

1. Data integrity: plan lost on a zero-delta line (Major)

A token_count can announce a new plan while billing nothing. The plan was read after the zero-delta and day-range returns, so such a line exited first and the next billed record inherited the previous plan.

Capture now happens in both parse paths immediately after the replay gate, before any early return. Deliberately still after the gate, so a child session's replayed parent history cannot install a stale plan.

Regression test zero_delta_plan_switch_still_attributes_the_next_billed_record: a line that announces team with unchanged cumulative totals, followed by a billed delta carrying no rate_limits, must attribute to team.

My first attempt at that fixture was wrong — I used last_token_usage (a per-request delta) where the zero-delta case needs total_token_usage (cumulative). The test failed loudly, which is what it's for.

2. Correctness: the notice inferred account identity (Minor)

The copy said "not just the signed-in account", which infers accounts from plan buckets — the exact overclaim this work exists to prevent. Several plans prove several plans. One account changing plans looks identical, as on this machine where prolite and plus are one account after an upgrade.

Now reads: These totals are not account-scoped. Local logs record the plan but not the account, and this machine shows 3 plans (…). Backend and bridge docs aligned.

Two edge cases found while checking the rest

  • unattributed could falsely trigger the notice. It's a bucket, not a plan, so one plan plus unlabeled records would have rendered "2 plans". It can no longer trigger alone, and shows as "unlabeled" when it accompanies a real split. Sub-1% shares render "<1%" instead of "0%".
  • Incremental parsing would drop the plan on resume — state seeds model and totals but not plan. Every caller passes offset 0 today so nothing is broken; noted at the seam so enabling it later doesn't silently lose attribution.

Verified both notice states render against the real stylesheet. Shared 660 passed, desktop 395, frontend 278. Clippy -D warnings, fmt, tsc all clean.

…count

Both from the CodeRabbit review of #97, and both were real.

A token_count can announce a new plan while billing nothing. The plan
was read after the zero-delta and day-range returns, so such a line
exited first and the next billed record inherited the previous plan.
Capture now happens in both parse paths straight after the replay gate,
before any early return. It stays after the gate so a child session's
replayed parent history cannot install a stale plan.

The disclosure also inferred account identity from plan buckets, which
is the exact overclaim this work exists to prevent. Several plans prove
several plans; one account changing plans looks identical, as it does on
this machine where prolite and plus are one account after an upgrade.
The copy now states that the totals are not account-scoped and what was
observed, and stops short of counting accounts.

Two edge cases found while checking the rest:

`unattributed` is a bucket, not a plan, so one plan plus some unlabeled
records would have rendered "2 plans". It can no longer trigger the
notice alone, and is listed as "unlabeled" when it accompanies a real
split. Shares below 1% render as "<1%" rather than "0%".

Incremental parsing would drop the plan on resume, since the state seeds
the model and totals but not the plan. Every caller passes offset 0
today, so nothing is wrong now; noted at the seam so enabling it later
does not silently lose attribution.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
ceiling 3c03c60 Commit Preview URL

Branch Preview URL
Jul 21 2026, 09:34 PM

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 39 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 215e5565-3d47-45e9-abdd-19756377dd5d

📥 Commits

Reviewing files that changed from the base of the PR and between 2139450 and 3c03c60.

📒 Files selected for processing (4)
  • apps/desktop-tauri/src-tauri/src/commands/chart.rs
  • apps/desktop-tauri/src/surfaces/settings/providers/sections/charts/ChartsSection.tsx
  • apps/desktop-tauri/src/types/bridge.ts
  • rust/src/core/jsonl_scanner.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/plan-attribution-review

Comment @coderabbitai help to get the list of available commands.

@btsouth
btsouth merged commit 831d487 into main Jul 21, 2026
11 checks passed
@btsouth
btsouth deleted the fix/plan-attribution-review branch July 21, 2026 21:40
@btsouth btsouth mentioned this pull request Jul 21, 2026
btsouth added a commit that referenced this pull request Jul 21, 2026
Release prep. Version bumped in all five tracked locations
(`version.env` incl. BUILD_NUMBER 100→101, both Rust manifests,
`package.json`, `tauri.conf.json`) plus `Cargo.lock`.

## What ships

**Added** — multi-plan disclosure (SOU-297 step 1, #97/#98). Codex
records the plan behind each request, so when a machine's totals span
more than one plan the Charts page says so rather than letting them read
as the signed-in account's.

**Fixed**
- Codex cache rate double-counted cached input, showing ~49% for a model
that was really ~97% (SOU-295, #96).
- Archived Codex sessions were missing from the Charts page, reset
windows, and API value card (SOU-296, #96).
- Cache percentages now say which window they measure (SOU-300, #99).

Also folds in a one-line doc fix flagged during the #90 review:
`CostUsageReport.current_windows` said "token totals" after it started
carrying cost too.

## Note for release notes

The archived-sessions fix will **increase** existing users' totals if
they archive Codex tasks — on the maintainer's machine 81 of 97 rollouts
were archived and therefore invisible. That is a correction, not a
regression, and the notes say so.

## Validation

`local-check.ps1 -All -Version 1.4.0` passes; release-doctor reports all
five version locations at 1.4.0 and the changelog entry present, with
only the expected pre-tag warnings (no local tag, no local assets dir).

Co-authored-by: tsouth89 <tsouth89@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant