From c99dd9d4b425bef069a40ac32ea182395c8f09dd Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Thu, 3 Sep 2026 15:00:03 +0300 Subject: [PATCH] fix(offload): let a replayed summary/off decision declare its loss, or the pipeline reverts it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #193. Under marker_mode summary/off an offloader takes a deliberate LOSSY drop: nothing is stashed and no <> is written, so it returns no cache keys. components/pipeline.go:135 treats that combination — the request shrank, no cache keys, not Skipped — as a contract violation and REVERTS the component, unless rep.Irreversible says the loss was chosen. commitMark's non-full branch sets that flag, so the turn that TAKES the decision is fine. Every later turn replays it through reapplyFrozen, which never did. Measured on a two-turn mask fixture at marker_mode: summary, same content both turns: turn 1 (fresh, via commitMark): keys=[] Irreversible=true -> kept turn 2 (replay, reapplyFrozen): keys=[] Irreversible=false -> REVERTED So from turn 2 onward, for the rest of the session, the component is discarded and the transcript is forwarded verbatim. That is worse than a lost saving. Earlier turns sent the REDUCED bytes, so sending the original re-writes the provider's whole cached suffix at ~11.5x the read price — every turn, for every message the component had reduced. It is the cache-destructive direction reapplyFrozen exists to avoid, reached through reapplyFrozen. And it is invisible from the component's side: it acts, computes a replacement, and the pipeline throws the work away afterwards. Affects mask, collapse, failed_run, readlifecycle, skeleton, cmdfilter and agentdiet whenever configured with marker_mode summary or off. The default (full) is unaffected: a full-mode frozen replacement always carries a marker, so keys is non-empty and the precondition never holds. len(keys) == 0 is a sound test for "degraded mode" rather than a proxy: every freeze() site is downstream of a tryMark/commitMark pair, so a full-mode frozen replacement always carries a marker — which means the blanket flag cannot mask a full-mode bug. Threading rep through the seven call sites is mechanical. Found while auditing replay paths for the #188 review, which flagged the same omission in two NEW replay branches introduced there. This instance predates that work — at the merge base reapplyFrozen takes no rep parameter at all — so it is fixed here, off main, rather than shipping only when #188 does. Verification: gofmt -l . clean, go vet ./... clean, go test ./... all packages pass (Go 1.26.4, eval box). TestASummaryModeReplayIsNotRevertedFromTurnTwoOnward asserts turn 2's exact conjunction, with turn 1 asserted first so a fixture that stopped reaching summary mode fails loudly instead of passing vacuously. Verified by reverting the subject: the test fails with a replayed summary-mode decision rewrote the message, returned NO cache key and did not set rep.Irreversible Signed-off-by: DAVID AMID Assisted-By: Claude Opus 5 (1M context) --- components/offload/agentdiet.go | 2 +- components/offload/cmdfilter.go | 2 +- components/offload/collapse.go | 2 +- components/offload/failed_run.go | 2 +- components/offload/freeze_test.go | 5 +- components/offload/mask.go | 2 +- components/offload/readlifecycle.go | 2 +- components/offload/skeleton.go | 2 +- components/offload/state.go | 22 ++++- components/offload/summarymodereplay_test.go | 86 ++++++++++++++++++++ 10 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 components/offload/summarymodereplay_test.go diff --git a/components/offload/agentdiet.go b/components/offload/agentdiet.go index 5f5a0e8c..41aac25f 100644 --- a/components/offload/agentdiet.go +++ b/components/offload/agentdiet.go @@ -399,7 +399,7 @@ func (d *AgentDiet) Offload(req *bschemas.BifrostChatRequest, rep *components.Re if !schema.Rewritable(*msg) { continue } - if fk, saved, ok := reapplyFrozen(c, d.Name(), msg); ok { + if fk, saved, ok := reapplyFrozen(c, rep, d.Name(), msg); ok { rep.TokensBefore += saved // best-effort; the pipeline recomputes exactly keys = append(keys, fk...) changed++ diff --git a/components/offload/cmdfilter.go b/components/offload/cmdfilter.go index 23815646..a67f323a 100644 --- a/components/offload/cmdfilter.go +++ b/components/offload/cmdfilter.go @@ -140,7 +140,7 @@ func (f *Cmdfilter) Offload(req *schemas.BifrostChatRequest, rep *components.Rep // reapplyFrozen also re-Puts the stashed original for every marker in the // replacement, so the expand loop keeps working across turns, and it declines // content the agent has expanded (kept-verbatim). - if fk, _, ok := reapplyFrozen(c, f.Name(), m); ok { + if fk, _, ok := reapplyFrozen(c, rep, f.Name(), m); ok { changed++ keys = append(keys, fk...) continue diff --git a/components/offload/collapse.go b/components/offload/collapse.go index bd993838..7e39982c 100644 --- a/components/offload/collapse.go +++ b/components/offload/collapse.go @@ -110,7 +110,7 @@ func (cl *Collapse) Offload(req *schemas.BifrostChatRequest, rep *components.Rep // on purpose — with max_frac set, CtxWindow can resolve differently mid-session // (model swap, refreshed modelinfo), and a threshold that drifts above this output // would otherwise flip it collapsed→full inside the cached prefix. - if fk, _, ok := reapplyFrozen(c, cl.Name(), m); ok { + if fk, _, ok := reapplyFrozen(c, rep, cl.Name(), m); ok { changed++ keys = append(keys, fk...) continue diff --git a/components/offload/failed_run.go b/components/offload/failed_run.go index c8da6489..99dd07d4 100644 --- a/components/offload/failed_run.go +++ b/components/offload/failed_run.go @@ -120,7 +120,7 @@ func (fr *FailedRun) Offload(req *schemas.BifrostChatRequest, rep *components.Re // Reapply a previously-frozen collapse on EVERY turn (cache-stable), regardless // of the tail boundary — the agent re-sends the original, so we must re-collapse // it to the same bytes or it reverts to full and churns the cache. - if fk, _, ok := reapplyFrozen(c, fr.Name(), m); ok { + if fk, _, ok := reapplyFrozen(c, rep, fr.Name(), m); ok { changed++ keys = append(keys, fk...) continue diff --git a/components/offload/freeze_test.go b/components/offload/freeze_test.go index 6c49816c..f2132140 100644 --- a/components/offload/freeze_test.go +++ b/components/offload/freeze_test.go @@ -142,10 +142,11 @@ func TestFrozenCountersMove(t *testing.T) { st := store.NewMemory(store.Options{}) c := &components.Ctx{Session: "sCount", Store: st} msg := tool("some tool output") - reapplyFrozen(c, "mask", &msg) // miss: nothing frozen yet + var rep components.Report + reapplyFrozen(c, &rep, "mask", &msg) // miss: nothing frozen yet freeze(c, "mask", "some tool output", "short") msg2 := tool("some tool output") - reapplyFrozen(c, "mask", &msg2) // hit + reapplyFrozen(c, &rep, "mask", &msg2) // hit h1, m1 := FrozenStats() if h1 <= h0 || m1 <= m0 { t.Fatalf("hits/misses must both advance: %d->%d, %d->%d", h0, h1, m0, m1) diff --git a/components/offload/mask.go b/components/offload/mask.go index ee2b0b2d..c74aeaeb 100644 --- a/components/offload/mask.go +++ b/components/offload/mask.go @@ -77,7 +77,7 @@ func (m *Mask) Offload(req *bschemas.BifrostChatRequest, rep *components.Report, // the tail boundary: the agent re-sends the original, so we must re-mask it to the // same bytes or it reverts full→masked→full and churns the provider KV cache. This // also skips kept-verbatim content (see reapplyFrozen). - if fk, _, ok := reapplyFrozen(c, m.Name(), msg); ok { + if fk, _, ok := reapplyFrozen(c, rep, m.Name(), msg); ok { changed++ keys = append(keys, fk...) continue diff --git a/components/offload/readlifecycle.go b/components/offload/readlifecycle.go index a617f7ab..19033b5d 100644 --- a/components/offload/readlifecycle.go +++ b/components/offload/readlifecycle.go @@ -157,7 +157,7 @@ func (rl *ReadLifecycle) Offload(req *bschemas.BifrostChatRequest, rep *componen // Replay a frozen decision on EVERY turn, at any depth: the agent re-sends the // original each turn, so not re-offloading it would flip the message // offloaded→full→offloaded and churn the provider's KV cache. - if fk, _, ok := reapplyFrozen(c, rl.Name(), msg); ok { + if fk, _, ok := reapplyFrozen(c, rep, rl.Name(), msg); ok { changed++ keys = append(keys, fk...) continue diff --git a/components/offload/skeleton.go b/components/offload/skeleton.go index fae1ed10..c26031fe 100644 --- a/components/offload/skeleton.go +++ b/components/offload/skeleton.go @@ -148,7 +148,7 @@ func (s *Skeleton) Offload(req *schemas.BifrostChatRequest, rep *components.Repo // Replay a frozen decision at ANY depth: the agent re-sends the original every // turn, so not re-eliding it would flip the message skeleton→full→skeleton and // churn the provider's KV cache. Same contract as mask/failed_run/readlifecycle. - if fk, _, ok := reapplyFrozen(c, s.Name(), m); ok { + if fk, _, ok := reapplyFrozen(c, rep, s.Name(), m); ok { emitted++ keys = append(keys, fk...) continue diff --git a/components/offload/state.go b/components/offload/state.go index cf968b91..5f8d50c0 100644 --- a/components/offload/state.go +++ b/components/offload/state.go @@ -200,7 +200,7 @@ func frozenLost(c *components.Ctx, key string) bool { // exists and still shrinks it. It also refreshes the expand originals for any markers // in the replacement (the agent re-sent the full original as m's content), so // restoration keeps working across turns. Returns the marker keys + whether it acted. -func reapplyFrozen(c *components.Ctx, comp string, m *bschemas.ChatMessage) ([]string, int, bool) { +func reapplyFrozen(c *components.Ctx, rep *components.Report, comp string, m *bschemas.ChatMessage) ([]string, int, bool) { content := schema.MessageText(*m) if isKeptVerbatim(c, contentKey(content)) { return nil, 0, false // agent expanded this; replaying the collapse would loop @@ -217,6 +217,26 @@ func reapplyFrozen(c *components.Ctx, comp string, m *bschemas.ChatMessage) ([]s return nil, 0, false } keys := expand.ParseMarkers(rs) + if len(keys) == 0 { + // A REPLAY WITH NO MARKERS IS A DEGRADED-MODE REPLAY, and it has to say so. + // + // Under marker_mode summary/off nothing was stashed, so the frozen replacement carries no + // <> to parse and this returns no cache keys — while still shrinking the message. + // components/pipeline.go reverts exactly that combination ("dropped content without + // stashing a cache_key") unless rep.Irreversible says the loss was chosen. + // + // The turn that MADE the decision sets it, through commitMark's non-full branch. Every + // later turn replays it through here and did not, so from turn 2 onward a summary-mode + // offloader had its whole component reverted and the transcript sent verbatim — a + // full-suffix cache write at ~11.5x the read price, on every turn, for the rest of the + // session. Measured on a two-turn mask fixture: turn 1 Irreversible=true, turn 2 + // Irreversible=false with the message rewritten and no keys returned. + // + // The blanket flag cannot mask a FULL-mode bug: every freeze() site is downstream of a + // tryMark/commitMark pair, so a full-mode frozen replacement always carries a marker and + // len(keys) == 0 implies a degraded mode. + rep.Irreversible = true + } for _, k := range keys { c.Store.Put(k, []byte(content)) // refresh the stashed original for expand } diff --git a/components/offload/summarymodereplay_test.go b/components/offload/summarymodereplay_test.go new file mode 100644 index 00000000..8f34878a --- /dev/null +++ b/components/offload/summarymodereplay_test.go @@ -0,0 +1,86 @@ +package offload + +import ( + "context" + "strings" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/schema" + "github.com/rossoctl/context-guru/store" +) + +// THE DEFECT +// +// Under marker_mode summary/off an offloader takes a deliberate LOSSY drop: nothing is stashed and +// no <> is written, so the component returns no cache keys. components/pipeline.go treats +// that combination — the request shrank, no cache keys, not Skipped — as a contract violation and +// REVERTS the component, unless rep.Irreversible says the loss was chosen. +// +// commitMark's non-full branch sets that flag, so the turn that TAKES the decision is fine. Every +// later turn replays the frozen decision through reapplyFrozen, which never set it. So from turn 2 +// onward, for the whole session: +// +// turn 1: keys=[] Irreversible=true -> kept +// turn 2: keys=[] Irreversible=false -> REVERTED, transcript sent verbatim +// +// A revert is not merely a lost saving. Earlier turns sent the reduced bytes, so sending the +// original re-writes the provider's whole cached suffix at ~11.5x the read price — and it happens +// on every turn, for every message the component had reduced. The component reports itself as +// working: it acts, it computes a replacement, and the pipeline throws it away afterwards. +// +// Found while auditing replay paths for a related review; the defect is independent of that work. +func TestASummaryModeReplayIsNotRevertedFromTurnTwoOnward(t *testing.T) { + body := strings.Repeat("a line of log output that goes on for a while\n", 60) + st := store.NewMemory(store.Options{}) + c, err := newMask([]byte("keep_recent: 0\nmin_tokens: 20\nmarker_mode: summary\n")) + if err != nil { + t.Fatal(err) + } + comp := c.(components.Offload) + ctx := &components.Ctx{Ctx: context.Background(), Session: "s", Store: st, MaxCachedIdx: -1} + req := func() *bschemas.BifrostChatRequest { + m := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleTool} + schema.SetMessageText(&m, body) + return &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{m}} + } + + // Turn 1 takes the decision. This turn was never broken — commitMark sets the flag — and it is + // asserted so a fixture that stopped reaching summary mode fails here rather than passing below. + r1 := components.Report{Kind: "offload"} + if _, err := comp.Offload(req(), &r1, ctx); err != nil { + t.Fatal(err) + } + if !r1.Irreversible || len(r1.CacheKeys) != 0 { + t.Fatalf("turn 1: Irreversible=%v keys=%v, want true/none — the fixture is not in a "+ + "degraded marker mode, so turn 2 is not the case under test", + r1.Irreversible, r1.CacheKeys) + } + + // Turn 2 replays it, through reapplyFrozen. + req2 := req() + r2 := components.Report{Kind: "offload"} + if _, err := comp.Offload(req2, &r2, ctx); err != nil { + t.Fatal(err) + } + if got := schema.MessageText(req2.Input[0]); got == body { + t.Fatalf("turn 2 did not replay the frozen decision, so the revert precondition is not " + + "reachable and this assertion would pass vacuously") + } + if r2.Skipped { + t.Fatal("turn 2 reported Skipped, so the pipeline would not revert it and this test " + + "proves nothing") + } + if len(r2.CacheKeys) != 0 { + t.Fatalf("turn 2 returned cache keys (%v); in summary mode nothing is stashed, so the "+ + "fixture is not exercising the degraded path", r2.CacheKeys) + } + if !r2.Irreversible { + t.Error("a replayed summary-mode decision rewrote the message, returned NO cache key and " + + "did not set rep.Irreversible. That is exactly what components/pipeline.go reverts as " + + "\"offload dropped content without stashing a cache_key\", so from turn 2 onward the " + + "whole component is discarded and the transcript goes upstream verbatim — flipping " + + "content earlier turns had already sent reduced, at ~11.5x the read price, every turn") + } +}