diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e030ef..838ff19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ All notable changes to smith. The format follows [Keep a Changelog](https://keep ### Fixed +- **Compaction no longer summarizes its own summary while the turn that is actually growing sits untouched**: the recency window counts *turns*, and a long agentic run — one prompt, then a hundred tool calls — is a single turn, so the window that exists to protect the last three turns covered the entire history instead. All four cheap stages became no-ops (`window_start` returns 0 once there are no more real turns than the window is wide, and every stage breaks on the first message), `safe_cut_index` found no boundary whose tail fit and fell back to the newest one, and the prefix left to summarize was — from the second compaction onward — the summary the first one had written, and nothing else. The reported result was `~105421 → ~105341 tokens, 0% of the budget reclaimed · summarize`, every turn, each one paying for a provider call and a full prompt-cache invalidation to swap one summary for another while the 76k of tool results behind the cut were never a candidate. The escape hatch that ignores the recency window already existed but was wired to `cut.zero?` — the one shape this case never takes. It now runs whenever the tail that would survive the cut is itself over target, which is the same dead end reached by a different road: `cut.zero?` is the case where there is no boundary, this is the case where no boundary helps. Afterwards the boundary is re-chosen against the shortened history, and a history that now fits is returned as it is rather than summarized for nothing. Two guards stand behind that: a prefix that is only a previous summary is refused before the call is made, recognised by the `SUMMARY_PREFIX` the writer and the reader now share rather than by a flag, which a resumed session's JSON round trip would not carry; and a summary that comes back longer than the turns it replaced is discarded, because paying for the call, losing the detail and growing the request is the one outcome with nothing to recommend it. On the reproduction — a short turn, then thirty 8 KB tool results — the same history now goes `60212 → 34249`, 74% of the budget reclaimed, target reached, and no provider call at all (#118). + - **A resumed session no longer forgets what earlier runs cost**: `Agent` counts tokens for a *run* — its counter is built at zero and nothing seeds it — while the session record holds a *lifetime*, and the loop's write-back assigned the one straight onto the other. So the first turn back overwrote everything a session had ever spent with the few hundred tokens of the turn just taken, and `smith sessions`, `smith stats` and `smith sessions export` reported the last run as though it were the whole session. The record now keeps the total it stood at when its agent was built and writes that plus the run's counter, so a session resumed twice shows all three runs in the index. A sum against a baseline rather than an addition, deliberately: the write-back runs after *every* turn and the counter is the run's running total, not the turn's increment, so `+=` would have counted turn one again on turn two, and again on turn three. The baseline is taken where the counter is born — in `build_agent`, the one place all five routes to a new agent pass through, `/resume` inside either running loop among them, which is the route a baseline taken at startup misses and the one that silently wiped the session being switched to. It rides on the session rather than on the CLI because such a switch rebuilds the agent before it re-points the interrupt handler and can yield in between, so a `^C` dispatched in that window can still reach a handler holding the session being *left* — which has to be written against its own baseline rather than the incoming one's. Nothing the agent counts changed, and neither did the two things that read it: `--max-budget-usd` is still a per-run limit through `spent_usd`, and the token line at the end of a run still reports that run rather than quietly becoming a lifetime figure (#102). - **`/clear` followed by `/resume` no longer destroys the session you left**: `/clear` empties the agent's conversation, and the loop's write-back copies that conversation onto the session record — so switching away straight after a clear wrote an emptied `session.json` and a `0` message count into the index, without asking and without a word. The `transcript.jsonl` beside it survived, being append-only, but nothing else did. `/clear` now writes nothing at all: it starts the running context over, and the record keeps what is on disk until a real turn adds something — so `/clear` plus a turn is still a fresh start, while `/clear` plus a switch loses nothing. The todo list and the calibration ratio are kept with the transcript they belong to, rather than being replaced by the empty list and the neutral ratio a clear leaves behind. The guard sits in the one method every persisting path calls, rather than at the call site the report happened to find — `/resume` in the plain loop and in the fullscreen one, the `^C` handler, the second `^C` in the fullscreen UI, that loop's exit and a prompt a `user_prompt_submit` hook blocks are each such a path, and that list is illustrative rather than exhaustive. It also covers a case the report thought was already safe: `/clear` then `/quit` costs nothing in the plain loop, which does not persist on the way out, but the fullscreen loop persists after `app.run` returns and lost the session the same way. Marked on the agent rather than inferred from an empty message list, which cannot tell a session that was cleared from one that was never used — and those two have to be written differently (#105). - **A `mode:` smith cannot read now costs a subagent its tools, not the reader the promise**: `mode: inspekt`, `mode: read-only` — anything that was not `work` or `inspect` — resolved to `work`, so a definition whose author was asking for a read-only agent got `bash`, `write_file` and `edit_file` instead, and nothing said so. `smith agents list` printed `work` as though the file had asked for it, which made the one view that exists to show configuration the view that hid the typo. An unreadable value falls back to `inspect` now — the conservative direction, since a mode is a security statement and a typo in one must not be able to grant more than the file asked for — and says so by path and by the value it could not read, on the `warn_io` the catalog already uses. The definition still loads: a typo should make an agent careful, not unusable. The listing quotes what was declared next to what took effect, so `inspect (fallback; 'inspekt' is not a mode)` reads as smith's own choice rather than as configuration. `mode: Inspect` was never a typo and still is not — case is folded before the value is read. The same string arriving from a *model*, through the `agent` tool's `mode` parameter, is refused outright with the two valid names, exactly as an unknown `agent_type` already was: the schema declares an enum but not every provider enforces one, and a value the model invented must not become `work` (#106). diff --git a/README.md b/README.md index d0e1c3b..0cd4637 100644 --- a/README.md +++ b/README.md @@ -1014,9 +1014,9 @@ Below the trigger nothing happens at all and the transcript is left byte-identic 1. **Drop stale thinking.** Bulky, worthless once its turn is over, and free in fidelity terms. The current turn is preserved byte for byte, because Anthropic validates the signature on a thinking block. 2. **Supersede duplicate reads.** Read the same file five times and only the newest is kept in full; the earlier ones become a note saying why they are not there. Only genuinely read-only tools take part — `read_file`, `grep`, `glob`. `bash` is deliberately excluded: running `make test` twice is not a duplicate, it is a before and an after. 3. **Truncate stale tool results.** Largest first, so reclaiming a given number of tokens mangles as few results as possible. A result reached once keeps a 512-byte head; one already carrying a truncation marker collapses to a single line. -4. **Summarize the oldest turns** via a separate, tool-free provider call, replacing them with a single summary message. If that call fails, the prefix is dropped outright rather than failing the turn. +4. **Summarize the oldest turns** via a separate, tool-free provider call, replacing them with a single summary message. If that call fails, the prefix is dropped outright rather than failing the turn. Two things it will not do: summarize a prefix that is only the summary the last compaction wrote, and keep a summary that came back longer than the turns it replaced. Both are calls that cost money and reclaim nothing. -The **last three real turns are left alone** by stages 2 and 3, so compaction cannot truncate the file being edited out from under the model. Real turns: the agent injects user messages of its own when a stop hook asks it to keep going or a response hits the output limit, and counting those would let three of them inside a single turn consume the whole window. The window is given up only as a last resort — one oversized `cat` inside the only turn there is gets cut anyway, because protecting the work in hand is worth less than a request the provider will accept at all. +The **last three real turns are left alone** by stages 2 and 3, so compaction cannot truncate the file being edited out from under the model. Real turns: the agent injects user messages of its own when a stop hook asks it to keep going or a response hits the output limit, and counting those would let three of them inside a single turn consume the whole window. The window is given up when no turn boundary leaves a tail that fits: stages 2 and 3 then run again over everything, the current turn included, because protecting the work in hand is worth less than a request the provider will accept at all. That is not an exotic case. A turn is a unit of *conversation*, not of size, and a long agentic run — one prompt, then a hundred tool calls — is a single turn, so a coding session reaches it routinely. Largest first still applies, and the pass stops the moment the target is met, so small recent results survive it. Every stage preserves the pairing between `tool_use` and `tool_result` blocks — providers reject a request where one half is missing, so compaction only ever shortens content or removes whole turns. Checkpoints name the message they were taken at rather than a position in the transcript, so summarizing a prefix cannot move them: a rewind still lands on the turn you picked, and a checkpoint whose message was summarized away restores its files and says the transcript was left alone. diff --git a/spec/smith/context_spec.cr b/spec/smith/context_spec.cr index f09da6a..da5bc95 100644 --- a/spec/smith/context_spec.cr +++ b/spec/smith/context_spec.cr @@ -742,3 +742,96 @@ describe "the cost of compacting a long history" do assert_tool_pairing(result.messages) end end + +# One prompt, then a hundred tool calls. The recency window counts turns, so a +# run of this shape is a *single* turn and the window covers all of it — which +# is how compaction came to summarize its own summary every turn while the +# thing that was actually growing sat behind the cut, untouched. +private def one_long_turn(steps : Int32, result_bytes : Int32 = 8_000) + messages = [] of Smith::LLM::Message + + messages << user_msg("the first question") + messages << assistant_tool_call("call-first") + messages << tool_result("call-first", "y" * 200) + + messages << user_msg("yes, go ahead") + steps.times do |i| + messages << assistant_tool_call("call-#{i}") + messages << tool_result("call-#{i}", "x" * result_bytes) + end + + messages +end + +private def tool_result_bytes(messages : Array(Smith::LLM::Message)) + messages.sum do |message| + message.content.sum { |block| block.type.tool_result? ? (block.text.try(&.bytesize) || 0) : 0 } + end +end + +describe "a long agentic run inside one turn" do + it "shortens the turn that is the whole context instead of summarizing the prefix" do + messages = one_long_turn(30) + result = compact_with_summary(messages, budget(70_000)) + + # It used to report `["summarize"]` here, having replaced three messages + # with a summary and left 240 KB of tool results in the turn behind the cut. + result.stages.should eq(["truncate"]) + result.reached_target?.should be_true + tool_result_bytes(result.messages).should be < tool_result_bytes(messages) + assert_tool_pairing(result.messages) + end + + it "shortens the turn rather than summarizing again on the next compaction" do + # The second compaction of the same history is where the old behaviour + # settled: the prefix is the summary the first one wrote, and nothing else. + # Shortening the turn is what makes the call unnecessary in the first place. + messages = [Smith::LLM::Message.user("#{Smith::Context::SUMMARY_PREFIX}what came before")] + + one_long_turn(30)[3..] + + calls = 0 + result = Smith::Context.compact(messages, budget(70_000)) do |_| + calls += 1 + "what came before" + end + + calls.should eq(0) + result.stages.should eq(["truncate"]) + result.reached_target?.should be_true + end + + it "refuses to summarize a prefix that is only the last summary" do + # Nothing here is truncatable, so the desperate pass cannot help and the + # cut lands back on the same boundary. What is left to summarize is the + # summary itself — a call that buys the difference between one summary and + # the next, every turn, forever. + messages = [ + Smith::LLM::Message.user("#{Smith::Context::SUMMARY_PREFIX}what came before"), + user_msg("yes, go ahead"), + Smith::LLM::Message.assistant("x" * 300_000), + ] + + calls = 0 + result = Smith::Context.compact(messages, budget(70_000)) do |_| + calls += 1 + "what came before" + end + + calls.should eq(0) + result.strategy.none?.should be_true + result.compacted?.should be_false + result.messages.should eq(messages) + end + + it "keeps the staged history when the summary comes back longer than the turns it replaces" do + # Nothing bounds what a provider answers with. Paying for the call, losing + # the detail and growing the request is the one outcome worth refusing. + messages = conversation(10, result_bytes: 60_000) + result = Smith::Context.compact(messages, budget(1_000)) { |_| "z" * 4_000_000 } + + result.strategy.summarized?.should be_false + result.stages.should_not contain("summarize") + result.after_tokens.should be <= result.before_tokens + assert_tool_pairing(result.messages) + end +end diff --git a/src/smith/context.cr b/src/smith/context.cr index 8e4d5a9..09745f8 100644 --- a/src/smith/context.cr +++ b/src/smith/context.cr @@ -20,6 +20,12 @@ module Smith TRUNCATION_MARKER = "truncated by smith to stay within the context window" SUPERSEDED_MARKER = "superseded by a later identical call" + # What a summarized prefix is introduced by. Named for the same reason the + # two markers above are, and for one more: the compaction that writes a + # summary and the compaction that has to recognise its own work a turn + # later are the same code reading the same string. + SUMMARY_PREFIX = "Summary of the earlier conversation: " + # How many real turns are left alone. One turn is a user message and # everything the agent did about it, so three of them cover "read the file, # edit it, run the tests" whole. A constant rather than config: an extra @@ -346,27 +352,44 @@ module Smith # Stage 3 — replace the oldest turns with a summary. cut = safe_cut_index(staged, target) - if cut.zero? - # No boundary to cut at, so the recency window is the last thing left to - # give. One oversized `cat` inside the current turn is the case this - # exists for: protecting the work in hand is worth less than a request - # the provider will accept at all. + + # Whether cutting there is the answer, or only the best boundary on + # offer. `safe_cut_index` returns the earliest turn boundary whose tail + # fits and falls back to the newest one when none does — and a tail that + # is itself over target is the ordinary shape of a long agentic run, not + # an exotic one: the recency window counts turns, and such a run is a + # single turn. Summarizing the prefix then reclaims whatever the prefix + # happens to be, which after the first compaction is the previous summary + # and nothing else. So this is the same dead end `cut.zero?` is, reached + # by a different road, and it has to give way the same way. + if estimate_tokens(staged[cut..]) > target + # The recency window is the last thing left to give. One oversized + # `cat` inside the current turn is the case this started as; a hundred + # tool results inside it is that case at the scale a coding session + # reaches — and protecting the work in hand is worth less than a + # request the provider will accept at all. # Attachments first, here as everywhere: shortening the text of a # result while its image stays behind gives up the readable part and # keeps the expensive one. if drop_stale_media(working, target, window_turns: 0, tool_only: true) stages << "attachments" unless stages.includes?("attachments") - staged = working.messages - after = budget.charged(working.tokens) end if truncate_old_tool_results(working, target, window_turns: 0) stages << "truncate" unless stages.includes?("truncate") - staged = working.messages - after = budget.charged(working.tokens) end - # Report honestly rather than claiming a compaction that did not happen. + staged = working.messages + after = budget.charged(working.tokens) + + # Shorter than it was, so a boundary that did not fit may fit now — and + # if the whole history fits there is nothing left for a summary to do. + cut = working.fits?(target) ? 0 : safe_cut_index(staged, target) + end + + # No boundary to cut at, or none still worth cutting. Report honestly + # rather than claiming a compaction that did not happen. + if cut.zero? strategy = stages.empty? ? Strategy::None : Strategy::Truncated return Result.new(staged, strategy, before, after, budget, stages) end @@ -374,6 +397,17 @@ module Smith prefix = staged[0...cut] tail = staged[cut..] + # A prefix that is nothing but the summary the last compaction wrote. + # Summarizing that again spends a provider call and a prompt-cache + # invalidation to reclaim the difference between one summary and the + # next, which is noise — and once a single turn has outgrown the target + # it is what *every* turn would do, forever. Refused rather than merely + # allowed to be cheap. + if prefix.size == 1 && summary?(prefix.first) + strategy = stages.empty? ? Strategy::None : Strategy::Truncated + return Result.new(staged, strategy, before, after, budget, stages) + end + strategy = Strategy::Summarized replacement = begin summarize.call(prefix) @@ -385,19 +419,44 @@ module Smith "Ask the user if you need details from before this point." end + compacted = [LLM::Message.user("#{SUMMARY_PREFIX}#{replacement}")] + tail + raw_compacted = estimate_tokens(compacted) + + # A summary can come back longer than the turns it replaced — nothing + # bounds what the provider answers. Keeping it would mean paying for the + # call, losing the detail *and* growing the request, so the staged + # history stands and the summary is thrown away. + if raw_compacted >= working.tokens + strategy = stages.empty? ? Strategy::None : Strategy::Truncated + return Result.new(staged, strategy, before, after, budget, stages) + end + stages << (strategy.dropped? ? "drop" : "summarize") - compacted = [LLM::Message.user("Summary of the earlier conversation: #{replacement}")] + tail Result.new( compacted, strategy, before, - budget.charged(estimate_tokens(compacted)), + budget.charged(raw_compacted), budget, stages ) end + # Whether a message is a summary an earlier compaction wrote. Read off the + # text rather than a flag on the message: a resumed session brings its + # history back from disk through the same JSON as any other message, and a + # flag that does not survive that round trip would make the guard hold in a + # fresh session and lapse in a resumed one. + private def self.summary?(message : LLM::Message) : Bool + return false unless message.role.user? + + first = message.content.first? + return false if first.nil? + + (first.text || "").starts_with?(SUMMARY_PREFIX) + end + # The history under compaction, with a running byte total. # # The estimator used to be re-run over the whole array once per truncation