fix(expand): stop sending recovered content upstream twice, and split the two skip reasons apart

[amiddavid]

Closes #201. Independent of #204 and #205 — all three branch from main.

Does the whole issue: document the cross-turn behaviour, count the flip, and fix the duplication. One measurement changes what the third one is for.

The repair path sent the recovered content upstream twice, on every turn, forever

Not in the issue. Two mechanisms, each individually right, compose into it: repairExpandErrors rewrites the client's No such tool available tool_result with the original on every later turn (the client keeps its own copy of the error), and the same call marks that content kept-verbatim, so skipReduce also leaves the original message uncompacted at its own position. One copy in place, one in the tool_result.

Measured, both arms, codesafe, one 200-line tool output:

	upstream bytes	copies
without the expand round-trip	252	1 (marker + head peek)
with it, every later turn	21,511	2 (400 probe hits)

An ~85x blowup against the compacted form, permanently — far larger than the one-off prefix flip the issue describes, which is what makes bullet 3 a fix for a live defect rather than a design preference. The control arm is load-bearing: without it, "the content appears once" would also pass on a pipeline that never compacted that message.

The fix is a pointer, not a deletion. The issue proposed deleting our tool_use/tool_result pair. Deleting messages shrinks the message count, and modes.Boundary reads n < prev as compactionResets.Add(1) with boundary 0 — so our own deletion would be attributed to the agent compacting its transcript. That is the counter-conflation defect again, inside the fix for it. A pointer keeps the message count unchanged: no boundary disturbed, no index shifting, no message left with an empty content array.

The pointer is written only when the content really is there. The agent's own compaction can drop the message the content came from while keeping the round-trip, and then the tool_result is the model's only copy. contentPresent checks by exact comparison against the values gjson decodes, not a substring scan of the raw body — JSON escaping makes a raw scan both false-negative and false-positive. It excludes the block being written, which the existing idempotency test caught me on: on a second pass the repaired block held the original, matched itself, and the repair replaced the content with a note pointing at what it had just removed.

The split bullet 2 asks for already existed, at 3 of 11 sites

kept_verbatim_after_expand was raised by extract_sweep, extract_llm and failed_run; eight others raised the conflated marker_or_kept_verbatim for the same condition, which also covers the benign "already carries a marker". Gates reach /stats per component, so that was a published counter reporting two causes with opposite readings — and a published experimental figure was corrected twice off it before anyone noticed the label was the problem. Fourth instance of the #200 rule.

skipReduce now returns which reason, as two named constants, and all eleven sites raise it. marker_or_kept_verbatim is retired, with the three component docs, two offload tests and one dash fixture updated. smartcrush raised no gate at all here, so its share of both reasons was invisible even in the conflated form.

dedup, extract and linecap have no reapplyFrozen path, so skipReduce is the only place they can report an expansion — this gives those three a clean signal for the first time, and that is where most of the previously invisible share sits. (Same four the reviewer independently flagged on #204.)

The flip is counted, and only when there is one

reapplyFrozen declined on kept-verbatim before looking up the frozen decision, so it could not tell an established compaction being abandoned — a suffix cache-write at ~11.5× a read — from content that was never compacted, where nothing flips. It now consults the decision inside that branch only: one extra Get on content the agent actually expanded, not on the hot path.

expand_prefix_flips at /stats, cg_expand_prefix_flips_total at /metrics. Documented as per turn per message, not per distinct content: every later turn re-observes the same abandonment while only the first is a real cache-write. It is a deliberate cost — re-compacting would bounce the agent into another expand — so counting it makes the trade visible instead of assumed.

Verification

gofmt -l . clean, go vet ./... clean, go test ./... all packages pass, go test -race clean over components/offload, expand, proxy, metrics (Go 1.26.4, eval box).

Four new tests, each revert-verified. The two that could pass for the wrong reason were checked in both directions:

Mutation	Failure
conflate the two gates again	want gate already_marked, got map[kept_verbatim_after_expand:1]
never count the flip	counted 0 flips, want 1
count every kept-verbatim skip as a flip	counted 1 flips for content that was never compacted: the counter is measuring expansions, not cache-writes
always write the content	turn 1 sent the content 2.0 times (400 probe hits, 21511 bytes) + carries no pointer
assume presence instead of checking it	the content is nowhere in the request: the repair replaced the model's only copy with a pointer to something that is not there

Two contract tests updated as designed: TestStatsShapeIsUnchanged and TestEverySnapshotFieldIsExportedOrExempt — the latter in notExportedWhy because the series is sourced from offload's counter rather than off s, since a promLine off s would export a permanent 0 while passing the test (the silent-zero shape #205 is about).

Not in this change

The in-place replacement of a marker by its content. The duplication it would have fixed is fixed above at a fraction of the risk. What remains of it — deleting the dead round-trip from the transcript — needs the boundary threading described above for roughly fifty bytes a turn. The gating signals it would use (ColdCache / TailOnlyCold / compaction_resets) all ship, so deferring it loses no ground.

🤖 Generated with Claude Code

[amiddavid]

I went at the measurement first, as invited, and it holds up. The control arm is doing real work and the one asymmetry I found in it runs the conservative way: the control's tool message is last in a two-message transcript while the treatment's is index 1 of 4, and depth makes compaction more likely, not less — so a control that compacts at the tail is the harder case to pass. The exact-200 assertion in the treatment arm is only safe because kept-verbatim leaves that message untouched, which means a stray head-peek on that path would fail it loudly rather than pass it silently; that is the right way round. Not deleting the round-trip because modes.Boundary reads n < prev as a compactionResets is the correct call, and exceptPath plus the story of how the idempotency test found it is the most useful comment in the diff.

One finding I would fix before merge, and it is the same class as the one you avoided by not deleting messages. contentPresent establishes "the content is in the transcript" against the body as it is before the pipeline runs (proxy.go:1094, ahead of applyMode), and the claim that the pointer cannot go stale rests on kept-verbatim being honoured. Ten components honour it. summarize honours neither skipReduce nor isKeptVerbatim and replaces msgs[start:end] wholesale, so it can delete the message the pointer names, in the same request that wrote the pointer. Detail inline at contentPresent.

One verified note on the flip counter's lookup, and three small ones. Everything else — the gate split across all eleven sites, the named constants, smartcrush finally raising anything, the flip counter's both-direction verification, and leaving the in-place replacement out with the residual stated rather than buried — I have no objection to.

Also: this branch and #204 both edit docs/reference/routes.md's stash rows, so whichever merges second needs a rebase, and the stash_expired remedy this table still gives as "raise ttl_seconds" is what #204 corrects.

[amiddavid]

The presence check cannot see what the pipeline is about to remove, and one component removes messages without consulting the mark this relies on.

The ordering is: repair (proxy.go:1094) → applyMode → pipeline. So contentPresent inspects the pre-pipeline body, and RestoredInPlace's claim that the note cannot go stale — "MarkKeptVerbatim is re-applied … BEFORE the pipeline reads the transcript" — is only true for components that check the mark. Ten do:

$ git grep -n 'skipReduce(\|isKeptVerbatim(' components/offload/*.go | grep -v _test
agentdiet cmdfilter collapse dedup extract extract_llm extract_sweep
failed_run linecap mask readlifecycle skeleton   (+ state.go itself)

summarize is in neither list — git grep -n 'isKeptVerbatim\|skipReduce\|keptKey' components/offload/summarize.go is empty — and it does not edit messages in place, it replaces the whole span msgs[start:end] with one summary message. So under preset: summarize the sequence is: repair sees the original at its position → writes the pointer → summarize summarises that span away → the model receives [expand: the content for id HASH is present in the transcript above] and, above it, a summary plus a marker for a different id. Before this change it received the content.

The likely ordering is the bad one, not the rare one: the expand round-trip is the newest thing in the transcript at repair time, so it sits in summarize's kept tail, while the original is older and therefore inside the span.

There is a pre-existing half to this that the fix would also close. Summarize re-compacting content the agent just expanded is the per-turn bounce loop cg:keep: exists to prevent — the reason every other offloader consults it. So the cheapest fix is at that end rather than this one: have summarizeSpan (or the span loop) skip a message whose contentKey is kept-verbatim, which restores the invariant this note depends on and gives summarize the guarantee the other ten already give. Doing it in contentPresent instead — e.g. only pointing when the summarize component is absent — would leave the loop.

If you would rather not touch summarize on this PR, the honest alternative is to say so where RestoredInPlace states the invariant: the note is safe for pipelines whose components honour cg:keep:, and preset: summarize is not one of them. I would not leave the current sentence, because it states the property unconditionally and a reader will rely on it.

[amiddavid]

This Get is not free in the way the comment says: it slides a pinned entry's TTL, so it keeps the abandoned decision alive for the whole session.

"One extra Get on rare content" accounts for the CPU. Memory.Get also does setExpiry(e) unless noSlide, and store.FrozenPrefix is in DefaultPinPrefixes — so reading the decision here renews the lifetime of a pinned entry that this branch has just decided will never be replayed again. Verified on this branch, 100s TTL, ten turns 60s apart:

turns run (this branch): frozen decision still alive 600s into a 100s TTL: true
no turns run (control):  frozen decision still alive 600s into a 100s TTL: false

Why it is worth a line rather than a shrug: pinned entries count against Memory.exemptRoom(), which is what gates stash admission, so a pin that no longer expires is pressure on the rewind reserve — stash_refused, declined removals, lost savings. That is the budget #188 introduced and #204 is currently reworking, reached from a new direction by a diagnostic.

I am not sure it is wrong. cg:keep: is evictable and not pinned, so if the mark lapses the replay resumes and the decision is wanted again — on that reading, keeping it alive is a small improvement. But that is a resource decision, and right now it is being made accidentally by a counter. Either state it ("and the read deliberately keeps the decision alive, because the keep-mark can lapse before it does"), or take a non-sliding probe — the store has no Has/Peek today, and one narrow addition would serve this and any future diagnostic read.

Worth noting a second effect if you do add one: counting once per (component, content) rather than per turn would also retire the "per turn per message, not per distinct content" caveat you have had to document in three places.

[amiddavid]

Small one, and the only place the eleven-site conversion changed behaviour rather than shape. The original was if content == "" || skipReduce(c, content), where || short-circuits — skipReduce never ran for empty content. Hoisting the call into the init statement runs it unconditionally, so an empty message now takes a store Get on keptKey(contentKey("")).

Harmless in outcome (the continue happens either way) but it inverts the ordering skipReduce's own new comment establishes — marker first precisely so the store is not asked on the cheap path — and if the empty string were ever marked kept-verbatim it would raise a spurious kept_verbatim_after_expand on every empty message. Reordering restores both:

if content == "" {
    continue
}
if gate, skip := skipReduce(c, content); skip {
    rep.Gate(gate)
    continue
}

[amiddavid]

The new block landed between CompactionResets' comment and its field, so that comment now reads as documentation for ExpandPrefixFlips: everything from "auto-compaction should be non-zero, and a run where it stays 0 while savings fall off a cliff mid-session…" is about compaction_resets, and it now sits above the wrong name. Moving the ExpandPrefixFlips block plus its field below CompactionResets fixes it and needs nothing else — in a file where the comments are the reference, this one will be read as authoritative and it is describing the neighbour.

[amiddavid]

This opens a second | Field | Meaning | table immediately after the stash table, which splits that table from the paragraph explaining it — the stash_refused "leading indicator" prose that follows now reads as commentary on expand_prefix_flips. Either add the row to the table above (it is the same /stats field list) or give it its own short subsection with a heading, since the flip is a different mechanism from the reserve counters and arguably deserves one.

Same file as #204's stash-row edits, so this will conflict; flagging so whoever merges second rebases rather than resolves by picking a side.

[amiddavid]

The summarize fix is right, and the part you worked out beyond my suggestion — that setting end to the kept-verbatim index leaves a tool_result at the head of the tail with its tool_use still in the span — is correct and is the half I had not thought through. I traced the retreat against summarizeSpan's two rejection shapes and it holds, including the two-results case and the retreat-to-start case. store.Peek is the right shape: non-sliding, and declining to remove an expired entry so a probe cannot move stash_expired or the reserve's byte total is the detail I would have missed. Declining the per-(component, content) recount as a separate decision rather than a quiet semantics change is the right call.

But the lesson you say paid for itself did not get applied to the fix it was about. The three new summarize tests all drive trimSpanForKeptVerbatim directly with a stub predicate, so none of them touches summarize.Offload. I removed the call site and ran the suite:

$ # summarize.Offload: trimSpanForKeptVerbatim call replaced with `_ = trimSpanForKeptVerbatim`
$ go test ./components/offload/ -run 'TestSummarize|TestTheTrimIsANoOp'
ok      github.com/rossoctl/context-guru/components/offload  0.238s
$ go test ./...
(28 packages ok, 0 failures)

The regression you just fixed — the one you say you would have shipped — is reachable again by deleting one statement, with the entire suite green. That is the same defect as the A == A loop on #204, and you caught its sibling one file over in TestTheFlipProbeDoesNotRenewTheDecisionItAsksAbout, which is exactly the right test. Detail and the cheap fix inline.

Two smaller things: an ordering point that is free to fix, and a consequence of the trim that the new counter does not see. Nothing else outstanding — the agentdiet early continue, the metrics.go move and the routes.md subsection are all as reported, and taking this branch off the stash rows does make the #204 rebase trivial.

[amiddavid]

Nothing asserts that summarize.Offload calls this. All three new tests call trimSpanForKeptVerbatim directly and pass func(text string) bool { return text == expanded } — a stub, not isKeptVerbatim. Verified by mutation on this branch: replacing the call at summarize.go:158 with _ = trimSpanForKeptVerbatim leaves these tests green and the whole suite green (28 packages ok, 0 failures).

So the regression is protected by the helper's own correctness and by nothing else. Three things could reintroduce it with every test passing: deleting the statement, passing a predicate that does not consult the store, or moving it below the point end is used.

The tell is visible in the fixture itself — line 38 builds st := store.NewMemory(...) and calls MarkKeptVerbatim(st, expanded), and then st is never used, because the stub replaced it. Dead setup is usually the signature of an assertion that stopped reaching the thing it was written for.

The harness for the real test already exists: commitgate_summarize_test.go drives Summarize.Offload end to end with a fake model and a ctx helper (:99, :159, :181). One assertion there — a store carrying the mark, Offload called, and the expanded message still present in req.Input afterwards — fails on all three mutations above. Keep these three as the boundary-arithmetic tests they are (the two-results and retreat-to-start cases are worth having in isolation); the point is that the wiring needs one of its own, for the reason TestTheFlipProbeDoesNotRenewTheDecisionItAsksAbout states in its own header: the drift is between what the call site does and what the helper offers, so the assertion has to live where the call site is.

[amiddavid]

Free to fix, and it only goes one way: this runs before the trigger gate at :165, so a turn where summarize declines still pays the scan. The scan is a contentKey hash plus a store Get for every message in the span, and it does not short-circuit in the common case — with nothing expanded it walks the whole span to return end unchanged.

Summarize previously did zero store lookups per span message; the other ten offloaders pay one per candidate, so the cost is in line with the rest of the pipeline, but paying it on turns where the component does nothing is not. Moving the trim below the trigger check and re-testing end <= start afterwards keeps every property (the trigger reads req, not end) and skips the work on declined turns:

if !s.trigger.Fires(req, c.CtxWindow) || end <= start {
    rep.Skipped = true
    return nil, nil
}
if trimmed := trimSpanForKeptVerbatim(...); trimmed != end {
    rep.Gate(GateKeptVerbatim)
    end = trimmed
}
if end <= start {
    rep.Skipped = true
    return nil, nil
}

As a side benefit the gate then means what it says: right now kept_verbatim_after_expand can be raised on a turn where summarize was never going to act, so the per-component gate count includes turns with no decision behind them.

[amiddavid]

A consequence of the summarize trim that expand_prefix_flips does not see, and I think it wants a sentence somewhere rather than code.

The trim lowers end below the expanded message, so a checkpoint whose boundary reached past it now fails the boundary > end guard at summarize.go:421 and cannot be reused. Summarize therefore re-summarizes a shorter span from scratch, which produces different summary text at a fixed position in the prefix — a suffix cache-write. Before the trim, that checkpoint kept matching and the summary stayed byte-identical (at the cost of the content, which is the bug you fixed). So the fix trades content loss for one cache-write, which is the correct trade and the same trade ExpandPrefixFlips was added to make visible.

It is invisible, though: expandFlips.Add(1) has exactly one call site, in reapplyFrozen's kept-verbatim branch, so the counter only sees the replay family. metrics.Snapshot and routes.md both describe it in general terms — "turns where an established compaction was abandoned because the agent had expanded that content" — which now has a second instance in a component the counter never reaches.

Either narrow the wording to the replay path, or count it here (a checkpoint refused because the trim moved end is exactly the event, and it is distinguishable from the ordinary boundary > end refusal by whether the trim fired this turn). I would take the wording change on this PR and leave the counting to a follow-up, since one more increment site changes what the number means and you have just declined a semantics change for that reason.

[amiddavid]

Verified independently, and both mutations fail. I rebuilt them from scratch rather than taking the report, and checked they compile first — for the reason you gave, a mutation that does not build is not evidence:

--- BASELINE ---   PASS
--- MUTATION A --- (call site replaced with `if false {`, compiles)
  FAIL summarize_keptverbatim_test.go:166: summarize removed content the agent had expanded
--- MUTATION B --- (predicate replaced with `func(string) bool { return false }`, compiles)
  FAIL summarize_keptverbatim_test.go:166: summarize removed content the agent had expanded

And the head independently: gofmt -l clean, go vet ./... clean, go test ./... with zero non-ok packages.

The precondition is what makes the new test carry its weight — asserting the unmarked fixture does get summarized away, so "the content survived" cannot pass on a fixture summarize never touched. That is the same guard the control arm gives the duplication measurement, applied one layer down, and it is the part I would have been most likely to leave out. The reordering below the trigger gate keeps the end <= start guarantee, and the note on the three boundary tests saying what they do and do not establish is worth more than the tests it annotates.

All three PRs are LGTM from me and I have nothing outstanding on any of them. #204 and #205 were done in earlier rounds; this closes #208. Two small notes below, neither needing a reply — one is an observation about the new fixture, the other is just confirming a scoping decision reads correctly.

On the process note: recording that two mutation attempts did not compile, and that neither counted, is the right instinct and it is rarer than it should be. A mutation that fails to build produces the same red as a mutation the test caught, which makes "the test failed" the least reliable sentence in a verification report unless someone checked which kind of red it was.

[amiddavid]

Observation, no action needed. expanded here is a hand-copy of bulkResult's format string (summarize_shape_test.go:41), so the test depends on bulkResult("t2") producing byte-identical text. That coupling is invisible from this file.

It is safe as written, and that is worth saying explicitly rather than leaving to luck: the precondition fails loudly if the coupling ever breaks — "the fixture does not contain the content to begin with" fires before anything else runs, so a bulkResult edit cannot silently turn this into a test of an absent string. That is the difference between fragile and brittle, and it is the right side of it.

If you ever want to remove the duplication, expanded := schema.MessageText(bulkResult("t2")) says the same thing with the coupling stated. Not worth a commit on its own.

[amiddavid]

This reads correctly, and the sentence that does the work is the last one — "do not read a zero here as expansion cost nothing" in metrics.Snapshot. A counter that names a general phenomenon while measuring one instance of it is only safe if the doc says which instance, and all three surfaces now do.

One thing worth confirming when #209 is picked up: kept_verbatim_after_expand on summarize's gate row is the population-at-risk signal you named, and it is now raised only below the trigger gate — so it counts turns where summarize would have acted and the trim stopped it, which is exactly the right denominator for that question and a tighter one than it would have been before this commit's reordering. Worth writing into #209 while the reason is fresh, since from the issue's side it will look like an unrelated ordering change.