Skip to content

fix(engine,cli): report weighted tokens and label both units (#129) - #188

Merged
pbean merged 1 commit into
mainfrom
fix/label-token-units-129
Jul 19, 2026
Merged

fix(engine,cli): report weighted tokens and label both units (#129)#188
pbean merged 1 commit into
mainfrom
fix/label-token-units-129

Conversation

@pbean

@pbean pbean commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Closes #129.

The problem

Token accounting used two units and labelled neither.

Every budget judges the cost-weighted total — cache reads discounted by limits.cache_read_weight (default 0.1, matching ~0.1x vendor billing). But the run-finished summary and bmad-loop status printed the raw total, cache reads at full price. On agentic work cache reads are 80–95% of the raw count, so the number an operator reads overstated spend ~6.5x against the figure the engine actually enforced — and neither said which unit it was.

Separately, session-end journal entries carried tokens as a bare scalar. The weight cannot be backed out of it, so a session's weighted spend was unreconstructible after the fact; the weighted figure existed only for sessions that tripped the budget guard.

The split was historical, not intentional-forever: aa73202 weighted the budgets and deliberately left displays raw; be7836b then fixed the TUI only, explicitly as "a pure observer". This finishes the job for the CLI surfaces, as discussed on the issue.

What changed

  • RunSummary carries weighted_tokens beside total_tokens (raw) and renders <weighted> weighted tokens (<raw> raw incl. cache reads). render() is a single chokepoint for stdout, the ATTENTION file and the desktop notification, so all three move together. A run with no usage tracked at all (usage_parser = "none", Copilot's shutdown-only flush) still renders one plain 0 tokens rather than asserting free work twice over.
  • session-end entries carry tokens_weighted beside tokens. null — never 0 — when the usage read failed, and both absent on an aborted end, preserving the tokens.py doctrine that untracked ≠ zero. Distinct from a tripped session's budget_weighted, which is the guard's mid-session sample at trip time rather than the end-of-session total.
  • bmad-loop status gained a run-level tokens: line (it printed no run total at all before) and shows weighted with raw alongside per story.
  • The diagnose bundle reports weighted beside total, per task and per run — including the Tasks table, which rendered a single unlabelled raw number.
  • Docs corrected where they still claimed "displayed totals stay raw", plus the settings description cache_read_weight never had.

Two decisions worth reviewing

Displays read the run's persisted policy snapshot, not live policy. _resume_paused_run reloads policy.toml without re-stamping state.policy_snapshot, so the two can disagree on a resumed run. Sourcing the summary from live policy would make the CLI and the TUI print different totals for the same run from the same state.json — reintroducing the exact bug class this issue is about. The invariant is: every display surface is reproducible from state.json alone. Filed as a follow-up, since the engine-vs-display gap itself is out of scope here.

Weighted totals sum per task, not over an aggregated TokenUsage. weighted_total rounds internally, so sum-of-rounds ≠ round-of-sum under banker's rounding (verified: 30 vs 32 on a three-task fixture). Per-task summation is what tui/widgets.py does, which is what makes the CLI and TUI agree to the token.

Both are commented at the call sites, because both look like pointless complexity to a future reader.

Visible output change

Per-story status cells go from <raw>t to <weighted>t (<raw> raw) — differently scaled and differently shaped, so scripts scraping that column need updating. status has no --json, which is why the text layout carries this weight; filed as a follow-up. Called out in the CHANGELOG.

Testing

2507 passed, 1 skipped; trunk check clean.

Coverage here was the actual hazard: nothing asserted RunSummary.render()'s text or the status token cell, so the whole format change initially landed with the suite still green. New tests close that, and the load-bearing ones were each confirmed to fail without the fix:

  • weight source — 2320 (live policy) vs 1320 (snapshot), using deliberately different weights so the number identifies which was read
  • summation order — 32 (round-of-sum) vs 30 (sum-of-rounds)
  • the cache_read_weight = 0 guard — a cache-read-only story must render 0, not - (which means no tokens at all). status gated correctly by accident before, since raw was the displayed value; the natural edit after this change regresses it, which the test now catches.

Tests use a non-default weight (0.5) so assertions can't be satisfied by a silent fall-back to the 0.1 default.

Driven end-to-end on a real run as well: summary.render(), ATTENTION, journal.jsonl, bmad-loop status, diagnose, and the TUI observer path all report the same two numbers (164,000 weighted / 1,244,000 raw — a 7.59x gap). That E2E is what caught the diagnose Tasks table still emitting a lone raw figure.

Follow-ups filed

Summary by CodeRabbit

  • New Features

    • Added weighted and raw token totals to run summaries, status views, diagnostics, and session journal entries.
    • Added clearer reporting for cache-read weighting and per-story token usage.
    • Added bmad-loop status documentation covering run summaries and stories mode.
  • Documentation

    • Clarified token accounting, budget behavior, journal fields, and TUI displays.
    • Documented the distinction between weighted totals, raw totals, and budget sampling.

Token accounting used two units and named neither. Every budget judges the
cost-weighted total (cache reads discounted by limits.cache_read_weight,
default 0.1, matching ~0.1x vendor billing), but the run-finished summary and
`bmad-loop status` printed the raw total with cache reads at full price. On
agentic work cache reads are 80-95% of the raw count, so the operator-facing
number overstated spend ~6.5x against the figure the engine enforced, and
neither said which unit it was.

The split was historical: aa73202 weighted the budgets and deliberately left
displays raw; be7836b then fixed the TUI only, as "a pure observer". This
finishes the job for the CLI surfaces.

- RunSummary carries weighted_tokens beside total_tokens (raw) and renders
  "<weighted> weighted tokens (<raw> raw incl. cache reads)". render() is the
  single chokepoint for stdout, the ATTENTION file and the desktop
  notification, so all three change together. No usage tracked at all still
  renders one plain "0 tokens" rather than asserting free work twice.
- session-end journal entries carry tokens_weighted beside tokens, so
  per-session spend is reconstructible after the fact -- the raw scalar alone
  cannot be un-weighted. null (never 0) when the usage read failed, and absent
  on an aborted end, preserving "untracked != zero"; distinct from a tripped
  session's budget_weighted (the guard's sample at trip time).
- `bmad-loop status` gained a run-level tokens: line and shows weighted with
  raw alongside per story. Visible output change: cells go from "<raw>t" to
  "<weighted>t (<raw> raw)".
- The diagnose bundle reports weighted beside total, per task and per run.

Displays resolve the weight from the run's persisted policy snapshot, not live
policy, so every observer reproduces the same number from state.json alone --
resume reloads policy.toml without re-stamping the snapshot, so sourcing this
from live policy would make the CLI and the TUI disagree for one run. Weighted
totals sum per task, matching the TUI: weighted_total rounds internally, so
sum-of-rounds and round-of-sum differ under banker's rounding.

Both behaviours are pinned by tests that fail without the fix. Also documents
the weighted/raw split where the docs still claimed "displayed totals stay
raw", corrects the TUI guide's task table (stale since 0.7.12 -- it described
`tokens` as raw and omitted the `raw` column), and gives cache_read_weight the
settings description it never had.
@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The change adds cost-weighted token accounting to run summaries, status output, diagnostics, and session-end journal entries while retaining raw totals. Persisted cache-read weighting now controls calculations, with tests and documentation covering successful, aborted, and unreadable usage cases.

Changes

Weighted token accounting

Layer / File(s) Summary
Engine summaries and session journals
src/bmad_loop/engine.py, tests/test_engine.py
Run summaries expose weighted and raw totals, while session-end entries persist tokens_weighted with explicit null or absent-field behavior for failure and abort paths.
Status and diagnostics reporting
src/bmad_loop/cli.py, src/bmad_loop/diagnostics.py, tests/test_cli.py, tests/test_diagnostics.py
Status and diagnostic outputs calculate weighted totals from persisted cache-read weighting and display them alongside raw totals.
Configuration and documentation
src/bmad_loop/data/settings/core.toml, README.md, docs/FEATURES.md, docs/tui-guide.md, CHANGELOG.md
Configuration descriptions and documentation define weighted/raw token fields, display formats, cache-read weighting, and journal semantics.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Engine
  participant RunState
  participant Usage
  participant Journal
  Engine->>Usage: Read session token usage
  Engine->>RunState: Read persisted cache_read_weight
  RunState-->>Engine: Return weighting factor
  Engine->>Usage: Compute weighted total
  Engine->>Journal: Write raw and weighted session totals
Loading

Possibly related PRs

Suggested reviewers: dracic

Poem

I’m a rabbit counting tokens bright,
Weighted by cache reads, neat and light.
Raw totals hop beside the score,
Journals keep both forevermore.
Status beams the numbers clear—
A tidy burrow for every ear.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes add tokens_weighted to session-end data and update run summaries to lead with weighted tokens while showing raw totals, matching #129.
Out of Scope Changes check ✅ Passed All changes support token accounting, reporting, documentation, or tests for the linked issue; no unrelated functionality was added.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: weighted token reporting in engine and CLI with both units labeled.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/label-token-units-129

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

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.

Run summaries report raw tokens while budgets use weighted; neither unit is labeled

1 participant