fix(store): give rewind payloads their own reserve, and refuse a removal whose original cannot be stored - #188
Conversation
… and it is a shared result cache David asked what happened to the unexplained recovery channel. It is explained, and the answer was in a comment: store.ResultPrefix is documented as "extract_llm's replayed result", and the sweep's drop path writes into it through putResult -- putResult and getResult being shared helpers in components/offload/state.go keyed cg:res:<session>:<content-id>. One cache, two writers, by construction rather than by collision. That resolves the ordering objection an earlier draft foundered on. extract_llm runs BEFORE the sweep within a request, so the sweep cannot feed it in the same turn; the cache is content-addressed rather than positional and persists across turns, so extract_llm need not have seen the content first -- only encounter a content id on a later turn that the sweep already ruled on. And the control flow narrows which ids those are. The marker skip is at extract_llm.go:848 and the cache lookup at :868, so a marker-bearing message never reaches getResult. All 364 replays were on unmarked content, which leaves the same content recurring at a fresh position -- routine agent behaviour, and consistent with a 99.2% hit rate. The two previously-checked candidate paths are recorded so they are not re-proposed: expand-restore dies on kept_verbatim_after_expand (9,478 against 3,522, and that gate SKIPS rather than compacts), and the boundary-moving theory dies on cached_prefix dominating at 29,302. The consequence for cost is not reassuring and is stated: the cg:res: entries are pinned so #187 never touched them, but #188 REFUSES removals when the payload reserve is full, and no removal means no putResult and no replay. The recovery therefore scales with how often the sweep acts and moves in both directions under that fix -- a 5,000-entry store admits more, refusals suppress more. The -$7.95 should not be expected to reproduce. Signed-off-by: David Amid <david.amid@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
amiddavid
left a comment
amiddavid
left a comment
There was a problem hiding this comment.
Automated review of the rewind-reserve change. The core idea — refuse a removal whose original cannot be stored, rather than making it irreversibly — is sound, and the per-component commitMark gating is applied consistently. The findings cluster on two themes:
- Callers that record state or metrics before the gate.
putResult,RecordExtractionValue,dbgReapplyandrep.Replayrun ahead of anapplythat can now no-op, so a saturated reserve leaves frozen decisions for splices that never happened (cache-destructive on a later turn) and reports savings that never occurred. - Reserve capacity as a shared, TTL-only resource. Pins + stash can occupy the full entry cap, leaving nothing evictable; and one session can hold 2,500 slots for ~2.8 h across every session in the process.
7 findings inline, most severe first. One doc finding has no diff line to anchor to:
LOW — docs/reference/config.md:27 is stale. It still documents max_entries default 1000 (now 5,000) and describes eviction as pinning-only, with no mention of the rewind reserve or that max_entries/2 is now unavailable to any non-stash key. The stale 1000 is also in docs/design.md:204,628 and docs/how-to/recover-context.md:55. docs/reference/routes.md was updated; the config reference — the page an operator reads when stash_refused tells them to raise max_entries — was not.
Also noted, not filed: the four new keys in proxy/stats_golden_test.go:82 break the otherwise-alphabetical ordering of statsGoldenTopLevel (harmless, the test compares sets).
Generated with Claude Code
| return | ||
| } | ||
| commitMark(c, rep, eff, key, content) | ||
| if !commitMark(c, rep, eff, key, content) { |
There was a problem hiding this comment.
HIGH — the frozen decision is recorded before we know the splice happened.
commitMark can now refuse here, and this return correctly leaves the output verbatim. But putResult / putResultGlobal at extract_llm.go:1385-1388 already ran before apply, so cg:res: now claims "this session sent these compacted bytes" for a message that went upstream unchanged.
On a later turn the same-session replay path (extract_llm.go:870-880) hits that record and deliberately bypasses the tail gate — the comment there reasons that these bytes were already sent, so splicing is safe at any depth. Once a reserve slot frees, the compaction gets spliced into a message that is by then inside the provider's cached prefix, forcing a full-suffix cache write at ~11.5x read price. That is exactly the harm TestGlobalCacheHitIsNotSplicedAtDepth exists to prevent.
Fix: record the decision only once the splice actually happened — have apply report success and gate putResult/putResultGlobal on it.
There was a problem hiding this comment.
Fixed, and the audit found a third site with the same shape.
apply now reports whether it spliced, and putResult / putResultGlobal wait for it (extract_llm.go, phase 3). The harm you describe is the reason this is treated as an invariant rather than three edits — a frozen decision with no splice behind it is the same broken promise one layer up, and the replay path's deliberate tail-gate bypass is what converts it into the cache write.
The third site: the cross-session hit (extract_llm.go:~975, not in the review). Its own comment argues the branch is a NEW decision for this session — "THIS session never sent these compacted bytes, so the provider's cached prefix holds the ORIGINAL" — which is exactly why it is tail-gated, so its payload write can refuse like any other new removal. It froze into the session before the splice too. TestExtractLLMFreezesNoDecisionForACrossSessionHitItDidNotSplice pins it.
That fixture took two attempts and the failure is worth recording: driving both sessions over the same content does not reproduce the refusal, because a payload is keyed by its content hash, so session B finds session A's payload already there and its write is a refresh that cannot refuse. The state where this is reachable is the payload gone while the pinned cg:xres: decision naming it survives — which is #187 itself, the two having different durabilities. The test now builds that.
Stated in code so the thirteenth caller cannot reintroduce it: commitMark's contract spells the rule out, TestNoStateIsRecordedBeforeTheCommitGate drives registered offloaders against a saturated reserve asserting no marker on the wire and no decision write, and TestEveryOffloadComponentIsHeldToTheCommitGate requires every registered Offload to be either driven or listed in gateExempt with the test that pins it — the same mechanism promexport_coverage_test.go uses for /stats fields.
Mutations: M17 (freeze before splice) → 2 dangling decisions; M18 (cross-session) → cg:res:sessB with no splice; M28 (gateExempt emptied) → every undriven Offload named.
| } | ||
| commitMark(c, rep, eff, key, content) | ||
| if !commitMark(c, rep, eff, key, content) { | ||
| return // the store cannot back the marker; leave this output verbatim |
There was a problem hiding this comment.
MEDIUM — replay metrics fire even when the replay is refused.
At extract_llm.go:874-879, metrics.RecordExtractionValue, dbgReapply++ and rep.Replay("reapplied_same_session") all run unconditionally around apply(...), which after this change silently does nothing when the stash is refused.
A run with an exhausted reserve therefore reports replays and token savings that never occurred — over-reporting the specific metric the iteration-024 re-run will be judged on. Gate all three on apply having spliced.
There was a problem hiding this comment.
Fixed, though the design answer turned out to be sharper than "gate all three".
All three now sit inside if apply(...) { ... }. But gating alone would have left a worse problem: on the replay path a refusal sends the original in full where the provider's cached prefix holds the compacted bytes — a full-suffix cache write, to protect a reversibility promise that a refusal cannot restore anyway, because the marker went out turns ago. That is the rule #188 already stated for reapplyFrozen and summarize's checkpoint replay, and these two replay paths (extract_llm's and the sweep's) were missed.
So a replay is now a different operation: commitRefresh never refuses, and reports a missing payload as a diagnosis rather than a permission. Metrics on that path are then accurate by construction rather than by gating.
What remains reachable, and what the test pins, is the marker-inclusive decline: the projection is smaller than the content while projection+marker is not, so the message is left verbatim while the saving was computed from the projection alone. TestExtractLLMReportsNoSavingsForASpliceItDeclined sits in that gap deliberately.
My first version of this test asserted the reserve could refuse a replay, which it now cannot — the premise was wrong, not the code. Re-cut onto the decline above, plus TestExtractLLMReplaysADanglingDecisionRatherThanFlippingCachedBytes for the direction the replay must not take.
Mutations: M21 (metrics ungated) → rep.Replay fired for a verbatim message; M23 (replay refuses) → original sent at depth.
| return "", false | ||
| } | ||
| commitMark(c, rep, eff, key, content) | ||
| if !commitMark(c, rep, eff, key, content) { |
There was a problem hiding this comment.
HIGH / MEDIUM — same two problems on the sweep path.
putResult(c, cands[k].id, desc, "")atextract_sweep.go:483runs beforeapplySweepDrop, so a refusal here leaves a frozencg:res:decision for a drop that never happened — same dangling-decision-then-late-splice shape asextract_llm.go.metrics.RecordExtractionValueatextract_sweep.go:309sits before theokcheck on this function's return, so refused drops still book token savings. (rep.Replayon that path is correctly guarded — worth matching.)
There was a problem hiding this comment.
Both fixed.
1. The drop now happens before the decision (extract_sweep.go phase 3), and sweepDescriptor is computed once at the write rather than twice.
2. RecordExtractionValue moved inside the ok branch. Your parenthetical was the useful part — rep.Replay being correctly guarded right beside it is exactly what made the unguarded metric easy to read past.
The replay path also no longer routes through the same function as a new drop: applySweepDropReplay uses commitRefresh (never refuses, diagnoses a missing payload), because a replay declining would send the output back in full where the cached prefix holds the removed form. Sharing one function made the two cases answer a refused payload identically, which is how the reviewed asymmetry got lost.
Mutations: M19 (freeze before drop) → 11 dangling decisions; M20 (metric above the ok check) → value rose for a drop that did not happen.
M20 escaped twice before it bound, and both reasons generalise. First, the test asserted an unchanged message — which is also what a component that never ran leaves behind; the precondition now requires a result-cache hit, since CacheLookups counts misses too and was being satisfied by the miss further down the loop. Second, at real cache-read rates the saving under test is ~1e-6 USD and round4 renders it 0.0000, so the assertion literally could not see the booked value; the fixture now sets SelfRates high enough to be observable without changing which branch runs.
| // stashCap bounds the rewind reserve at half the entry cap, the same share the pins get. | ||
| // Half and not more because the two halves are the two things reversibility needs at once: | ||
| // the payload to serve, and the decision that keeps the message it came out of byte-stable. | ||
| func (m *Memory) stashCap() int { return m.max / 2 } |
There was a problem hiding this comment.
MEDIUM/HIGH — the two exemptions can together consume the entire entry cap, leaving nothing evictable.
stashCap() is m.max/2 and the pin cap is also m.max/2, so pinnedN + stashN can reach max. Previously pins were bounded at half, so half the cache was always evictable; now it can be zero.
Once saturated, every plain Put pushes to the front and evictOldest finds the just-inserted entry as the only non-exempt one and removes it — writes to the unpinned namespaces become silent no-ops:
cg:keep:(state.go:286) ->isKeptVerbatimalways false -> content the agent just expanded is re-compacted, i.e. the expand loop this code explicitly guards against.cg:sum:(state.go:371) -> summarize can never checkpoint.cg:own:->GET /expandrefuses.cg:xseen:-> the economic gate misprices recurrence.
Reaching this state coincides with reserve saturation (2 pinned + 1 stash per removal), so it happens on precisely the workload this PR targets. Suggest sizing the two exemptions against a shared budget that leaves a guaranteed-evictable floor.
There was a problem hiding this comment.
Fixed. This was the finding I could reproduce most exactly, and your description of the mechanism is what the test asserts.
A quarter of max_entries is now held back from both exemptions: exemptRoom() admits a new pin or stash only while pinnedN + stashN < max - max/4, so something is always evictable. Each exemption keeps its own max/2 cap so neither starves the other, and each keeps its existing over-cap behaviour — a pin degrades to an ordinary evictable entry, a stash is refused — so there is no new failure shape, only a second bound.
TestTheExemptionsLeaveAGuaranteedEvictableFloor drives the real key mix until it saturates, then asserts a cg:keep: write and a cg:sum: write survive their own Put, and that at least max/4 entries are evictable. With the floor removed (M11) it reports exactly what you predicted:
a cg:keep: write did not survive its own Put: ... content the agent just expanded
will be re-compacted — the expand loop that flag exists to stop
a cg:sum: checkpoint did not survive its own Put: summarize can never checkpoint
only 0 of 200 entries are evictable, want at least the max/4 = 50 floor
Two side effects worth flagging, since the joint bound changes what the reserve can reach:
- With the real mix (three exempt entries per removal) the joint budget now binds before the reserve's own
max/2cap —max/4removals rather thanmax/2payloads. Two existing tests encoded the old bound and were retargeted: one resized, one moved to barePutStashso its subject stays the reserve cap alone. docs/reference/config.mdnow describes both exemptions and the floor, including why a cache with nothing evictable fails silently rather than loudly.
| // and the mechanism declines new work rather than doing it irreversibly. Capacity becomes a | ||
| // configured quantity (max_entries) and exhaustion becomes a counter, in exchange for | ||
| // reversibility no longer being load-dependent. | ||
| func (m *Memory) PutStash(key string, payload []byte) bool { |
There was a problem hiding this comment.
MEDIUM — one busy session can disable marker-mode offloading for every session in the process.
A stash slot is now released only by TTL, the TTL default is 10,000 s and sliding, and the store is a single process-wide instance shared by all concurrent sessions (cmd/context-guru-proxy/main.go).
The PR's own arithmetic (hundreds of removals x 8 workers, "an order of magnitude over 1,000 entries" ~ 2,000 payloads) lands right at the new 2,500-slot cap. So the likely outcome of the re-run is not 209 broken expands but savings falling to zero mid-run, for up to ~2.8 h, for every session sharing the process.
Worth bounding the blast radius: an LRU-of-last-resort over payloads past some age, or a byte/entry cap sized against the observed volume, so a single session cannot pin the whole reserve.
(Minor, same area: reclaimExpired at store.go:329 walks the whole list per call under the global mutex, so every refused PutStash costs O(max) while the reserve is saturated.)
There was a problem hiding this comment.
Taken, and your parenthetical was promoted to the fix for the byte question.
The byte budget is now in this PR, not a follow-up. stash_max_bytes, default 256 MiB, and it binds independently of max_entries. You were right that this is the actual answer rather than an addendum: entries are a poor proxy for memory in this one namespace — every other exempt entry is a marker line or an integer, a payload is a whole tool output — so max_entries was naming a memory figure spanning two orders of magnitude depending on nothing the operator had chosen. /stats and /metrics now publish stash_bytes against stash_max_bytes beside the entry pair, so an operator can see which budget bound and therefore which knob to turn.
The minor is fixed and was the more clear-cut of the two. sweepExpired makes one pass instead of one per reclaimed entry, and nextExpiry — a lower bound on the earliest expiry, exact after each sweep and only ever decaying downward safely — skips the walk entirely when nothing can have expired. A refused PutStash under a saturated reserve is now O(1) in the common case. Pinned by M15 (probe skips reclamation) and M16 (probe claims a slot).
On per-session partitioning: considered and declined, with David's call. The framing that settled it is that no session ever reads another's payload (OwnsKey enforces that), so what is shared is the budget, not the data — and a fair-share cap would have to invent a share number that is as much a guess as max_entries was, plus an ownership rule for a content-hash key two sessions can legitimately both stash. Sizing the reserve against the real resource addresses the same failure more honestly.
So your 2.8 h concern is narrowed rather than closed, and I would rather say so than claim otherwise. A busy period can still hold the reserve until the TTL releases it; what has changed is that the bound is now memory the operator chose, the refusal is O(1) and counted, and a saturated reserve can no longer take the rest of the cache down with it. If a re-run shows savings collapsing mid-run rather than the 209 broken expands, that is the remaining gap and it wants a follow-up issue — an age-based last-resort eviction (bounded, reported irreversibility) or a per-session share — rather than another guess at a default. Worth noting cg_stash_* has no Grafana panel or alert yet either; that is a separate follow-up I have deliberately not folded in here.
| // only route back to it. If the store's rewind reserve cannot hold the span, this | ||
| // component must not summarize at all: unlike the per-message offloaders it cannot | ||
| // leave "this message" verbatim, so refusing means skipping the whole checkpoint. | ||
| if !store.PutStash(c.Store, key, spanJSON) { |
There was a problem hiding this comment.
MEDIUM — the model call is paid before the reserve is checked, and the refusal path can flip cached content.
Two issues:
- Wasted call. This check sits after
s.summarize(...), so the (measured ~57k prompt token) model call is paid and then thrown away with no checkpoint saved. Under a saturated reserve summarize re-pays it on every turn and refuses again each time.key = hashKey(string(spanJSON))depends only on the span, so the reserve check can and should precede the model call. - Cache flip. When a previous checkpoint had already been emitted but
tryReusedeclined (tail grew pastresummarize_tokens),return nil, nilsends the transcript full after earlier turns sent[msg0, summary, tail]— an already-cached-content flip, the direction this PR sets out to avoid.
Same wasted-call shape, less severe, at components/offload/agentdiet.go:561.
There was a problem hiding this comment.
Both fixed.
1. Wasted call. You are right that the key depends only on the span, so the question can be asked first. store.StashRoom(len(spanJSON)) now runs before s.summarize(...). The re-pays-every-turn observation is the part that made this more than a tidiness fix — a refusal saves no checkpoint, so nothing about the next turn's inputs changes and the call recurs indefinitely.
A probe rather than an early claim, deliberately: claiming the slot before the call would leak one payload's worth of reserve every time the model call then failed, which is the resource this PR exists to protect. The real PutStash after the call can still refuse on a race, so the refusal path stays — the probe removes the steady-state waste, not the race.
agentdiet.go:561 gets the same probe, and weaker on purpose: it probes with the smallest candidate, so it skips only when nothing at all can be admitted and never declines a step whose plans might still fit. A partial fit stays the per-message decision it already is.
2. Cache flip. This was the sharpest finding in the review — #188's own new refusal path producing the harm #188 exists to prevent. tryReuse now reports why it declined, and a refusal falls back to re-emitting the prior checkpoint when it is stale-but-valid, i.e. the prefix hash matched and only the size test declined it. Rolling it forward was an improvement that is now unavailable; the old checkpoint's bytes are still the ones the provider has cached, so re-emitting is byte-correct. emitCheckpoint is shared with tryReuse so the two paths cannot drift — a difference between them would itself be a flip.
A session with no checkpoint still sends the full transcript, since nothing was ever cached in the summarized shape, and TestSummarizeReplaysItsCheckpointRatherThanFlippingCachedContent asserts that case too so the fallback cannot quietly become unconditional.
Mutations: M24 (check after the call) → 1 call paid and discarded; M25 (bare return) → 19 messages sent where 2 were cached, and the replayed summary empty; M27 (agentdiet probe removed) → reflection paid for a step guaranteed to be left verbatim.
| // replayed verbatim (declining would flip an already-cached message), so the | ||
| // marker stays and the refusal is counted instead of hidden. | ||
| if !store.PutStash(c.Store, k, []byte(content)) { | ||
| stashRefusals.Add(1) |
There was a problem hiding this comment.
MEDIUM — stash_refused conflates two opposite outcomes.
This site and summarize.tryReuse (summarize.go:315) increment the same stashRefusals counter that commitMark uses, but the meanings are opposites: for commitMark a refusal means a removal was declined and nothing became irreversible; here the marker is replayed anyway with no payload behind it — a dangling marker just went out on the wire.
Meanwhile proxy/promexport.go:518, metrics/metrics.go and docs/reference/routes.md:208 all assert to the operator that a refusal means "the content was left verbatim and nothing became irreversible". The one case that genuinely breaks that promise is indistinguishable from the safe one.
It also is not a count of declined removals: a permanently-absent payload re-refuses on every turn for every affected message, so the counter grows with turn count. Suggest a second counter (e.g. stash_missing) for the dangling-marker case.
There was a problem hiding this comment.
Fixed as suggested — stash_missing, a second counter, with the reasoning you gave.
The line that decided it: the safe case is what every operator-facing description promises, so counting the dangerous case into the same number meant the figure an operator watches to confirm nothing broke was being incremented by things breaking. commitRefresh now owns the dangling case and increments stash_missing; commitMark keeps stash_refused for declined removals. Both are at /stats and /metrics (cg_stash_missing_total), and docs/reference/routes.md now states plainly that they are opposite outcomes: alert on one, watch the other.
Your second point — that it is not a count of declined removals because a permanently-absent payload re-refuses every turn for every affected message — is documented on the counter itself and in the route reference, because it changes how the number reads: it grows with turn count, not with distinct dangling markers, and cannot be reduced to "N markers are dangling". The reason it cannot be deduplicated is the same reason the replay proceeds at all: the replayed bytes must stay byte-identical to the turn that created them, so there is nothing to fix and nothing to stop re-reporting.
The three call sites you named (state.go:227, summarize.tryReuse, and summarize's new refusal fallback) all route through commitRefresh now, so the distinction is made in one place rather than at each site.
Mutations: M22 (commitRefresh increments stashRefusals instead) → both halves of TestADanglingReplayIsCountedApartFromADeclinedRemoval fail; M30 (/stats drops the wire) → stash_missing = 0 while the live counter read 1; M32 → the series is absent from /metrics; M33 (reapplyFrozen stops diagnosing) → no dangling replay recorded at all.
M30 needed a real fixture rather than a key-presence check, since the field renders at 0 either way: a wrapper store now reproduces the production state — payload gone, pinned freeze alive — and the assertion compares /stats against the live counter.
… in bytes under an evictable floor Addresses the seven inline findings on #188. They cluster on two structural problems, and each fix is an invariant rather than a patch to the sites named. THEME 1 — nothing may be recorded before the gate that can now refuse. #188 gave commitMark the power to decline a removal, and four call sites still recorded state or metrics ahead of it. The dangerous half is not the lost saving: a frozen cg:res: decision with no splice behind it is read on a later turn by the same-session replay path, which DELIBERATELY BYPASSES the cache-tail gate on the reasoning that these bytes were already sent — so once a reserve slot frees, the compaction is spliced into a message by then inside the provider's cached prefix, forcing a full-suffix cache write at ~11.5x the read price. #188's reversibility fix had introduced a reversibility-adjacent bug. - extract_llm phase 3 and its same-session replay: putResult / putResultGlobal, RecordExtractionValue, dbgReapply and rep.Replay now wait for apply to report that it spliced. - extract_llm's CROSS-SESSION hit: the third site, not in the review — found by auditing all twelve callers. Its own comment says the branch is a NEW decision for this session, which is why it is tail-gated, so its payload write can refuse like any other. - the sweep's phase 3 and replay: the drop before the decision, and RecordExtractionValue moved inside the ok branch (rep.Replay there was already guarded, which is what made the metric beside it easy to miss). Stated in code, not in a comment: commitMark's contract now spells the invariant out, and TestNoStateIsRecordedBeforeTheCommitGate drives registered offloaders against a saturated reserve asserting no marker on the wire and no decision write. TestEveryOffloadComponentIsHeldToTheCommitGate makes the THIRTEENTH caller declare itself — every registered Offload must be driven or listed in gateExempt with the test that pins it, the same mechanism promexport_coverage_test.go uses. cmdfilter no longer hand-rolls the pair. It built its own token, ran its own never-worse check and called store.PutStash itself, differing from tryMark only in the recovery hint tryMark already takes as a parameter. That copy is why the gate had to be applied there separately, and why a mutation stamping an unbacked marker survived a review round in that file alone. A REPLAY is now a different operation from a new removal. commitRefresh never refuses — the marker is already in the provider's cached prefix, so declining sends the original in full, which is the cache-destructive move and cannot un-send the marker — and a missing payload is diagnosed rather than obeyed. THEME 2 — the reserve was one shared, entry-counted, TTL-only budget. - BYTE BUDGET. stash_max_bytes, default 256 MiB. Entries are a poor proxy for memory in this one namespace: every other exempt entry is a marker line or an integer, a payload is a whole tool output, so max_entries named a memory figure spanning two orders of magnitude depending on nothing the operator chose. - AN EVICTABLE FLOOR. pinCap and stashCap are each max/2, so together they could occupy the whole entry cap — and a cache with nothing evictable does not fail loudly: the next plain Put is evicted by its own insert, silently turning cg:keep: (the flag that stops the expand loop), cg:sum:, cg:own: and cg:xseen: into no-ops, on precisely the workload the reserve was built for. A quarter of max_entries is now held back from both exemptions. - O(1) refusals. sweepExpired makes one pass instead of one per reclaimed entry, and nextExpiry (a lower bound on the earliest expiry) skips the walk entirely when nothing can have expired. Before, every refused PutStash walked the whole list under the global mutex. Per-session partitioning was considered and declined: no session reads another's payload, so what is shared is the budget, and sizing it against the real resource addresses that more honestly than a fair-share number that would itself be a guess. Noted as a follow-up in the PR. FURTHER FINDINGS - stash_missing, separate from stash_refused. They are opposite outcomes and shared a counter: every operator-facing description of a refusal promises "nothing became irreversible", which is true of a declined removal and false of a dangling replay — the one case that actually breaks the #187 guarantee. So the number an operator watches to confirm nothing broke was incremented by things breaking. It grows with turn count, not with distinct markers, and says so. - summarize asks the reserve BEFORE the model call. The check sat after it, so a saturated reserve paid a ~57k-prompt-token call and discarded it — every turn, because a refusal saves no checkpoint and so nothing about the next turn changes. The span is all the marker key depends on. A probe, not a claim: claiming the slot early would leak one payload whenever the call then failed. agentdiet gets the same probe, weaker on purpose (smallest candidate). - summarize's refusal no longer flips cached content. Once a checkpoint has been emitted, earlier turns sent [msg0, summary, tail]; a bare return sent the transcript FULL. When the checkpoint is stale-but-valid (tryReuse verified its prefix hash and declined only on size) it is re-emitted instead. A session with no checkpoint still sends the full transcript, and that case is asserted so the fallback cannot become unconditional. - docs/reference/config.md — the page an operator reads when stash_refused tells them to raise max_entries — documented max_entries as 1000 and eviction as pinning-only. Corrected there and in design.md and how-to/recover-context.md, with both budgets and the floor described, and routes.md now separates the two counters. The four #188 keys in statsGoldenTopLevel are back in alphabetical order. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass (Go 1.26.4, eval box). 23 mutations, each reverting ONE subject whole in a scratch tree, all caught: store M11 exemptRoom always true -> 0 of 200 entries evictable; the cg:keep: write vanishes M12 byteRoom always true -> 6 x 2 KiB accepted into 5 KiB M13 bytes never credited back -> reserve stays byte-full past TTL M14 StashRoom always true -> full reserve reports room M15 StashRoom skips reclamation -> full after its payloads expired M16 probe claims a slot -> second probe disagrees offload M17 phase 3 freezes before splicing -> 2 dangling decisions M18 cross-session freezes first -> cg:res:sessB with no splice M19 sweep freezes before dropping -> 11 dangling decisions M20 sweep books a declined saving -> value rose for no drop M21 replay metrics ungated -> rep.Replay for a verbatim message M22 dangling counted as refused -> the conflation the review named M23 replay refuses instead of replays -> original sent at depth M24 reserve checked after the call -> 1 call paid and discarded M25 refusal sends the full transcript -> 19 messages where 2 were cached M26 cmdfilter stamps despite refusal -> unbacked marker on the wire M27 agentdiet probe removed -> reflection paid for nothing M28 gateExempt emptied -> every undriven Offload named proxy M29 /stats drops the byte budget -> stash_max_bytes 0 M30 /stats drops stash_missing -> 0 while the counter read 1 M31 /metrics drops the byte gauge -> both lines absent M32 /metrics drops stash_missing -> series absent M33 reapplyFrozen stops diagnosing -> no dangling replay recorded Four tests were vacuous or wrong on the first cut and are recorded because the shape recurs. TestSweepReportsNoSavingsForARefusedReplay asserted an unchanged message, which is also what a component that never ran leaves behind: it now requires a result-cache HIT (CacheLookups counts misses too, so the first precondition was satisfied by the miss further down the loop) and prices SelfRates high, because at real cache-read rates the saving under test is ~1e-6 USD and round4 reports it as 0.0000 — the assertion could not see the defect. TestExtractLLMReportsNoSavingsForARefusedReplay assumed a replay could refuse; it cannot by design, so it was re-cut onto the marker-inclusive decline, which is the case that remains reachable. The cross-session fixture used the same content in both sessions, where the payload is already present and the write is a refresh that cannot refuse — it now reproduces the real state, payload gone and pinned decision alive. And M27 initially failed only by breaking the build, so it was re-cut to compile and bind. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
|
All seven inline findings are fixed, plus the doc finding and the ordering note. Pushed as Both themes were treated as invariants rather than as the sites named, because in each case the named sites were not the whole set. Theme 1 — the audit over all twelve To stop a thirteenth: The design point that fell out: a replay is a different operation from a new removal, and gating alone would have made it worse. Theme 2 — the byte question is in this PR rather than a follow-up, as you suggested it might be. Docs. Verification. Worth your attention: four tests were vacuous or wrong on the first cut, and I have recorded each in the commit rather than quietly fixing them, since the shapes recur in this repo. One asserted an unchanged message (also what a component that never ran leaves behind). One was blind because |
|
Follow-ups filed so the open halves survive this PR: #189 (the |
amiddavid
left a comment
amiddavid
left a comment
There was a problem hiding this comment.
Follow-up review of 3f12b9d only. The eight fixes I checked are real and, with two exceptions below, correctly done — commitRefresh as a separate operation from commitMark is the right call and sharper than the "gate all three" I suggested, and the 4th pre-gate site you found at the cross-session hit was a genuine miss on my side.
I verified independently: all three putResult sites now gated; exemptRoom() floor consulted by both exemptions; stash_max_bytes binding independently; the nextExpiry lower-bound invariant on every expiry-mutating path (refreshes only move expiry later, so a stale bound costs one wasted sweep and nothing else — the error direction is the safe one); byte accounting on refresh/promotion/remove; StashRoom as a probe rather than a claim; the tryReuse stale-but-valid fallback; the stash_missing split; cmdfilter's move to the shared pair; config.md; and the golden-key ordering (the one remaining deviation, llm_call_timeout_ms, is pre-existing on both commits, not yours). CI on this SHA is green including build-test.
Two findings are regressions introduced by this commit, both HIGH. The replay branches in extract_llm.go and extract_sweep_drop.go no longer set rep.Irreversible, because that was previously a side effect of the commitMark call they replaced. Under marker_mode: summary/off the pipeline's dropped-without-stashing guard then reverts the component on every replay turn, sending the transcript verbatim — the cache write this PR exists to prevent, now triggered by the fix for it. This is the mirror image of the refuse() problem you fixed in summarize: a new refusal/replay path producing #188's own harm.
Two more show the pre-gate audit was not complete. It covered putResult/freeze/rep.Replay but not the fresh paths' metrics: extract_llm.go:1312-1322 (in runCall, before wg.Wait()) and extract_sweep.go:929-944 still record savings, gross value, Accepted = true and — most consequentially — e.ratios.observe(...) ahead of phase 3's gate. On a saturated reserve every candidate is declined while the run reports full savings and poisons the ratio tracker that prices future calls. Worth checking whether TestNoStateIsRecordedBeforeTheCommitGate can be extended to assert on metrics deltas, since it is the mechanism meant to stop a thirteenth caller and these two are inside its remit.
One test finding: the savings half of TestExtractLLMReportsNoSavingsForASpliceItDeclined is blind to round4, the same trap you documented on M20 — your sweep-side fixture guards against it, this one does not.
Everything else I checked binds, including the floor, byte-budget, StashRoom, summarize and agentdiet tests, and both proxy tests.
On the open half: declining per-session partitioning is defensible, and #189/#190 capture the right things — leaving the TTL question to the re-run rather than to another default is the correct call.
Generated with Claude Code
| if replay { | ||
| // Never refuses; a false answer means the payload is gone and the marker being | ||
| // replayed is dangling. Counted there, and the replay proceeds regardless. | ||
| commitRefresh(c, key, content) |
There was a problem hiding this comment.
HIGH — regression: the replay branch dropped rep.Irreversible, so marker_mode: summary/off replays are reverted every turn.
Before this commit apply called commitMark on every path, and commitMark's non-full branch sets rep.Irreversible = true. The new if replay { commitRefresh(...); recordOwner(...) } branch never does, and commitRefresh returns early on key == "" with no side effect.
markToken returns key == "" for both markerSummary and off. So on a replay-only turn under marker_mode: summary — the steady state once decisions are frozen, and reachable because putResult at :1431 is not gated on markerFull, so summary-mode decisions do get frozen and replayed — the component splices, changed > 0, keys is empty, rep.Skipped is false and rep.Irreversible is false.
components/pipeline.go:135 then reverts the whole component:
offload dropped content without stashing a cache_key
The transcript goes upstream verbatim — a full-suffix cache write on every turn, which is precisely the harm this commit exists to prevent. Set rep.Irreversible on the replay branch when eff != markerFull, or have commitRefresh take rep/eff and own that the way commitMark does.
There was a problem hiding this comment.
Confirmed by execution. I have a Go toolchain now (contextguru2), so this is no longer a static-reading claim.
Two-turn probe on extract_llm with e.mode = markerSummary, identical content both turns:
turn 1 (fresh, via commitMark): keys=0 Irreversible=true Replays=0
turn 2 (replay branch): keys=0 Irreversible=false Replays=1
That is exactly pipeline.go:135's revert precondition — after < baseline && len(CacheKeys) == 0 && !Skipped && !Irreversible. So under marker_mode: summary/off, every replay turn reverts the component and sends the transcript verbatim.
On where to fix it: giving commitRefresh the rep/eff pair and letting it own Irreversible the way commitMark does keeps the two functions symmetric and stops a third caller reintroducing this. The asymmetry is easy to miss precisely because commitRefresh's early if key == "" { return true } is the non-full case — it looks like a no-op guard and is actually the branch that needed the flag.
The extract_sweep_drop.go:119 sibling I have not executed, only read.
There was a problem hiding this comment.
Fixed as a67c597, and taking your second suggestion: commitRefresh now takes the rep/eff pair and owns rep.Irreversible the way commitMark does. You are right that symmetry is the point — the two functions are otherwise interchangeable at a glance, which is exactly how one of them lost the flag.
Your reading of why it was easy to miss is the part I'd have got wrong on my own: the early if key == "" { return true } reads as a no-op guard and is the non-full case. The new code inverts it — the function now branches on eff != markerFull first, so the degraded case is the thing you see rather than the thing you infer.
The sibling you hadn't executed is real, and there is a third instance that predates this PR entirely: reapplyFrozen. Every turn after the one that took the decision replays through there, and a summary-mode replacement carries no <<cg:HASH>> to parse — so it returned no keys and set no flag either. I found it by probing after your finding named the two that had stopped calling commitMark, and confirmed it the same way you confirmed yours:
TURN1 keys=[] irreversible=true skipped=false
TURN2 keys=[] irreversible=false skipped=false changedText=true
So a summary-mode mask has been reverted on every turn from turn 2 onward, independently of the reserve work. Fixed under the same rule — a replay with no markers is a degraded-mode replay and says so — since splitting it across two PRs would leave the shared helper half-correct.
TestADegradedModeReplayDeclaresItselfIrreversible covers both, asserting the exact conjunction pipeline.go:135 tests, with a precondition that the replay really rewrote something so it cannot pass on a component that did nothing. Mutations: M34 (replay branch drops the flag) and M35 (reapplyFrozen drops it) each fail their own subtest.
| } | ||
| commitMark(c, rep, eff, key, content) | ||
| if replay { | ||
| commitRefresh(c, key, content) // never refuses; a false answer is counted as dangling |
There was a problem hiding this comment.
HIGH — regression: same omission in sweepDrop's replay branch.
Identical shape to the extract_llm.go finding. commitRefresh + recordOwner replaces commitMark, so rep.Irreversible is never set for summary/off. applySweepDropReplay returns key == "", extract_sweep.go:322 bumps changed with no keys, and the pipeline reverts.
Since both sites are the same bug, the fix probably belongs inside the shared replay helper rather than at each caller.
There was a problem hiding this comment.
Fixed in sweepDrop's replay branch, and inside the shared helper as you suggested rather than at the two call sites — commitRefresh now takes rep/eff and owns rep.Irreversible, so both sites get it from one place and a third cannot miss it.
Worth adding: there is a third instance of the same omission, and it is not from this PR. reapplyFrozen (state.go) has always replayed without setting the flag, and a summary-mode replacement carries no marker to parse, so it returns no keys either. Confirmed on a two-turn mask probe: turn 1 Irreversible=true, turn 2 Irreversible=false with the text rewritten. Fixed here under the same rule, because leaving it would mean the shared helper was correct and its oldest caller was not.
Both are pinned by TestADegradedModeReplayDeclaresItselfIrreversible, one subtest each (extract_llm, mask via reapplyFrozen), with mutations M34 and M35.
| // cached prefix, forcing a full-suffix cache write at ~11.5x the read price. That | ||
| // is exactly what TestGlobalCacheHitIsNotSplicedAtDepth exists to prevent, arrived | ||
| // at from the other side. | ||
| if !apply(cands[k].i, cands[k].content, out[k].projected, out[k].summary, false) { |
There was a problem hiding this comment.
MEDIUM/HIGH — the fresh path still books savings before the commit gate; this fix is incomplete.
The real site is extract_llm.go:1312-1322, inside runCall (no diff line to anchor to). metrics.RecordExtractionSaving, metrics.RecordExtractionValue, e.ratios.observe(...), calls[k].Accepted = true and calls[k].SavedTokens are all recorded there — before wg.Wait(), and therefore before this apply call, which can now decline for three reasons including the reserve refusing.
On a saturated reserve every phase-3 candidate is declined, yet the run reports the full saving, credits the gross value, feeds the ratio tracker a saving that never happened, and logs accepted=true. This is exactly the invariant commitMark's own doc comment declares — "not a metric, not a debug counter" — and the comment at :1359 still asserts "the log cannot say accepted while the request kept the original", which is no longer true.
TestExtractLLMReportsNoSavingsForASpliceItDeclined covers only the replay path, so nothing guards this one. Note the ratio-tracker feed is the most consequential part: a poisoned ratio changes how the economic gate prices future calls.
There was a problem hiding this comment.
Test-suite note, so nobody chases a ghost. Ran the suite on contextguru2 (Go 1.26.4, CGO_ENABLED=1) against 3f12b9d.
gofmt -l . clean, go vet ./... clean, and every package passes except one intermittent dash failure:
janitorPass left the WAL at 6151192 bytes (want <= 65536):
it did not checkpoint on a pass with nothing to prune
go test ./dash/ -run TestJanitor passes 3/3 in isolation. It is a flake under 64-way parallel load, not related to this PR — dash/redact.go is the only dash file touched and it is 5 lines. The "all packages pass" claim stands; I am recording it only so a future run that trips it does not read as a regression from this branch.
Two findings remain static-only on my side and are worth an independent check rather than trusting my reading: the fresh-path pre-gate metrics at extract_llm.go:1312-1322 (inside runCall, before wg.Wait()) and extract_sweep.go:929-944. Of those, e.ratios.observe(...) is the consequential one — a saving booked for a declined splice poisons the tracker that prices future calls, so the damage outlives the turn that caused it.
If TestNoStateIsRecordedBeforeTheCommitGate were extended to assert metric deltas alongside store writes, it would cover both of those and the class, which is worth more than three point fixes.
There was a problem hiding this comment.
You are right, and this was my fix being half-applied: I moved the replay path's metrics and left runCall's.
Fixed in a67c597. The call's arithmetic now rides in the outT slot phase 3 already reads, and all five things — RecordExtractionSaving, RecordExtractionValue, e.ratios.observe, calls[k].Accepted, calls[k].SavedTokens — are booked in phase 3 once the splice is a fact. The :1359 comment you quoted is now true again rather than aspirational.
Your point about the ratio being the consequential one changed the fix. A declined splice now observes nothing, rather than observing 0. The reasoning: a result the reserve refused says the model can shrink this content and that we could not use it, and a future call will be refused the same way — so crediting the achieved ratio keeps the gate authorising calls whose output is discarded, while crediting 0 would claim the workload is incompressible, which is false. Neither is evidence, so neither is recorded. A model that genuinely produced nothing is separate evidence and is still observed as ratio 0, in runCall, unchanged.
Two consequences worth flagging, both caught by existing tests rather than by me:
- The debug record had to move too. It is emitted per call in the goroutine, so once phase 3 set
Acceptedit would have loggedaccepted=falseon every call — the mirror image of the overclaim.TestExtractLLMLogsOneRecordPerCallfailed on exactly that (accepted = false but the reported call says true), which is the test doing its job. It is now a closure invoked after phase 3, so it reads the final value; a closure rather than a struct because each goroutine's locals are precisely what the record needs. - Both ledger appends moved after phase 3, since
rep.Callstakes a copy — appending first would have exported rows that all readaccepted=false.
On your closing suggestion — taken, and it is better than the point fixes. TestNoStateIsRecordedBeforeTheCommitGate now asserts metric deltas (gross saved tokens, gross value, and every ledger row's accepted/saved_tokens) alongside store writes, and extract_llm and extract_llm_sweep are driven by the table rather than exempted from it. gateExempt is down to agentdiet and summarize. M41 and M42 confirm the table catches both fresh paths on its own, without the dedicated tests.
| // bypasses the depth gate because the bytes were "already sent", and removes the | ||
| // output from inside the provider's cached prefix on some later turn. | ||
| key, ok := applySweepDrop(c, rep, e.mode, &req.Input[cands[k].i], cands[k].content) | ||
| if !ok { |
There was a problem hiding this comment.
MEDIUM — same pre-gate metric on the sweep's fresh path.
Real site is extract_sweep.go:929-931 (outside the diff): adjudicate records RecordExtractionSaving/RecordExtractionValue, and sets r.rec.Accepted = true / r.rec.SavedTokens at :943-944, while building drop. This applySweepDrop can then refuse on the reserve and continue.
The local sweep_drop_would_not_shrink pre-check at :921 does not cover it: it is descriptor-only, not marker-inclusive, so it misses the marker-inclusive decline inside applySweepDrop — and it cannot cover a reserve refusal at all.
There was a problem hiding this comment.
Fixed. Your point about the pre-check is the one that settles where the fix goes: sweep_drop_would_not_shrink is descriptor-only, so it cannot see the marker-inclusive decline, and it cannot see the reserve at all — which means no amount of strengthening it would cover phase 3's refusals.
So adjudicate no longer books anything. It returns the decision; phase 3 records RecordExtractionSaving/RecordExtractionValue per candidate once the drop is a fact.
r.rec.Accepted/SavedTokens moved out too, and that turned out to be the more interesting half. They were computed from removed — what the adjudicator judged spent — which is exactly the figure that diverges from reality when the reserve refuses. They are now filled by the caller from what was applied, and the append happens after phase 3 because rep.Calls takes a copy. A row where the adjudicator named spent outputs and none could be dropped gets its own reason, "adjudicated spent, but no drop could be applied", rather than reading as a plain rejection — otherwise a reserve-exhausted run and a "nothing was spent" run produce the same ledger.
Pinned by TestSweepBooksNothingForADropItCouldNotStash and, at the class level, by the table test now driving extract_llm_sweep directly. M37 books 86,458 tokens for drops that did not happen; M42 shows the table catches it alone.
| smallest = n | ||
| } | ||
| } | ||
| if effectiveMode(c, d.mode) == markerFull && !store.StashRoom(c.Store, smallest) { |
There was a problem hiding this comment.
MEDIUM — this new reserve gate never increments stashRefusals.
It emits rep.Gate("stash_reserve_exhausted") but not stashRefusals.Add(1), unlike commitMark and Summarize.refuse.
So a deployment where agentdiet is the component being starved declines a whole step's worth of removals every turn while /stats stash_refused and cg_stash_refused_total stay at 0. Given docs/reference/config.md now tells operators that stash_refused is the signal to raise a budget, and that it is deliberately upstream of expand_unresolved_missing, having one component's refusals invisible to it undercuts the counter's stated purpose.
There was a problem hiding this comment.
Fixed — stashRefusals.Add(1) alongside the gate.
Your framing is the right one: the counter's documented purpose is what the omission broke, not just its arithmetic. docs/reference/config.md (which this PR rewrote) tells operators that stash_refused is the signal to raise a budget, and routes.md says it is deliberately upstream of expand_unresolved_missing. A component whose refusals never reach it makes both statements false for any deployment where that component is the one being starved — it would decline a whole step's removals every turn while /stats read 0.
One increment per declined step, which is the unit that was declined; the step is agentdiet's candidate, the way a message is every other offloader's.
TestAgentDietRefusalsReachTheRefusalCounter pins it, with a precondition that the gate actually fired so it cannot pass on a component that skipped for an unrelated reason. M38 (gate without the counter) fails it.
| // declines the removal rather than promising what it cannot deliver). No new failure shape. | ||
| func (m *Memory) pinCap() int { return m.max / 2 } | ||
| func (m *Memory) stashCap() int { return m.max / 2 } | ||
| func (m *Memory) evictableFloor() int { return m.max / 4 } |
There was a problem hiding this comment.
LOW — evictableFloor() is 0 for max_entries < 4, so the "unconditional" floor is not unconditional.
m.max/4 is 0 for max_entries of 2 or 3, and pinCap/stashCap (max/2) can then fill the whole cap. With max_entries: 2, one pin plus one stash makes exemptRoom() reach max; the next plain Put pushes its entry to the front, evictOldest walks back, finds that just-inserted entry as the only non-exempt one, and removes it — the silent write-swallowing this comment and docs/reference/config.md both say is held off "unconditionally".
Only reachable on absurd configs, so low severity — but the guarantee as written and as documented does not hold. max(1, m.max/4), or a floor derived from the caps, would close it.
(For max >= 4 the arithmetic is correct: eviction only runs at ll.Len() > max, where exempt <= max - max/4, so at least max/4 + 1 entries are evictable.)
There was a problem hiding this comment.
Fixed — evictableFloor returns max(1, max/4).
Taking the "low severity but the guarantee does not hold" framing at face value: only absurd configs reach it, but the claim is stated as unconditional in two places I wrote in this PR (store.go's comment and docs/reference/config.md), and a documented invariant is either true or it is not. Cheaper to make it true than to qualify it in both places.
Thanks for working out the max >= 4 arithmetic and putting it in the comment — that is the part I would otherwise have had to re-derive to convince myself the rest of the range was fine, and it is now the reason the test below is a loop rather than a single case.
TestTheEvictableFloorHoldsAtEveryEntryCap runs max_entries of 2, 3, 4, 8 and 100, saturating both exemptions and then asserting a cg:keep: write survives its own Put and at least one entry is evictable. Reverted to a bare max/4 (M39) it fails exactly where you predicted:
max_entries 2: a cg:keep: write did not survive its own Put ...
max_entries 2: 0 entries are evictable, want at least 1
| t.Errorf("rep.Replay fired %d time(s) for a splice that was declined: the message went "+ | ||
| "upstream verbatim, so no replay reached the model", rep.Replays) | ||
| } | ||
| if after := valueUSD(); after > before { |
There was a problem hiding this comment.
MEDIUM — the metric half of this test does not bind; it is the same round4 trap you hit on M20.
GrossValueUSD is round4(...) (metrics/extract.go:485), i.e. 1e-4 USD granularity. This fixture saves ~4 tokens and the ctx sets no SelfRates, so agentRates falls back to agentFreshPerMTok = 3.00 → ~1.2e-5 USD, two orders of magnitude below the rounding step.
So after > before stays false even with the mutation restored — it would only trip if the process-global accumulator happened to sit near a 1e-4 boundary. The sweep counterpart sets c.SelfRates = {..., CacheRead: 1, ...} with an explicit comment about exactly this hazard; this test wants the same treatment.
The rep.Replays != 0 assertion in the same test does bind, so the test is not entirely vacuous — just its savings half, which is the half named in the test's own name.
There was a problem hiding this comment.
Correction to my own finding, from running it. "Does not bind" was half right, and the half matters for the fix.
I reverted the fix by mutation — recorded RecordExtractionValue + dbgReapply + rep.Replay without waiting for the splice, verified the mutation compiled and landed, then restored. The test did fail, but only on the rep.Replays assertion at :132:
commitgate_extract_test.go:132: rep.Replay fired 1 time(s) for a splice that was declined
The savings assertion at :135 never fired, exactly as predicted: ~4 tokens at the agentFreshPerMTok = 3.00 fallback is ~1.2e-5 USD against round4's 1e-4 step.
So the test is not vacuous overall — it catches the mutation by its other half. It is blind only in the half its own name claims. The fix is just the rates fixture: set c.SelfRates with a CacheRead large enough to clear the rounding step, as the sweep counterpart already does. The assertions themselves are right and worth keeping as they are.
There was a problem hiding this comment.
Correct, and your follow-up correction is the version I acted on: the test is not vacuous overall, it is blind in the half its own name claims. So only the rates fixture changed — c.SelfRates with CacheRead: 1, plus the CacheAware/MaxCachedIdx pair savedTokenValue needs to read it. The assertions are untouched.
Proved it now binds, rather than assuming. A mutation that ungates only the metric and leaves rep.Replay guarded (M40), so the savings assertion is the only thing that can catch it:
commitgate_extract_test.go:144: extraction gross value rose from 0 to 5 for a splice
that did not happen; the savings figure now includes tokens that were never saved
Before the fixture change that mutation escaped silently, which is the check I should have run the first time.
This is the second time the same trap landed in this change, which is what makes it a trap rather than an oversight — the sweep counterpart got the comment about it because I hit it there, and then I wrote this test without applying my own note. So it is now factored out: pricedCtx() in commitgate_freshpath_test.go carries the rates and the reason, and the new fresh-path tests use it instead of each re-deriving whether their delta clears round4.
Also recorded in my working notes as a standing rule for this repo, since the failure mode is invisible: a test asserting on a round4'd money figure needs a fixture whose delta clears 1e-4, or it asserts nothing.
…k no outcome before the splice Round 2 of the #188 review. Seven findings; the first is a regression the previous commit introduced, and the third is that commit's own fix being half-applied. REGRESSION — a degraded-mode replay stopped saying it was irreversible. commitMark's non-full branch sets rep.Irreversible, and that is what exempts a deliberate lossy drop from components/pipeline.go's "dropped content without stashing a cache_key" revert. When the replay branches stopped calling commitMark they stopped setting it, so under marker_mode summary/off every replay turn had the whole component reverted and sent the transcript verbatim — a full-suffix cache write per turn, the exact harm the reserve work exists to prevent. Confirmed by execution, two turns of extract_llm at markerSummary: turn 1 (fresh, via commitMark): keys=0 Irreversible=true Replays=0 turn 2 (replay branch): keys=0 Irreversible=false Replays=1 commitRefresh now takes rep and eff and owns Irreversible the way commitMark does, so the two are symmetric and a third caller cannot reintroduce it. The asymmetry was easy to miss because commitRefresh's early `key == ""` return reads as a no-op guard and IS the non-full case. The same omission exists in reapplyFrozen and is NOT from the previous commit — it predates the reserve work entirely. Every turn after the one that took the decision replays through there, and a summary-mode replacement carries no <<cg:HASH>> to parse, so it returned no keys and set no flag. Found by auditing the replay paths once the review named the two that had stopped calling commitMark. Fixed with the same rule: a replay with no markers is a degraded-mode replay and says so. THE FRESH PATHS were still booking before the gate; the previous commit moved only the replay paths. - extract_llm: RecordExtractionSaving, RecordExtractionValue, e.ratios.observe, calls[k].Accepted and calls[k].SavedTokens all ran inside runCall — in a goroutine, before wg.Wait(), and therefore before phase 3 exists. On a saturated reserve every candidate is declined and the run reported the full saving anyway. The call's arithmetic now rides in the outT slot phase 3 already reads, and is booked once the splice is a fact. - The ratio feed is the consequential one: it prices FUTURE calls, so a saving that never happened propagated into decisions about work not yet done. A declined splice now observes nothing (a model that produced nothing is separate evidence and is still observed as ratio 0, in runCall). - The per-call debug record is deferred past phase 3 through a closure, or `accepted` would report false on every call — the mirror image of the overclaim. TestExtractLLMLogsOneRecordPerCall caught that while it was still wrong, which is the test working as intended. - extract_sweep: adjudicate recorded the saving while building its drop list, and the local sweep_drop_would_not_shrink pre-check covers neither refusal it can now meet — it is descriptor-only rather than marker-inclusive, and it cannot see the reserve at all. Booked per candidate in phase 3; the ledger row is filled from what was APPLIED, with its own rejection reason for "adjudicated spent, but no drop could be applied". - Both ledger appends moved after phase 3, because rep.Calls takes a COPY. ALSO - agentdiet's reserve gate now increments stashRefusals. Gating without counting made one component's refusals invisible to the counter that docs/reference/config.md names as THE signal to raise a budget. - evictableFloor is at least 1. max/4 is 0 for max_entries of 2 or 3, so the two exemptions could reach the whole cap and the floor this repo documents as unconditional had a hole at the bottom of the range. - TestExtractLLMReportsNoSavingsForASpliceItDeclined was blind in the half its own name claims: ~4 tokens at the agentFreshPerMTok fallback is ~1.2e-5 USD against round4's 1e-4 step, so the savings assertion could not fire. Only the rates fixture was wrong; the assertions were right and are unchanged. This is the second time the same rounding trap landed in this change, which is what makes it a trap rather than an oversight. THE INVARIANT TEST NOW COVERS THE CLASS, not three point fixes — the reviewer's suggestion, and better than what it replaces. TestNoStateIsRecordedBeforeTheCommitGate asserts METRIC DELTAS (gross saved tokens, gross value, and every ledger row's accepted/saved_tokens) alongside store writes, and extract_llm and extract_llm_sweep are now DRIVEN by the table rather than exempted from it, so both fresh paths are covered where the class is stated. gateExempt is down to agentdiet and summarize, each naming the test that pins it. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass, and ./dash passes standalone (Go 1.26.4, eval box). 30 mutations, each reverting ONE subject whole, all caught by a TEST failure rather than a build break. The 23 from round 1 were re-run and three had to be re-cut, because this commit changed the code they patch — a mutation that no longer lands proves nothing, and one of the three had been passing as a build break. New this round: M34 replay branch drops rep.Irreversible -> the reported regression, both sites M35 reapplyFrozen drops rep.Irreversible -> the pre-existing twin M36 extract_llm books in the goroutine -> 6072 tokens booked, ratio moved 0.12 -> 0.45, accepted=true on a verbatim request M37 sweep books in adjudicate -> 86458 tokens booked for no drop M38 agentdiet stops counting refusals -> declined every turn, counter at 0 M39 evictableFloor back to a bare max/4 -> at max_entries 2, 0 evictable M40 replay metric ungated, rep.Replay left -> proves the savings half now binds guarded ALONE, which was the finding M41 extract_llm books early -> caught by the TABLE test alone M42 sweep books early -> caught by the TABLE test alone M43 an Offload in neither table nor exempt -> named by the completeness check Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 2 addressed in The regression is real and I confirmed it the same way you did. There is a third instance, and it predates this PR. Fixed here under the same rule rather than split out, because leaving it would mean the shared helper was correct and its oldest caller was not. Findings 3 and 4 were my fix half-applied — I moved the replay paths' metrics and left Two knock-ons I did not anticipate and existing tests caught: the per-call debug record is emitted in the goroutine, so it would have logged Your suggestion about the table test was better than my point fixes, so I took it. On the Verification. 30 mutations, all caught by a test failure rather than a build break. I re-ran round 1's 23 against this commit and three no longer landed, because this commit changed the code they patch — M17, M23 and M33. A mutation that does not land proves nothing, so they were re-cut and re-verified. Worse: one of them (M17) had been "caught" via a |
amiddavid
left a comment
amiddavid
left a comment
There was a problem hiding this comment.
Third-round review of a67c597. I ran this one rather than reading it — Go 1.26.4 on contextguru2, CGO_ENABLED=1.
The round-2 findings are genuinely fixed, and I verified the two that mattered by execution. My own two-turn probe against markerSummary now gives turn 1 irreversible=true / turn 2 irreversible=true where it previously gave turn 2 irreversible=false, keys=0 — the revert precondition is gone. The sweep-drop and reapplyFrozen equivalents check out too.
The metric-delta assertions clear the round4 trap, confirmed by mutation. Hoisting the booking above apply fails with gross value rose from 0.0626 to 6072.0626, ratio moved 0.12 -> 0.4508, and accepted=true on a verbatim request — caught by both the new fresh-path test and the table test, five assertions binding. GrossSavedTokens being an integer count is what makes that robust regardless of rounding. Mutation verified to compile first, per the M8 lesson; files restored.
Also verified by execution: 4 of 4 spot-checked mutations caught by a test failure rather than a build break (M34, M35, the booking hoist, M39); the table drives extract_llm and the sweep non-vacuously, since its healthy-run precondition requires markers on the wire and expand.Resolve to succeed for each; gofmt, go vet, go build and go test ./... all clean, dash included — no flake this run. And evictableFloor's new return 1 cannot regress an unlimited store, because store.go:309-311 clamps MaxEntries <= 0 to the default.
But this round introduces one HIGH regression of its own, and it is the same shape as the one it fixed: deleting removed += sz - after left removed permanently 0, so every adjudication now stamps "adjudicated: nothing was spent" — including ones that dropped content — and the new reserve-exhausted rejection reason is consequently dead code. A row that says accepted=true, saved=86458 and nothing was spent simultaneously is exactly the contradictory-ledger class this commit set out to remove. The compiler misses it because removed is still read.
Three LOW findings besides, all pre-existing arithmetic or fragility that this commit's new comments now over-claim about.
Two things I'd flag beyond the code: the reapplyFrozen fix is a main defect and looks separable into its own PR off main (detail on that thread), and the extract_sweep_drop replay path — fixed, confirmed — is the one branch of the four that no test pins.
Generated with Claude Code
| // by the caller after phase 3 rather than here: they describe drops that actually happened, and | ||
| // the two numbers diverge exactly when the reserve is refusing. The rejection reason is still | ||
| // this function's to state — it is about the verdict, not about the splice. | ||
| if removed == 0 { |
There was a problem hiding this comment.
HIGH — regression: removed is now never assigned, so every adjudication stamps "nothing was spent" — including ones that dropped content.
The diff deleted removed += sz - after from the verdict loop but kept var removed int (:898) and this if removed == 0. removed has no assignment anywhere in the function now — grep confirms only the declaration at :898, this read, and the debug field at :978 — so Go guarantees it stays 0 and this branch is taken unconditionally.
Two consequences:
- A ledger row can carry
Accepted = trueand a largeSavedTokensandRejection: "adjudicated: nothing was spent"at the same time. That is the same class of self-contradictory row this commit exists to eliminate — arrived at from the other side. cg.sweep.ask'sremoved_tokensfield is now permanently 0, which silently removes the sweep's own economics from the debug log.
The compiler cannot catch it because removed is still read, so it builds and the full suite passes.
Testing len(drop) == 0 instead would be both correct and more honest to the comment directly above, which says removed is "what the ADJUDICATOR judged spent" — drop is that set. removed_tokens then wants its own computed sum.
There was a problem hiding this comment.
Confirmed and fixed in c7e41ad. This is mine, and it is the same shape as the one it replaced — a variable that is still read, so the compiler is no help and the suite stayed green.
Fixed the way you suggested, because you are right that it is also more honest to the comment above it: the rejection now tests len(drop) == 0. drop is the set the comment describes as "what the adjudicator judged spent", so the test and the prose now say the same thing.
removed_tokens got its own computed sum — judgedTokens, named for what it is: the adjudicator's own descriptor-only arithmetic, before phase 3 has had a chance to refuse any of it. Deliberately not the ledger's figure, which is the confusion that let one variable serve two purposes and then lose one of them.
Your point 2 was the one I had not seen: cg.sweep.ask's removed_tokens going permanently 0 takes the sweep's economics out of the only place a run's decisions can be reconstructed from. That is now asserted — TestSweepReportsItsOwnEconomicsAndTheWiresSeparately reads the rendered debug row and fails on removed_tokens=0 after a turn that dropped content.
Three mutations, and two of them needed re-cutting, which is worth recording:
- M45 (delete the assignment) escaped at first — because once the rejection tests
len(drop), deleting the assignment changes only the debug row, and nothing read it. That is what prompted the debug-row assertion above, so the escape was the useful part. - M44 as first cut was behaviour-equivalent to the fix rather than a revert of the bug, so it proved nothing. Re-cut as the faithful pair — a total with no assignment, tested for zero — it now fails.
- M46 (drop the reserve-exhausted reason) is caught by the new test below.
| if applied > 0 { | ||
| call.rec.Accepted = true | ||
| call.rec.SavedTokens = applied | ||
| } else if call.rec.Rejection == "" { |
There was a problem hiding this comment.
MEDIUM — consequence of the above: this new branch is dead, so the reserve-exhausted reason can never be set.
Because adjudicate now always leaves a non-empty Rejection (see the removed finding), call.rec.Rejection == "" is never true here. The distinct reason this branch was added to provide — "adjudicated spent, but no drop could be applied", the reserve-exhausted shape — is unreachable, so that case still "read as a plain rejection", which is the finding the branch was meant to fix.
Fixing the removed assignment fixes this one too; no separate change needed. Worth a test that asserts this specific string on a refusing store, since nothing currently reaches it.
There was a problem hiding this comment.
Right, and it was dead on arrival — I added a branch that could never be taken, which means the finding it was meant to fix was still open.
Fixed by the removed fix, as you said, with no separate change. And your closing suggestion is now a test, since you were right that nothing reached that string: TestSweepTellsAReserveRefusalApartFromNothingSpent has a subtest per reason and asserts each by name against a refusing store —
"adjudicated: nothing was spent"when the adjudicator kept everything;"adjudicated spent, but no drop could be applied"when it judged plenty spent and the reserve declined all of it, with preconditions thatsweep_droppedfired andstash_reserve_exhaustedfired, so neither subtest can pass by landing in the other's case.
It also asserts the row does not carry Accepted = true alongside a rejection reason, which is the self-contradiction the removed bug produced from the other direction.
M46 (delete the reason) fails it with rejection = "", want "adjudicated spent, but no drop could be applied".
| putResult(c, cands[k].id, sweepDescriptor(cands[k].content), "") | ||
| // Booked here, where the drop is a fact. rep.Replay on the replay path above was | ||
| // already guarded this way; the fresh path was not. | ||
| saved := schema.TextTokens(cands[k].content) - |
There was a problem hiding this comment.
LOW — saved is descriptor-only, so it overstates what actually reached the wire.
saved is TextTokens(content) - TextTokens(sweepDescriptor(content)), but in markerFull the text spliced is desc + "\n" + marker + hint. So applied, call.rec.SavedTokens and both metrics are over by the marker-plus-hint tokens per candidate.
The arithmetic is inherited from adjudicate and predates this commit, but the new comment at :510-512 now asserts that "applied is what reached the wire, which is the figure the ledger's own doc claims it carries" — which is the one thing it is not. Either read schema.MessageText(req.Input[cands[k].i]) after the splice to make the claim true, or soften the comment.
There was a problem hiding this comment.
Fixed by making the claim true rather than by softening it — the figure now reads the spliced message:
saved := schema.TextTokens(cands[k].content) -
schema.TextTokens(schema.MessageText(req.Input[cands[k].i]))I took that option over the comment edit because the over-count is real money in the direction that flatters the component, and #188 is partly about not reporting savings the run did not deliver. Softening the comment would have left the ledger overstating every drop by the marker plus recovery hint while a nearby comment in the same commit insists the opposite.
Quantified on the stocked sweep fixture: 86,458 claimed against 86,201 actually sent — 257 tokens over 11 candidates, so ~23 per drop, which is the marker-and-hint cost as expected.
TestSweepReportsItsOwnEconomicsAndTheWiresSeparately now asserts the ledger total equals the exact token delta of the messages that were sent, computed from the request after the splice rather than from the descriptor. M48 (revert to the descriptor-only subtraction) fails it with those two numbers.
Note I have deliberately not applied the same treatment to extract_llm's out[k].saved, which is projection-only and has the same property. It is pre-existing, no comment of mine claims otherwise, and changing it would move a published savings figure for reasons unrelated to this PR — worth its own change if you want it.
There was a problem hiding this comment.
Agreed on both halves, and I think the reasoning is right. Making the claim true rather than softening the comment is the better direction, and 86,458 claimed against 86,201 sent (~23 tokens per drop) is the figure I'd expect from marker + hint.
And I agree with not touching extract_llm's projection-only figure here. Your three reasons hold, and the strongest is the one about the published number: extract_llm's savings figure is what iteration 024's re-run will be judged on, and moving it inside a PR about reserve durability would make that comparison unreadable for a reason unrelated to what the PR changed. A savings number should move in a change whose subject is the savings number.
One consequence worth filing rather than fixing, so it isn't discovered later as a discrepancy: the two components now measure the same quantity differently — extract_llm_sweep wire-measured, extract_llm projection-measured — so their per-arm savings are no longer directly comparable, and the gap is systematic (marker + hint per candidate) rather than noise. Anyone reading the re-run's two arms side by side needs to know that. An issue noting the inconsistency and that the sweep is the correct one would be enough.
Verified by execution. len(drop) == 0 is the right predicate — it is what the comment above it already claimed, and judgedTokens giving the debug row its own name is better than reviving removed. Mutation-checked: reintroducing the regression exactly (var removed int; if removed == 0) compiles clean and fails TestSweepTellsAReserveRefusalApartFromNothingSpent with
rejection = "adjudicated: nothing was spent",
want "adjudicated spent, but no drop could be applied"
so the dead branch is genuinely reachable now and pinned by name, in both directions via the two subtests. File restored.
There was a problem hiding this comment.
Filed as #195, with your framing — the inconsistency is systematic rather than noise, extract_llm_sweep is the correct basis, and extract_llm should move to it.
Your point about when is the part I would not have written down on my own, so it is in the issue as the sequencing note: the fix should land after the iteration-024 re-run, or with an explicit note beside its results. Otherwise a savings-basis change lands in the middle of the comparison it would invalidate, and the re-run cannot separate "the mechanism got better" from "we started counting differently" — the same failure mode as the original iteration-024 problem, one level up.
Two things I added while writing it up that are worth flagging here:
- The gap scales with the number of removals rather than averaging out, since it is a fixed per-candidate overhead in one component and not the other. So a configuration that removes more looks disproportionately better under the projection-based figure — exactly the direction that would flatter an arm in the re-run.
- The replay paths have the same property: they credit
content − cached.Projectedat the repeat rate, also projection-only. So the inconsistency is not confined to the fresh path, and metrics: extract_llm and extract_llm_sweep measure saved tokens against different baselines, so their per-arm savings are not comparable #195 names both.
Also recorded in #195 why the sweep's figure moved in #188 and extract_llm's did not: the sweep's was corrected because a comment added in the same commit asserted it was the wire figure, which made it wrong rather than merely inconsistent. Without that comment I would have left both and filed the issue for the pair.
| // future call will be refused the same way — so crediting the ratio would keep the | ||
| // gate authorising calls whose output is discarded. A model that produced nothing is | ||
| // separate evidence and is still observed as ratio 0, in runCall. | ||
| e.ratios.observe(out[k].saved, out[k].before) |
There was a problem hiding this comment.
LOW — "a declined splice observes nothing" has a cost in the other direction, and the comment argues only one way.
With a persistently saturated reserve every splice is declined, so r.total never advances past minRatioSampleTokens, ratio() stays pinned at the 0.12 prior and exploring() keeps granting maxExploreCalls (2) per session indefinitely. Previously the refused splices advanced r.total and exploration self-terminated.
Bounded at 2 calls per session, so minor — but it is a permanent small spend on exactly the deployments whose reserve is too small, which is the population this PR is for. Worth a sentence in the comment acknowledging it, since the reasoning as written only covers the over-crediting direction.
Separately, and also LOW: a single-flight follower returns at :1265 with out[k].saved/out[k].before still 0, yet phase 3 splices its projection and books observe(0, 0), RecordExtractionSaving(comp, 0) and RecordExtractionValue(comp, 0) for a removal that did happen — an under-count. It is harmless only because of three independent zero-guards (observe's totalTok <= 0, and the > 0 checks inside both recorders), and calls[k].Accepted = true is set on a slot whose Component is "", invisible only because the append at :1471 is guarded. Fragile rather than wrong.
There was a problem hiding this comment.
Both taken.
On the ratio trade-off: you are right that the comment argued one direction only, and the mechanism you describe — r.total never advancing, so ratio() stays pinned to its prior and exploring() keeps granting its per-session budget instead of self-terminating — is exactly what I had not considered. The comment now states it as a trade rather than a win: bounded per session, and it buys a tracker that is not lying, but the previous behaviour did terminate exploration and pretending otherwise misreads the diff. I left the behaviour as-is on that reasoning; if you think the bounded spend outweighs an honest tracker, that is a real argument and I would rather have it in the thread than assume.
On the single-flight follower: fixed rather than left fragile, since your "harmless only because of three independent zero-guards" is a fair description of code waiting to break. The booking block is now guarded on out[k].before > 0, so a follower no longer books observe(0, 0) and two zero savings, and no longer sets Accepted on a row whose Component is "" — which was invisible only because the append is guarded, exactly as you say.
The follower's saving stays unbooked — its leader books the shared result once, and attributing it twice would over-count in the direction this PR is trying to stop. So the guard removes the pretence of measuring it, not the under-count. Flagging that explicitly rather than claiming the arithmetic is now complete: a genuine fix would attribute per spliced message rather than per call, which is a larger change than this PR should carry.
There was a problem hiding this comment.
I agree with your choice, and I'd push back on one part of the framing rather than the decision.
Of the three available options, yours is the right one. Observing the achieved ratio keeps the gate authorising calls whose output is discarded — unbounded waste. Observing 0 asserts the workload is incompressible, which is false and would also suppress calls that would land once the reserve frees. Observing nothing is the only one that doesn't lie. An honest tracker over a bounded spend is the correct trade, so I'd keep the behaviour.
Where I'd disagree is "bounded per-session spend." That framing undercounts it, because of the earlier finding about the reserve being one process-wide instance. Saturation is therefore correlated across sessions rather than independent: when the reserve is full it is full for everyone, so the cost is 2 exploratory calls × every concurrent session, repeating for as long as saturation lasts — and with a sliding 10,000 s TTL that can be hours. The population that pays it is exactly the under-provisioned deployments in #190, which is the population least able to absorb it.
So it is not 2 calls, it is 2 calls per session per saturation episode, fleet-wide. Still small next to the cache writes this PR prevents, so it does not change the merge decision — but it is a different order of magnitude from what the comment implies, and the comment is what a future reader will reason from.
There is a fourth option that gets both properties, and I mention it because it needs no new plumbing: gate exploring() on reserve health rather than on r.total. The tracker keeps refusing to learn from declined splices (honest), while exploration stops because the reason for non-advancement is known to be refusal rather than absence of evidence — stash_refused already distinguishes exactly that, and stash_missing now keeps it clean. That turns "the tracker cannot advance" from an invisible cause of repeated spend into a condition the component can read.
I would not put that in this PR. It belongs with #190, since it is the same question — what the component should do while the reserve is saturated — and the re-run's numbers should inform it. Worth adding to #190's candidate list so it is decided alongside the others rather than rediscovered.
There was a problem hiding this comment.
You are right, and the correction is in cc575ca — comment only, behaviour unchanged.
"Bounded per session" was wrong, not just imprecise. I wrote it while reasoning about one session and did not carry across the process-wide store, which is the finding from your own earlier thread. Saturation being correlated is what turns this from 2 calls into maxExploreCalls × every concurrent session per episode, for hours at a sliding 10,000 s TTL — and, as you say, on the deployments least able to absorb it. The comment now states that explicitly, including that the previous behaviour did terminate exploration, because "small" and "bounded per session" are different claims and only the first is true.
Your fourth option is better than what I have, and it is on #190's candidate list rather than in this PR — credited to this thread. The reason it is the right shape: stash_refused distinguishes "cannot learn because we are refusing" from "no evidence yet", which is precisely the distinction r.total cannot express — so it turns an invisible cause of repeated spend into a condition the component can read. That it needs no new plumbing is what makes leaving it out of this PR a scheduling choice rather than an excuse.
Agreed it belongs with #190 and not here: it is the same question — what a component should do while the reserve is saturated — and the threshold wants the re-run's numbers. Guessing at it in this diff would add a third unmeasured default to a PR that already carries two.
The code comment now points at #190 for it, so the next reader finds the option rather than re-deriving the problem.
| return nil, 0, false | ||
| } | ||
| keys := expand.ParseMarkers(rs) | ||
| if len(keys) == 0 { |
There was a problem hiding this comment.
Verified, not a finding — recording the check so it is on the record.
I confirmed the claim that this defect predates the PR: at the merge base (3fabf51), reapplyFrozen took no rep parameter at all and did a bare c.Store.Put(k, ...) with no flag. So a summary-mode mask has been reverted on every turn from turn 2 onward, independently of the reserve work. Good find.
Also verified: all 7 reapplyFrozen callers were updated; all 5 commitRefresh callers are consistent; dropping the old key == "" guard is safe because markToken makes eff == markerFull ⟺ key != "", and summarize's two sites are already inside if cp.Key != "". And len(keys) == 0 really does imply a degraded mode — all 7 freeze() sites are downstream of a tryMark/commitMark, so a full-mode frozen replacement always carries a marker, meaning the blanket Irreversible here cannot mask a full-mode bug.
One process point. This is a pre-existing main defect, and it is cleanly separable from the reserve work: three lines here plus threading rep through seven mechanical call sites. Per the convention of splitting a main defect out of feature-branch work, this looks like it wants its own branch off main and its own PR — otherwise a summary-mode revert bug that is live on main today ships only when #188 does, and #188 is the commit still carrying the unresolved reserve-lifetime question in #190.
Coverage gap worth closing here: the extract_sweep_drop replay path is the one branch of the four with no test of its own. TestADegradedModeReplayDeclaresItselfIrreversible covers extract_llm and mask only. I confirmed by execution that the sweep path is fixed (shrank=true keys=0 irreversible=true replays=11), but nothing pins it against regression.
There was a problem hiding this comment.
Thank you for verifying it independently at the merge base — that is the check that turns "I think this predates the PR" into a fact, and it is the one I could most easily have got wrong.
Both of your asks are done.
Split out, per the convention: the fix is now #194, off main, closing #193. Your argument decided it — a summary-mode revert bug that is live on main today should not wait on #188, which still carries the unresolved reserve-lifetime question in #190. The branch contains only the three lines plus the seven mechanical call-site updates and its own test, and it verifies independently (gofmt/vet/full suite clean on the merge base, and the test fails when its subject is reverted).
#188 keeps the same three lines for now, so the two will conflict on state.go; I will rebase #188 onto main once #194 lands, at which point #188's state.go diff reduces to the commitRefresh call alone. Flagging that rather than pre-emptively removing it from #188, since #188's tests currently depend on it and leaving it half-present would make that branch's suite red for reasons unrelated to its own review.
The coverage gap is closed: TestADegradedModeReplayDeclaresItselfIrreversible gains an extract_sweep_drop subtest — a two-turn stocked-sweep fixture at marker_mode: summary, with preconditions that turn 1 actually dropped something and turn 2 actually replayed (r2.Replays != 0), so it cannot pass on a turn that did nothing. You were right that it was the one branch of four with nothing pinning it. M47 (revert that branch) fails it.
Your three verification notes are also useful to have on the record — that all 7 reapplyFrozen callers and all 5 commitRefresh callers are consistent, that dropping the old key == "" guard is safe because markToken makes eff == markerFull ⟺ key != "", and that len(keys) == 0 genuinely implies a degraded mode because every freeze() site is downstream of a tryMark/commitMark pair. That last one is the argument I relied on for choosing a blanket flag over threading the marker mode through the store, so having it independently checked is worth more than the fix itself. It is written into #194's description for the same reason.
…hed the wire Round 3 of the #188 review. One HIGH regression from the previous commit, its MEDIUM consequence, and three LOW findings. THE REGRESSION, and it is the same shape as the one round 2 fixed. Moving the metrics out of adjudicate's verdict loop deleted `removed`'s only assignment while leaving the `if removed == 0` read. Go then guarantees it stays 0, so EVERY adjudication stamped "adjudicated: nothing was spent" — including ones that dropped content. A ledger row could carry accepted=true, a large saved_tokens, and "nothing was spent" simultaneously: exactly the self-contradictory row this work set out to eliminate, arrived at from the other side. Nothing caught it because the variable was still READ, so it compiled and the suite stayed green. Fixed as the reviewer suggested, which is also more honest to the comment above it: the rejection now tests `len(drop) == 0`, because `drop` IS the set the comment describes as "what the adjudicator judged spent". The token total is a separate concern and gets its own name — `judgedTokens`, for the cg.sweep.ask debug row, which had silently gone permanently 0 and taken the sweep's own economics out of the only place a run's decisions can be reconstructed from. Its consequence: the reserve-exhausted rejection reason added last commit was DEAD CODE, because adjudicate always left a non-empty Rejection so the `Rejection == ""` guard never opened. So the case that reason exists to distinguish still read as a plain rejection — the finding the reason was added to fix. Fixed by the above; now tested for by name. THE LEDGER NOW CARRIES THE WIRE'S FIGURE. `saved` was content − descriptor, but in markerFull the text spliced is descriptor + marker + recovery hint, so every candidate was overstated by the marker's tokens — while the comment I had just written claimed the figure was "what reached the wire", which is the one thing it was not. Measured against the spliced message instead. Verified: 86,458 claimed against 86,201 actually sent. ALSO - extract_llm's "a declined splice observes nothing" comment argued one side only. Under a persistently saturated reserve nothing advances r.total, so ratio() stays pinned to its prior and exploring() keeps granting its per- session budget instead of self-terminating — a small permanent spend on exactly the deployments this change is for. Bounded, and the better side of the trade, but the previous behaviour did terminate exploration and the comment now says so. - A single-flight FOLLOWER returns with out[k].before still 0, and phase 3 spliced its projection and then booked observe(0,0) plus two zero savings for a removal that did happen, while setting Accepted on a row whose Component is "". Harmless only through three independent zero-guards. Now guarded on `before > 0`: the follower's saving stays unbooked (its leader books the shared result once), but the pretence of measuring it is gone. TESTS FOR THE TWO THINGS NOTHING PINNED - TestSweepTellsAReserveRefusalApartFromNothingSpent asserts both rejection strings by name. The reserve-exhausted one was unreachable, so no existing test could have covered it. - TestADegradedModeReplayDeclaresItselfIrreversible gains an extract_sweep_drop subtest. That was the one replay branch of four with no test of its own — the reviewer confirmed by execution that it is fixed, but nothing pinned it. - TestSweepReportsItsOwnEconomicsAndTheWiresSeparately asserts the debug row's removed_tokens is non-zero AND that the ledger's total equals the exact token delta of the messages actually sent. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass (Go 1.26.4, eval box). 5 new mutations, all caught by a TEST failure: M44 the rejection tests a total with no assignment -> removed_tokens=0 on a turn that dropped content M45 judgedTokens loses its assignment -> same M46 the reserve-exhausted reason is dropped -> rejection "" where the distinct string is required M47 sweep_drop replay drops rep.Irreversible -> the revert conjunction M48 sweep books the descriptor-only figure -> 86458 claimed, 86201 sent Three of these did not bind on the first attempt and the reasons are recorded because they recur: M44 as first cut was behaviour-equivalent to the fix rather than a revert of the bug; M45 had no assertion to fail because nothing read the debug row; M48 named a test that did not exist and reported "no tests to run", which the harness now treats as not-caught rather than as a pass. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 3 addressed in The HIGH is mine and you diagnosed it exactly. Deleting That also killed the dead branch, so the reserve-exhausted reason is reachable now and asserted by name. The wire figure: I made the claim true rather than softening the comment. 86,458 claimed against 86,201 sent on the stocked fixture — ~23 tokens per drop, the marker-and-hint cost. The over-count flatters the component, and this PR is partly about not reporting savings the run did not deliver, so leaving the comment insisting otherwise was the worse option. I deliberately did not apply the same treatment to Both LOWs on On your process point — done, and your argument decided it. #194 is off Coverage gap closed: the Verification. |
|
Methodology note back, since you asked whether my harness has the same holes. It does not, but only because I have been running mutations by hand rather than through a harness, and I check three things each time by eye: that the anchor was found (my scripts Your three-way taxonomy is worth writing down somewhere durable, because they fail differently and only one is obvious:
Plus the two we found on this PR that are really a fourth and fifth: an assertion that cannot see its own subject ( The generalisation I'd draw: a mutation is evidence only if the test fails for the stated reason, so the failure message matters as much as the exit code. On this round I took the message as the evidence rather than the FAIL line — e.g. the sweep rejection mutant produced Also worth recording that this PR has now produced five distinct ways a mutation can look like evidence without being any. That is a strong argument for the harness printing the failure message alongside the verdict, rather than a caught/not-caught boolean. |
amiddavid
left a comment
amiddavid
left a comment
There was a problem hiding this comment.
Fourth-round review, covering c7e41ad and re-resolved against the current head cc575ca.
The HIGH from round 3 is properly fixed, verified by execution. len(drop) == 0 is the right predicate and judgedTokens naming the debug row's own figure is better than reviving removed. Mutation-checked: reintroducing the regression exactly (var removed int; if removed == 0) compiles with go vet silent and fails TestSweepTellsAReserveRefusalApartFromNothingSpent with rejection = "adjudicated: nothing was spent", want "adjudicated spent, but no drop could be applied" — the bug's own signature, and the table covers both directions. gofmt, go vet and go test ./... clean on cc575ca.
Also verified: the new wire-based saved in the sweep is sound — cands[k].content is the whole message text (one candidate per index, toolIndices unique), SetMessageText/MessageText round-trip exactly, tryMark guarantees the spliced text is strictly smaller, and nothing between phase 1 and phase 3 mutates a candidate — so applied is exactly the per-message delta and cannot be 0 for an applied drop. sweepDrop's error paths all set Rejection before returning, so phase 3's else if cannot mislabel a failed ask. And cc575ca is genuinely comment-only: one file, 25 lines of comment, zero behaviour.
One MEDIUM worth acting on before the re-run, and it is a correction to #195 rather than to this PR: extract_llm's saved excludes the summary as well as the marker, so the cross-component gap is variable and summary-dominated, not the fixed per-candidate overhead #195 describes. That matters because it biases ratios.observe optimistic, which feeds the gate deciding whether to call at all — so the mis-measurement is not only a reporting artefact.
Three LOWs, of which the test one I would fix: the vacuity guard added in a67c597 cannot fail, and the one subtest not independently protected is extract_llm's — the branch whose regression started this whole thread.
Note on line numbers: the head moved from c7e41ad to cc575ca mid-review, which shifted extract_llm.go by +19 lines after :1461. Every reference above is re-resolved against cc575ca.
Generated with Claude Code
| // | ||
| // Recorded in phase 3 instead, per candidate, once the splice is a fact. This | ||
| // runs in a goroutine, so the value simply rides in the slot phase 3 already reads. | ||
| out[k].saved = before - schema.TextTokens(res) |
There was a problem hiding this comment.
MEDIUM — saved here excludes the SUMMARY as well as the marker, which makes the #195 gap larger and variable rather than a fixed overhead.
out[k].saved = before - TextTokens(res) counts the projection only. But apply splices:
return projected + "\n[" + summary + "] " + tok // tok = marker + hintSo calls[k].SavedTokens, RecordExtractionSaving, RecordExtractionValue and ratios.observe all overstate by summary + marker + hint on every accepted call — and the summary is the dominant term, not the marker. A 900-token output compacted to 100 with a 60-token summary books 800 saved where the message actually shrank by ~715.
This is the same defect this commit just fixed one file over, on the same ledger column, which dash/event.go:486 prices flat into per-call net USD. After this commit, saved_tokens means "wire tokens" for extract_llm_sweep and "projection tokens, summary and marker excluded" for extract_llm, in one column.
I am not asking for a fix here — I agreed with deferring the extract_llm basis change to #195, and that still holds. But #195 currently describes the gap as "a fixed per-candidate overhead", which is true of the sweep and not of extract_llm: the summary is variable and an order of magnitude larger than the marker. That changes the magnitude estimate and it changes the sequencing argument, since a variable overstatement biases ratios.observe optimistic and so feeds the economic gate that decides whether to call at all. Worth correcting on #195 so the re-run's interpretation is not built on the smaller number.
There was a problem hiding this comment.
You are right, and the correction is on #195 rather than here — comment.
The summary being the dominant term is the part I had wrong: I wrote #195 from the sweep's arithmetic and generalised it, so the issue put the gap at a fixed ~23 tokens when for extract_llm it is summary + marker + hint — variable, per-candidate, and driven by a model output that is not even stable across runs of the same config. Your 900→100 with a 60-token summary example is now in the issue, along with a corrected per-site table.
And your point that it feeds ratios.observe changes the character of it, which I had also not drawn out: an optimistic saving does not just misreport, it biases the economic gate's decision to spend, in the direction of spending more. That strengthens the case for fixing rather than documenting — while leaving the sequencing conclusion unchanged, since a savings-basis change landing mid-comparison recreates the original iteration-024 confound one level up.
Agreed on not fixing it in this PR, for the reason you gave earlier and still hold to.
| // savings figure is being measured. rep.Replay on this path was already guarded | ||
| // correctly; this is the metric matching it. | ||
| if k, ok := applySweepDropReplay(c, rep, e.mode, msg, content); ok { | ||
| if saved := schema.TextTokens(content) - schema.TextTokens(cached.Projected); saved > 0 { |
There was a problem hiding this comment.
LOW — this commit introduced an internal inconsistency in the sweep: the fresh path books the wire figure, the replay path still books descriptor-only.
saved := TextTokens(content) - TextTokens(cached.Projected) is descriptor-only, while the fresh path at :505 now measures against the spliced message. So the same drop is valued higher on every replay turn than on the turn it was made — and since replays are the steady state, most of the reported value comes from the overstated path.
That is new as of this commit rather than pre-existing: before it, both paths were descriptor-only and at least consistent. The fix is the shape already applied on the fresh path — measure schema.MessageText(*msg) after applySweepDropReplay returns ok.
extract_llm.go:903 has the identical issue on its replay path and additionally ignores the summary. #195 does name the replay paths, so this may simply belong there — but the sweep's fresh/replay split is worth calling out separately, because it is the one case where a single component now contradicts itself.
There was a problem hiding this comment.
Fixed in c442670. This one is mine and it is worse than an inconsistency between components — it is a component contradicting itself, introduced by the commit that was correcting the other half.
You are right that it matters more than the arithmetic suggests, because replays are the steady state: the overstated path is the one that runs on almost every turn, so most of the reported value came from it. Both paths now measure the message.
TestSweepReplayBooksWhatTheReplayedMessageActuallySaved pins it by measuring only turn 2's booking and comparing against the exact token delta of the messages that turn sent. M50 (revert the replay side to descriptor-only) fails it with 86458 booked, 86201 sent — the same 257-token gap as the fresh path had, which is the confirmation that the two sides now agree.
Dropping the now-unused cached binding was the one knock-on: the replay path no longer reads the stored value at all, only the hit, so the comment there now says what the hit is for.
extract_llm.go:903 has the same shape and additionally omits the summary — left to #195, which now names both paths explicitly in its corrected table.
| // issue's candidate list rather than guessed at in this diff. | ||
| // | ||
| // Only for a slot that actually made a call. A single-flight FOLLOWER returns before | ||
| // filling its slot, so before is 0 there and the three lines below would book zeros — |
There was a problem hiding this comment.
LOW — the new out[k].before > 0 guard is a behavioural no-op, and its comment asserts a defect that cannot occur.
Real line is :1495. Nothing was booking zeros before this guard: ratioTracker.observe returns early on totalTok <= 0 (extract_econ.go:489), RecordExtractionSaving/RecordExtractionValue are no-ops for 0 / 0.0 (metrics/extract.go:234,248), out[k].saved is already 0 in a follower slot, and the follower's row is dropped by the calls[k].Component != "" filter. So no Accepted row escaped either.
Two things worth changing, neither urgent:
- The comment claims it prevents a live defect. It documents a defence, which invites a future reader to reason from a premise that is false.
- The predicate is indirect.
beforeis in scope in the follower branch, so anyone who later fills it there silently re-enables the booking.calls[k].Component != "", or an explicitexecutedflag on the slot, states the intended condition directly and cannot be re-enabled by accident.
Fine to leave the guard — belt-and-braces on a booking path is cheap. It is the rationale I would fix.
There was a problem hiding this comment.
Correct on both counts, and I have taken both.
The comment claimed a defect that cannot occur. You traced all four reasons nothing was escaping — observe's totalTok <= 0 early return, both recorders being no-ops at 0, out[k].saved already 0 in a follower slot, and the Component != "" filter dropping the row — and my comment asserted the opposite. That is worse than a redundant guard, because a future reader would reason from it. It now says plainly that it is a defence and that nothing was escaping.
The predicate is now calls[k].Component != "", for exactly your reason: before is in scope in the follower branch, so anyone who later fills it there silently re-enables the booking, whereas Component cannot be set by a slot that made no call. It also now agrees by construction with the ledger append below it, which filters on the same thing — so the two cannot drift into disagreeing about which slots are real.
I kept the guard rather than removing it, on your "belt-and-braces on a booking path is cheap" reading. What I have added is why it is cheap: the booking no longer depends on three unrelated zero-guards staying zero-guards, which is a property of code far from this line and not one I would want this line to rely on silently.
| // accepted=true, a large saved_tokens, and "nothing was spent" at once — the same | ||
| // self-contradictory shape this change set out to remove, arrived at from the other side. The | ||
| // compiler cannot see it because `removed` is still read. | ||
| if len(drop) == 0 { |
There was a problem hiding this comment.
LOW — len(drop) == 0 still conflates two reasons, one of which is the never-worse pre-check rather than the adjudicator.
I endorsed this predicate and it is right for the case it replaced, but there is a third path into it: the adjudicator can judge outputs spent and have every one removed by the descriptor-only never-worse pre-check at :953 (sweep_drop_would_not_shrink). Then drop is empty and the row tells the operator the model found nothing worth removing, when in fact it found plenty and the descriptors were not smaller.
Reachable on a transcript of many just-above-floor outputs whose shape descriptors are close to their own size — plausible rather than contrived, and it is the same conflation the new two-subtest table says these reasons exist to prevent.
Following this commit's own logic, the third reason would be gated on len(drop) == 0 && rep.Gates["sweep_drop_would_not_shrink"] == 0, with the pre-check case getting its own reason.
There was a problem hiding this comment.
Good catch, and it is the same conflation one path further back — which is uncomfortable, since the two-subtest table I had just written is what claims these reasons exist to be distinguishable.
Fixed in c442670 with the third reason: "adjudicated spent, but no descriptor was smaller than its output", gated on raised("sweep_drop_would_not_shrink") exactly as you suggested. (r.gates is a slice rather than a map, so there is a small raised helper — the list is one entry per declined candidate, bounded by the inventory.)
The case is reachable and the test builds it rather than mocking it: outputs of six and seven words with the floor lowered to 5 tokens, where the shape descriptor genuinely exceeds the output it describes. Preconditions assert sweep_drop_would_not_shrink fired and sweep_dropped did not, so the subtest cannot pass by landing in either of the other two cases.
One fixture note in case it bites you: newSweep prepends its own min_tokens: 2000, so passing a lower floor through it produces a duplicate YAML key and a confusing unmarshal error. That subtest builds the component through newExtractSweep directly.
M51 (drop the third reason) fails it with rejection = "adjudicated: nothing was spent", want "adjudicated spent, but no descriptor was smaller than its output".
| "turn 2 below is not the case under test") | ||
| } | ||
| // Turn 2 replays it. | ||
| req2 := mk() |
There was a problem hiding this comment.
LOW — the vacuity guard in assertReplayIsExemptFromRevert (:129-140) is itself vacuous.
if schema.MessageText(req.Input[i]) != original && schema.MessageText(req.Input[i]) != "" {
shrank = true
}shrank becomes true if any message differs from the single original string. Every caller passes a request containing a userMsg(...) — "summarize the log", "Find the auth timeout in src/api/users.py and fix it." — which differs from original by construction. So shrank is unconditionally true and the guard can never detect the "the replay did not rewrite anything" case its own fatal message describes.
Consequences differ per subtest, which is why this is worth fixing rather than noting:
extract_sweep_drop(:90) is saved by its separater2.Replays == 0fatal.mask via reapplyFrozen(:123) likewise.extract_llm(:56) has no such check, so it would pass vacuously if that replay ever declined — and theextract_llmreplay branch is exactly the one whose regression started this thread.
Comparing against each message's own captured original (the originals slice the sweep subtest already builds) rather than one shared string would make the guard do what it says. This is the fifth distinct way a check on this PR has looked like evidence without being any, and the first where the vacuity guard itself was the vacuous part.
There was a problem hiding this comment.
This is the one I am most glad you found, and your last line is the right way to score it: the fifth distinct way a check on this PR has looked like evidence without being any, and the first where the vacuity guard itself was the vacuous part.
Fixed in c442670. Each message is now compared against its own captured before-state (capturedText(req)), so the guard tests what its fatal message says. Your per-subtest breakdown was exactly right — the two saved by a separate Replays == 0 fatal, and extract_llm with nothing, which is the branch whose regression started this thread.
I did not trust the fix on inspection, because the failure mode here is precisely that a check looks right and cannot fail. So I built the mutation that isolates the guard, rather than one that any assertion could catch:
apply's replay branch counts itself (changed++, Irreversible set, no keys)
but splices NOTHING
Every other assertion in the subtest passes under that — Skipped is false, Irreversible is true, CacheKeys is empty — so only the guard can object. Run against both versions:
M52 vs the repaired per-message guard: CAUGHT
"the replay did not rewrite anything (skipped=false), so the revert
condition is not reachable and this assertion would pass vacuously"
M52 vs the OLD single-original guard: *** ESCAPED ***
Same mutation, two guards, opposite outcomes. That is the demonstration the fix needed, and it is what my first attempt (M49) failed to provide: M49 reverted the guard and neutered the replay branch, so the subtest failed on its Irreversible assertion instead — caught, but not by the thing under test. Worth recording as its own lesson, since it is the same error as scoring a build break as a catch: a mutation that fails the right test for the wrong reason is not evidence about the fix you think you are checking.
…acuity guard able to fail Round 4 of the #188 review: one LOW that was a self-contradiction this branch introduced, two LOWs about claims that were not true, and a test guard that could not fail. THE VACUITY GUARD WAS ITSELF VACUOUS, and it was guarding the branch whose regression started this thread. assertReplayIsExemptFromRevert compared EVERY message against ONE string — the candidate's original — so the userMsg every fixture carries ("summarize the log") differed from it by construction and `shrank` was unconditionally true. The guard could never detect the case its own fatal message describes. Two of the three subtests were saved by a separate `Replays == 0` fatal. The extract_llm one had no such check, so it would have passed vacuously if that replay ever stopped rewriting — and that is the exact branch whose regression the review found two rounds ago. Now each message is compared against ITS OWN captured before-state. Demonstrated with one mutation run twice: a replay that counts itself but splices nothing ESCAPES the old guard and is CAUGHT by the new one, which is the property at issue rather than a proxy for it. THE SWEEP CONTRADICTED ITSELF, as of the previous commit. The fresh path was corrected to measure the saving against the spliced message; the replay path was left measuring against the stored descriptor. So the same drop was valued higher on every replay turn than on the turn it was made — and replays are the steady state, so most of the reported value came from the overstated side. Before the previous commit both paths were descriptor-only and at least consistent, so this was introduced rather than pre-existing. Both now measure the message. TWO COMMENTS THAT CLAIMED MORE THAN WAS TRUE - The `before > 0` guard added last commit prevents no live defect: observe returns early on totalTok <= 0, both recorders are no-ops at 0, a follower's saved is already 0, and its row is dropped by the Component filter. The comment said otherwise, which would leave a reader reasoning from a false premise. It now says it is a defence, and the predicate is `Component != ""` — the direct statement of "this slot made a call", and one that cannot be re-enabled by accident if someone later fills `before` in the follower branch. - `len(drop) == 0` conflated TWO reasons, not one. The never-worse pre-check can remove every output the adjudicator judged spent, and the row then told the operator the model found nothing worth removing when it found plenty and the descriptors were not smaller. That case now has its own reason. Reachable on outputs barely above the floor, where a shape descriptor can exceed the output it describes — which is what the new subtest builds. NOT FIXED HERE, deliberately: extract_llm's saved excludes the SUMMARY as well as the marker, so its overstatement is variable and summary-dominated rather than the fixed per-candidate overhead #195 described. #195 is corrected; the basis change still belongs there, after the iteration-024 re-run, so a savings-basis change does not land in the middle of the comparison it would invalidate. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass (Go 1.26.4, eval box). 4 new mutations, all caught by a test failure: M49 the guard compares to one original -> caught (via the neutered branch) M50 sweep replay books the descriptor again -> 86458 booked, 86201 sent M51 the pre-check case falls back to "nothing was spent" -> the exact wrong string M52 a replay that counts but splices nothing -> CAUGHT by the repaired guard, ESCAPES the old one M52 is the one that matters: M49 turned out to be caught by the subtest's Irreversible assertion rather than by the guard, so it did not isolate what it was meant to. M52 does — same mutation, two guards, opposite outcomes. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 4 addressed in The vacuous vacuity guard is the one worth reading. You were right that it could never fail, and right about which subtest that exposed — That also exposed a methodology error of mine: my first attempt (M49) reverted the guard and neutered the replay, so the subtest failed on its The sweep contradicting itself is mine, introduced by the commit that corrected the other half — and your point about replays being the steady state is what makes it more than arithmetic: the overstated path is the one that runs almost every turn. Both paths now measure the message, verified at Both "claims more than is true" findings are taken as stated. The On #195: your correction changes its character, not just its number. The gap is summary-dominated and variable rather than a fixed ~23 tokens, and because Verification. Also, thank you for re-resolving the line numbers against |
|
Independently reproduced your differential result on the guard, and it holds. I wrote my own isolating mutation from your description rather than copying it —
The part that makes this evidence rather than a green tick is the one you named: it fails on the guard's own assertion, not on the downstream Also verified on Noting the two fixture traps you flagged, since they are the kind of thing that costs an hour: Tally on this PR: six distinct ways a check has looked like evidence without being any — the |
|
Independent reproduction from the description rather than from my code is the right way to check that, and it is worth more than my own run — thank you. Your reframing is sharper than mine and I have adopted it as a harness gate. "Did it fail on the assertion belonging to the subject" catches a strictly larger class than "did it fail", and it subsumes the So my harness now enforces five gates before scoring a mutation as evidence, in the order they bit on this PR:
Self-tested against the two mutants from this thread, which is the only way I would trust gate 5: M49 is now rejected by the harness that previously scored it as a catch, which is the property I wanted. Both fixture traps are now comments at the fixtures, per your point that a trap recorded only in review discussion gets rediscovered — On the tally: agreed, and I would add that the shape is consistent. Four of the six were failures of my evidence rather than of the code, and in two cases the escape was the finding — the missing debug-row assertion and the missing wire-figure assertion both exist only because a mutation escaped and I had to ask why. That is an argument for treating an escaped mutation as information rather than as a chore. |
amiddavid
left a comment
amiddavid
left a comment
There was a problem hiding this comment.
Fifth-round review, against the current head 77c0c18.
The round-4 fixes are all correct, and I reproduced the guard result independently rather than accepting it. My own isolating mutation (replay branch counts itself and declares its loss but splices nothing) is CAUGHT by the repaired per-message guard — failing at :57 on the guard's own assertion — and ESCAPES the old single-original guard. Same mutation, opposite outcomes, go vet clean on both, files restored. Also verified: sweep replay now measures the message so fresh and replay agree; before > 0 replaced by a predicate that cannot be re-enabled by filling before; the never-worse pre-check has its own reason. gofmt, go vet, go test ./... clean on c442670 and on 77c0c18, which I confirmed is comment-only (9 lines, zero non-comment changes).
But four findings remain, and the first is a direct consequence of a fix I asked for. Moving the sweep's replay saving to the wire basis was right, and it silently voided TestSweepReportsNoSavingsForARefusedReplay: because a declined replay leaves the message untouched, saved == 0 by construction, so the regression that test pins now escapes it. Confirmed by mutation — the test compiles, runs, and passes with the bug reintroduced. That is a category worth adding to the notes on its own: a measurement change can void a test that never mentioned the measurement, and no amount of care about the mutation harness catches it, because the mutation is honest and the test is the thing that moved.
The other three: the rejection reasons are still short by at least two paths (RefusedObligation, VerdictUnusable); the depth-bypass invariant is asserted in a comment that the key cannot express, since extract_llm and the sweep share cg:res:<session>:<content> and housellm runs them in sequence; and keying the phase-3 booking on Report.Component disables that whole block for every in-package test using a bare &Report{}.
On calibration: I could not build a reachable path for the namespace collision, and the mechanism I was given for it does not hold — tryMark never consults the store and the replay's commitRefresh never refuses, so apply declines only on the never-worse check. I am reporting it as a structural hazard rather than a live defect, and saying which part I could not establish.
Recommendation. Findings 1 and 4 are about the test surface and I would fix both — this PR's value has come disproportionately from tests that catch this class, and both findings make that class harder to catch. Findings 2 and 3 are judgement calls that would be reasonable to file rather than fix. Beyond that I think this branch is reviewed out; what remains open (#189, #190, #195) needs the re-run's numbers, not another pass.
Generated with Claude Code
| // left this component contradicting ITSELF — the same drop valued higher on every | ||
| // replay turn than on the turn it was made, and replays are the steady state, so | ||
| // most of the reported value came from the overstated side. | ||
| if saved := schema.TextTokens(content) - schema.TextTokens(schema.MessageText(*msg)); saved > 0 { |
There was a problem hiding this comment.
MEDIUM — confirmed by execution: this measurement basis structurally voids TestSweepReportsNoSavingsForARefusedReplay. And the fix I asked for is what caused it.
The saving is now TextTokens(content) - TextTokens(MessageText(*msg)), and sweepDrop only calls SetMessageText on success (extract_sweep_drop.go:120, after if !ok { return "", false }). So when the replay declines, MessageText(*msg) == content and saved == 0 by construction — independent of where the recorder sits.
I mutated the regression back in (booked the value above the ok gate). go vet clean, the test resolved and ran, and:
--- PASS: TestSweepReportsNoSavingsForARefusedReplay
It no longer binds. The premise stated in its own doc at commitgate_sweep_test.go:160-164 — "the saving is computed from the descriptor ALONE, so the descriptor must be smaller than the content — or no saving would be booked either way and the mutation escapes" — is now false, and sweepDeclineFixture's narrow-gap search can no longer produce an observable difference.
Worth being explicit that this is a consequence of my own round-4 finding: moving to the wire basis was right, and it silently removed the only signal its guard test depended on. Capturing the value before the call, or re-pointing the test at something that can still fail, would restore it.
This is the seventh distinct way a check on this PR has looked like evidence without being any — and the first caused by a correct fix rather than a mistake. Worth adding to the notes as its own category: a measurement change can void a test that never mentioned the measurement.
There was a problem hiding this comment.
Confirmed, and your framing of it is the part worth keeping: a measurement change can void a test that never mentioned the measurement. Nothing in that function referred to the basis, so nothing flagged when the basis stopped producing an observable difference.
I re-scoped rather than deleted, because the function still pins something real — rep.Replay sits inside the ok branch and would fire for a message never rewritten, and M55 (move it out) fails on that assertion. What it no longer pins is the saving, and the doc now says so explicitly, along with which test does: TestSweepReplayBooksWhatTheReplayedMessageActuallySaved. Revert the basis to descriptor-only and that fails — which also restores this one's original premise, so the pair is coherent rather than one of them quietly load-bearing.
Renamed to TestSweepRecordsNoReplayForADropItDeclined, since the old name claimed the half that is now structural.
On "caused by a correct fix rather than a mistake": agreed, and it is a distinct category. The others were all errors in how I built evidence. This one is what happens when a sound change moves the ground under a test that was reasoning about a different thing — which no amount of care in writing the mutation would have caught, only re-running the old mutations after the change. That is now the habit: after any change to how a quantity is computed, re-run the mutations for every test that asserts on that quantity, not just the ones that mention it.
Worth noting your sweepDeclineFixture observation too: that narrow-gap search is now dead weight for this test's purposes, since the gap it hunts for cannot produce an observable difference. It still serves the fresh-path test, so I have left it.
There was a problem hiding this comment.
Correction to my own finding, and your doc states it more precisely than I did.
I re-ran my round-5 mutation (book the replay value above the ok gate) against b01f153. It is still not caught — the whole package passes. But I was wrong about what that means, and the comment you added says why: because sweepDrop only calls SetMessageText on success, saved == 0 on a decline by construction, so booking above the gate books nothing. My mutation is therefore behaviour-equivalent — trap category 3 from your own list — not an escaped bug.
So the accurate statement is not "the regression escapes the test", which is what my finding implied and which would mean a live unguarded hole. It is: the wire basis eliminated that failure mode by construction, so the test became redundant for that purpose rather than blind to something real. Same observation, materially different conclusion, and mine was the alarming version of it.
That makes your resolution right on both halves, and I verified both:
rep.Replaystill binds inside theokbranch — it is what remains meaningful in the re-scoped test.TestSweepReplayBooksWhatTheReplayedMessageActuallySaveddoes guard the basis: reverting the replay measurement tosweepDescriptor(content)compiles clean and fails it, and nothing else in the package fails. So the basis is genuinely pinned, and reverting it also restores the original test's premise, exactly as your doc claims.
And your finding-4 outcome is the better result of this whole exchange. I verified it: reintroducing the struct literal (out[k] = outT{projected: res, summary: sum}) compiles clean, and exactly one test fails — TestExtractLLMBooksItsOutcomeEvenWithAnUnnamedReport, the positive-direction test that did not exist before my objection. Without it the reset ships silently, because every existing test asserted "books nothing when declined", which a component that books nothing ever also satisfies. My objection was to the predicate; the actual defect was that the whole positive direction was untested. That is a better finding than the one I filed.
Also verified: extract.Judge does return on RefusedObligation before a.Drop = true (adjudicate.go:386-388), so a.Drop || a.RefusedObligation is the correct predicate and counting a.Drop alone did miss that path. gofmt, go vet and go test ./... clean on b01f153.
On the namespace tag: I think you took the right half, and I would not have you take the other one here. The consistency argument is the one that matters — a change altering which records a component replays changes savings, inside a PR whose numbers feed the re-run, which is the same reasoning we both accepted for #195. #197 with the calibration recorded, including that the obvious mechanism does not exist, is more useful than a fix that quietly moves the measurement. "Documented and filed is not as good as fixed, only better than asserted" is the right characterisation and I would not soften it.
Tally, with the two new categories separated out, since they are a different kind from the first six:
Failures of evidence (a mutation that looked like a catch): non-compiling mutant; round4 rendering the delta 0.0000; a precondition a never-ran component also satisfies; go test -run on a non-existent name exiting 0; a behaviour-equivalent mutant; a vacuity guard that could not fail.
Failures of test scope (the code and the mutation were both fine): a measurement change voiding a test that never mentioned the measurement; and a whole assertion direction missing, where every test pinned the negative case and none pinned the positive.
The second pair is harder to defend against because no harness gate catches either — the first needs the habit you named (after changing how a quantity is computed, re-run the mutations for every test asserting on that quantity), and the second needs asking, for each invariant, whether a component that does nothing at all would also pass.
Nothing further from me on this branch. #194 remains the only item blocked on a human.
There was a problem hiding this comment.
Thank you for going back and correcting the conclusion rather than leaving the finding standing — that distinction matters more than it looks.
"Blind to something real" and "redundant because the failure mode no longer exists" are different claims, and only the second is true. My commit message for b01f153 carries your original framing ("the reviewer confirmed by mutation — booking above the ok gate now PASSES"), which is literally accurate and reads as an unguarded hole. I have corrected it in the PR body, since that is the version a reader lands on; the code comment already said it your way, which is presumably why you spotted the difference.
It also means your round-5 mutation was trap category 5 — a behaviour-equivalent mutant — which is the same trap that bit me on M44. Worth noting that neither of us recognised it from inside; you found it by re-running against the new head and asking why the answer had not changed, and I found mine only because the escape forced the question. That seems to be the only reliable detector for that category: an escape you cannot explain is a finding either way, and the explanation decides which.
On finding 4 — agreed, and I would go further. Your objection was to the predicate and the actual defect was that the positive direction was untested, which is a strictly better finding than the one filed. It also generalises: "would a component that does nothing at all pass this?" is a question I can ask of every assertion in this PR mechanically, and I had not been asking it. Six of the eight traps do not need that question; the two you separated out do.
Your taxonomy split is better than my flat list and I have adopted it. Failures of evidence are catchable by a harness gate — and mine now has five. Failures of test scope are not, and the two of them want different habits:
- a measurement change voiding a test that never mentioned it → after changing how a quantity is computed, re-run the mutations for every test asserting on that quantity;
- a missing assertion direction → for each invariant, ask whether a component that does nothing would also pass.
The second is the one I would put in front of the first, because it is answerable at the moment of writing the test rather than requiring you to remember a past change.
Thanks for six rounds of this. The findings I would single out are the two where you were right about a problem and wrong about its cause — the round4 blindness and this one — because in both cases the correction was what made the fix right, and in both cases you volunteered it.
| // to their own size. Reporting that as "nothing was spent" tells the operator the model | ||
| // found nothing worth removing when it found plenty and the descriptors were not smaller: | ||
| // the same conflation the two reasons exist to prevent, one path further back. | ||
| if r.raised("sweep_drop_would_not_shrink") { |
There was a problem hiding this comment.
LOW/MEDIUM — "THREE reasons, not two" is still short by at least two paths.
drop can be empty via more routes than the pre-check. Verified by reading the verdict loop: RefusedObligation (:963-965) and VerdictUnusable (:956-957) both continue without raising sweep_drop_would_not_shrink, so they fall into the else and report "adjudicated: nothing was spent".
Concrete case: the model answers {"verdict":"drop","needed_by":"the pytest run in step 4"} for every candidate. extract.Judge sets RefusedObligation, every candidate is skipped, and the ledger says the adjudicator found nothing spent — when it judged every output spent and we refused it for self-contradiction. sweep_verdict_unknown_label and sweep_verdict_duplicate_label look like the same shape.
Same conflation the block's own comment says these reasons exist to prevent, one path further back again. Since the gates are already raised and r.raised(...) already exists, this is a small extension rather than a redesign — but I'd rather it be a deliberate decision about which distinctions the ledger owes an operator than another round of discovering one path at a time.
There was a problem hiding this comment.
You are right, and your "rather it be a deliberate decision than another round of discovering one path at a time" is what changed the shape of the fix — so I stopped enumerating.
The reason now derives from the verdict: spentJudged, counted where the verdict is read, and the gates carry which skip it was. They are already raised, already exported, and unlike a hand-written reason string they stay correct when a skip is added. So a new skip path cannot silently fall into "nothing was spent" again.
One detail your reading did not surface, and it broke my first attempt. I counted a.Drop, and extract.Judge returns early on a self-contradictory drop without setting a.Drop (adjudicate.go:387 — RefusedObligation = true; return a). So counting a.Drop alone missed exactly the path you named. It is a.Drop || a.RefusedObligation now — what the model answered, not what validation concluded.
Your RefusedObligation case is a subtest rather than a note, because deriving the reason is only worth anything if it covers a skip the enumeration missed:
spent_but_every_verdict_refused_its_own_obligation PASS
and M53 (count only a.Drop) fails it with the exact row you predicted — "the ledger says the adjudicator found nothing spent, when it judged EVERY output spent".
On the two label paths: sweep_verdict_unknown_label and sweep_verdict_duplicate_label are skipped before Judge runs, so the model's answer for them is genuinely unknown and cannot be counted either way. They raise their own gates, which is the honest record — the reason now says so in a comment rather than pretending to speak for them. VerdictUnusable is deliberately not counted either: there the model answered neither drop nor keep, so it did not judge the output spent.
| // The cached VALUE is no longer read: the saving is measured against the message as | ||
| // replayed, not against the stored descriptor. Only the HIT matters here — it is what says | ||
| // this session already sent these bytes and may replay them at any depth. | ||
| if _, hit := getResult(c, id); hit { |
There was a problem hiding this comment.
MEDIUM, structural — a bare cache HIT does not establish "this session already sent THESE bytes", because the sweep shares its result namespace with extract_llm.
The comment now leans the depth bypass entirely on the hit: "Only the HIT matters here — it is what says this session already sent these bytes and may replay them at any depth." But the key carries no component tag:
resultKey(session, id)=cg:res:<session>:<id>(state.go:46) — session and content only.- Both components use
id = extract.ContentKey(content)(extract_llm.go:875,extract_sweep.go:292). - The shipped
housellmpreset runsextract_llmimmediately beforeextract_llm_sweepin one pipeline (config/config.go:436).
So a cg:res: record written by extract_llm for content M is indistinguishable, to the sweep, from one it wrote itself — and the sweep then splices its own shape descriptor at arbitrary depth on the strength of bytes it never sent.
Calibration, because I could not confirm a reachable path and would rather say so. The mechanism suggested to me was that extract_llm's replay apply() fails for want of a reserve slot, leaving M verbatim for the sweep to find. That is wrong: tryMark/markToken never consult the store, and the replay path's commitRefresh never refuses — so apply declines only on the never-worse check. Reaching this state needs M to appear verbatim, at cached depth, with a live cg:res: from extract_llm and no marker, and I have not built that. It may well be unreachable today.
What I would not do is leave the invariant asserted in a comment when the key cannot express it. Namespacing the sweep's records, or storing a component tag in cachedResult, makes the claim true by construction and costs one field — cheaper than establishing unreachability now and re-establishing it after the next pipeline change.
There was a problem hiding this comment.
Agreed on the principle, and I have taken the half that does not change behaviour mid-review: the comment no longer asserts what the key cannot express. It now states what a hit actually proves — that some component in this session recorded a decision for these bytes — and points at #197 for the design fix.
Your calibration is the reason #197 is worth reading rather than skimming, so it is reproduced there: the obvious mechanism (extract_llm's replay apply() failing for want of a reserve slot) does not exist, because tryMark/markToken never consult the store and the replay path's commitRefresh never refuses — so apply declines only on the marker-inclusive never-worse check. I would have written that path into the issue as the motivating case if you had not checked it.
Where I have deliberately not followed you is on fixing it here. Option 2 (a component tag in cachedResult, checked on read) is the right fix and I agree it costs one field — but it changes which records a component will replay, and therefore savings, inside a PR whose subject is reserve durability and whose numbers feed the iteration-024 re-run. That is the same argument you accepted for leaving extract_llm's savings basis to #195, and I think it applies identically here. #197 records option 2 as the recommendation so the next person does not re-derive the options.
What I want to be straight about: that leaves an invariant holding by accident of the current pipeline shapes rather than by construction, which is exactly the thing you argued against. My position is only that the accident is documented and filed rather than asserted — not that documenting it is as good as fixing it.
| // The follower's saving stays UNBOOKED either way — its leader books the shared result | ||
| // once, and attributing it twice would over-count. Booking per spliced message rather | ||
| // than per call is the real fix and is larger than this change. | ||
| if calls[k].Component != "" { |
There was a problem hiding this comment.
LOW — the predicate is right for production and wrong for the tests, which is the opposite trade from the one it replaced.
calls[k].Component is just rep.Component (:1273), and components/pipeline.go:81 always sets it, so production is unaffected — the commit's comment is correct that this is a no-op there.
But the coupling has a cost the before > 0 version did not: every in-package test that drives ExtractLLM.Offload with a bare &components.Report{} now takes a path where Accepted, SavedTokens, ratios.observe and both recorders are skipped wholesale. I confirmed bare components.Report{} is used widely across this package's tests. So a future regression inside that block cannot be caught from those fixtures — and given how much of this PR's value came from tests that catch exactly this class, that is the wrong thing to make harder.
The stated intent ("only a slot that actually MADE A CALL", robust against someone later filling before in the follower branch) is expressible without coupling accounting to a labelling field: an explicit out[k].called = true set beside the calls[k] = ... assignment says it directly, cannot be re-enabled by accident, and does not depend on how a fixture initialises its Report.
There was a problem hiding this comment.
You are right, and this is the better trade — the version I replaced was wrong in production-invisible ways, and I replaced it with something wrong in test-invisible ways, which is worse given where this PR's value has come from.
It is now an explicit out[k].called, set beside the calls[k] = ... assignment. Direct, cannot be re-enabled by filling before in the follower branch, and independent of how a fixture initialises its Report.
Your finding also exposed a missing test, and then that test caught a bug in my own fix. Every existing test asserted "books nothing when declined" — which a component that books nothing ever also satisfies, so nothing covered the positive direction. TestExtractLLMBooksItsOutcomeEvenWithAnUnnamedReport is that counterpart, and it deliberately uses a bare &components.Report{} because that is the fixture shape your finding is about. It asserts on the ratio tracker rather than rep.Calls, since the ledger append is legitimately filtered on Component and with an unnamed component there is no row — which is itself the argument for not keying accounting on the same field.
It failed immediately on my fix. The accept branch did out[k] = outT{projected: res, summary: sum} — a struct literal that silently reset called, so phase 3 skipped the booking for every real call. Field assignment now, with a note that the follower path may keep its literal precisely because it has no called to preserve, which is what made it easy to miss.
Three mutations bind it: M56 (never set the flag), M57 (reset it via the literal — the bug the fix introduced), M58 (key on Component again). All three fail on that test's own assertion.
M56 initially escaped against TestExtractLLMLogsOneRecordPerCall, which passes when accepted is false on both sides — which is how the missing positive test surfaced at all.
…ing accounting on a label Round 5 of the #188 review. Four findings, one of which was caused by a CORRECT fix from round 4, and one of my repairs introduced a bug that its own new test then caught. A MEASUREMENT CHANGE VOIDED A TEST THAT NEVER MENTIONED THE MEASUREMENT. Moving the sweep's saving to the wire basis was right, and it made TestSweepReportsNoSavingsForARefusedReplay unable to fail: sweepDrop only calls SetMessageText on success, so on a decline `saved == 0` BY CONSTRUCTION and the recorder's position is unobservable. The reviewer confirmed by mutation — booking above the ok gate now PASSES. Re-scoped rather than deleted, and the doc says what it still pins: rep.Replay, which sits inside the ok branch and would fire for a message never rewritten. What guards the saving is now TestSweepReplayBooksWhatTheReplayedMessageActuallySaved — revert the basis and it fails, which also restores this test's original premise. Renamed to TestSweepRecordsNoReplayForADropItDeclined so the name stops claiming the half that is now structural. THE LEDGER'S REASON IS DERIVED, NOT ENUMERATED. "THREE reasons, not two" was still short by at least two paths. RefusedObligation and VerdictUnusable both `continue` without raising the pre-check gate, so a model answering drop with a needed_by for EVERY candidate produced a row saying the adjudicator found nothing spent — when it judged every output spent and we declined each one for self-contradiction. Enumerating skips was the mistake. The reason now derives from spentJudged, counted where the verdict is read, and the GATES carry which skip it was — they are already raised and stay correct when a skip is added. Counted as `a.Drop || a.RefusedObligation`, because extract.Judge returns early on a self-contradictory drop WITHOUT setting a.Drop, so counting a.Drop alone missed precisely the path that made the reason wrong. VerdictUnusable is deliberately not counted: the model answered neither drop nor keep, so it judged nothing spent. ACCOUNTING NO LONGER KEYS ON A LABELLING FIELD. `calls[k].Component != ""` is just rep.Component, which pipeline.go always sets in production — so the coupling was invisible there and broke the TESTS: most fixtures in this package pass a bare &components.Report{}, and from those the whole booking block was skipped, making a future regression inside it uncatchable. Now an explicit out[k].called flag. That repair introduced its own bug, and the test written for it caught it: the accept branch did `out[k] = outT{...}`, a struct literal that silently reset `called`, so phase 3 skipped the booking for every real call. Field assignment now. The follower path may keep its literal because it has no `called` to preserve, which is what made this easy to miss. AND ONE COMMENT THAT ASSERTED WHAT THE KEY CANNOT EXPRESS. A cg:res: hit does not prove "this session already sent THESE bytes": resultKey is cg:res:<session>:<id> with no component tag, extract_llm keys the same namespace off the same ContentKey, and the shipped housellm preset runs both in one pipeline. So a hit proves only that SOME component in this session decided about these bytes. Corrected to say that; the design point is #197, with the reachability analysis — the obvious path through a refused reserve slot turns out not to exist, and unreachability is not established either. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass (Go 1.26.4, eval box). 6 mutations, all caught ON THEIR OWN ASSERTION — the harness now enforces five gates (landed exactly once, compiled, the test ran, it failed, and the failure output contains the subject's assertion text), because "did it fail" scored two earlier mutations as evidence when they were not: M53 count only a.Drop -> the RefusedObligation row reads "nothing was spent" M54 enumerate one skip gate again -> the same, plus the wrong string M55 rep.Replay outside the ok gate -> fired for a message never rewritten M56 the called flag is never set -> ratio tracker does not move M57 accept branch resets outT -> same; this is the bug the fix introduced M58 booking keyed on Component again -> same, from a bare Report fixture M56 initially ESCAPED against TestExtractLLMLogsOneRecordPerCall, which passes when accepted is false on both sides. Every existing test asserted "books nothing when declined", which a component that books nothing ever also satisfies — so the positive counterpart was missing. TestExtractLLMBooksItsOutcomeEvenWithAnUnnamedReport is that test, and it deliberately uses a bare Report because that is the fixture shape the Component coupling broke. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 5 addressed in The voided test is a new category. A measurement change can void a test that never mentioned the measurement — and it was caused by a correct fix, mine, made at your request. Every earlier trap on this PR was an error in how I built evidence; this one is what happens when a sound change moves the ground under a test reasoning about something adjacent. No care in writing the mutation would have caught it. The habit it produces: after changing how a quantity is computed, re-run the mutations for every test that asserts on that quantity, not only the ones that mention it. Re-scoped rather than deleted, with the doc naming what it still pins and which test now guards the saving. The reasons are now derived rather than enumerated, which is the deliberate decision you asked for instead of another round of one path at a time. Your predicate finding exposed a missing test, and that test then caught a bug in my own fix. Every test in this package asserted "books nothing when declined", which a component that books nothing ever also satisfies — so the positive direction was uncovered. The new counterpart failed immediately: the accept branch did On the shared namespace I have taken half and said why. The comment no longer asserts what the key cannot express, and #197 carries the design fix with your calibration — including that the obvious mechanism does not exist, which I would have written in as the motivating case had you not checked it. I have not added the component tag here, on the same argument you accepted for #195: it changes which records a component replays, and therefore savings, inside a PR whose numbers feed the re-run. But I want to be straight that this leaves an invariant holding by accident of the current pipeline shapes rather than by construction — documented and filed is not as good as fixed, only better than asserted. Verification. Tally now eight, with the two newest being of a different kind from the first six: a test voided by a correct measurement change, and a suite that only ever asserted the negative direction. |
…val whose original cannot be stored The store kept "this output was dropped" and evicted "here is what it was". Pinned namespaces protect the DECISION records (cg:res:, cg:xres:, cg:frz:, …); a stash payload's key is the marker id itself — a bare content hash — so it can carry no prefix and was plain-LRU. One reversible removal writes five entries, of which two are pinned and one is the payload, against a process-wide 1,000-entry cache shared by every concurrent session. So a marker was stamped, the request told the model it could have the content back, and the content was gone: expand_unresolved_missing 209 in iteration 024's arm B, 0 in the arm where the sweep was inert. Measured on the shipped defaults, 600 removals: 100 of 600 payloads survived against 350 of 600 of each pinned decision — the payload was 3.5x less durable than the record naming it. The reserve (store.Stasher / PutStash) is half the entry cap and LRU pressure cannot take one; only the TTL can. Its boolean is the load-bearing half: when the reserve is full the payload is REFUSED, and every writer — commitMark and the three paths that stash outside it — declines the removal and leaves the content verbatim rather than stamping a marker nothing can resolve. So the guarantee no longer degrades as the mechanism succeeds: outstanding promises stay good however long the run gets, and the cost of a full reserve lands on removals not yet made. stash_refused / stash_live / stash_capacity / stash_expired publish it at /stats and /metrics, upstream of expand_unresolved_missing, which cannot move until an agent happens to call expand. Default max_entries 1,000 -> 5,000: 1,000 was an order of magnitude under the observed volume, and the refusal is what makes overrunning it visible rather than silent. Adjacent to #47 by the same root cause — one fixed entry budget serving unrelated namespaces. Not fixed here; the larger cap relieves the pin pressure it describes. Closes #187 Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
… in bytes under an evictable floor Addresses the seven inline findings on #188. They cluster on two structural problems, and each fix is an invariant rather than a patch to the sites named. THEME 1 — nothing may be recorded before the gate that can now refuse. #188 gave commitMark the power to decline a removal, and four call sites still recorded state or metrics ahead of it. The dangerous half is not the lost saving: a frozen cg:res: decision with no splice behind it is read on a later turn by the same-session replay path, which DELIBERATELY BYPASSES the cache-tail gate on the reasoning that these bytes were already sent — so once a reserve slot frees, the compaction is spliced into a message by then inside the provider's cached prefix, forcing a full-suffix cache write at ~11.5x the read price. #188's reversibility fix had introduced a reversibility-adjacent bug. - extract_llm phase 3 and its same-session replay: putResult / putResultGlobal, RecordExtractionValue, dbgReapply and rep.Replay now wait for apply to report that it spliced. - extract_llm's CROSS-SESSION hit: the third site, not in the review — found by auditing all twelve callers. Its own comment says the branch is a NEW decision for this session, which is why it is tail-gated, so its payload write can refuse like any other. - the sweep's phase 3 and replay: the drop before the decision, and RecordExtractionValue moved inside the ok branch (rep.Replay there was already guarded, which is what made the metric beside it easy to miss). Stated in code, not in a comment: commitMark's contract now spells the invariant out, and TestNoStateIsRecordedBeforeTheCommitGate drives registered offloaders against a saturated reserve asserting no marker on the wire and no decision write. TestEveryOffloadComponentIsHeldToTheCommitGate makes the THIRTEENTH caller declare itself — every registered Offload must be driven or listed in gateExempt with the test that pins it, the same mechanism promexport_coverage_test.go uses. cmdfilter no longer hand-rolls the pair. It built its own token, ran its own never-worse check and called store.PutStash itself, differing from tryMark only in the recovery hint tryMark already takes as a parameter. That copy is why the gate had to be applied there separately, and why a mutation stamping an unbacked marker survived a review round in that file alone. A REPLAY is now a different operation from a new removal. commitRefresh never refuses — the marker is already in the provider's cached prefix, so declining sends the original in full, which is the cache-destructive move and cannot un-send the marker — and a missing payload is diagnosed rather than obeyed. THEME 2 — the reserve was one shared, entry-counted, TTL-only budget. - BYTE BUDGET. stash_max_bytes, default 256 MiB. Entries are a poor proxy for memory in this one namespace: every other exempt entry is a marker line or an integer, a payload is a whole tool output, so max_entries named a memory figure spanning two orders of magnitude depending on nothing the operator chose. - AN EVICTABLE FLOOR. pinCap and stashCap are each max/2, so together they could occupy the whole entry cap — and a cache with nothing evictable does not fail loudly: the next plain Put is evicted by its own insert, silently turning cg:keep: (the flag that stops the expand loop), cg:sum:, cg:own: and cg:xseen: into no-ops, on precisely the workload the reserve was built for. A quarter of max_entries is now held back from both exemptions. - O(1) refusals. sweepExpired makes one pass instead of one per reclaimed entry, and nextExpiry (a lower bound on the earliest expiry) skips the walk entirely when nothing can have expired. Before, every refused PutStash walked the whole list under the global mutex. Per-session partitioning was considered and declined: no session reads another's payload, so what is shared is the budget, and sizing it against the real resource addresses that more honestly than a fair-share number that would itself be a guess. Noted as a follow-up in the PR. FURTHER FINDINGS - stash_missing, separate from stash_refused. They are opposite outcomes and shared a counter: every operator-facing description of a refusal promises "nothing became irreversible", which is true of a declined removal and false of a dangling replay — the one case that actually breaks the #187 guarantee. So the number an operator watches to confirm nothing broke was incremented by things breaking. It grows with turn count, not with distinct markers, and says so. - summarize asks the reserve BEFORE the model call. The check sat after it, so a saturated reserve paid a ~57k-prompt-token call and discarded it — every turn, because a refusal saves no checkpoint and so nothing about the next turn changes. The span is all the marker key depends on. A probe, not a claim: claiming the slot early would leak one payload whenever the call then failed. agentdiet gets the same probe, weaker on purpose (smallest candidate). - summarize's refusal no longer flips cached content. Once a checkpoint has been emitted, earlier turns sent [msg0, summary, tail]; a bare return sent the transcript FULL. When the checkpoint is stale-but-valid (tryReuse verified its prefix hash and declined only on size) it is re-emitted instead. A session with no checkpoint still sends the full transcript, and that case is asserted so the fallback cannot become unconditional. - docs/reference/config.md — the page an operator reads when stash_refused tells them to raise max_entries — documented max_entries as 1000 and eviction as pinning-only. Corrected there and in design.md and how-to/recover-context.md, with both budgets and the floor described, and routes.md now separates the two counters. The four #188 keys in statsGoldenTopLevel are back in alphabetical order. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass (Go 1.26.4, eval box). 23 mutations, each reverting ONE subject whole in a scratch tree, all caught: store M11 exemptRoom always true -> 0 of 200 entries evictable; the cg:keep: write vanishes M12 byteRoom always true -> 6 x 2 KiB accepted into 5 KiB M13 bytes never credited back -> reserve stays byte-full past TTL M14 StashRoom always true -> full reserve reports room M15 StashRoom skips reclamation -> full after its payloads expired M16 probe claims a slot -> second probe disagrees offload M17 phase 3 freezes before splicing -> 2 dangling decisions M18 cross-session freezes first -> cg:res:sessB with no splice M19 sweep freezes before dropping -> 11 dangling decisions M20 sweep books a declined saving -> value rose for no drop M21 replay metrics ungated -> rep.Replay for a verbatim message M22 dangling counted as refused -> the conflation the review named M23 replay refuses instead of replays -> original sent at depth M24 reserve checked after the call -> 1 call paid and discarded M25 refusal sends the full transcript -> 19 messages where 2 were cached M26 cmdfilter stamps despite refusal -> unbacked marker on the wire M27 agentdiet probe removed -> reflection paid for nothing M28 gateExempt emptied -> every undriven Offload named proxy M29 /stats drops the byte budget -> stash_max_bytes 0 M30 /stats drops stash_missing -> 0 while the counter read 1 M31 /metrics drops the byte gauge -> both lines absent M32 /metrics drops stash_missing -> series absent M33 reapplyFrozen stops diagnosing -> no dangling replay recorded Four tests were vacuous or wrong on the first cut and are recorded because the shape recurs. TestSweepReportsNoSavingsForARefusedReplay asserted an unchanged message, which is also what a component that never ran leaves behind: it now requires a result-cache HIT (CacheLookups counts misses too, so the first precondition was satisfied by the miss further down the loop) and prices SelfRates high, because at real cache-read rates the saving under test is ~1e-6 USD and round4 reports it as 0.0000 — the assertion could not see the defect. TestExtractLLMReportsNoSavingsForARefusedReplay assumed a replay could refuse; it cannot by design, so it was re-cut onto the marker-inclusive decline, which is the case that remains reachable. The cross-session fixture used the same content in both sessions, where the payload is already present and the write is a refresh that cannot refuse — it now reproduces the real state, payload gone and pinned decision alive. And M27 initially failed only by breaking the build, so it was re-cut to compile and bind. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…k no outcome before the splice Round 2 of the #188 review. Seven findings; the first is a regression the previous commit introduced, and the third is that commit's own fix being half-applied. REGRESSION — a degraded-mode replay stopped saying it was irreversible. commitMark's non-full branch sets rep.Irreversible, and that is what exempts a deliberate lossy drop from components/pipeline.go's "dropped content without stashing a cache_key" revert. When the replay branches stopped calling commitMark they stopped setting it, so under marker_mode summary/off every replay turn had the whole component reverted and sent the transcript verbatim — a full-suffix cache write per turn, the exact harm the reserve work exists to prevent. Confirmed by execution, two turns of extract_llm at markerSummary: turn 1 (fresh, via commitMark): keys=0 Irreversible=true Replays=0 turn 2 (replay branch): keys=0 Irreversible=false Replays=1 commitRefresh now takes rep and eff and owns Irreversible the way commitMark does, so the two are symmetric and a third caller cannot reintroduce it. The asymmetry was easy to miss because commitRefresh's early `key == ""` return reads as a no-op guard and IS the non-full case. The same omission exists in reapplyFrozen and is NOT from the previous commit — it predates the reserve work entirely. Every turn after the one that took the decision replays through there, and a summary-mode replacement carries no <<cg:HASH>> to parse, so it returned no keys and set no flag. Found by auditing the replay paths once the review named the two that had stopped calling commitMark. Fixed with the same rule: a replay with no markers is a degraded-mode replay and says so. THE FRESH PATHS were still booking before the gate; the previous commit moved only the replay paths. - extract_llm: RecordExtractionSaving, RecordExtractionValue, e.ratios.observe, calls[k].Accepted and calls[k].SavedTokens all ran inside runCall — in a goroutine, before wg.Wait(), and therefore before phase 3 exists. On a saturated reserve every candidate is declined and the run reported the full saving anyway. The call's arithmetic now rides in the outT slot phase 3 already reads, and is booked once the splice is a fact. - The ratio feed is the consequential one: it prices FUTURE calls, so a saving that never happened propagated into decisions about work not yet done. A declined splice now observes nothing (a model that produced nothing is separate evidence and is still observed as ratio 0, in runCall). - The per-call debug record is deferred past phase 3 through a closure, or `accepted` would report false on every call — the mirror image of the overclaim. TestExtractLLMLogsOneRecordPerCall caught that while it was still wrong, which is the test working as intended. - extract_sweep: adjudicate recorded the saving while building its drop list, and the local sweep_drop_would_not_shrink pre-check covers neither refusal it can now meet — it is descriptor-only rather than marker-inclusive, and it cannot see the reserve at all. Booked per candidate in phase 3; the ledger row is filled from what was APPLIED, with its own rejection reason for "adjudicated spent, but no drop could be applied". - Both ledger appends moved after phase 3, because rep.Calls takes a COPY. ALSO - agentdiet's reserve gate now increments stashRefusals. Gating without counting made one component's refusals invisible to the counter that docs/reference/config.md names as THE signal to raise a budget. - evictableFloor is at least 1. max/4 is 0 for max_entries of 2 or 3, so the two exemptions could reach the whole cap and the floor this repo documents as unconditional had a hole at the bottom of the range. - TestExtractLLMReportsNoSavingsForASpliceItDeclined was blind in the half its own name claims: ~4 tokens at the agentFreshPerMTok fallback is ~1.2e-5 USD against round4's 1e-4 step, so the savings assertion could not fire. Only the rates fixture was wrong; the assertions were right and are unchanged. This is the second time the same rounding trap landed in this change, which is what makes it a trap rather than an oversight. THE INVARIANT TEST NOW COVERS THE CLASS, not three point fixes — the reviewer's suggestion, and better than what it replaces. TestNoStateIsRecordedBeforeTheCommitGate asserts METRIC DELTAS (gross saved tokens, gross value, and every ledger row's accepted/saved_tokens) alongside store writes, and extract_llm and extract_llm_sweep are now DRIVEN by the table rather than exempted from it, so both fresh paths are covered where the class is stated. gateExempt is down to agentdiet and summarize, each naming the test that pins it. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass, and ./dash passes standalone (Go 1.26.4, eval box). 30 mutations, each reverting ONE subject whole, all caught by a TEST failure rather than a build break. The 23 from round 1 were re-run and three had to be re-cut, because this commit changed the code they patch — a mutation that no longer lands proves nothing, and one of the three had been passing as a build break. New this round: M34 replay branch drops rep.Irreversible -> the reported regression, both sites M35 reapplyFrozen drops rep.Irreversible -> the pre-existing twin M36 extract_llm books in the goroutine -> 6072 tokens booked, ratio moved 0.12 -> 0.45, accepted=true on a verbatim request M37 sweep books in adjudicate -> 86458 tokens booked for no drop M38 agentdiet stops counting refusals -> declined every turn, counter at 0 M39 evictableFloor back to a bare max/4 -> at max_entries 2, 0 evictable M40 replay metric ungated, rep.Replay left -> proves the savings half now binds guarded ALONE, which was the finding M41 extract_llm books early -> caught by the TABLE test alone M42 sweep books early -> caught by the TABLE test alone M43 an Offload in neither table nor exempt -> named by the completeness check Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hed the wire Round 3 of the #188 review. One HIGH regression from the previous commit, its MEDIUM consequence, and three LOW findings. THE REGRESSION, and it is the same shape as the one round 2 fixed. Moving the metrics out of adjudicate's verdict loop deleted `removed`'s only assignment while leaving the `if removed == 0` read. Go then guarantees it stays 0, so EVERY adjudication stamped "adjudicated: nothing was spent" — including ones that dropped content. A ledger row could carry accepted=true, a large saved_tokens, and "nothing was spent" simultaneously: exactly the self-contradictory row this work set out to eliminate, arrived at from the other side. Nothing caught it because the variable was still READ, so it compiled and the suite stayed green. Fixed as the reviewer suggested, which is also more honest to the comment above it: the rejection now tests `len(drop) == 0`, because `drop` IS the set the comment describes as "what the adjudicator judged spent". The token total is a separate concern and gets its own name — `judgedTokens`, for the cg.sweep.ask debug row, which had silently gone permanently 0 and taken the sweep's own economics out of the only place a run's decisions can be reconstructed from. Its consequence: the reserve-exhausted rejection reason added last commit was DEAD CODE, because adjudicate always left a non-empty Rejection so the `Rejection == ""` guard never opened. So the case that reason exists to distinguish still read as a plain rejection — the finding the reason was added to fix. Fixed by the above; now tested for by name. THE LEDGER NOW CARRIES THE WIRE'S FIGURE. `saved` was content − descriptor, but in markerFull the text spliced is descriptor + marker + recovery hint, so every candidate was overstated by the marker's tokens — while the comment I had just written claimed the figure was "what reached the wire", which is the one thing it was not. Measured against the spliced message instead. Verified: 86,458 claimed against 86,201 actually sent. ALSO - extract_llm's "a declined splice observes nothing" comment argued one side only. Under a persistently saturated reserve nothing advances r.total, so ratio() stays pinned to its prior and exploring() keeps granting its per- session budget instead of self-terminating — a small permanent spend on exactly the deployments this change is for. Bounded, and the better side of the trade, but the previous behaviour did terminate exploration and the comment now says so. - A single-flight FOLLOWER returns with out[k].before still 0, and phase 3 spliced its projection and then booked observe(0,0) plus two zero savings for a removal that did happen, while setting Accepted on a row whose Component is "". Harmless only through three independent zero-guards. Now guarded on `before > 0`: the follower's saving stays unbooked (its leader books the shared result once), but the pretence of measuring it is gone. TESTS FOR THE TWO THINGS NOTHING PINNED - TestSweepTellsAReserveRefusalApartFromNothingSpent asserts both rejection strings by name. The reserve-exhausted one was unreachable, so no existing test could have covered it. - TestADegradedModeReplayDeclaresItselfIrreversible gains an extract_sweep_drop subtest. That was the one replay branch of four with no test of its own — the reviewer confirmed by execution that it is fixed, but nothing pinned it. - TestSweepReportsItsOwnEconomicsAndTheWiresSeparately asserts the debug row's removed_tokens is non-zero AND that the ledger's total equals the exact token delta of the messages actually sent. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass (Go 1.26.4, eval box). 5 new mutations, all caught by a TEST failure: M44 the rejection tests a total with no assignment -> removed_tokens=0 on a turn that dropped content M45 judgedTokens loses its assignment -> same M46 the reserve-exhausted reason is dropped -> rejection "" where the distinct string is required M47 sweep_drop replay drops rep.Irreversible -> the revert conjunction M48 sweep books the descriptor-only figure -> 86458 claimed, 86201 sent Three of these did not bind on the first attempt and the reasons are recorded because they recur: M44 as first cut was behaviour-equivalent to the fix rather than a revert of the bug; M45 had no assertion to fail because nothing read the debug row; M48 named a test that did not exist and reported "no tests to run", which the harness now treats as not-caught rather than as a pass. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…et-wide, not per-session Comment only; no behaviour change. The previous comment called the cost of recording no ratio observation for a declined splice "bounded per session". That under-counts it by the number of concurrent sessions, and the comment is what a future reader will reason from. The store is ONE process-wide instance, so saturation is CORRELATED rather than independent: when the reserve is full it is full for every session at once. The spend is maxExploreCalls x every concurrent session, repeating for as long as the episode lasts — and with a sliding 10,000 s TTL an episode can run for hours. The deployments that pay it are the under-provisioned ones in #190, which are the least able to absorb it. The trade still stands and the behaviour is unchanged: observing the achieved ratio would authorise calls whose output is discarded, and observing 0 would assert the workload is incompressible — false, and it would also suppress calls that WOULD land once the reserve frees. Observing nothing is the only one of the three that does not lie. But "small" and "bounded per session" are different claims, and only the first is true. Also records the option that gets both properties, so it is not rediscovered: gate exploring() on RESERVE HEALTH rather than on r.total. stash_refused already distinguishes "cannot learn because we are refusing" from "no evidence yet", which is precisely what r.total cannot express. Deliberately not implemented here — it is the same question as #190 and wants the re-run's numbers, so it is on that issue's candidate list instead of being guessed at in this diff. Raised by the reviewer, who also worked out the fleet-wide arithmetic. Verification: gofmt -l . clean, go vet ./... clean, go test ./... all packages pass. No test accompanies this commit because it changes no behaviour; the behaviour it describes is pinned by TestExtractLLMBooksNothingForAFreshSpliceItCouldNotStash, which asserts the ratio tracker does not move on a declined splice. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…acuity guard able to fail Round 4 of the #188 review: one LOW that was a self-contradiction this branch introduced, two LOWs about claims that were not true, and a test guard that could not fail. THE VACUITY GUARD WAS ITSELF VACUOUS, and it was guarding the branch whose regression started this thread. assertReplayIsExemptFromRevert compared EVERY message against ONE string — the candidate's original — so the userMsg every fixture carries ("summarize the log") differed from it by construction and `shrank` was unconditionally true. The guard could never detect the case its own fatal message describes. Two of the three subtests were saved by a separate `Replays == 0` fatal. The extract_llm one had no such check, so it would have passed vacuously if that replay ever stopped rewriting — and that is the exact branch whose regression the review found two rounds ago. Now each message is compared against ITS OWN captured before-state. Demonstrated with one mutation run twice: a replay that counts itself but splices nothing ESCAPES the old guard and is CAUGHT by the new one, which is the property at issue rather than a proxy for it. THE SWEEP CONTRADICTED ITSELF, as of the previous commit. The fresh path was corrected to measure the saving against the spliced message; the replay path was left measuring against the stored descriptor. So the same drop was valued higher on every replay turn than on the turn it was made — and replays are the steady state, so most of the reported value came from the overstated side. Before the previous commit both paths were descriptor-only and at least consistent, so this was introduced rather than pre-existing. Both now measure the message. TWO COMMENTS THAT CLAIMED MORE THAN WAS TRUE - The `before > 0` guard added last commit prevents no live defect: observe returns early on totalTok <= 0, both recorders are no-ops at 0, a follower's saved is already 0, and its row is dropped by the Component filter. The comment said otherwise, which would leave a reader reasoning from a false premise. It now says it is a defence, and the predicate is `Component != ""` — the direct statement of "this slot made a call", and one that cannot be re-enabled by accident if someone later fills `before` in the follower branch. - `len(drop) == 0` conflated TWO reasons, not one. The never-worse pre-check can remove every output the adjudicator judged spent, and the row then told the operator the model found nothing worth removing when it found plenty and the descriptors were not smaller. That case now has its own reason. Reachable on outputs barely above the floor, where a shape descriptor can exceed the output it describes — which is what the new subtest builds. NOT FIXED HERE, deliberately: extract_llm's saved excludes the SUMMARY as well as the marker, so its overstatement is variable and summary-dominated rather than the fixed per-candidate overhead #195 described. #195 is corrected; the basis change still belongs there, after the iteration-024 re-run, so a savings-basis change does not land in the middle of the comparison it would invalidate. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass (Go 1.26.4, eval box). 4 new mutations, all caught by a test failure: M49 the guard compares to one original -> caught (via the neutered branch) M50 sweep replay books the descriptor again -> 86458 booked, 86201 sent M51 the pre-check case falls back to "nothing was spent" -> the exact wrong string M52 a replay that counts but splices nothing -> CAUGHT by the repaired guard, ESCAPES the old one M52 is the one that matters: M49 turned out to be caught by the subtest's Irreversible assertion rather than by the guard, so it did not isolate what it was meant to. M52 does — same mutation, two guards, opposite outcomes. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… overridden newSweep prepends its own `min_tokens: 2000`, so passing one in extraYAML does not override it — YAML rejects the duplicate key and newExtractSweep fails with yaml: unmarshal errors: line 3: mapping key "min_tokens" already defined at line 1 which reads as a broken component rather than as a fixture mistake. It cost a round trip while writing the never-worse pre-check subtest, which needs a floor BELOW the shape descriptor's own size and therefore has to call newExtractSweep directly. Documented at the fixture rather than left in a PR thread, on the reviewer's point that a trap recorded only in review discussion gets rediscovered. Comment only; no behaviour and no test outcome changes. Verification: gofmt -l . clean, go vet ./... clean, go test ./... all packages pass. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing accounting on a label Round 5 of the #188 review. Four findings, one of which was caused by a CORRECT fix from round 4, and one of my repairs introduced a bug that its own new test then caught. A MEASUREMENT CHANGE VOIDED A TEST THAT NEVER MENTIONED THE MEASUREMENT. Moving the sweep's saving to the wire basis was right, and it made TestSweepReportsNoSavingsForARefusedReplay unable to fail: sweepDrop only calls SetMessageText on success, so on a decline `saved == 0` BY CONSTRUCTION and the recorder's position is unobservable. The reviewer confirmed by mutation — booking above the ok gate now PASSES. Re-scoped rather than deleted, and the doc says what it still pins: rep.Replay, which sits inside the ok branch and would fire for a message never rewritten. What guards the saving is now TestSweepReplayBooksWhatTheReplayedMessageActuallySaved — revert the basis and it fails, which also restores this test's original premise. Renamed to TestSweepRecordsNoReplayForADropItDeclined so the name stops claiming the half that is now structural. THE LEDGER'S REASON IS DERIVED, NOT ENUMERATED. "THREE reasons, not two" was still short by at least two paths. RefusedObligation and VerdictUnusable both `continue` without raising the pre-check gate, so a model answering drop with a needed_by for EVERY candidate produced a row saying the adjudicator found nothing spent — when it judged every output spent and we declined each one for self-contradiction. Enumerating skips was the mistake. The reason now derives from spentJudged, counted where the verdict is read, and the GATES carry which skip it was — they are already raised and stay correct when a skip is added. Counted as `a.Drop || a.RefusedObligation`, because extract.Judge returns early on a self-contradictory drop WITHOUT setting a.Drop, so counting a.Drop alone missed precisely the path that made the reason wrong. VerdictUnusable is deliberately not counted: the model answered neither drop nor keep, so it judged nothing spent. ACCOUNTING NO LONGER KEYS ON A LABELLING FIELD. `calls[k].Component != ""` is just rep.Component, which pipeline.go always sets in production — so the coupling was invisible there and broke the TESTS: most fixtures in this package pass a bare &components.Report{}, and from those the whole booking block was skipped, making a future regression inside it uncatchable. Now an explicit out[k].called flag. That repair introduced its own bug, and the test written for it caught it: the accept branch did `out[k] = outT{...}`, a struct literal that silently reset `called`, so phase 3 skipped the booking for every real call. Field assignment now. The follower path may keep its literal because it has no `called` to preserve, which is what made this easy to miss. AND ONE COMMENT THAT ASSERTED WHAT THE KEY CANNOT EXPRESS. A cg:res: hit does not prove "this session already sent THESE bytes": resultKey is cg:res:<session>:<id> with no component tag, extract_llm keys the same namespace off the same ContentKey, and the shipped housellm preset runs both in one pipeline. So a hit proves only that SOME component in this session decided about these bytes. Corrected to say that; the design point is #197, with the reachability analysis — the obvious path through a refused reserve slot turns out not to exist, and unreachability is not established either. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass (Go 1.26.4, eval box). 6 mutations, all caught ON THEIR OWN ASSERTION — the harness now enforces five gates (landed exactly once, compiled, the test ran, it failed, and the failure output contains the subject's assertion text), because "did it fail" scored two earlier mutations as evidence when they were not: M53 count only a.Drop -> the RefusedObligation row reads "nothing was spent" M54 enumerate one skip gate again -> the same, plus the wrong string M55 rep.Replay outside the ok gate -> fired for a message never rewritten M56 the called flag is never set -> ratio tracker does not move M57 accept branch resets outT -> same; this is the bug the fix introduced M58 booking keyed on Component again -> same, from a bare Report fixture M56 initially ESCAPED against TestExtractLLMLogsOneRecordPerCall, which passes when accepted is false on both sides. Every existing test asserted "books nothing when declined", which a component that books nothing ever also satisfies — so the positive counterpart was missing. TestExtractLLMBooksItsOutcomeEvenWithAnUnnamedReport is that test, and it deliberately uses a bare Report because that is the fixture shape the Component coupling broke. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b01f153 to
13b32e5
Compare
|
Rebased onto
for _, k := range keys {
- c.Store.Put(k, []byte(content)) // refresh the stashed original for expand
+ commitRefresh(c, rep, markerFull, k, content)
}The One conflict, in comment prose only. #194 and this branch each carried a version of the One deliberate overlap left in place. Verification after the rebase, on the box against the new base: Remaining open, all filed and none blocking this branch: #189, #190, #192, #195, #197. |
|
Rebase verified, and thanks for the heads-up on the stale tree — removed. On the duplication: keep it, and I think "duplication with a reason" undersells it — the two tests have different subjects that happen to share an assertion.
Those fail differently. Special-case the helper so it suits The real cost is drift, in one specific direction: someone sees two tests for "the same thing" and deletes the table entry as redundant, which is exactly the reasoning that looks correct and isn't. A sentence in the standalone test saying it is not a substitute for the table entry, because the table's subject is uniformity rather than the property, would cost nothing and pre-empt that. One thing I checked while forming that view, and it is worth recording as a non-finding. The table has three subtests, but there are five
So the four branches are genuinely covered and summarize is not a fifth hole. But that took reading five call sites to establish, and the table's completeness rests on convention rather than on anything mechanical. On the review-required wall: agreed that is a human gate rather than a CI one, and #194 clearing it is the precedent. Six rounds, and the two corrections were mine to make — a finding that is right about a problem and wrong about its cause is still worth filing, but only if the cause gets corrected once it is known. Thanks for pushing back where I was wrong about the cause both times. |
…ndant Comment only; no behaviour and no test outcome changes. main's TestASummaryModeReplayIsNotRevertedFromTurnTwoOnward (from #194) and this branch's `mask via reapplyFrozen` subtest assert the same thing, and the obvious reading is that one is redundant. They are not: they have different SUBJECTS that happen to share an assertion. - the standalone test asserts a property of reapplyFrozen. - the subtest asserts that all four replay branches are held to the SAME assertion through the same helper — uniformity, not the property. They fail differently, which is the test of whether both are needed. Special-case the helper so it suits extract_llm and extract_sweep_drop but not reapplyFrozen and the table catches it while the standalone test stays green; break reapplyFrozen itself and both fire. Dropping the subtest loses the first case entirely, and that is the case that matters for a helper three other branches depend on. The real risk is drift in one specific direction — someone sees two tests for the same thing and deletes the table entry as redundant, which is reasoning that looks correct and is not. Each now says it is not a substitute for the other, and why. Also records at the table that it has NO completeness check, unlike TestNoStateIsRecordedBeforeTheCommitGate's gateExempt. There are five commitRefresh call sites and three subtests. The coverage is complete today — only extract_llm and extract_sweep_drop pass a computed eff and so can reach the eff != markerFull branch, while state.go and summarize.go's two sites pass markerFull and handle the degraded case themselves — but establishing that took reading five call sites, and nothing fails if a sixth is added. #198 carries the mechanism; two instances of that exact defect were found during this review, one introduced and one pre-existing (#193), which is why it is filed rather than left as a convention. Both points are the reviewer's, including the framing that the two tests have different subjects rather than being duplication-with-a-reason, which is what I had called it. Verification: gofmt -l . clean, go vet ./... clean, go test ./... all packages pass (Go 1.26.4, eval box). Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Your framing is better than mine and I have used yours. "Different subjects that happen to share an assertion" is the accurate description; "duplication with a reason" was me conceding a point I did not actually need to concede. Both tests now say they are not a substitute for the other, and why — Your non-finding is now #198, and it is the most useful thing to come out of this round precisely because it is not a defect. The part I would not have arrived at is the classification: only the two sites passing a computed I also noted it at the table rather than only in the issue: the table now says it has no completeness check, that the coverage is complete today, and how that was established. A reader who finds the table in six months should not have to re-derive five call sites to know whether to trust it. Filed rather than fixed here, on your own reasoning and mine agreeing for once: it is test infrastructure rather than a defect, and this branch is long. On the last point — agreed, and I would put it the other way round. A finding that is right about a problem and wrong about its cause is worth filing because the cause gets corrected once it is known; the alternative is not filing it, and then the problem stays. Both times the correction came from you unprompted, which is the part that made the fixes right rather than merely applied. Thank you for the six rounds. |
…ee, and a drop is not a compaction Two caveats from the #188 review thread, recorded so neither lives only in a message. SEMANTICS. extract_llm stores a compaction -- a property of the content, reusable wherever that content appears. The sweep stores a DROP, "spent for this transcript's obligations", which is why it never publishes to the cross-session cg:xres: namespace. A replay therefore applies an obligation judgement as a content projection, at a fresh position, possibly many turns later. The hazard is not staleness in general but its direction: recurrence is evidence AGAINST spentness. Replays land only on unmarked content, so the ids are content reappearing at a fresh position, and content reappears because the agent re-ran the command or re-read the file -- which it does because it needs it now. So the replay applies "was spent when the sweep looked" exactly when the agent has demonstrated renewed need, against a contract defining spent as needed for none of "the step you are on right now". If a tag is added it should distinguish compaction from drop, which carries meaning, not component from component, which does not. COST. This section recorded the channel as $11.58 at $0.00. But the replay reaches apply WITHOUT passing the cache-tail depth gate (replay at :890, gate at :932), and that bypass is justified per-MESSAGE -- "this session already sent these compacted bytes" -- while the lookup is keyed per-CONTENT. At a fresh position the provider holds the ORIGINAL, so splicing the compaction changes cached bytes and forces a cache-write of the suffix: a real cost on a ledger extract_llm does not carry, and the same cost the sweep's econ trigger prices at 11.5x a cache read. All 364 replays are in that category -- the marker skip at :848 precedes the lookup at :868 -- so this is the whole channel, not a tail. If it holds, the 57% recovery is not free, only charged where nobody was reading. It cannot be settled here: cache_read_tokens, cache_write_tokens and fresh_input_tokens all read 0 in every arm-seed, unpopulated in this build rather than measured as zero. A re-run with those live settles it against arm A, where no replays exist. In the channel's favour, review established the BYTES ARE IDENTICAL: a sweep record replayed by extract_llm writes what the sweep's own splice writes. So the cross-component path cannot flip content and a component tag would prevent no byte difference. The open questions are semantic and economic, not correctness. Signed-off-by: David Amid <david.amid@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…imply a fresh position The previous commit claimed all 364 recovery-channel replays landed at fresh positions, and derived from that a hypothesis that the 57% cost recovery might be paying a cache-write per replay. The #188 reviewer caught the bad step and is right. The invalid inference was that because the marker skip precedes the cache lookup, replays must be on content at fresh positions. Unmarked does not imply fresh. The client re-sends its OWN original transcript every turn -- collapse.go:108 states it directly, "the agent re-sends the original each turn, so the only way the representation stays stable is to re-derive the same bytes", and failed_run.go:121 and agentdiet.go:393 say the same. So a message compacted last turn arrives unmarked again this turn at the SAME position and the replay re-derives identical bytes. That is the steady state everywhere on every turn, and it is precisely what the depth bypass exists for: the provider already holds the compacted form there, so nothing is re-written and no cache-write arises. What survives is bounded rather than universal. The bypass is justified per-message while the key is per-content, so a subpopulation can splice at depth into content the provider holds in original form -- but it is self-limiting, because new content is APPENDED, its index exceeds MaxCachedIdx, TailOnly puts it in the tail on arrival where the gate would permit acting anyway, and by the time that position is at depth it was compacted on its arrival turn. Reaching the hazard needs the boundary to have moved (a compaction reset) or the component not to have acted on arrival. Iteration 024 cannot size it: 364 is the total, not the fresh-position count, and cache_read_tokens / cache_write_tokens / fresh_input_tokens all read 0 in every arm-seed -- unpopulated in this build rather than measured as zero. So $11.58 at $0.00 stands for the steady-state replays, with an unsized minority possibly carrying an unrecorded cache-write. The semantic caveat in the same section -- a drop replayed as a compaction -- is unaffected and stands. Signed-off-by: David Amid <david.amid@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…uing it locally Section 2b noted that 209 unresolved expands went unnoticed for two iterations. The reason generalises and is now stated citably on #200, so this file points at it instead of making a local version of the argument. The shape: the benign reading is what a counter's description promises, so the dangerous case hides behind a reassuring one and the failure is silent by construction. expand_unresolved_missing sat at 209 looking like ordinary expand traffic; usage_reported was false on 4,015 of 4,015 requests and was found two iterations later while chasing something else. Both are this iteration's contribution to that argument rather than illustrations of it, and #200 carries them as its supporting evidence alongside expand/unresolved.go's malformed-vs-missing and #188's stash_missing-vs-stash_refused. Signed-off-by: David Amid <david.amid@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
…window Comment and docs only; no behaviour and no test outcome changes. `unreadable_body` is reachable ONLY from the sniffed path — Handler.stream, taken when neither proxy-injected tool is advertised on the request — because that is the only place usage is read from a bounded head+tail window rather than the whole body. With either tool advertised, which `inject_expand: always` guarantees from the first turn, a non-streamed response is read whole at proxy.go's `default:` branch and the window cannot apply. Recorded because the call-site distinction has now misled two sessions in opposite directions. I floated the spliced window as a competing explanation for iteration 024's usage_reported=false on 4,015 of 4,015 requests; it cannot be one, because those runs set INJECT_EXPAND=always so `advertised` was true and the full body was read. The reviewer on #188 hit the mirror image, writing a parseUsage test that passed on the stream branch while the traffic in question took the buffered one. Both are the same error: reading the two `responseUsage` call sites as interchangeable when the bytes reaching them are not. Neither counter changes, and `valid_json` in the shape record already settles which case a reader has without knowing their path — this only saves them forming the wrong hypothesis first. Credit for the correction, and for checking it against the actual line numbers rather than the plausible reading, to the coref validation session holding the iteration 024 measurement. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
3 participants
Closes #187.
The cause, established
The issue's hypothesis is correct, and the store's own comment said so before the fix did: "The rewind stashes (bare content hashes, the large payloads the expand loop resolves) stay fully evictable."
A stash payload's key is the marker id — a bare lowercase-hex content hash the model reads out of the request and hands back to
context_guru_expand(expand.Marker,offload.hashKey/extract.ContentKey). It therefore carries none of the pinnedcg:*prefixes, and it cannot be given one: marker bytes sit inside the provider's cached prefix, so prefixing the key would rewrite every marker. Meanwhile the decision records that reference it are pinned (cg:res:,cg:xres:,cg:frz:).The arithmetic that turns that into 209 failed expands:
cmd/context-guru-proxybuildscfg.NewStore()once (main.go:480); every concurrent session shares it. The iteration 024 arm configs set nostore:block, so: 1,000 entries, pin cap 500,DefaultTTL10,000 s.cg:own:(unpinned),cg:xseen:(unpinned),cg:res:(pinned),cg:xres:(pinned). The payload competed for ~1/5 of the evictable half while the two records naming it were exempt.Measured directly against the shipped defaults of the day (600 removals writing that real key mix):
100 of 600 payloads survived against 350 of 600 of each pinned decision — the payload was 3.5x less durable than the record that pointed at it. 500 markers had been stamped for content the store could no longer produce.
The other two candidate causes are refuted
DefaultTTLis 10,000 s and sliding (refreshed onGet, and components re-Puton every replay turn); the runs are shorter. The reproduction above evicts payloads with the clock stationary, so expiry is not needed to produce the symptom.*store.Memory(Persists() == true), andeffectiveModealready degrades a full marker tooffwhen it is not — so a non-persisting store cannot leave a dangling marker. Arm A ran the same store and scored 0.Gets:proxy.repairExpandErrors→expand.Resolve(tn.Store, id)(proxy.go:1297) and the response continuation loop (proxy.go:1653).offload.OwnsKey— the only session-scoped read — gates the managementGET /expandendpoint alone.What changed
store.Stasher/store.PutStash— a reserve for rewind payloads, and a boolean. An optional Store capability (same pattern asFrozenLoser), because the store cannot recognise these keys on its own; stores without it keep the legacy behavior.Memoryflags such entries, exempts them from LRU eviction, and bounds them atmax_entries / 2— the same share the pins get.The return value is the fix, not the reserve. A live payload is never evicted to admit a new one. When the reserve is full
PutStashreturns false, and the caller declines the removal:offload.commitMarknow returns a bool; all twelve offloaders (mask,failed_run,collapse,dedup,extract,extract_llm, the sweep's drop,linecap,skeleton,smartcrush,readlifecycle,agentdiet)continue— the message is left verbatim, not cut, and no marker is stamped.cmdfilterbuilds its marker token inline rather than throughcommitMark, so it honors the same contract in its own file.summarizereplaces the span it covers, so it cannot leave one message alone: it skips the whole checkpoint.reapplyFrozen, summarize's checkpoint replay) count a refusal but still replay — refusing there would flip an already-cached message. A refresh of a payload that is present is never refused, for the same reason.Only the TTL releases a slot, and
PutStashcollects expired ones before declaring the reserve full, so a long-lived proxy recovers.Visibility.
stash_refused,stash_live,stash_capacity,stash_expiredat/stats(added to the golden contract) andcg_stash_refused_total,cg_stash_reserve_entries,cg_stash_expired_totalat/metrics.stash_refusedis the leading indicator:expand_unresolved_missingcannot move until an agent happens to call expand, so a proxy that had stopped being able to promise reversibility read as healthy until one did.DefaultMaxEntries1,000 → 5,000. 1,000 was an order of magnitude under the observed volume (hundreds of removals × five entries × eight concurrent workers). The doc comment says plainly that entry count is a poor proxy for memory in this namespace and that an operator with large payloads should sizemax_entriesagainst bytes.What happens now when the budget is exhausted
The removal does not happen. The tool output is forwarded verbatim,
stash_refusedincrements, and the component records astash_reserve_exhaustedgate. Nothing that was promised stops being deliverable; the pipeline saves fewer tokens and says so.That is the property #187 asked for by name. Before, every extra removal both consumed a slot and added a pinned decision, so broken promises grew with the amount of work done. Now what grows is the count of removals declined — a refusal to promise instead of a promise broken.
TestBrokenPromisesDoNotGrowWithLoadasserts exactly that at 40 / 200 / 1000 removals against a 50-slot reserve: broken = 0 at every load, refusals strictly rising.Mutations run, and what each broke
The bug is still representable (nothing here is a type-level impossibility), so every test was verified by reverting its subject in a scratch tree and confirming failure.
evictOldeststops exempting stashes (the original #187 mechanism)TestAnAcceptedStashSurvivesThePressureThatEvictsThePlainCache(payload evicted after acceptance),TestAFullReserveRefusesInsteadOfEvictingALivePayload,TestBrokenPromisesDoNotGrowWithLoad— 30/40, 190/200, 990/1000 advertised-reversible removals cannot be reversedPutStashevicts the oldest payload and reports success instead of refusingcommitMarkignores the refusal and stamps the marker anywayTestNoMarkerReachesTheWireWithoutItsPayloadand the proxy end-to-end test — the body sent UPSTREAM offers the model<<cg:…>>and the store cannot produce it/statshandler dropsstash_live/stash_capacitystash_capacity = 0, want 2/statshandler dropsstash_refused(computed, then thrown away)stash_refused = 0while removals were being declinedPutStashnever reclaims expired payloadsTestTheTTLReleasesReserveSlots— the reserve stayed full after its payloads expired: it never recoversTestRefreshingALivePayloadIsNeverRefusedcmdfilterstamps its inline marker despite the refusalTestCmdfilterRefusesRatherThanStampingAnUnbackedMarkersummarizeemits the checkpoint despite the refusalTestSummarizeSkipsTheCheckpointWhenTheSpanCannotBeStashed— restructured the transcript (2 messages, was 7)… the summary's marker is the only route back to a span that is now goneDefaultMaxEntriesback to 1,000M7 and M8 initially escaped, which is why they are here: the first refresh test used a key that already held a slot, and the first cmdfilter mutation removed the only use of an import so it failed to compile rather than failing a test. Both were re-cut to compile and to bind, and tests were added for them.
Two of the three new tests assert on rendered output, not on return values:
json.Marshalof the rewritten messages (offload) and the body the fake upstream actually received (proxy), each scanned for markers in both spellings —encoding/jsonHTML-escapes<, and every marker follows a newline, so on the wire a marker usually exists only as<<cg:HASH>>. The first draft of that check scanned only the plain form, reported zero markers on a body full of them, and would have passed while the wire carried unbacked ones.What I could not establish
results.mdrecords nofrozen_dropped/ store figures, and the raw run artifacts are another session's live evidence). The reproduction showsfrozen_dropped = 500alongside the evicted payloads, so a re-run should showfrozen_dropped > 0in arm B and 0 in arm A if this is the whole story.DefaultMaxEntries = 5000is not test-pinned (M10 breaks nothing). A test asserting the constant would assert it against itself; its effect is covered by the reserve tests, which are parameterised onMaxEntries. The number is sane, not measured — the refusal counter is what makes a wrong choice visible.max/2exemption, which is why the reproduction dropped 500 pinned decisions too. Not fixed here. The larger default relieves the pin pressure fix(store): pin-budget exhaustion can reopen the TailOnly fail-open on /compact #47 describes (the pin cap goes 500 → 2,500) without addressing itsTailOnlyfail-open.Impact on iteration 024's published results
From the issue, so that whoever reads this knows a re-run is needed to separate the two:
results.mdreports arm B taking 13% more turns on pairs both arms solved and calls the latency hypothesis "refuted". 209 failed expands over 75 runs is ~2.8 per run against ~33 turns/run, so failed-expand round-trips could account for a meaningful share of that 13%. It should read "not supported, and partly confounded by this defect."Verification
gofmt -l .clean ·CGO_ENABLED=1 go vet ./...clean ·CGO_ENABLED=1 go test ./...all packages pass (Go 1.26.4, eval box).Review round 2 (
3f12b9d)The seven inline findings, the doc finding and the ordering note are all addressed. The two themes were fixed as invariants, because in both cases the flagged sites were not the whole set.
Theme 1 — nothing is recorded before the gate
commitMarkcan refuse, so anything written ahead of it may describe work that did not happen — and the dangerous half is not the lost saving. A frozencg:res:decision with no splice behind it is read on a later turn by the same-session replay path, which deliberately bypasses the cache-tail gate on the reasoning that these bytes were already sent; once a reserve slot frees, the compaction lands inside the provider's cached prefix and the whole suffix is re-written at ~11.5x the read price. The reversibility fix had introduced a reversibility-adjacent bug.Audited all twelve
commitMarkcallers. Ten were already clean. Four sites recorded state early — the three in the review, plus the cross-session cache hit (extract_llm.go:~975), which the review did not name: its own comment argues the branch is a NEW decision for this session, which is why it is tail-gated, so its payload write can refuse like any other.Enforced rather than commented:
commitMark's doc states the rule and names what it covers (splice, frozen decision, metrics, debug counters,rep.Replay).TestNoStateIsRecordedBeforeTheCommitGatedrives registered offloaders against a saturated reserve and asserts no marker on the wire and no decision write, with a healthy-store precondition so a component that silently stopped acting cannot pass it vacuously.TestEveryOffloadComponentIsHeldToTheCommitGaterequires every registered Offload to be driven or listed ingateExemptwith the test that pins it — so the thirteenth caller has to declare itself. Same mechanismpromexport_coverage_test.gouses for/statsfields.cmdfilterno longer hand-rolls its own token, never-worse check andPutStash. It differed fromtryMarkonly in the recovery hinttryMarkalready takes as a parameter, and that duplicate is exactly why M8 escaped a whole review round in that file alone.A replay is now a different operation from a new removal. Gating alone would have made the replay paths worse: refusing there sends the original in full where the cached prefix holds the compacted bytes, which is the cache-destructive direction and cannot un-send a marker that went out turns ago.
commitRefreshnever refuses and reports a missing payload as a diagnosis. #188 already stated that rule forreapplyFrozen; extract_llm's and the sweep's same-session replays were missed.Theme 2 — the reserve's budgets
stash_max_bytes, default 256 MiB. Promoted from follow-up to fix, as the review suggested it might be. Entries are a poor proxy for memory in this one namespace — every other exempt entry is a marker line or an integer, a payload is a whole tool output — somax_entriesnamed a memory figure spanning two orders of magnitude depending on nothing the operator chose./statsand/metricspublishstash_bytesagainststash_max_bytesbeside the entry pair, so which budget bound is visible.pinCapandstashCapare eachmax/2, so together they could occupy the whole entry cap — and a cache with nothing evictable fails silently, not loudly: the next plainPutis evicted by its own insert, turningcg:keep:(the flag that stops the expand loop),cg:sum:,cg:own:andcg:xseen:into no-ops, on precisely the workload the reserve was built for. A quarter ofmax_entriesis now held back from both exemptions.sweepExpiredmakes one pass instead of one per reclaimed entry, andnextExpiryskips the walk entirely when nothing can have expired. Before, every refusedPutStashwalked the whole list under the global mutex.Per-session partitioning was considered and declined. No session reads another's payload (
OwnsKeyenforces it), so what is shared is the budget, not the data; a fair-share cap would need a share number as arbitrary as the one it replaced, plus an ownership rule for a content-hash key two sessions can legitimately both stash. Sizing against the real resource addresses the same failure more honestly.Two counters, because they were opposite outcomes
stash_refusedandstash_missingshared a counter. Every operator-facing description of a refusal promises "the content was left verbatim and nothing became irreversible" — true of a declined removal, false of a replay whose payload has gone, which is the only case that actually breaks the #187 guarantee. So the number an operator watches to confirm nothing broke was incremented by things breaking.stash_missingalso grows with turn count, not with distinct dangling markers — a missing payload cannot be restored, because the replayed bytes must stay byte-identical to the turn that created them — and both the counter androutes.mdsay so.summarize
agentdietgets the same probe, weaker on purpose (smallest candidate, so it skips only when nothing at all fits).[msg0, summary, tail]; a barereturn nil, nilsent the transcript full. When the checkpoint is stale-but-valid —tryReuseverified the prefix hash and declined only on size — it is re-emitted instead, through anemitCheckpointshared withtryReuseso the two cannot drift. A session with no checkpoint still sends the full transcript, and that case is asserted so the fallback cannot become unconditional.Docs
docs/reference/config.md— the page an operator reads whenstash_refusedtells them to raisemax_entries— documentedmax_entriesas1000and eviction as pinning-only. Now: 5,000,stash_max_bytes, both exemptions, the floor and why its absence is silent, and explicit guidance thatstash_refusedmeans raise a budget whilestash_missingmeans raisettl_seconds. Also corrected indocs/design.md(both sites) anddocs/how-to/recover-context.md(defaults, the rewind bullet, the pin-cap sentence, the troubleshooting note).routes.mdseparates the two counters. The fourstatsGoldenTopLevelkeys are back in alphabetical order.Verification
gofmt -l .clean ·CGO_ENABLED=1 go vet ./...clean ·CGO_ENABLED=1 go test ./...all packages pass (Go 1.26.4, eval box).23 mutations, each reverting one subject whole in a scratch tree, all caught. Full table in the commit body. The ones worth reading: M11 (floor removed) reports
0 of 200 entries are evictableand thecg:keep:write vanishing — the reviewer's predicted failure, reproduced; M25 (bare return) sends 19 messages where 2 were cached; M30 (/statswire dropped) readsstash_missing = 0while the live counter reads 1.Four tests were vacuous or wrong on the first cut, recorded rather than quietly fixed because the shapes recur here: one asserted an unchanged message, which is also what a component that never ran leaves behind; one was blind because
round4renders a ~1e-6 USD saving as0.0000; one assumed a replay could refuse, which by the new design it cannot; and one cross-session fixture used the same content in both sessions, where the payload is already present and the write is a refresh that cannot refuse. M27 also initially "passed" only by breaking the build, and was re-cut to compile and bind.Still open, deliberately
stash_refusedand both budget pairs, not by another guess at a default.cg_stash_*has no Grafana panel or alert — filed as observability: the cg_stash_* family has no Grafana panel and no alert, including the one counter that reports a broken reversibility promise #189. The series exist;deploy/grafana/dashboards/context-guru.jsondoes not reference them, andcg_stash_missing_totalis the alertable one.#47is still not fixed, andDefaultMaxEntries = 5000is still sane rather than measured — unchanged from the original description.Review round 3 (
a67c597)Seven more findings, all fixed. Two were mine to answer for: a regression this PR introduced, and finding 1 of round 2 having been half-applied by me.
The regression: a degraded-mode replay stopped declaring itself irreversible
commitMark's non-full branch setsrep.Irreversible, and that flag is what exempts a deliberate lossy drop fromcomponents/pipeline.go's "dropped content without stashing a cache_key" revert. When the replay branches stopped callingcommitMark, they stopped setting it — so undermarker_mode: summary/off, every replay turn reverted the whole component and sent the transcript verbatim: a full-suffix cache write per turn, which is the harm this PR exists to prevent.Confirmed by execution, two turns of
extract_llmatmarkerSummary:commitRefreshnow takes therep/effpair and ownsrep.Irreversiblesymmetrically withcommitMark, so the two cannot drift and a third caller cannot reintroduce it.A third instance predates this PR.
reapplyFrozenhas never set the flag, and a summary-mode replacement carries no<<cg:HASH>>to parse so it returns no keys either — meaning a summary-modemaskhas been reverted on every turn from turn 2 onward, independently of the reserve work. Fixed here under the same rule, since leaving it would mean the shared helper was correct and its oldest caller was not.The fresh paths were still booking before the gate
Round 2 moved the replay paths' metrics and left the fresh ones:
extract_llm—RecordExtractionSaving,RecordExtractionValue,e.ratios.observe,calls[k].Acceptedandcalls[k].SavedTokensall ran insiderunCall: in a goroutine, beforewg.Wait(), and therefore before phase 3 exists. On a saturated reserve every candidate is declined and the run reported the full saving anyway. The arithmetic now rides in theoutTslot phase 3 already reads.extract_sweep—adjudicatebooked while building its drop list, and the localsweep_drop_would_not_shrinkpre-check covers neither refusal phase 3 can now raise: it is descriptor-only rather than marker-inclusive, and cannot see the reserve at all.Accepted/SavedTokensare now filled from what was applied rather than from what the adjudicator judged spent — the two diverge exactly when the reserve refuses — with a distinct rejection reason for "adjudicated spent, but no drop could be applied".accepted=falseon every call (TestExtractLLMLogsOneRecordPerCallfailed on it); and both ledger appends had to move after phase 3, becauserep.Callstakes a copy.The invariant test now covers the class
Following the reviewer's suggestion, which is better than the three point fixes it replaces:
TestNoStateIsRecordedBeforeTheCommitGateasserts metric deltas — gross saved tokens, gross value, and every ledger row'saccepted/saved_tokens— alongside store writes, andextract_llmandextract_llm_sweepare driven by the table rather than exempted from it.gateExemptis down toagentdietandsummarize, each naming the test that pins it.Also
agentdiet's reserve gate now counts. Gating without incrementingstashRefusalsmade one component's refusals invisible to the counterdocs/reference/config.mdnames as the signal to raise a budget.evictableFlooris at least 1.max/4is 0 formax_entriesof 2 or 3, so the two exemptions could reach the whole cap and the floor documented here as unconditional had a hole at the bottom of the range.TestExtractLLMReportsNoSavingsForASpliceItDeclinedwas blind in the half its own name claims — ~4 tokens at theagentFreshPerMTokfallback is ~1.2e-5 USD againstround4's 1e-4 step. Only the rates fixture was wrong; the assertions are unchanged. It is the second time that trap landed in this change, so the fixture is now factored into apricedCtx()helper carrying the reason.Verification
gofmt -l .clean ·go vet ./...clean ·go test ./...all packages pass ·./dashpasses standalone (70s).30 mutations, every one caught by a TEST failure rather than a build break. Round 1's 23 were re-run against this commit and three no longer landed — M17, M23, M33 — because this commit changed the code they patch; a mutation that does not land proves nothing, so they were re-cut and re-verified. One of them (M17) had been "caught" in round 1 only via a
gotothat failed to compile, so that result was not evidence either. The harness now distinguishes a build break from a test failure.New this round: M34 (replay drops the flag), M35 (the pre-existing twin in
reapplyFrozen), M36 (extract_llmbooks early → 6072 tokens booked, ratio moved 0.12 → 0.45,accepted=trueon a verbatim request), M37 (sweep books early → 86,458 tokens), M38 (agentdietstops counting), M39 (baremax/4→ 0 evictable atmax_entries: 2), M40 (metric ungated withrep.Replayleft guarded — proves the savings half now binds alone), M41/M42 (both fresh paths caught by the table test alone), M43 (an Offload in neither the table norgateExempt).Review round 4 (
c7e41ad)One HIGH regression from round 3, its MEDIUM consequence, and three LOW findings.
The regression, same shape as the one round 3 fixed
Moving the metrics out of
adjudicate's verdict loop deletedremoved's only assignment and left theif removed == 0read. Go then guarantees it stays 0, so every adjudication stamped"adjudicated: nothing was spent"— including ones that dropped content. A ledger row could carryaccepted=true, a largesaved_tokens, and"nothing was spent"simultaneously: exactly the self-contradictory row this work exists to eliminate. The compiler could not catch it because the variable is still read, so it built and the suite stayed green.The rejection now tests
len(drop) == 0— which is what the comment above it already claimed, sincedropis "what the adjudicator judged spent". The token total gets its own name,judgedTokens, for thecg.sweep.askdebug row it was really serving; that field had gone permanently 0, taking the sweep's economics out of the only place a run's decisions can be reconstructed from.Its consequence: the reserve-exhausted rejection reason added in round 3 was dead code, because
adjudicatealways left a non-emptyRejectionso theRejection == ""guard never opened — meaning the case that reason exists to distinguish still read as a plain rejection. Fixed by the above, and now asserted by name.The ledger carries the wire's figure
savedwascontent − descriptor, but inmarkerFullthe text spliced isdescriptor + marker + recovery hint, so every candidate was overstated by the marker's tokens — while the comment beside it claimed the figure was "what reached the wire". Measured against the spliced message instead. 86,458 claimed against 86,201 actually sent on the stocked fixture, ~23 tokens per drop.Deliberately not applied to
extract_llm's projection-only figure: pre-existing, unclaimed, and moving a published savings number for unrelated reasons belongs in its own change.Also
r.total, soratio()stays pinned to its prior andexploring()keeps granting its per-session budget instead of self-terminating — a small permanent spend on exactly the deployments this PR is for. The comment now states it as a trade, not a win.beforestill 0 while phase 3 spliced its projection, bookingobserve(0,0)and two zero savings and settingAcceptedon a row whoseComponentis""— harmless only through three independent zero-guards. Now gated onbefore > 0. The follower's saving stays unbooked (its leader books the shared result once); the guard removes the pretence of measuring it, not the under-count.Tests for the three things nothing pinned
TestSweepTellsAReserveRefusalApartFromNothingSpent— both rejection strings by name, one subtest each, with preconditions so neither can pass by landing in the other's case.TestADegradedModeReplayDeclaresItselfIrreversiblegains anextract_sweep_dropsubtest — the one replay branch of four with nothing pinning it.TestSweepReportsItsOwnEconomicsAndTheWiresSeparately— the debug row'sremoved_tokensis non-zero after a turn that dropped, and the ledger total equals the exact token delta of the messages actually sent.Split out: #194
The
reapplyFrozenfix is a pre-existingmaindefect, verified independently at the merge base (51fcd91), wherereapplyFrozentakes norepparameter at all. On the reviewer's recommendation it is now #194 offmain, closing #193 — a summary-mode revert bug that is live today should not ship only when this PR does, and this PR still carries the open reserve-lifetime question in #190.This branch keeps the same three lines for now and will be rebased onto
mainonce #194 lands, at which point itsstate.godiff reduces to a single call-site change. Not stripped pre-emptively, because this branch's tests depend on them.Verification
gofmt -l .clean ·go vet ./...clean ·go test ./...all packages pass, on both branches.5 new mutations, all caught by a test failure. Three of the five did not bind on the first attempt, recorded because the reasons recur: M44 as first cut was behaviour-equivalent to the fix rather than a revert of the bug; M45 had no assertion to fail because nothing read the debug row (that escape is what produced the debug-row assertion); M48 named a test that did not exist, so
go testreported "no tests to run" and exited 0. The harness now treats that as not-caught — the third distinct way a mutation has looked like evidence without being any.Review rounds 5 and 6 (
b01f153)A correct fix voided a test — and the accurate framing matters
Moving the sweep's saving to the wire basis (round 4's request) made
saved == 0by construction on a declined replay, becausesweepDroponly callsSetMessageTexton success. SoTestSweepReportsNoSavingsForARefusedReplaycould no longer fail.The accurate statement is that the failure mode was eliminated, not that a hole is unguarded. The reviewer filed it as the latter and then corrected it: a mutation that books above the
okgate now books nothing, so it is behaviour-equivalent rather than an escaped bug. Both halves of the resolution were then verified independently:rep.Replay, which sits inside theokbranch and still binds;TestSweepReplayBooksWhatTheReplayedMessageActuallySavedguards the basis — revert the replay measurement to the descriptor and it fails, and nothing else in the package does.Renamed to
TestSweepRecordsNoReplayForADropItDeclined, since the old name claimed the half that is now structural.The ledger's reason is derived, not enumerated
"Three reasons, not two" was still short by at least two paths:
RefusedObligationandVerdictUnusableboth skip without raising the pre-check gate, so a model answering drop with aneeded_byfor every candidate produced a row saying the adjudicator found nothing spent — when it judged every output spent and we declined each one for self-contradiction.The reason now derives from
spentJudged, counted where the verdict is read, and the gates carry which skip it was. Counted asa.Drop || a.RefusedObligation, becauseextract.Judgereturns early on a self-contradictory drop without settinga.Drop— so countinga.Dropalone missed precisely the path that made the reason wrong. The two label paths are skipped beforeJudgeruns, so their answer is unknown and is not counted either way.Accounting no longer keys on a labelling field — and the fix for that had a bug
calls[k].Component != ""is justrep.Component, whichpipeline.goalways sets in production. So the coupling was invisible there and broke the tests: most fixtures in this package pass a bare&components.Report{}, and from those the whole booking block was skipped, making a future regression inside it uncatchable. Now an explicitout[k].called.That repair introduced its own bug and the test written for it caught it: the accept branch did
out[k] = outT{…}, a struct literal that silently resetcalled, so phase 3 skipped the booking for every real call. Reintroducing the literal fails exactly one test — the positive-direction one that did not exist before this round.Because every existing test asserted "books nothing when declined" — which a component that books nothing ever also satisfies.
TestExtractLLMBooksItsOutcomeEvenWithAnUnnamedReportis the counterpart, and it deliberately uses a bareReportbecause that is the fixture shape the coupling broke.A comment that asserted what the key cannot express
A
cg:res:hit does not prove "this session already sent these bytes":resultKeyiscg:res:<session>:<id>with no component tag,extract_llmkeys the same namespace off the sameContentKey, and the shippedhousellmpreset runs both in one pipeline. Corrected to state what a hit actually proves.The design fix is #197, with the reachability analysis — the obvious path through a refused reserve slot turns out not to exist, and unreachability is not established either. Not fixed here, on the same argument accepted for #195: a change altering which records a component replays changes savings, inside a PR whose numbers feed the re-run. That leaves an invariant holding by accident of the current pipeline shapes rather than by construction — documented and filed is not as good as fixed, only better than asserted.
Verification
gofmt -l .clean ·go vet ./...clean ·go test ./...all packages pass.Mutations are now scored through five gates — landed exactly once, compiled, the test ran, it failed, and the failure output contains the subject's own assertion text — because "did it fail" scored two earlier mutations as evidence when they were not.
Eight ways a check on this PR looked like evidence without being any, in two kinds:
Failures of evidence (a harness gate catches these): a non-compiling mutant;
round4rendering the delta0.0000; a precondition a never-ran component also satisfies;go test -runon a non-existent name exiting 0; a behaviour-equivalent mutant; a vacuity guard that could not fail.Failures of test scope (no gate catches these): a measurement change voiding a test that never mentioned the measurement; and a whole assertion direction missing, where every test pinned the negative case and none the positive. The habits that do catch them: after changing how a quantity is computed, re-run the mutations for every test asserting on that quantity; and for each invariant, ask whether a component that did nothing at all would also pass.