From ffd608a3e26460ca49fa7e10ba7cb37ea75bc4c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Fri, 11 Sep 2026 01:34:57 +0200 Subject: [PATCH 1/3] fix: a session that switched models is priced per stretch, not at the one it ended on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- CHANGELOG.md | 4 +- README.md | 2 +- spec/smith/session_usage_spec.cr | 62 +++++++++++++++ src/smith/agent.cr | 13 ++++ src/smith/cli.cr | 47 +++++++++++- src/smith/session.cr | 127 +++++++++++++++++++++++++++++-- src/smith/session_export.cr | 54 ++++++++++++- src/smith/stats.cr | 35 +++++---- 8 files changed, 316 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 56918fb..ab3be61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,13 +9,15 @@ All notable changes to smith. The format follows [Keep a Changelog](https://keep - **`${VAR}` works in a stdio server's `env`, as it already did in an HTTP server's `headers`**: `{"env": {"GITHUB_TOKEN": "${GITHUB_TOKEN}"}}` in `mcp.json` now hands the child what smith's own environment holds, instead of the fifteen literal characters `${GITHUB_TOKEN}` — which a server receives as a token and fails on somewhere far from the cause. The two halves of an entry had drifted: `headers` was given the expansion when HTTP servers arrived, `env` two methods away kept taking values verbatim, and the only ways left to give a subprocess a secret were to write it into a file that gets committed or to rely on the child inheriting smith's entire environment. The second is what #109 is about stopping, and it cannot be stopped while there is no deliberate way to pass one — so this comes first. One implementation serves both now, so a `${VAR}` cannot come to mean two things depending on which half of an entry it was written in, and the warning for an unset variable names the entry it was written in — `env 'GITHUB_TOKEN'` where before there was no warning at all, `header 'Authorization'` exactly as before, its closing words now reading *using an empty value* rather than *sending it empty*, which is the one thing about this that an existing HTTP user will notice. Only the exact `${NAME}` shape is a reference: a bare `$`, a `$5` and a `${not-a-name}` are left as written, and numbers and booleans keep arriving as the strings they obviously mean, so `"PORT": 8080` still needs no quotes (#122). - **Plugins & marketplaces, in Claude Code's own format**: `smith plugin marketplace add owner/repo` registers a marketplace — a repository or a directory with `.claude-plugin/marketplace.json` — and `smith plugin install @` copies one of its plugins into `~/.smith/plugins/installed/`, from where its `skills//SKILL.md`, its root `SKILL.md` and its `agents/*.md` load like any other. `marketplace list|remove|update` and `plugin list|uninstall|update` complete the set; removing a marketplace removes the plugins that came from it, and a version that has not moved reports "up to date" instead of copying again. A plugin skill is `:` and a plugin agent `:` — the namespaced name is the guaranteed address, so installing a plugin can never change what an existing `/deploy` means. The bare name works too, but only where no other source claims it; where one does, the clash is reported once and only the full name resolves. `smith skills list` and `smith agents list` name the origin of every plugin entry. Deliberately not loaded, and said out loud rather than dropped: `hooks` — code execution outside the approval gate, which needs the trust store's digest model first — plus `.mcp.json`/`mcpServers`, `lspServers`, `commands`, themes and monitors, each named in a warning and in the install summary. `npm` and `command` sources are refused permanently, `git-subdir` and `archive` are Phase 2, and a marketplace source may only be `https://`, `owner/repo` or a local path: `ext::` is remote code execution and the rest cannot be checked. A source leaving the marketplace root, a name that could not be a directory and a symlink inside a plugin tree are all refused — a marketplace is third-party data. Startup pays one directory read and nothing else, because the `installed///` path segments *are* the provenance — and nothing a plugin brought with it warns on that path either: a marketplace ships definitions written for another harness by the dozen, so what smith could not read in one is said by `smith plugin install`, and again on demand by `smith skills list` and `smith agents list`, while a definition you wrote yourself keeps warning at startup exactly as before. `smith doctor` draws the same line by volume: your own broken files in full, a plugin's counted by the reason they share (#98). - **`smith update` replaces the binary from the newest release**: it reads the latest tag from the GitHub API, compares it against `Smith::VERSION`, downloads the archive for this platform over HTTPS, verifies its SHA-256 against the release's new `SHA256SUMS`, and renames the new file over the old one in the same directory, which is the only replacement a running executable survives. `--check` reports and changes nothing. The download runs through the same post-DNS SSRF guard `web_fetch` uses, plus three rules of its own: a non-https URL is refused rather than upgraded, since it came from an API answer and not from a human; the redirect off `github.com` to `objects.githubusercontent.com` **is** followed, where `web_fetch` refuses a cross-host hop, so an allow-list of GitHub's own hosts takes over that job and is applied afresh on every one of at most five hops; and a `smith` member that is not a plain regular file is refused, because a symlinked one would be chmod'ed and renamed straight through the link. Releases now carry a `SHA256SUMS`, and one that does not is refused unless it predates this feature or `--allow-unverified` is passed. Release builds are stamped as such at compile time, so a `make build` from a working tree refuses to overwrite itself instead of destroying a build nothing can reproduce — as do a Homebrew or Nix install, a distribution directory, and a directory the current user cannot write, each naming the command to run instead. A build newer than the newest release is never downgraded (#96). -- **`/model` switches the model mid-session**: bare it reports the model and provider in use, with a name it switches the model from the next request onward — the provider client, its API key and its connection stay as they are, which is all `-m` ever decided at startup. The new model is written to the session straight away, so `smith resume` comes back on it, and subagents spawned afterwards follow it. `--max-budget-usd` now adds each turn up at the rates in force when that turn was billed, so a switch cannot re-price money already spent. No allow-list stands in the way of a model released next week — a name must be a single word that is not a flag, a path or a provider, and anything else the provider rejects at the next request as an ordinary turn error. One session still records one model, so a session that switched is reported under the model it ended on by `smith stats` and the `COST` column (#94). +- **`/model` switches the model mid-session**: bare it reports the model and provider in use, with a name it switches the model from the next request onward — the provider client, its API key and its connection stay as they are, which is all `-m` ever decided at startup. The new model is written to the session straight away, so `smith resume` comes back on it, and subagents spawned afterwards follow it. `--max-budget-usd` now adds each turn up at the rates in force when that turn was billed, so a switch cannot re-price money already spent. No allow-list stands in the way of a model released next week — a name must be a single word that is not a flag, a path or a provider, and anything else the provider rejects at the next request as an ordinary turn error. One session recorded one model when this landed, so a session that switched was reported under the model it ended on by `smith stats` and the `COST` column; #103 has since split that (#94). - **`smith skills list` and `smith agents list`**: the two catalogs smith already builds are now readable. Both print each entry with its origin file and any file of the same name it shadows, and a warning about a file that lost a name clash names the file that won rather than reading as though the entry in use were broken. `skills list` prints every skill with its description, and — this is the point — names each `SKILL.md` that did not read as written: a `---` block that is never closed, a byte-order mark in front of it, or a line that is not `key: value` (a `tools:` over a YAML list arrives as no value at all). Such a file still loads, but under its directory name and without its description, which until now was invisible until `$skill-name` quietly failed to expand. `agents list` prints each definition with its path, description, provider, model, mode and effective tool list, so a definition that names no `tools` shows what its mode implies rather than a blank; the same header check warns for agent definitions too, through the `warn_io` they already warn on (#93). - **`smith sessions export `**: takes a run with you — Markdown on stdout by default, `--json` for the structured log, `--out ` for a file. The reference resolves the way `resume` and `sessions delete` resolve one, and the export reads the raw `transcript.jsonl` in preference to the compaction-shortened session file, naming its source and both message counts so a disagreement is visible. Attachments render as placeholders rather than base64, tool calls and thinking are abbreviated, non-UTF-8 and control bytes from tool output are scrubbed so the export stays text, unknown rates print `n/a`, and damage degrades: a broken index, an unreadable transcript line or a missing session file cost a warning, not the export. Nothing on the path builds a provider or needs an API key (#95). - **`smith doctor`**: one command that asks every source a run depends on before a session exists — which provider keys are set (`set`/`missing`, never a value), which `config.toml` was loaded and what the `[defaults]` chain resolves to, whether Ollama answers and with which models, a real `initialize` handshake per MCP server, the configured search backend and its key, the sandbox by an actual trial run rather than the presence of a binary, and the version, `SMITH_HOME`, instruction files and catalog counts — with any skill or agent file that did not load named rather than merely missing from the count. Every probe carries its own deadline and they run at once, so a wholly unreachable setup answers in about three seconds, and a probe that could not reach a verdict fails rather than passing quietly. Nothing it starts outlives it, and nothing a server or a config file wrote reaches the output. A stdio server inherits smith's environment, an HTTP server picks its own error body, and either can write anything into a JSON-RPC error message — so a server's words are replaced by what smith worked out for itself (a status, an error code, a timeout) rather than filtered, and the rule travels with the value so a new way for a server to talk cannot become a new way for it to be quoted. From the config file: an argument list is summarised by its length and a url is cut back to scheme, host and port. `smith mcp list` shows that trimmed form in its `COMMAND` column and keeps the server's own words, where they are the answer being looked for. A `fail` sets the exit code to 1; a `warn` leaves it alone (#92). ### Fixed +- **A session that switched models is no longer priced as though it had only ever used the last one**: `Session::Data` held one `model` and one `usage`, so everything that turns tokens into money — `smith stats`, the `COST` column of `smith sessions`, the running cost line, `smith sessions export` — charged an entire session at the rate of whatever model it happened to end on. Measured on the case #99 left behind: 100k prompt and 20k completion tokens spent on `claude-opus-5`, then `/model claude-haiku-4-5`, and the report reads $0.20 for $1.00 of work. It breaks the rule `pricing.cr` is built on, that a wrong cost figure is worse than none. Usage is now recorded per provider/model stretch 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, by the model that was asked, and the running cost line reads `spent_usd` directly whenever a budget is set, so the line and `BudgetExceeded` cannot disagree after a switch the way they did. 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. A session that never switches is unchanged in every output. Two rules worth knowing: `smith stats` lists a session under every model it used and still counts it as one session, and a session with any stretch on a model with no known price reports `n/a` rather than a sum quietly missing a part (#103). + - **A forked session no longer makes `smith stats` count its parent's history again**: `fork` copied `usage` along with the transcript, and `Stats.aggregate` sums that field over the rows of the index — so every fork added an entire history to the grand total that had only ever been spent once. Forking a long session three times reported roughly four times what it cost. The copying was always there, but it used to mean something smaller: before #102 the field held the *last run*, and a fork inherited one run's tokens; #102 made it the session's *lifetime*, and the same line quietly began inheriting all of it. A fork now starts at zero, which is the only reading of "what this session spent" that a sum over sessions can be taken of. Nothing is undercharged by that, which is worth stating because the obvious worry is the wrong one: the baseline for a run is taken from this same field, the agent is built with the whole inherited transcript, and the first response is billed for all of it — as prompt tokens, or as cache reads where the parent's cache is still warm, which is the likely case precisely here, a fork's transcript being byte-identical to the one the parent just sent. Both are priced, so re-sending what it inherited appears in the fork's own `COST` column either way. What stays with the parent is what the parent spent *producing* the transcript, counted once, where it was spent. A fork made before this release does not merely keep its inherited figure, it carries it: the baseline for every later run is read from that same field, so the excess is re-added on every save and the row stays over by exactly the inherited amount for the life of the session. It is not migrated, because `parent_id` says which session a fork came from but nothing recorded what that session had spent *at the time of the fork*, and the parent has run since — so any retroactive correction would be a guess, which is the one thing a cost figure may not be. There is no in-product way to reset it short of deleting the session (#117). - **A server that explains itself and exits is no longer quoted as having said nothing**: the line an MCP server writes to stderr on its way out is the whole answer to "why will this not start" — a missing argument, a token refused on sight — and whether it survived was a race between two fibers over one file descriptor. `Process#wait` closes all three pipes in its `ensure`, so the fiber that reaps the process ends the stderr drain not by reaching the end of the stream but by taking the descriptor away from it, discarding whatever the process wrote and nothing had read yet; `close` did the same from the other side, closing the read end before the drain had got there. Neither was waiting for anything — the drain fiber only had to have been given a turn first, and usually it was, because `wait` blocks on a channel before it closes anything and that turn fell out of the blocking. "Usually" is the whole complaint: it showed as `spec/mcp/manager_spec.cr:208` failing on macOS CI for pull requests that touch nothing nearby, `TOKEN-from-the-child` missing from a line reading `could not write to the MCP server: … Broken pipe`, green on a re-run of the same commit — and what it stands for is `smith mcp list` printing a bare `Broken pipe` for a server that said exactly why it quit. Both sides now yield to the drain first, and both are capped, which is the part that took a second attempt: an *unbounded* wait before reaping turned out to cost 6.3 seconds per server at shutdown, because stderr ends when the **last** write end closes and a grandchild that inherited fd 2 — a wrapper that backgrounds something, a server with a worker — holds it open long after its parent is gone, so reaping waited for a shutdown that was waiting for the reaping and only both graces timing out broke the circle. Bounded at 250 ms, the same wait ends in a scheduler turn wherever the drain can finish, and gives up where it never will. The price is bounded and named: a server that both answers in under a quarter of a second *and* leaves a helper holding stderr open pays the remainder of the cap once, at `max(0, 250 ms − however long the transport has been up)` — not per shutdown, and concurrently rather than one after another where several servers are involved, so `smith doctor`, which performs a full handshake before it shuts anything down, is past the cap before it gets there. How often the loss actually struck is not quoted here on purpose: it is a scheduling race, the probe that reproduces it does so only on a loaded machine, and the honest summary is that it happened, that it is now covered from both sides, and that the covering is bounded (#114). diff --git a/README.md b/README.md index 4e90c94..10272a5 100644 --- a/README.md +++ b/README.md @@ -1075,7 +1075,7 @@ The built-in commands, resolved before skill expansion (a skill of the same name `/model` is the one built-in that works both bare and with an argument: on its own it reports the model and provider in use, and with a name it switches the model from the next request onward. Only the name on the wire changes — the provider client, its API key and its connection stay as they are, which is exactly what `-m` decides at startup. The new model is written to the session immediately, so `smith resume` comes back on it. Switching *provider* is not offered: that needs a different client and a different API key, so it stays a restart with `--provider`. -**Known limitation — cost reporting after a switch.** A session records *one* model, so a session that switched models is reported under the model it ended on: `smith stats`, the `COST` column of `smith sessions` and the running cost line price its whole token usage at that model's rate. Splitting a session's usage per model is a larger change to the session record than switching the model is. `--max-budget-usd` is *not* affected — it adds each turn up at the rates that were in force when that turn was billed, so a switch never moves money already spent. +**Cost reporting after a switch.** A session records what each model used separately, so `smith stats`, the `COST` column of `smith sessions`, the running cost line and `smith sessions export` all price each stretch at its own rate rather than at the model the session happens to have ended on. `smith stats` lists the session under every model it used, and counts it once. A session recorded before this existed has no split and is read as the one model it named, which is what it always meant — nothing needs migrating. If any stretch ran on a model with no known price the whole figure is `n/a` rather than a sum missing one part, the same rule that governs a single-model session. The name is not validated against a list, because there is no honest offline list of every model a provider will accept — a hardcoded one would reject models released next week. What is checked is what can be: a name must be a single word, and a provider name given by mistake (`/model anthropic`) is named as such. A model that does not exist is rejected by the provider at the next request and reported as a turn error; the session stays open, and another `/model` puts it right. diff --git a/spec/smith/session_usage_spec.cr b/spec/smith/session_usage_spec.cr index 412f935..2737bd7 100644 --- a/spec/smith/session_usage_spec.cr +++ b/spec/smith/session_usage_spec.cr @@ -269,6 +269,68 @@ describe "a session's lifetime usage across resumes" do end end + it "prices a session that switched models at both rates, not at the one it ended on" do + # #103. `claude-opus-5` and `claude-haiku-4-5` are an order of magnitude + # apart, so pricing everything at the model a session happens to end on is + # not a rounding error — it is the difference the report exists to show. + with_cli do |cli| + session = cli.store_for_spec.create(model: "claude-opus-5", provider: "anthropic") + agent = cli.build_agent_for_spec(BillingProvider.new, session) + # What `-m` decides at startup; the harness's provider would otherwise + # answer on its own default and the switch below would be the second of + # three models rather than the second of two. + agent.model = "claude-opus-5" + + agent.send("expensive turn") + agent.model = "claude-haiku-4-5" + agent.send("cheap turn") + cli.persist_for_spec(session, agent) + + # Two segments, one per model, each with its own half of the tokens. + saved = cli.store_for_spec.load(session.id) + saved.segments.map(&.model).sort!.should eq(["claude-haiku-4-5", "claude-opus-5"]) + saved.segments.each(&.usage.total_tokens.should eq(120)) + # And the total is still the total. + saved.usage.total_tokens.should eq(240) + + # The COST column and `smith stats` both price per segment now. + entry = cli.store_for_spec.list.find { |row| row.id == session.id }.not_nil! + opus = Smith::Pricing.estimate(TURN_USAGE, "anthropic", "claude-opus-5").not_nil! + haiku = Smith::Pricing.estimate(TURN_USAGE, "anthropic", "claude-haiku-4-5").not_nil! + + entry.cost.not_nil!.should be_close(opus + haiku, 1e-9) + Smith::Stats.aggregate([entry]).cost.not_nil!.should be_close(opus + haiku, 1e-9) + + # The old behaviour, for contrast: everything at the ending model would + # have been this, and it is not what is reported any more. + ended_on = Smith::Pricing.estimate(saved.usage, "anthropic", "claude-haiku-4-5").not_nil! + entry.cost.not_nil!.should_not be_close(ended_on, 1e-9) + + # `smith stats` breaks it down by model rather than filing it under one. + by_model = Smith::Stats.aggregate([entry]).by_model + by_model.map(&.model).sort!.should eq(["claude-haiku-4-5", "claude-opus-5"]) + # One session, counted once, however many models it used. + Smith::Stats.aggregate([entry]).with_usage.should eq(1) + end + end + + it "reads a record written before the split as the one model it recorded" do + # No migration: an old session file has a total and a model and no + # segments, which is exactly one segment, and it prices as it always did. + with_cli do |cli| + session = cli.store_for_spec.create(model: "claude-opus-5", provider: "anthropic") + session.usage = TURN_USAGE + session.usage_segments.clear + cli.store_for_spec.save(session) + + entry = cli.store_for_spec.list.find { |row| row.id == session.id }.not_nil! + expected = Smith::Pricing.estimate(TURN_USAGE, "anthropic", "claude-opus-5").not_nil! + + entry.cost.not_nil!.should be_close(expected, 1e-9) + Smith::Stats.aggregate([entry]).by_model.map(&.model).should eq(["claude-opus-5"]) + end + end + it "leaves --max-budget-usd a per-run limit" do # The budget runs off `spent_usd`, which the agent counts for itself and # which no baseline touches. A resumed session with a long history starts diff --git a/src/smith/agent.cr b/src/smith/agent.cr index 8e8b27f..e9cdabe 100644 --- a/src/smith/agent.cr +++ b/src/smith/agent.cr @@ -72,6 +72,16 @@ module Smith # and ending the run over money never spent when it is to a dearer one. getter spent_usd : Float64 = 0.0 + # What this run used, split by the model in force when each response + # arrived. The same reason `spent_usd` adds up per response rather than + # pricing the total once: after `/model` there is no single model the run + # was charged at, so there is no single one to attribute its tokens to. + # + # Keyed by model alone, not provider: `/model` changes the model and + # leaves the provider, its key and its connection exactly as they were. + # Whoever persists this knows which provider it belongs to. + getter usage_by_model : Hash(String, LLM::Usage) = Hash(String, LLM::Usage).new + def initialize( @provider : LLM::Provider, @registry : Tools::Registry = Tools::Registry.default, @@ -523,6 +533,9 @@ module Smith private def update_usage(u : LLM::Usage) @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 # A stretch with no known rate contributes nothing rather than a guess — # the CLI is what says so out loud when a budget is set. if rates = @rates diff --git a/src/smith/cli.cr b/src/smith/cli.cr index 9bda220..2e6a690 100644 --- a/src/smith/cli.cr +++ b/src/smith/cli.cr @@ -816,7 +816,13 @@ module Smith # five call sites. A run start, `smith resume`, `smith -c` and the # `/resume` inside either loop all reach a new agent through here, and # the last of those is the one a baseline taken at process start misses. - session_data.try { |data| data.usage_before_run = data.usage } + session_data.try do |data| + data.usage_before_run = data.usage + # Taken here for the same reason and at the same moment as the total + # above — the run's split has to be added to what the session already + # had, not written over it. + data.segments_before_run = data.segments + end agent = Agent.new( provider: provider, @@ -922,7 +928,7 @@ module Smith shutdown_bash_jobs 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)) # A failed provider call must not report success to a calling script. exit(renderer.exit_code) @@ -989,6 +995,16 @@ module Smith # it in would count turn one again on turn two, and again on turn three. # Against a baseline it is the same answer however often it is written. session_data.usage = session_data.usage_before_run + agent.cumulative_usage + # The provider comes from the record because `/model` cannot change it: + # it switches the model and leaves the client, its key and its + # connection alone. The agent therefore counts by model and this is + # where the pair is put back together. + session_data.usage_segments = Session.merge_segments( + session_data.segments_before_run, + agent.usage_by_model.map do |model, usage| + Session::UsageSegment.new(session_data.provider, model, usage) + end + ) session_data.todos = @todos.items session_data.context_ratio = agent.context_ratio @session_store.save(session_data) @@ -1067,7 +1083,7 @@ module Smith shutdown_bash_jobs shutdown_mcp persist(session_data, agent) - renderer.finish(agent.cumulative_usage, cost_for(session_data.provider, agent.model, agent.cumulative_usage)) + renderer.finish(agent.cumulative_usage, run_cost(session_data.provider, agent)) exit(renderer.exit_code) end @@ -1231,7 +1247,7 @@ module Smith else submit(agent, trimmed) persist(session_data, agent) - if cost = cost_for(session_data.provider, agent.model, agent.cumulative_usage) + if cost = run_cost(session_data.provider, agent) app.cost_text = "#{Smith::Pricing.format(cost)}" end app.turn_finished @@ -2069,6 +2085,29 @@ module Smith Pricing.estimate(usage, provider_name, model, @config.pricing) end + # What the run has cost, priced per stretch rather than all at the model + # it happens to have ended on. + # + # With a budget set the agent has already added it up, per response, at + # the rates in force when each one arrived — so reading that is not an + # optimisation but the only way the line and `BudgetExceeded` can agree. + # They did not, after a switch: one summed per turn and the other priced + # the lot at the current model. + private def run_cost(provider_name : String, agent : Agent) : Float64? + return agent.spent_usd unless @max_budget_usd.nil? + + # Nothing counted yet: there are 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? + + Session.cost_of( + agent.usage_by_model.map do |model, usage| + Session::UsageSegment.new(provider_name, model, usage) + end, + @config.pricing + ) + end + # A budget without a price for the model in use is not a budget. Saying so # once, loudly, beats letting an automated run believe it is capped. private def budget_rates(provider_name : String, model : String) : Pricing::Rates? diff --git a/src/smith/session.cr b/src/smith/session.cr index b877b55..6fe2fb5 100644 --- a/src/smith/session.cr +++ b/src/smith/session.cr @@ -15,6 +15,67 @@ module Smith::Session class NotFound < ArgumentError end + # What one provider/model stretch of a session used. A session that never + # switched has exactly one of these; `/model` adds another. + # + # An array rather than a hash keyed by "provider/model", because a model + # name is allowed to contain a slash — `anthropic/claude-sonnet-5` is how + # OpenRouter spells one — and a key that cannot be taken apart again is not + # a key. + struct UsageSegment + include JSON::Serializable + + getter provider : String + getter model : String + getter usage : Smith::LLM::Usage + + def initialize(@provider : String, @model : String, @usage : Smith::LLM::Usage) + end + + def same_target?(other : UsageSegment) : Bool + Smith::Pricing.key_for(provider, model) == Smith::Pricing.key_for(other.provider, other.model) + end + + def +(other : UsageSegment) : UsageSegment + UsageSegment.new(provider, model, usage + other.usage) + end + end + + # Adds two lists of segments together, one entry per provider/model. Used + # wherever a run's segments meet the ones a session already had. + def self.merge_segments(base : Array(UsageSegment), addition : Array(UsageSegment)) : Array(UsageSegment) + result = base.map { |segment| segment } + + addition.each do |segment| + if index = result.index { |existing| existing.same_target?(segment) } + result[index] = result[index] + segment + else + result << segment + end + end + + result + end + + # What a list of segments cost, or nil if any one of them cannot be priced. + # + # nil rather than the sum of the parts that *are* known: a figure that + # silently omits one model is worse than no figure, which is the rule + # `pricing.cr` is built on. A session priced `n/a` because one stretch ran + # on an unknown model is telling the truth about itself. + def self.cost_of(segments : Array(UsageSegment), overrides : Smith::Pricing::Overrides? = nil) : Float64? + return nil if segments.empty? + + total = 0.0 + segments.each do |segment| + cost = Smith::Pricing.estimate(segment.usage, segment.provider, segment.model, overrides) + return nil if cost.nil? + total += cost + end + + total + end + struct IndexEntry include JSON::Serializable @@ -34,6 +95,11 @@ module Smith::Session getter model : String? = nil getter usage : Smith::LLM::Usage? = nil + # Usage split by the model that incurred it. Absent from every row written + # before `/model` could switch one, which is why `segments` below falls + # back rather than this being read directly. + getter usage_segments : Array(UsageSegment) = Array(UsageSegment).new + def initialize( @id : String, @created_at : Time, @@ -45,9 +111,28 @@ module Smith::Session @provider : String? = nil, @model : String? = nil, @usage : Smith::LLM::Usage? = nil, + @usage_segments : Array(UsageSegment) = Array(UsageSegment).new, ) end + # What to price, in the shape everything downstream wants. + # + # A row from before this existed has no segments and needs no migration: + # it recorded one model and one block of usage, which is exactly one + # 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) + return @usage_segments unless @usage_segments.empty? + + provider = @provider + model = @model + usage = @usage + return Array(UsageSegment).new if provider.nil? || model.nil? || usage.nil? + + [UsageSegment.new(provider, model, usage)] + end + # What to type to get this session back. def reference : String @name || @id @@ -57,12 +142,7 @@ module Smith::Session # overrides. nil when the rate is unknown or the entry predates usage # tracking — a wrong cost figure is worse than no cost figure. def cost(overrides : Smith::Pricing::Overrides? = nil) : Float64? - provider = @provider - model = @model - usage = @usage - return nil if provider.nil? || model.nil? || usage.nil? - - Smith::Pricing.estimate(usage, provider, model, overrides) + Session.cost_of(segments, overrides) end end @@ -101,6 +181,14 @@ module Smith::Session property messages : Array(Smith::LLM::Message) property usage : Smith::LLM::Usage + # The same total, split by the model that incurred it. `usage` stays the + # sum because plenty reads it and none of that wants to know about models + # — the split is for pricing, where the rate differs per stretch. + # + # 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 + # Sessions written before the todo tool existed simply have no field. The # same goes for name and parent_id: both default so older files load. property todos : Array(Smith::TodoList::Item) = Array(Smith::TodoList::Item).new @@ -133,6 +221,12 @@ module Smith::Session @[JSON::Field(ignore: true)] property usage_before_run : Smith::LLM::Usage = Smith::LLM::Usage.new(0, 0, 0) + # The same baseline for the split, taken at the same moment and for the + # same reason: the run's segments have to be added to what the session + # stood at, not assigned over it. + @[JSON::Field(ignore: true)] + property segments_before_run : Array(UsageSegment) = Array(UsageSegment).new + def initialize( @id : String, @cwd : String, @@ -159,6 +253,21 @@ module Smith::Session txt.size > 60 ? "#{txt[0..57]}..." : txt end + # As `IndexEntry#segments`: an old record recorded one model and one block + # of usage, and that is one segment. + # + # A record that has spent nothing gets no segment at all, which matters + # more than it looks: this list is what `build_agent` takes as a baseline, + # and a zero-token segment for the model a fresh session merely *declares* + # would be merged into the run's real ones and reported as a model that + # was never asked anything. + def segments : Array(UsageSegment) + return @usage_segments unless @usage_segments.empty? + return Array(UsageSegment).new if @usage.total_tokens.zero? + + [UsageSegment.new(@provider, @model, @usage)] + end + def to_index_entry : IndexEntry IndexEntry.new( id: @id, @@ -170,7 +279,11 @@ module Smith::Session parent_id: @parent_id, provider: @provider, model: @model, - usage: @usage + usage: @usage, + # The resolved list, not the raw field: a row is written to be read by + # `smith stats` and the COST column, and neither should have to know + # that a record predating the split spells its one segment differently. + usage_segments: segments ) end diff --git a/src/smith/session_export.cr b/src/smith/session_export.cr index 6058e91..b54b5b4 100644 --- a/src/smith/session_export.cr +++ b/src/smith/session_export.cr @@ -33,6 +33,13 @@ module Smith end end + # One model's share of a session, with its price already worked out. + record SegmentCost, + provider : String, + model : String, + usage : LLM::Usage, + cost : Float64? + class Document getter id : String getter name : String? @@ -43,6 +50,14 @@ module Smith getter cwd : String? getter usage : LLM::Usage? getter cost : Float64? + + # One entry per model the session used, priced when the document was + # built — a `Document` renders, it does not carry the pricing table. + # + # A session that never switched has one and it says nothing the header + # does not; a session that did is otherwise a total with no way to tell + # where it came from. + getter usage_segments : Array(SegmentCost) = Array(SegmentCost).new getter todos : Array(TodoList::Item) getter messages : Array(LLM::Message) getter source : Source @@ -74,6 +89,7 @@ module Smith @cwd : String? = nil, @usage : LLM::Usage? = nil, @cost : Float64? = nil, + @usage_segments : Array(SegmentCost) = Array(SegmentCost).new, @todos : Array(TodoList::Item) = Array(TodoList::Item).new, @transcript_count : Int32? = nil, @transcript_skipped : Int32 = 0, @@ -132,6 +148,21 @@ module Smith # point. Same for the cost below. json.field("usage") { (usage = @usage) ? usage.to_json(json) : json.null } 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 + json.array do + @usage_segments.each do |segment| + json.object do + json.field "provider", segment.provider + json.field "model", segment.model + json.field("usage") { segment.usage.to_json(json) } + json.field "cost_usd", segment.cost + end + end + end + end + end json.field("todos") { @todos.to_json(json) } json.field("messages") { @messages.to_json(json) } end @@ -154,6 +185,18 @@ module Smith " completion, " << usage.cached_tokens << " cache\n" end md << "- **Cost:** " << Pricing.format(@cost) << "\n" + + # A session that switched models has a total that belongs to no single + # rate. Saying which stretch was which is the difference between a + # figure that can be checked and one that has to be taken on trust. + if @usage_segments.size > 1 + md << "- **By model:**\n" + @usage_segments.each do |segment| + md << " - " << segment.provider << "/" << segment.model << ": " + md << segment.usage.total_tokens << " tokens, " + md << Pricing.format(segment.cost) << "\n" + end + end md << "- **Exported from:** `" << @source.label << "` (" << source_note << ")\n" md << "\n_Tool arguments, tool results and thinking are abbreviated to " << ABBREVIATED_CHARS << " characters; `--json` exports the log in full._\n\n" @@ -232,6 +275,7 @@ module Smith provider = data.try(&.provider) || entry.try(&.provider) model = data.try(&.model) || entry.try(&.model) usage = data.try(&.usage) || entry.try(&.usage) + segments = data.try(&.segments) || entry.try(&.segments) || Array(Session::UsageSegment).new Document.new( id: id, @@ -244,7 +288,15 @@ module Smith updated_at: data.try(&.updated_at) || entry.try(&.updated_at), cwd: data.try(&.cwd), usage: usage, - cost: cost_of(usage, provider, model, overrides), + cost: Session.cost_of(segments, overrides), + usage_segments: segments.map do |segment| + SegmentCost.new( + provider: segment.provider, + model: segment.model, + usage: segment.usage, + cost: Pricing.estimate(segment.usage, segment.provider, segment.model, overrides) + ) + end, todos: data.try(&.todos) || Array(TodoList::Item).new, transcript_count: recorded.try(&.size), transcript_skipped: skipped_lines, diff --git a/src/smith/stats.cr b/src/smith/stats.cr index 971315c..722014a 100644 --- a/src/smith/stats.cr +++ b/src/smith/stats.cr @@ -68,22 +68,29 @@ module Smith entries.each do |entry| agg.sessions += 1 - usage = entry.usage - provider = entry.provider - model = entry.model - next if usage.nil? || provider.nil? || model.nil? + # One segment per model the session used — exactly one for a session + # that never switched, and for every row written before it could. + segments = entry.segments + next if segments.empty? + # Per entry, not per segment: this counts sessions, and a session that + # switched models is still one session. agg.with_usage += 1 - agg.prompt_tokens += usage.prompt_tokens - agg.completion_tokens += usage.completion_tokens - agg.cache_creation_tokens += usage.cache_creation_tokens - agg.cache_read_tokens += usage.cache_read_tokens - - key = Pricing.key_for(provider, model) - stat = models[key]? || ModelStat.new(provider, model) - stat.add(usage, Pricing.estimate(usage, provider, model, overrides)) - # ModelStat is a struct: write the updated copy back into the hash. - models[key] = stat + + segments.each do |segment| + usage = segment.usage + + agg.prompt_tokens += usage.prompt_tokens + agg.completion_tokens += usage.completion_tokens + agg.cache_creation_tokens += usage.cache_creation_tokens + agg.cache_read_tokens += usage.cache_read_tokens + + key = Pricing.key_for(segment.provider, segment.model) + stat = models[key]? || ModelStat.new(segment.provider, segment.model) + stat.add(usage, Pricing.estimate(usage, segment.provider, segment.model, overrides)) + # ModelStat is a struct: write the updated copy back into the hash. + models[key] = stat + end end known = models.values.sum { |s| s.cost || 0.0 } From 2029defb2bc27d989551e583d9aa8fa3f38c4012 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Fri, 11 Sep 2026 01:51:14 +0200 Subject: [PATCH 2/3] fix: guard emptiness on tokens that exist, and settle the split before /model moves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 2 +- README.md | 2 +- spec/smith/session_usage_spec.cr | 81 ++++++++++++++++++++++++++++++++ src/smith/cli.cr | 23 +++++++-- src/smith/llm/types.cr | 10 ++++ src/smith/session.cr | 9 ++-- src/smith/session_export.cr | 12 ++--- 7 files changed, 123 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab3be61..52e7e61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ All notable changes to smith. The format follows [Keep a Changelog](https://keep ### Fixed -- **A session that switched models is no longer priced as though it had only ever used the last one**: `Session::Data` held one `model` and one `usage`, so everything that turns tokens into money — `smith stats`, the `COST` column of `smith sessions`, the running cost line, `smith sessions export` — charged an entire session at the rate of whatever model it happened to end on. Measured on the case #99 left behind: 100k prompt and 20k completion tokens spent on `claude-opus-5`, then `/model claude-haiku-4-5`, and the report reads $0.20 for $1.00 of work. It breaks the rule `pricing.cr` is built on, that a wrong cost figure is worse than none. Usage is now recorded per provider/model stretch 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, by the model that was asked, and the running cost line reads `spent_usd` directly whenever a budget is set, so the line and `BudgetExceeded` cannot disagree after a switch the way they did. 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. A session that never switches is unchanged in every output. Two rules worth knowing: `smith stats` lists a session under every model it used and still counts it as one session, and a session with any stretch on a model with no known price reports `n/a` rather than a sum quietly missing a part (#103). +- **A session that switched models is no longer priced as though it had only ever used the last one**: `Session::Data` held one `model` and one `usage`, so everything that turns tokens into money — `smith stats`, the `COST` column of `smith sessions`, the running cost line, `smith sessions export` — charged an entire session at the rate of whatever model it happened to end on. Measured on the case #99 left behind: 100k prompt and 20k completion tokens spent on `claude-opus-5`, then `/model claude-haiku-4-5`, and the report reads $0.20 for $1.00 of work. It breaks the rule `pricing.cr` is built on, that a wrong cost figure is worse than none. Usage is now recorded per provider/model stretch 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, by the model that was asked, and the running cost line reads `spent_usd` directly whenever a budget is set, so the line and `BudgetExceeded` cannot disagree after a switch the way they did. 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. A session that never switches is unchanged in every output. Two rules worth knowing: `smith stats` lists a session under every model it used and still counts it as one session, and a session's own figure reports `n/a` if any stretch of it ran on a model with no known price, rather than a sum quietly missing a part — while the grand total in `smith stats` keeps doing what it always did, adding up what it can price and showing the rest as `n/a` rows in the breakdown, because one number describes one session and the other summarises many. The running cost line prices the same way rather than reading `Agent#spent_usd`, which the issue offered as a shortcut: the two agree wherever both are defined, and where they do not, `spent_usd` is the enforcement figure — it counts an unpriced stretch as nothing, and printing that as a cost would answer "unknown" with "free" (#103). - **A forked session no longer makes `smith stats` count its parent's history again**: `fork` copied `usage` along with the transcript, and `Stats.aggregate` sums that field over the rows of the index — so every fork added an entire history to the grand total that had only ever been spent once. Forking a long session three times reported roughly four times what it cost. The copying was always there, but it used to mean something smaller: before #102 the field held the *last run*, and a fork inherited one run's tokens; #102 made it the session's *lifetime*, and the same line quietly began inheriting all of it. A fork now starts at zero, which is the only reading of "what this session spent" that a sum over sessions can be taken of. Nothing is undercharged by that, which is worth stating because the obvious worry is the wrong one: the baseline for a run is taken from this same field, the agent is built with the whole inherited transcript, and the first response is billed for all of it — as prompt tokens, or as cache reads where the parent's cache is still warm, which is the likely case precisely here, a fork's transcript being byte-identical to the one the parent just sent. Both are priced, so re-sending what it inherited appears in the fork's own `COST` column either way. What stays with the parent is what the parent spent *producing* the transcript, counted once, where it was spent. A fork made before this release does not merely keep its inherited figure, it carries it: the baseline for every later run is read from that same field, so the excess is re-added on every save and the row stays over by exactly the inherited amount for the life of the session. It is not migrated, because `parent_id` says which session a fork came from but nothing recorded what that session had spent *at the time of the fork*, and the parent has run since — so any retroactive correction would be a guess, which is the one thing a cost figure may not be. There is no in-product way to reset it short of deleting the session (#117). diff --git a/README.md b/README.md index 10272a5..8f443fb 100644 --- a/README.md +++ b/README.md @@ -1075,7 +1075,7 @@ The built-in commands, resolved before skill expansion (a skill of the same name `/model` is the one built-in that works both bare and with an argument: on its own it reports the model and provider in use, and with a name it switches the model from the next request onward. Only the name on the wire changes — the provider client, its API key and its connection stay as they are, which is exactly what `-m` decides at startup. The new model is written to the session immediately, so `smith resume` comes back on it. Switching *provider* is not offered: that needs a different client and a different API key, so it stays a restart with `--provider`. -**Cost reporting after a switch.** A session records what each model used separately, so `smith stats`, the `COST` column of `smith sessions`, the running cost line and `smith sessions export` all price each stretch at its own rate rather than at the model the session happens to have ended on. `smith stats` lists the session under every model it used, and counts it once. A session recorded before this existed has no split and is read as the one model it named, which is what it always meant — nothing needs migrating. If any stretch ran on a model with no known price the whole figure is `n/a` rather than a sum missing one part, the same rule that governs a single-model session. +**Cost reporting after a switch.** A session records what each model used separately, so `smith stats`, the `COST` column of `smith sessions`, the running cost line and `smith sessions export` all price each stretch at its own rate rather than at the model the session happens to have ended on. `smith stats` lists the session under every model it used, and counts it once. A session recorded before this existed has no split and is read as the one model it named, which is what it always meant — nothing needs migrating. A *session's* figure — the `COST` column, the export — is `n/a` if any stretch of it ran on a model with no known price, rather than a sum quietly missing one part; the same rule that has always governed a single-model session. The grand total in `smith stats` is the other way round, as it always has been: it adds up what it can price and shows the unpriced models as their own `n/a` rows in the breakdown, so a partial total is visible as partial rather than silent. The difference is deliberate — one number describes one session and has to be right or absent, the other summarises many and says which parts it could not price. The name is not validated against a list, because there is no honest offline list of every model a provider will accept — a hardcoded one would reject models released next week. What is checked is what can be: a name must be a single word, and a provider name given by mistake (`/model anthropic`) is named as such. A model that does not exist is rejected by the provider at the next request and reported as a turn error; the session stays open, and another `/model` puts it right. diff --git a/spec/smith/session_usage_spec.cr b/spec/smith/session_usage_spec.cr index 2737bd7..96fa044 100644 --- a/spec/smith/session_usage_spec.cr +++ b/spec/smith/session_usage_spec.cr @@ -44,6 +44,10 @@ class Smith::CLI build_agent(provider, session_data) end + def switch_model_for_spec(session_data : Smith::Session::Data, agent : Smith::Agent, name : String) : Nil + switch_model(session_data, agent, name) + end + def persist_for_spec(session_data : Smith::Session::Data, agent : Smith::Agent) : Nil persist(session_data, agent) end @@ -331,6 +335,83 @@ describe "a session's lifetime usage across resumes" do end end + it "reads a pre-#103 index written on disk exactly as it always did" do + # The fallback that matters is `IndexEntry`'s: `smith stats` and the COST + # column read the index, not the session file, and a real installation's + # index.json has no `usage_segments` at all. Written out by hand rather + # than produced by this build, which could only ever write the new shape. + with_cli do |cli| + dir = cli.store_for_spec.sessions_dir + Dir.mkdir_p(dir) + + File.write(File.join(dir, "index.json"), <<-JSON) + [{"id":"session-old","created_at":"2026-01-01T00:00:00Z","updated_at":"2026-01-01T00:00:00Z", + "first_prompt":"before the split","message_count":2, + "provider":"anthropic","model":"claude-opus-5", + "usage":{"prompt_tokens":100000,"completion_tokens":20000,"total_tokens":120000}}] + JSON + + entry = cli.store_for_spec.list.find { |row| row.id == "session-old" }.not_nil! + expected = Smith::Pricing.estimate(entry.usage.not_nil!, "anthropic", "claude-opus-5").not_nil! + + entry.segments.size.should eq(1) + entry.cost.not_nil!.should be_close(expected, 1e-9) + + agg = Smith::Stats.aggregate([entry]) + agg.cost.not_nil!.should be_close(expected, 1e-9) + agg.by_model.map(&.model).should eq(["claude-opus-5"]) + agg.total_tokens.should eq(120_000) + end + end + + it "keeps a lifetime whole when the provider reported no total_tokens" do + # `total_tokens` is whatever the provider said, and three of the four + # adapters default it to 0 when the key is missing. Testing emptiness + # against it would have thrown a real history away on the next turn — and + # unrecoverably, since the next baseline reads the truncated list. + with_cli do |cli| + session = cli.store_for_spec.create(model: "claude-opus-5", provider: "anthropic") + session.usage = Smith::LLM::Usage.new(100_000, 20_000, 0) + cli.store_for_spec.save(session) + + resumed, agent = resume(cli, session.id) + agent.send("one more") + cli.persist_for_spec(resumed, agent) + + saved = cli.store_for_spec.load(session.id) + saved.segments.sum(&.usage.billed_prompt_tokens).should eq(100_100) + saved.segments.sum(&.usage.completion_tokens).should eq(20_020) + end + end + + it "does not re-attribute an old session's history to the model it switches to" do + # A record from before the split derives its one segment from `model`. If + # `/model` moved that field first, everything the session ever spent would + # be re-read as the new model's — #103's own bug, arriving through the + # door of the feature that motivated it. + with_cli do |cli| + session = cli.store_for_spec.create(model: "claude-opus-5", provider: "anthropic") + session.usage = Smith::LLM::Usage.new(100_000, 20_000, 120_000) + session.usage_segments.clear + cli.store_for_spec.save(session) + + before = cli.store_for_spec.list.find { |row| row.id == session.id }.not_nil!.cost.not_nil! + + loaded = cli.store_for_spec.load(session.id) + agent = cli.build_agent_for_spec(BillingProvider.new, loaded) + cli.switch_model_for_spec(loaded, agent, "claude-haiku-4-5") + cli.store_for_spec.save(loaded) + + after = cli.store_for_spec.list.find { |row| row.id == session.id }.not_nil!.cost.not_nil! + after.should be_close(before, 1e-9) + + # And it stays put across a reload, rather than being re-derived wrong. + reloaded = cli.store_for_spec.load(session.id) + reloaded.segments.map(&.model).should eq(["claude-opus-5"]) + reloaded.model.should eq("claude-haiku-4-5") + end + end + it "leaves --max-budget-usd a per-run limit" do # The budget runs off `spent_usd`, which the agent counts for itself and # which no baseline touches. A resumed session with a long history starts diff --git a/src/smith/cli.cr b/src/smith/cli.cr index 2e6a690..3e8887d 100644 --- a/src/smith/cli.cr +++ b/src/smith/cli.cr @@ -1398,6 +1398,15 @@ module Smith previous = agent.model agent.model = name + # Settle the split *before* the model moves. A record written before + # #103 carries no segments and derives its one from `model` — so + # overwriting `model` first would silently re-attribute everything the + # session ever spent to the model it is switching *to*, which is the + # very error #103 exists to remove, arriving through the door of the + # feature that motivated it. Once written down, the past cannot be + # re-read. + session_data.usage_segments = session_data.segments + # Persisted as well as applied, so `smith resume` comes back on the new # model — the index row is rebuilt from this same field on save. session_data.model = name @@ -2094,12 +2103,18 @@ module Smith # They did not, after a switch: one summed per turn and the other priced # the lot at the current model. private def run_cost(provider_name : String, agent : Agent) : Float64? - return agent.spent_usd unless @max_budget_usd.nil? - - # Nothing counted yet: there are no segments to price and no model to - # blame, so the answer is the one a zero-usage run always gave. + # 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? + # Priced from the segments rather than read off `Agent#spent_usd`, which + # the issue offered as the shortcut. Where both are defined they agree — + # same rates, same responses, only grouped differently — 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 enforcement deliberately counts a stretch with + # no known rate as nothing. Printing that as a *cost* would answer + # "unknown" with "free", against the rule `output.cr` states outright. Session.cost_of( agent.usage_by_model.map do |model, usage| Session::UsageSegment.new(provider_name, model, usage) diff --git a/src/smith/llm/types.cr b/src/smith/llm/types.cr index a701789..2a5abd5 100644 --- a/src/smith/llm/types.cr +++ b/src/smith/llm/types.cr @@ -200,6 +200,16 @@ module Smith::LLM @prompt_tokens + @cache_read_tokens + @cache_creation_tokens end + # Nothing was used. Deliberately not `total_tokens.zero?`: that field is + # whatever the provider reported, and three of the four adapters default + # it to 0 when the key is missing while `prompt_tokens` and + # `completion_tokens` hold real numbers. Anthropic computes it as input + # plus output, which leaves out billable cache tokens. Asking the four + # fields that carry the counts is the only test that cannot be lied to. + def empty? : Bool + billed_prompt_tokens.zero? && @completion_tokens.zero? + end + def initialize( @prompt_tokens : Int32 = 0, @completion_tokens : Int32 = 0, diff --git a/src/smith/session.cr b/src/smith/session.cr index 6fe2fb5..deeb736 100644 --- a/src/smith/session.cr +++ b/src/smith/session.cr @@ -123,7 +123,7 @@ module Smith::Session # 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) - return @usage_segments unless @usage_segments.empty? + return @usage_segments.dup unless @usage_segments.empty? provider = @provider model = @model @@ -262,8 +262,11 @@ module Smith::Session # would be merged into the run's real ones and reported as a model that # was never asked anything. def segments : Array(UsageSegment) - return @usage_segments unless @usage_segments.empty? - return Array(UsageSegment).new if @usage.total_tokens.zero? + # A copy: `build_agent` parks this as a baseline and `persist` merges + # against it, and neither should be able to reach back into the record + # it came from. + return @usage_segments.dup unless @usage_segments.empty? + return Array(UsageSegment).new if @usage.empty? [UsageSegment.new(@provider, @model, @usage)] end diff --git a/src/smith/session_export.cr b/src/smith/session_export.cr index b54b5b4..1a8f1a3 100644 --- a/src/smith/session_export.cr +++ b/src/smith/session_export.cr @@ -275,7 +275,11 @@ module Smith provider = data.try(&.provider) || entry.try(&.provider) model = data.try(&.model) || entry.try(&.model) usage = data.try(&.usage) || entry.try(&.usage) - segments = data.try(&.segments) || entry.try(&.segments) || Array(Session::UsageSegment).new + # `||` cannot do this: an empty array is truthy, so a session file that + # simply has not spent anything would stop the chain and hide the index + # row behind it — which turned a `$0.00` export into `n/a`. + segments = data.try(&.segments) || Array(Session::UsageSegment).new + segments = entry.try(&.segments) || Array(Session::UsageSegment).new if segments.empty? Document.new( id: id, @@ -321,12 +325,6 @@ module Smith candidate end - private def self.cost_of(usage : LLM::Usage?, provider : String?, model : String?, overrides : Pricing::Overrides?) : Float64? - return nil if usage.nil? || provider.nil? || model.nil? - - Pricing.estimate(usage, provider, model, overrides) - end - private def self.load_session(store : Session::Store, id : String, warnings : Array(String)) : Session::Data? store.load(id) rescue ArgumentError From 95a709b3adf2a5788ee682c2cd79fce3c8559106 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Fri, 11 Sep 2026 01:56:32 +0200 Subject: [PATCH 3/3] docs: two comments still describe the shortcut that was removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 2 +- src/smith/cli.cr | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52e7e61..4d5132f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ All notable changes to smith. The format follows [Keep a Changelog](https://keep ### Fixed -- **A session that switched models is no longer priced as though it had only ever used the last one**: `Session::Data` held one `model` and one `usage`, so everything that turns tokens into money — `smith stats`, the `COST` column of `smith sessions`, the running cost line, `smith sessions export` — charged an entire session at the rate of whatever model it happened to end on. Measured on the case #99 left behind: 100k prompt and 20k completion tokens spent on `claude-opus-5`, then `/model claude-haiku-4-5`, and the report reads $0.20 for $1.00 of work. It breaks the rule `pricing.cr` is built on, that a wrong cost figure is worse than none. Usage is now recorded per provider/model stretch 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, by the model that was asked, and the running cost line reads `spent_usd` directly whenever a budget is set, so the line and `BudgetExceeded` cannot disagree after a switch the way they did. 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. A session that never switches is unchanged in every output. Two rules worth knowing: `smith stats` lists a session under every model it used and still counts it as one session, and a session's own figure reports `n/a` if any stretch of it ran on a model with no known price, rather than a sum quietly missing a part — while the grand total in `smith stats` keeps doing what it always did, adding up what it can price and showing the rest as `n/a` rows in the breakdown, because one number describes one session and the other summarises many. The running cost line prices the same way rather than reading `Agent#spent_usd`, which the issue offered as a shortcut: the two agree wherever both are defined, and where they do not, `spent_usd` is the enforcement figure — it counts an unpriced stretch as nothing, and printing that as a cost would answer "unknown" with "free" (#103). +- **A session that switched models is no longer priced as though it had only ever used the last one**: `Session::Data` held one `model` and one `usage`, so everything that turns tokens into money — `smith stats`, the `COST` column of `smith sessions`, the running cost line, `smith sessions export` — charged an entire session at the rate of whatever model it happened to end on. Measured on the case #99 left behind: 100k prompt and 20k completion tokens spent on `claude-opus-5`, then `/model claude-haiku-4-5`, and the report reads $0.20 for $1.00 of work. It breaks the rule `pricing.cr` is built on, that a wrong cost figure is worse than none. Usage is now recorded per provider/model stretch 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, by the model that was asked, so the running cost line and `BudgetExceeded` cannot disagree after a switch the way they did — one summed per turn at the rates in force while the other priced the whole run at the model it had arrived at. 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. A session that never switches is unchanged in every output. Two rules worth knowing: `smith stats` lists a session under every model it used and still counts it as one session, and a session's own figure reports `n/a` if any stretch of it ran on a model with no known price, rather than a sum quietly missing a part — while the grand total in `smith stats` keeps doing what it always did, adding up what it can price and showing the rest as `n/a` rows in the breakdown, because one number describes one session and the other summarises many. The running cost line prices the same way rather than reading `Agent#spent_usd`, which the issue offered as a shortcut: the two agree wherever both are defined, and where they do not, `spent_usd` is the enforcement figure — it counts an unpriced stretch as nothing, and printing that as a cost would answer "unknown" with "free" (#103). - **A forked session no longer makes `smith stats` count its parent's history again**: `fork` copied `usage` along with the transcript, and `Stats.aggregate` sums that field over the rows of the index — so every fork added an entire history to the grand total that had only ever been spent once. Forking a long session three times reported roughly four times what it cost. The copying was always there, but it used to mean something smaller: before #102 the field held the *last run*, and a fork inherited one run's tokens; #102 made it the session's *lifetime*, and the same line quietly began inheriting all of it. A fork now starts at zero, which is the only reading of "what this session spent" that a sum over sessions can be taken of. Nothing is undercharged by that, which is worth stating because the obvious worry is the wrong one: the baseline for a run is taken from this same field, the agent is built with the whole inherited transcript, and the first response is billed for all of it — as prompt tokens, or as cache reads where the parent's cache is still warm, which is the likely case precisely here, a fork's transcript being byte-identical to the one the parent just sent. Both are priced, so re-sending what it inherited appears in the fork's own `COST` column either way. What stays with the parent is what the parent spent *producing* the transcript, counted once, where it was spent. A fork made before this release does not merely keep its inherited figure, it carries it: the baseline for every later run is read from that same field, so the excess is re-added on every save and the row stays over by exactly the inherited amount for the life of the session. It is not migrated, because `parent_id` says which session a fork came from but nothing recorded what that session had spent *at the time of the fork*, and the parent has run since — so any retroactive correction would be a guess, which is the one thing a cost figure may not be. There is no in-product way to reset it short of deleting the session (#117). diff --git a/src/smith/cli.cr b/src/smith/cli.cr index 3e8887d..a4c038e 100644 --- a/src/smith/cli.cr +++ b/src/smith/cli.cr @@ -2097,11 +2097,11 @@ module Smith # What the run has cost, priced per stretch rather than all at the model # it happens to have ended on. # - # With a budget set the agent has already added it up, per response, at - # the rates in force when each one arrived — so reading that is not an - # optimisation but the only way the line and `BudgetExceeded` can agree. - # They did not, after a switch: one summed per turn and the other priced - # the lot at the current model. + # This and `BudgetExceeded` disagreed after a switch: one summed per turn + # at the rates in force, the other priced the whole run at the model it + # had arrived at. Pricing per stretch settles that from this side — see + # below for why it is done here rather than by reading the agent's own + # total, which was the obvious way to make them agree and the wrong one. 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.