fix: attribute compaction summary costs to the model that generated them#3760
Merged
Conversation
Adds Model+Usage to compactor.Result and session.Item (migration 25), threads them through SessionSummaryEvent, persistence, and the server AddSummary path so the TUI /cost By Model breakdown includes compaction spend instead of silently dropping it.
Add chat.Usage.Add helper; validate/normalize summary attribution in server; thread model/usage through client API via session.Item; deep-copy usage pointer in persistence observer and snapshotItems; document last-model-wins in runUsage. Assisted-By: Claude
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The PR threads compaction model/usage attribution through a well-structured chain: compactor.Result → session.Item → SessionSummaryEvent → PersistenceObserver → SQLite store (migration 25). The implementation is sound:
- Deep-copy discipline is maintained throughout — usage pointers are copied in
Clone,snapshotItems,cloneSessionItem, and the persistence observer, preventing aliasing bugs. - Migration 25 correctly adds
NOT NULL DEFAULT ''columns tosession_itemswithout breaking existing rows. - Wire protocol preserves backward compat via the legacy
tokensJSON field inAddSummaryRequest. - Server-side validation (
validateSummaryAttribution) correctly guards against negative token counts, overlong model names, and control characters before persisting. - Cost dialog (
addCompactionCost) correctly attributes compaction spend to the model bucket and accumulates tokens into per-model totals. - Branch/clone:
cloneSessionItemcorrectly copies the newModelfield and deep-copies theUsagepointer;FirstKeptEntrypreservation in branch summary items is handled.
No high or medium severity bugs were found. Low-severity observations (not blocking):
runUsageincompactor.goreadssess.Messageswithout acquiring the session mutex — safe in the current single-owner context but inconsistent with the codebase'ssnapshotItems()convention.addCompactionCostsubstitutes a zerochat.Usage{}for nil — harmless, but leaves a zero-token row in the By Agent compaction bucket for pre-migration summary items.
melmennaoui
approved these changes
Jul 20, 2026
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
3 participants
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.
When an agent uses a
compaction_model, the/costdialog's "By Model" section showed nothing for that model — its token spend was silently folded into the session total and a generic "compaction" bucket, with no per-model breakdown. This made it impossible to reason about or compare compaction costs across models.The fix threads the compaction model name and token usage from the compaction sub-run through
compactor.Result, intosession.Item(newmodelandusage_jsoncolumns via SQLite migration 25), throughSessionSummaryEvent, and into the TUI cost dialog, which now attributes compaction spend to the model that generated the summary and includes its tokens in the per-model totals.Review follow-ups are bundled in a second commit: a shared
chat.Usage.Addhelper, server-side validation of summary attribution metadata (negative token counts, model name length and control characters, zero-usage normalization), Go client API carrying the full summary item with"tokens"wire-name compatibility forfirst_kept_entry, deep-copied usage pointers, and a standalone fix preservingFirstKeptEntrywhen branching summary items. New tests cover compactor model/usage reporting, SQLite round-trip, persistence observer, server validation, branch cloning, and the dialog's by-model attribution.