Anchor checkpoints to a message id instead of a position (#57) - #59
Merged
Conversation
A checkpoint recorded how many messages the transcript held when it was taken, and a rewind cut there. That number is only meaningful as long as nothing in front of it moves — and compaction moves it, by replacing a prefix of turns with a single summary. #55 handled that by shifting every stored index whenever compaction removed messages, which worked but left two rough edges. A position inside the replaced prefix could not be recovered at all, so it was marked lost. And every future change that reorders or removes messages would have had to remember to shift too: nothing about `message_index : Int32` says "this breaks if anyone edits the array in front of it", which is why the bug sat latent for exactly as long as the summarizer was unreachable. LLM::Message now carries an id, assigned on creation and preserved wherever compaction rewrites a message rather than removing it. A checkpoint stores that id; a rewind resolves it against the current transcript. Compaction needs no bookkeeping at all — a message that survived still resolves, and one that was summarized away does not, which is the same answer the shifting used to compute and the honest one either way. So Store#shift_message_indices, Entry#transcript_lost, the agent's call into them, and Context::Result#removed_prefix all go away. Checkpoints written before ids existed keep their index and keep working. The id never leaves smith: every provider builds its request from the role and the blocks, and the estimator does not count it. Closes #57. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #57. The last follow-up from #53.
The problem
A checkpoint recorded how many messages the transcript held when it was taken (
checkpoints.cr:122), and a rewind cut there (cli.cr:1258). That number is only meaningful as long as nothing in front of it moves — and compaction moves it, by replacing a prefix of turns with a single summary.#55 handled that by shifting every stored index whenever compaction removed messages. It worked, and it left two rough edges:
transcript_lostand its rewind restored files without touching the transcript. Honest, but bookkeeping that existed only to compute an answer the data could have given directly.message_index : Int32says "this breaks if anyone edits the array in front of it". That invisibility is why the bug sat latent for exactly as long as the summarizer was unreachable.The change
LLM::Messagecarries an id, assigned on creation and preserved wherever compaction rewrites a message rather than removing it — a truncated tool result is still the same message. A checkpoint stores that id; a rewind resolves it against the current transcript throughSession::Transcript.index_after.Compaction then needs no bookkeeping at all. A message that survived still resolves; one that was summarized away does not, which is the same answer the shifting used to compute and the honest one either way.
So these go away:
Store#shift_message_indices,Entry#transcript_lost, the agent's call into them, andContext::Result#removed_prefix.Resolution lives with the caller rather than in the store, which is what the store's own comment already asked for — "The registry has no business knowing about the transcript, so the position is handed in rather than looked up."
RestoreResultcarries the id; the CLI turns it into an index.Migration: checkpoints written before ids existed keep their
message_indexand keep working — the pre-#55 behaviour, rather than being silently wrong.Entry#message_indexis nilable now and never written for new entries.Random ids rather than a counter:
smith forkcopies a transcript, and two sessions handing out the same numbers would collide.The id never leaves smith. Every provider builds its request from the role and the blocks, and the estimator does not count it, so nothing about the wire format or the token budget changes.
Testing
857 examples, green.
🤖 Generated with Claude Code