Skip to content

fix(pr-cost): make the posted cost readable, not just correct - #37

Merged
cheshirecode merged 2 commits into
mainfrom
fix/pr-cost-readable-payload
Sep 11, 2026
Merged

fix(pr-cost): make the posted cost readable, not just correct#37
cheshirecode merged 2 commits into
mainfrom
fix/pr-cost-readable-payload

Conversation

@cheshirecode

Copy link
Copy Markdown
Owner

Why this exists

PR #35 fixed how the claude lane prices a session. It did not fix the thing that started that work: the comment a person reads.

A posted comment on a real PR read:

"tokens_in": 721979117,   "usd": 510.0

Its reader concluded the run had bought 722M tokens at input rates. 96.8% of that total was cache reads, billed at a tenth of the input rate. The reader's output already held the split; the pr-cost/v1 contract had no field for it, so the comment printed the merged number alone and the reader supplied the missing premise.

I found this while auditing whether the superseded fix/pr-cost-cache-visibility branch was safe to delete — it held a test file main lacked, and three of its nine tests covered behaviour I had not carried over. That is what surfaced the gap.

What changed

The contract gains five additive, nullable keys, so schema_version stays pr-cost/v1 and a payload written before them still validates:

tokens_in_uncached, tokens_in_cache_read, tokens_in_cache_write, usd_basis, scope

Two rules the collector now enforces:

  • The split must sum to tokens_in when all three parts are present. A split that does not add up is worse than no split: both numbers reach the comment and a reader cannot tell which to believe.
  • scope defaults to session-total whenever tokens_in is present. A session reader sums the whole session, which may cover other PRs and unrelated work; unlabelled, those numbers read as this PR's cost.

usd_basis moves out of the free-text note into its own field, so the comment can say what the figure was priced from. default-rates means flat lane rates were used, not the rates of the model named in the payload — which is how a wrong price can still look right.

The payoff

The comment now carries a human summary above the JSON, because the JSON alone is what was misread. On the session that produced the original comment:

**~$602.99** — claude-opus-5 — confidence: estimated
- Priced from: model-rates (the published rates of the model named above)
- Input 720,696,122 = 2,956 uncached + 697,885,763 cache read (96.8% of input)
  + 22,807,403 cache write. Cache reads bill at a fraction of the input rate,
  so this total is not input-priced.
- Output 1,038,408 tokens
- Covers: whole session, which may cover other PRs and unrelated work

The annotate recipe passes the three splits from the reader's existing keys, so they are a rename, not a recomputation.

Tests

All three new collector tests proved red against main, each for its own reason:

Test Red on main
test_scope_defaults_to_session_total_when_tokens_are_present KeyError: 'scope'
test_a_split_that_does_not_sum_to_tokens_in_is_refused PrCostError not raised (split of 300 against tokens_in 1000)
test_comment_body_shows_the_split_the_basis_and_the_scope '697,885,763 cache read' absent from the heading

The comment assertion reads the prose above the JSON block on purpose. The JSON always held these numbers, so matching the whole body would pass against the old comment.

A regression I caught and what caught it

Moving usd_basis out of --notes broke test_notes_follow_reader_basis, which asserted the basis by reading the note text. The property that file guards is unchanged — its docstring says the annotation preserves the reader's basis — so the check moved to the new field rather than being dropped. render_notes became render_flag, and the recipe gained three more checks for the token-split flags.

Worth stating plainly: python3 -m unittest discover skills/pr-cost/tests did not catch it. I had run that before editing annotate.md and not after. tests/run.sh caught it (156 pass / 1 fail), which is the argument for running the full suite rather than the narrow one.

Both new recipe assertions fail under their own mutation — deleting --usd-basis or --tokens-in-cache-read fails exactly the test that names it — proved against a scratchpad copy of the skill tree so the tracked recipe is never written.

Verification

tests/run.sh                                       -> 157 pass, 0 fail
python3 -m unittest discover skills/pr-cost/tests  -> 79 tests OK
pr_cost_doctor.py --self-check                     -> OK
ruff check (all changed files)                     -> clean

Plus the documented annotate recipe run end to end offline — synthetic transcript through reader, the five new flags, payload, comment — with the split verified to sum to tokens_in.

PR #35 fixed how the claude lane prices a session. It did not fix the
thing that started that work: the comment a person reads.

A posted comment on a real PR read `tokens_in 721,979,117` next to
`usd ~510`, and its reader concluded the run had bought 722M tokens at
input rates. 96.8% of that total was cache reads, billed at a tenth of
the input rate. The payload already held the split in the reader's
output; the pr-cost/v1 contract had no field for it, so the comment
printed the merged number alone and the reader supplied the missing
premise.

The contract gains five additive, nullable keys -- tokens_in_uncached,
tokens_in_cache_read, tokens_in_cache_write, usd_basis, scope -- so
schema_version stays pr-cost/v1: a payload written before them still
validates.

Two rules the collector now enforces:

- When all three parts are present they must sum to tokens_in. A split
  that does not add up is worse than no split: both numbers reach the
  comment and a reader cannot tell which to believe.
- scope defaults to session-total whenever tokens_in is present. A
  session reader sums the whole session, which may cover other PRs and
  unrelated work; unlabelled, those numbers read as this PR's cost.

The comment now carries a human summary above the JSON, because the JSON
alone is what was misread. On the session that produced the original
comment it renders:

  **~$602.99** - claude-opus-5 - confidence: estimated
  - Priced from: model-rates (the published rates of the model named above)
  - Input 720,696,122 = 2,956 uncached + 697,885,763 cache read
    (96.8% of input) + 22,807,403 cache write. Cache reads bill at a
    fraction of the input rate, so this total is not input-priced.
  - Output 1,038,408 tokens
  - Covers: whole session, which may cover other PRs and unrelated work

usd_basis moves out of the free-text note into its own field, where the
comment can say what the figure was priced from -- `default-rates` means
flat lane rates were used, NOT the rates of the model named in the
payload, which is how a wrong price can still look right.

The annotate recipe passes the three splits from the reader's existing
keys, so they are a rename rather than a recomputation.

All three tests proved red against main's collector, each for its own
reason: KeyError 'scope'; PrCostError not raised on a split summing to
300 against tokens_in 1000; and '697,885,763 cache read' absent from the
comment heading. The comment assertion reads the prose above the JSON
block on purpose -- the JSON always held these numbers, so matching the
whole body would pass against the old comment.

Verified: 79 pr-cost tests OK; doctor --self-check OK; ruff clean; and
the documented recipe run end to end offline, reader through payload to
comment, with the split summing to tokens_in.
test_notes_follow_reader_basis asserted the basis by reading --notes,
because free text was the only channel carrying it. The basis now has
its own --usd-basis field, so that assertion broke when the note stopped
repeating it.

The property this file guards is unchanged -- its docstring says the
annotation preserves the reader's reported basis -- so the check moves to
the field rather than being dropped. render_notes becomes render_flag,
which renders any one flag of the documented recipe through the same
key() helper, and the recipe now gets three more checks: the basis field
and the three token-split flags, which are what stop the comment showing
only a merged tokens_in. The surviving note caveat keeps its own test.

Both new assertions fail under their own mutation -- deleting
--usd-basis or --tokens-in-cache-read from the recipe fails exactly the
test that names it -- proved against a scratchpad copy of the skill tree
so the tracked recipe is never written.

Found by tests/run.sh, not by `unittest discover` over
skills/pr-cost/tests: I had run the narrower command before editing
annotate.md and did not re-run it after.
@cheshirecode
cheshirecode merged commit 804f999 into main Sep 11, 2026
5 checks passed
@cheshirecode
cheshirecode deleted the fix/pr-cost-readable-payload branch September 11, 2026 18:21
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