Repository navigation
fix(jsonl): dedup cumulative token counts by message.id (#8) - #9
Merged
Merged
Conversation
…c#549) Claude Code writes one JSONL entry per content block of a single API call, all sharing one message.id. The cumulative token path summed every entry, inflating Tokens Input/Output/Total by up to ~1.84x. Mirror the grouping already used for Tokens Last Turn: consecutive counted entries sharing an id contribute one usage (prompt side once, output as the running max). - transcripts without duplicate-id entries are unchanged (existing tests) - streaming partials resolve to the finalized value, counted once mid-stream - sidechain/API-error rows keep their current treatment in sums and recency Added vitest cases on a synthetic sirmalloc#549-shaped transcript asserting exact cumulative totals (12 in / 292 out / 16660 total vs inflated 16/393/...), streaming partials with ids, and a sidechain per-content-block group. Co-Authored-By: Claude Code <[email protected]>
axisrow
commented
Sep 25, 2026
axisrow
left a comment
axisrow
left a comment
Owner
Author
There was a problem hiding this comment.
Approving - no changes requested. Reviewed the full diff 51a8819..fa9b4e2 by tracing the implementation.
Verified:
- Grouping mirrors the existing last-turn dedup: one group window, delta-only output for a matching id, full accumulation for a new group. Entries without an id always start a new group, so no-id transcripts behave exactly as before; the existing no-id tests cover the parity claim.
- The one-time hasStopReasonField reset also resets the new dedup state (countedMessageId/countedOutputTokens), so legacy pre-stop-reason rows cannot spuriously match post-reset ids.
- The finishTokenMetrics revival path composes correctly: a trailing stop_reason=null entry either matches the last counted group (output delta applied) or forms a new group; no double counting, because stop-null entries are skipped at collect time once hasStopReasonField is set.
- The delta>0 guard gives max-wins on output, matching the Math.max semantics of the last-turn widget.
- The trackLastTurnTokens refactor is behavior-preserving: empty or undefined ids normalize to null and can never match.
- Test expectations trace correctly against the implementation (12/292/16356/16660/16258; 5/200/375/580/228; 910/700/5130/6740/40).
One informational note inline; nothing blocking.
Review follow-up on #9: the duplicate early return deliberately keeps the context-length snapshot from the group's first counted entry. Write the invariant down at the return so it survives as a conscious assumption. Co-Authored-By: Claude Code <[email protected]>
axisrow
commented
Sep 25, 2026
axisrow
left a comment
axisrow
left a comment
Owner
Author
There was a problem hiding this comment.
Re-reviewing the updated head e2db47b (follow-up to review 5319914008 on fa9b4e2). Approving - no changes requested.
The new commit is comment-only (4 lines at the duplicate early return in accumulateCumulativeTokenEntry) and documents exactly the invariant flagged in the previous review: within one message.id group the recency snapshot stays pinned to the first counted entry, which is safe because per-block entries repeat identical prompt-side usage and contextLength never reads output_tokens. No code changes; everything verified in the previous review still applies to this head:
- one group window, delta-only output for a matching id, full accumulation for a new group; no-id entries always start a new group, so no-id transcripts behave exactly as before;
- the one-time hasStopReasonField reset also resets the dedup state, so legacy rows cannot spuriously match post-reset ids;
- the finishTokenMetrics revival path composes without double counting;
- max-wins on output matches the last-turn widget Math.max semantics;
- test expectations trace correctly against the implementation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #8 — cumulative token over-reporting (upstream sirmalloc#549).
One JSONL entry per content block shares one
message.id, and the cumulative token path (Tokens Input / Output / Total) summed every entry, inflating counts by up to ~1.84x. This PR applies the same per-API-call grouping already used by theTokens Last Turnwidget to the cumulative path ingetTranscriptAnalysis: entries are grouped bymessage.idwith the maxoutput_tokenskept per group; entries without an id start a new group.Transcripts without duplicate-id entries produce identical numbers to before — all pre-existing tests pass unchanged.
Before / after (synthetic sirmalloc#549-shaped transcript)
Edge cases covered by tests:
contextLength);Tests
bun test: 2377 pass / 0 fail (single final run)bun run lint: clean, no suppressionsRefs: sirmalloc#549.
🤖 Generated with Claude Code