Price a session that switched models per stretch, not at the one it ended on - #128
Conversation
… one it ended on `Session::Data` held one `model` and one `usage`, so everything that turns tokens into money charged a whole session at the rate of whatever model it happened to end on: `smith stats`, the COST column, the running cost line, `smith sessions export`. The case #99 left behind — 100k prompt and 20k completion on `claude-opus-5`, then `/model claude-haiku-4-5` — reported $0.20 for $1.00 of work. That is the rule `pricing.cr` is built on, broken: a wrong cost figure is worse than no cost figure. Usage is recorded per provider/model stretch now, and priced per stretch. `Agent` already added *money* up per response at the rates in force, which is what made `--max-budget-usd` immune to this; it now counts tokens the same way, keyed by the model that was asked. The pair is put back together in `persist`, where the provider is known — `/model` switches the model and leaves the client, its key and its connection alone, which is why the agent need not track one. A list rather than a hash keyed by "provider/model": a model name may contain a slash, `anthropic/claude-sonnet-5` being how OpenRouter spells one, and a key that cannot be taken apart again is not a key. The baseline from #102 has a sibling, taken at the same moment and for the same reason: a run's split is added to what the session already had, not written over it. The running cost line reads `spent_usd` directly whenever a budget is set, so it and `BudgetExceeded` cannot disagree after a switch the way they did — one summed per turn while the other priced the lot at the current model. Nothing needs migrating. A record written before the split has one model and one block of usage, which *is* one segment, and is read as exactly that. One subtlety that cost a spec failure before it was found: a session that has spent nothing gets no segment at all, or the model a fresh session merely *declares* would enter the baseline and be reported as a model that was never asked anything. Two rules worth knowing, both in the README: `smith stats` lists a session under every model it used and still counts it once; and a session with any stretch on an unpriced model reports `n/a` rather than a sum quietly missing a part. Two specs, each verified against both halves — reverting the per-model persist or the per-segment aggregation fails them. Closes #103 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e /model moves Review found two ways to lose or misattribute a whole session's history. Both verified before fixing, both now have a spec that fails without the fix. `Data#segments` tested `usage.total_tokens.zero?`. That field is whatever the provider reported: openai, ollama and openrouter all default it to 0 when the key is missing while `prompt_tokens` and `completion_tokens` hold real numbers, and Anthropic computes it as input plus output, leaving out billable cache tokens. So a session holding 100k prompt and 20k completion with a reported total of 0 lost all of it on the next turn — from the segments, the COST column and `smith stats` — and lost it for good, since the following baseline reads the truncated list. `Usage#empty?` asks the four fields that carry the counts, which is the only test that cannot be lied to. `switch_model` wrote `session_data.model` and left the record to be saved with no segments. A record from before the split derives its one segment from `model`, so the fallback then re-read the entire lifetime as the model being switched *to*: the issue's own example, $1.00 of opus reported as $0.20 of haiku, arriving through the door of the feature that motivated #103. The split is written down before the model moves — once written, the past cannot be re-read. `run_cost` no longer short-circuits to `Agent#spent_usd` when a budget is set. The two agree wherever both are defined, so the disagreement with `BudgetExceeded` that #103 names is gone either way; where they differ, `spent_usd` is the wrong one to show. It is the enforcement figure and counts an unpriced stretch as nothing, so the line printed $0.00 for a model with no known rate where it used to print n/a, and a partial sum after a switch to one — answering "unknown" with "free", against the rule `output.cr` states outright. Three more, smaller: An empty segment list is truthy, so `||` in the export stopped there and hid the index row behind it, turning a never-run session's `$0.00` into `n/a` while the COST column still said `$0.00`. The README claimed one n/a rule for both a session's figure and the grand total, and only the first is true. They differ on purpose: one number describes one session and has to be right or absent, the other summarises many and shows which parts it could not price. Said that way now, in both places. `SessionExport.cost_of` had no callers left. `segments` hands back a copy, so a parked baseline cannot reach into the record it came from. The spec claim in the PR was also too strong, and the gap was real: nothing loaded a pre-#103 index from disk, which is the fallback that actually matters, since `smith stats` and the COST column read the index and not the session file. There is now a spec that writes one by hand — this build could only ever write the new shape — and it fails when that fallback is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review, final round. Both are leftovers from the commit before the one that deleted the `spent_usd` short-circuit, and both now say the opposite of the code they sit next to — the doc comment above `run_cost` contradicts the in-body comment eight lines below it. The reason each gives was never the point anyway. What `run_cost` and `BudgetExceeded` needed was to stop disagreeing after a switch, and pricing per stretch does that from this side; reading the agent's own total was the obvious way to get there and the wrong one, which the surviving comment already explains. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review durch einen Reviewer-Agenten, drei Runden: REQUEST CHANGES, dann APPROVE WITH NITS, dann grün. Zwei der Befunde waren Wege, die Geschichte einer Session zu verlieren oder falsch zuzuordnen. Blocker 1: der Leerheits-Guard prüfte ein Feld, das der Provider meldet
Eine Session mit 100k Prompt- und 20k Completion-Tokens und gemeldeter Null verlor beim nächsten Turn alles — aus den Segmenten, der COST-Spalte und
Blocker 2:
|
![code-review-smith[bot]](https://avatars.githubusercontent.com/in/4653728?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Code Review Smith
Reviewed
webmatze/smith #128
1. Summary
This is a well-reasoned, carefully-scoped fix: usage is split per provider/model stretch in Agent, persisted as a list of Session::UsageSegment, and priced per stretch everywhere a cost is shown. The compatibility story (old records read as the single segment they always meant, no migration), the usage.empty? test built on billed tokens rather than the provider-reported total_tokens, the array-instead-of-"provider/model"-hash rationale, and the settlement of usage_segments before session_data.model moves in switch_model are all the right calls, and the specs assert the negative case explicitly. My findings are about one unverified interaction (fork), an asymmetry between the two segments fallbacks, segment identity, and a spec that pokes agent.model instead of driving the real /model path.
2. Findings
🟡 Should consider
a) src/smith/session.cr:190 — does the fork path reset usage_segments?
#117 fixed the double-count by zeroing a fork's usage, on the reasoning that Stats.aggregate sums that field over index rows. This change moves the sum to segments, and to_index_entry (line 289) writes the resolved list, so usage_segments now dominates both stats and cost. The diff doesn't touch fork. If fork builds a fresh Session::Data, the property default saves you; if it loads the parent's record and zeroes usage, a fork inherits the parent's whole split and re-introduces exactly the #117 bug — now with a cost column that reads the parent's figure too. Please confirm, add usage_segments.clear next to the usage reset if it's missing, and pin it with a spec mirroring the #117 one. If it is missing, treat this as a must-fix.
b) src/smith/session.cr:125 — IndexEntry#segments lacks the usage.empty? guard that Data#segments (line 264) has.
The PR's own stated invariant is "a session that has spent nothing gets no segment", and the spec at 387 exists to protect it. But the index-side fallback only checks for nil, so a brand-new session's row — written with usage_segments: [] by to_index_entry and a zero usage — resolves to one phantom segment for the model it merely declared. Stats.aggregate (line 73) then counts it in with_usage and creates a models[key] row at 0 tokens / $0.00 for a model that was never asked anything, and session_export.cr:282 only avoids n/a by falling back into that phantom. Two rules for one concept is how this class of bug came about in the first place. Suggest return Array(UsageSegment).new if usage.nil? || usage.empty? here too — noting that it also stops never-run sessions counting towards with_usage, so check the existing stats specs before landing it.
c) src/smith/session.cr:36 — segment identity is the pricing key, not the pair that gets displayed.
same_target? compares Pricing.key_for(provider, model), but merge_segments then keeps the first segment's literal provider/model strings, and those are what the export prints and what ModelStat is constructed from. Any normalisation key_for does — case, an alias, a date suffix — silently collapses two distinct model names into one segment wearing the first one's name, and the second disappears from By model: with its tokens folded in. Segment identity should be the thing that's shown: provider == other.provider && model == other.model. Stats.aggregate already groups by Pricing.key_for at line 88, so the breakdown is unaffected either way, and provider always comes from one field so casing can't split a real stretch.
d) spec/smith/session_usage_spec.cr:289 — the headline spec switches by poking agent.model, not through switch_model.
The spec that names #103 sets agent.model = "claude-haiku-4-5" directly, so it exercises attribution + persist + merge but never the /model path this PR changed: the session_data.usage_segments = session_data.segments settlement (cli.cr:1408), the session_data.model write, and the save that rebuilds the index row from it. It also leaves session_data.model == "claude-opus-5" while the segments say otherwise, so nothing asserts the record ends up naming the model it switched to. The only spec using switch_model_for_spec (line 402) is the pre-split one and runs no turns. Drive this one through cli.switch_model_for_spec(session, agent, "claude-haiku-4-5") — the helper is already there — and add saved.model.should eq("claude-haiku-4-5"), so a regression in the settlement on a post-split record is caught and not just on a pre-split one.
e) src/smith/cli.cr:931 — two different sources for the provider name now feed one helper.
run_cost(provider.name, agent) here, but run_cost(session_data.provider, agent) at 1086 and 1250, while persist pairs every model key with session_data.provider at line 1005. If those two can ever disagree (a run started against a client other than the one the record names), the cost printed at the end of the run and the cost stored for it are priced at different providers' rates — and the stored one is what every later report reads. run_cost exists precisely to make the displayed figure and the enforcement figure agree; pick one provider source of truth for it as well.
🟢 Nitpicks
f) src/smith/cli.cr:2108 — the empty guard is a proxy. agent.usage_by_model.empty? means "nothing was counted" only because both are written in update_usage. agent.cumulative_usage.empty? — the method this PR just added — says the intent directly and can't drift if cumulative_usage is ever seeded from a baseline.
g) src/smith/agent.cr:538 — attribution reads @model at response time. The comment above it claims @model "is what was asked for and answered just now", which holds only while /model cannot be applied mid-turn. Capturing the model when the request goes out and passing it into update_usage keeps that claim true if the command ever becomes applicable while a turn streams.
h) src/smith/cli.cr:2110 — the PR description contradicts the code. The description's bullet says "the running cost line reads spent_usd directly whenever a budget is set"; the code and the CHANGELOG both say the opposite, and deliberately so. Worth correcting in the description so the merged record doesn't describe a design that was rejected.
i) src/smith/session_export.cr:153 — the new usage_by_model JSON field is undocumented. The README paragraph at 1078 says the export prices each stretch at its own rate but never mentions the new key or the By model: markdown block. The export's JSON is a machine-readable contract; a sentence in the README's export section (or the #95 CHANGELOG entry) naming the field and the >1-segment condition would keep consumers from discovering it by diff.
3. Verdict
Verdict: COMMENT
Generated by Code Review Smith v0.2.0 via the smith agent harness.
| # | ||
| # Empty in a record written before `/model` existed, and in one that has | ||
| # never run; `segments` is what to read. | ||
| property usage_segments : Array(UsageSegment) = Array(UsageSegment).new |
There was a problem hiding this comment.
usage_segments is now what stats and cost read (via to_index_entry at line 289), so the #117 fork fix — which zeroed usage to stop a fork re-adding its parent's history — only still holds if fork also clears this field. The diff doesn't touch the fork path. If fork loads the parent's record and zeroes usage rather than constructing a fresh Data, a fork inherits the parent's whole split and the double count is back, now with a cost column too. Please confirm, add usage_segments.clear beside the usage reset if it's missing, and pin it with a spec mirroring the #117 one; if it is missing this is a must-fix.
| # segment, and reading it as one says precisely what it always said. The | ||
| # fallback is the whole of the compatibility story — nothing rewrites an | ||
| # old row until a real turn saves the session anyway. | ||
| def segments : Array(UsageSegment) |
There was a problem hiding this comment.
IndexEntry#segments only guards against nil, while Data#segments (line 264) also guards with @usage.empty? — the invariant this PR states explicitly ("a session that has spent nothing gets no segment"). A brand-new session's index row is written with usage_segments: [] and a zero usage, so it resolves here to one phantom segment for the model the session merely declared: Stats.aggregate counts it in with_usage and opens a models[key] row at 0 tokens / $0.00 for a model never asked anything, and session_export.cr:282 avoids n/a only by falling back into it. Add return Array(UsageSegment).new if usage.nil? || usage.empty? so there is one rule, and check the existing stats specs since this also stops never-run sessions counting towards with_usage.
| end | ||
|
|
||
| def same_target?(other : UsageSegment) : Bool | ||
| Smith::Pricing.key_for(provider, model) == Smith::Pricing.key_for(other.provider, other.model) |
There was a problem hiding this comment.
Segment identity is Pricing.key_for(provider, model), but merge_segments keeps the first segment's literal strings and those are what the export prints and what ModelStat is built from. Any normalisation key_for performs (case, alias, date suffix) collapses two distinct model names into one segment wearing the first name, folding the second's tokens in and erasing it from By model:. Use the displayed pair instead — provider == other.provider && model == other.model; Stats.aggregate already groups by Pricing.key_for at line 88, so the breakdown is unaffected, and provider always comes from a single field so casing cannot split a real stretch.
| agent.model = "claude-opus-5" | ||
|
|
||
| agent.send("expensive turn") | ||
| agent.model = "claude-haiku-4-5" |
There was a problem hiding this comment.
The spec named after #103 switches models by assigning agent.model directly, so it never exercises the /model path this PR changed: the session_data.usage_segments = session_data.segments settlement (cli.cr:1408), the session_data.model write, and the save that rebuilds the index row from it. It also leaves session_data.model == "claude-opus-5" while the segments disagree, so nothing asserts the record ends up naming the model it switched to, and the only spec that calls switch_model_for_spec (line 402) is the pre-split one and runs no turns. Drive the switch through cli.switch_model_for_spec(session, agent, "claude-haiku-4-5") and add saved.model.should eq("claude-haiku-4-5"), so a regression in the settlement is caught on a post-split record too, not only on a pre-split one.
| shutdown_mcp | ||
| persist(session_data, agent) | ||
| renderer.finish(agent.cumulative_usage, cost_for(provider.name, agent.model, agent.cumulative_usage)) | ||
| renderer.finish(agent.cumulative_usage, run_cost(provider.name, agent)) |
There was a problem hiding this comment.
This call passes provider.name while lines 1086 and 1250 pass session_data.provider, and persist pairs every model key with session_data.provider at line 1005. If those two can ever disagree — a run started against a client other than the one the record names — the cost printed at the end of the run and the cost stored for it are priced at different providers' rates, and the stored one is what every later report reads. run_cost exists to make the displayed and enforced figures agree; give it one provider source of truth as well.
| private def run_cost(provider_name : String, agent : Agent) : Float64? | ||
| # Nothing counted yet: no segments to price and no model to blame, so | ||
| # the answer is the one a zero-usage run always gave. | ||
| return cost_for(provider_name, agent.model, agent.cumulative_usage) if agent.usage_by_model.empty? |
There was a problem hiding this comment.
agent.usage_by_model.empty? is a proxy for "nothing was counted", true only because both fields are written together in update_usage. agent.cumulative_usage.empty? — the method this PR adds — states the intent directly and cannot drift if cumulative_usage is ever seeded from a baseline.
| @cumulative_usage += u | ||
| # `@model` is what was asked for and answered just now, which is what | ||
| # makes this attribution right rather than approximate. | ||
| @usage_by_model[@model] = (@usage_by_model[@model]? || LLM::Usage.new(0, 0, 0)) + u |
There was a problem hiding this comment.
Attribution reads @model when the response arrives, and the comment above claims that is "what was asked for and answered just now" — which holds only while /model cannot be applied mid-turn. Capturing the model as the request goes out and passing it into update_usage keeps that claim true if the command ever becomes applicable while a turn is streaming.
| # the answer is the one a zero-usage run always gave. | ||
| return cost_for(provider_name, agent.model, agent.cumulative_usage) if agent.usage_by_model.empty? | ||
|
|
||
| # Priced from the segments rather than read off `Agent#spent_usd`, which |
There was a problem hiding this comment.
The code and the CHANGELOG both say the running cost line is priced from the segments rather than read off Agent#spent_usd, but the PR description states the opposite ("reads spent_usd directly whenever a budget is set"). Correct the description so the merged record doesn't describe the design that was deliberately rejected.
| json.field "cost_usd", @cost | ||
| # Only where it adds something: one segment is the header again. | ||
| if @usage_segments.size > 1 | ||
| json.field("usage_by_model") do |
There was a problem hiding this comment.
The new usage_by_model JSON key and the By model: markdown block are not documented: README line 1078 says only that the export prices each stretch at its own rate. The export's JSON is a machine-readable contract, so a sentence in the README's export section (or the #95 CHANGELOG entry) naming the field and the more-than-one-segment condition would save consumers from discovering it by diff.
1 participant
Closes #103
Session::Dataheld onemodeland oneusage, so everything that turns tokens into money charged an entire session at the rate of whatever model it ended on. The case #99 left behind: 100k prompt + 20k completion onclaude-opus-5, then/model claude-haiku-4-5, reported as $0.20 for $1.00 of work.The shape
Usage is recorded per provider/model stretch and priced per stretch.
Agentgainsusage_by_model. It already added money up per response at the rates in force — that is what made--max-budget-usdimmune to this — and now counts tokens the same way, keyed by the model that was asked. Keyed by model alone:/modelswitches the model and leaves the client, its key and its connection alone, so the provider is not the agent's to track.Session::UsageSegment— a list, not a hash keyed by"provider/model". A model name may contain a slash (anthropic/claude-sonnet-5is how OpenRouter spells one), and a key that cannot be taken apart again is not a key.Data#usagestays the total. Plenty reads it and none of that wants to know about models. The split is for pricing, where the rate differs per stretch.Stats.aggregateiterates segments. Per entry, not per segment, for the session count — a session that switched is still one session.spent_usddirectly whenever a budget is set. That is criterion 4: the line andBudgetExceededcould not agree after a switch, because one summed per turn and the other priced the lot at the current model.smith sessions exportprints the split, in Markdown and JSON, but only when there is more than one segment — one segment is the header again.No migration
A record written before the split has one model and one block of usage, which is one segment, and is read as exactly that. Old rows are rewritten only when a real turn saves the session anyway. A spec pins this.
One subtlety that cost a spec failure before it was found: a session that has spent nothing gets no segment. Otherwise the model a fresh session merely declares enters the baseline through
build_agentand gets merged into the run's real segments — and is then reported as a model that was never asked anything. The first version of this had that bug and the spec caught it as a phantom third model.Two rules, both now in the README
smith statslists a session under every model it used, and still counts it as one session.n/a— not a sum quietly missing a part. Same rule as a single-model session;pricing.crwould rather say nothing than say something wrong.The known-limitation note is gone
README.mdcarried it explicitly, and criterion 2 asks for its removal once the thing is true. The/modelCHANGELOG entry for #94 now points forward to #103 rather than describing the limit as current.Specs
Two, and each was verified against both halves of the change: reverting the per-model persist (one segment for the ending model) fails them, and so does reverting the per-segment aggregation (
entry.segments[0, 1]). One drives a real switch throughbuild_agent/persistand asserts the COST column,Stats.aggregate, the by-model breakdown and the session count — including an explicit assertion that the figure is not the old ending-model one. The other reads a segment-less record and pins the fallback.crystal spec: 1434 examples, 0 failures.crystal tool format --check: clean.🤖 Generated with Claude Code