diff --git a/apply/apply.go b/apply/apply.go index bc23d27..14ce934 100644 --- a/apply/apply.go +++ b/apply/apply.go @@ -132,11 +132,18 @@ func BodyFull(ctx context.Context, pipe *components.Pipeline, st store.Store, pr // Volatile-tail split, before anything else touches the body. This is a // body-level concern rather than a component one: the pipeline operates on // `messages`, but the block that needs splitting lives in the top-level `system` - // array, which components never see. Gated on cacheinject being configured, so it - // is opt-in via the same pipeline entry and adds no new config surface. See - // prefixsplit.go for what it splits and why. + // array, which components never see. See prefixsplit.go for what it splits and why. + // + // Gated on `cachesplit` OR `cacheinject`. The two were coupled only because the split + // had to hang off some existing config entry, but they are independent mechanisms + // with very different evidence: the split is measured (−34.1% cost, 0% → 96.7% hit in + // an isolated A/B), while breakpoint PLACEMENT has never been measured — so #32 drops + // cacheinject from the default presets and puts `cachesplit` there instead. Without + // its own gate the split would have gone down with cacheinject, turning "disable an + // unproven component" into a real cost regression. It stays opt-in rather than + // unconditional so `off` remains a true passthrough control for A/B runs. systemSplit := false - if !bypass && pipe != nil && pipe.Has("cacheinject") { + if !bypass && pipe != nil && (pipe.Has("cachesplit") || pipe.Has("cacheinject")) { body, systemSplit = splitVolatileTail(body, provider) } @@ -170,6 +177,10 @@ func BodyFull(ctx context.Context, pipe *components.Pipeline, st store.Store, pr CtxWindow: window, CacheAware: cacheAware, MaxCachedIdx: maxCachedIdx, + // Every breakpoint already on the wire — including the ones no component can + // see (`system`, `tools`, and the marks our own normalize drops). The + // provider's cap of four counts them all (issue #32, defect 2). + ExistingBreakpoints: wireBreakpoints(body), } // Canonical form of each normalized message BEFORE the pipeline, so a @@ -179,7 +190,7 @@ func BodyFull(ctx context.Context, pipe *components.Pipeline, st store.Store, pr normPre[i], _ = json.Marshal(norm[i]) } - pipe.Run(chat, c) + rr := pipe.Run(chat, c) // A component changed the message count (summarize restructures the transcript // to [msg0, , last-K]). Rebuild the messages array preserving each @@ -198,6 +209,9 @@ func BodyFull(ctx context.Context, pipe *components.Pipeline, st store.Store, pr // if no component changes a message. changed := systemSplit var changes []change + // Per-message count of changes this writeback threw away, attributed back to the + // components that made them once the loop is done. + discarded := map[int]int{} for i := range chat.Input { s := slots[i] switch s.kind { @@ -222,8 +236,20 @@ func BodyFull(ctx context.Context, pipe *components.Pipeline, st store.Store, pr } if !s.lossless { // bifrost can't round-trip this message; splicing our re-marshal would - // drop provider fields it doesn't model. Discard the change, keep the - // original bytes. ponytail: correctness over the marginal saving here. + // drop provider fields it doesn't model. But if the ONLY change is added + // `cache_control` — metadata, not content — write those keys at their exact + // paths on the original raw bytes: nothing else is read or rewritten, so no + // provider field can be dropped. See metawrite.go (issue #32). + if w, ok := metadataOnlyWrites(s.pre, post); ok { + if nb, ok := applyMetaWrites(out, s.path, len(chat.Input[i].Content.ContentBlocks), w); ok { + out = nb + changed = true + continue + } + } + // Anything else: discard the change, keep the original bytes. + // ponytail: correctness over the marginal saving here. + discarded[i]++ continue } if out, err = sjson.SetRawBytes(out, s.path, post); err != nil { @@ -235,9 +261,19 @@ func BodyFull(ctx context.Context, pipe *components.Pipeline, st store.Store, pr changes = append(changes, mkChange(s.path, schema.MessageText(pm), schema.MessageText(chat.Input[i]))) } } + pipe.RecordDiscards(rr, discarded) if changed && dumpPath != "" { dumpChanges(c.Session, changes) } + // A cap breach WE caused is a bug and must be loud. A request that arrived already + // over the cap is the client's to fix — we forward it as-is (fail open), and an ERROR + // blaming context-guru for it would be a false alarm. So compare against the inbound + // count and only shout when we added to an over-cap total. + if n := wireBreakpoints(out); n > maxWireBreakpoints && n > c.ExistingBreakpoints { + slog.Error("context-guru: cache breakpoint count exceeds the provider cap", + "breakpoints", n, "inbound", c.ExistingBreakpoints, + "cap", maxWireBreakpoints, "session", c.Session) + } return out, changed } diff --git a/apply/apply_test.go b/apply/apply_test.go index d63a903..6c143a8 100644 --- a/apply/apply_test.go +++ b/apply/apply_test.go @@ -3,6 +3,8 @@ package apply_test import ( "context" "encoding/json" + "errors" + "reflect" "strings" "testing" @@ -11,8 +13,10 @@ import ( "github.com/rossoctl/context-guru/components" _ "github.com/rossoctl/context-guru/components/all" "github.com/rossoctl/context-guru/config" + "github.com/rossoctl/context-guru/metrics" "github.com/rossoctl/context-guru/store" "github.com/tidwall/gjson" + "github.com/tidwall/sjson" ) func pipe(t *testing.T, yaml string) *config.Config { @@ -266,3 +270,291 @@ func TestNoMessagesForwardsUnchanged(t *testing.T) { t.Fatalf("no messages array => forward unchanged; got changed=%v %s", changed, out) } } + +// TestCacheinjectReachesTheWire is the #32 regression: cacheinject's only possible +// targets on Claude Code traffic are assistant messages carrying `tool_use`, which +// bifrost cannot round-trip — so every mark used to be discarded by the writeback +// loop (46 applied, 0 forwarded, measured over 40 real requests). cache_control is +// metadata, so it is written onto the ORIGINAL raw bytes and the unmodellable +// provider fields must survive verbatim. +func TestCacheinjectReachesTheWire(t *testing.T) { + cfg := pipe(t, "pipeline: [cacheinject]\n") + p, _ := cfg.Build(nil) + st := store.NewMemory(store.Options{}) + + body := []byte(`{"model":"claude-x","messages":[ + {"role":"user","content":"run the tool"}, + {"role":"assistant","content":[{"type":"text","text":"on it"},{"type":"tool_use","id":"toolu_abc","name":"Bash","input":{"command":"ls -la"}}]} + ]}`) + + out, changed := apply.Body(context.Background(), p, st, bschemas.Anthropic, body, "", false) + if !changed { + t.Fatal("expected cacheinject's breakpoint to change the body") + } + cc := gjson.GetBytes(out, "messages.1.content.1.cache_control") + if !cc.Exists() || cc.Get("type").String() != "ephemeral" { + t.Fatalf("breakpoint never reached the wire: %s", out) + } + // Provider fields bifrost drops on unmarshal must be intact. + blk := gjson.GetBytes(out, "messages.1.content.1") + if blk.Get("id").String() != "toolu_abc" || blk.Get("name").String() != "Bash" || + blk.Get("input.command").String() != "ls -la" { + t.Fatalf("tool_use provider fields corrupted: %s", blk.Raw) + } + if gjson.GetBytes(out, "messages.1.content.0.text").String() != "on it" { + t.Fatalf("sibling text block corrupted: %s", out) + } + // Everything except the added cache_control is byte-identical. + stripped, err := sjson.DeleteBytes(out, "messages.1.content.1.cache_control") + if err != nil { + t.Fatal(err) + } + if !jsonEq(t, stripped, body) { + t.Fatalf("body changed beyond the metadata write:\n old=%s\n new=%s", body, stripped) + } +} + +func jsonEq(t *testing.T, a, b []byte) bool { + t.Helper() + var av, bv any + if err := json.Unmarshal(a, &av); err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(b, &bv); err != nil { + t.Fatal(err) + } + return reflect.DeepEqual(av, bv) +} + +// TestWireBreakpointCapRealTrafficShape uses the exact shape 1,771 of 1,794 +// captured Claude Code requests carry — system=2, tools=0, messages=1 — on a long +// conversation, and asserts the total the PROVIDER sees never exceeds 4. Before #32 +// the component counted only the messages array, saw 1 existing breakpoint, and +// budgeted 3 more: 6 on the wire, which the provider rejects with a 400. +func TestWireBreakpointCapRealTrafficShape(t *testing.T) { + cfg := pipe(t, "pipeline: [cacheinject]\n") + p, _ := cfg.Build(nil) + + msgs := make([]any, 0, 60) + for i := 0; i < 60; i++ { + role, blk := "user", map[string]any{"type": "text", "text": strings.Repeat("turn ", i%7+1)} + if i%2 == 1 { + role = "assistant" + } + m := map[string]any{"role": role, "content": []any{blk}} + if i == 59 { // the caller's own trailing breakpoint + blk["cache_control"] = map[string]any{"type": "ephemeral"} + } + msgs = append(msgs, m) + } + body, _ := json.Marshal(map[string]any{ + "model": "claude-x", + "system": []any{ + map[string]any{"type": "text", "text": "tools preamble", "cache_control": map[string]any{"type": "ephemeral"}}, + map[string]any{"type": "text", "text": "main system prompt", "cache_control": map[string]any{"type": "ephemeral"}}, + }, + "messages": msgs, + }) + + out, _ := apply.Body(context.Background(), p, store.NewMemory(store.Options{}), bschemas.Anthropic, body, "", false) + if n := countWireBreakpoints(t, out); n > 4 { + t.Fatalf("%d breakpoints on the wire — the provider caps at 4 and 400s above it", n) + } +} + +// countWireBreakpoints counts cache_control across system, tools and messages the +// way the provider does — deliberately re-derived in the test rather than reusing +// the implementation's counter, so a bug in that counter cannot hide the cap breach. +func countWireBreakpoints(t *testing.T, body []byte) int { + t.Helper() + var req struct { + System []map[string]json.RawMessage `json:"system"` + Tools []map[string]json.RawMessage `json:"tools"` + Msgs []struct { + CacheControl json.RawMessage `json:"cache_control"` + Content []map[string]json.RawMessage `json:"content"` + } `json:"messages"` + } + if err := json.Unmarshal(body, &req); err != nil { + t.Fatalf("output is not valid JSON: %v", err) + } + n := 0 + count := func(blocks []map[string]json.RawMessage) { + for _, b := range blocks { + if _, ok := b["cache_control"]; ok { + n++ + } + } + } + count(req.System) + count(req.Tools) + for _, m := range req.Msgs { + if len(m.CacheControl) > 0 { + n++ + } + count(m.Content) + } + return n +} + +// contentRewriter is a test-only component that rewrites an assistant message's +// text — a CONTENT change on a message bifrost cannot round-trip, so the writeback +// layer must discard it. No shipped component targets assistant turns, so the +// discard path needs one to exercise it. +type contentRewriter struct{} + +func (contentRewriter) Name() string { return "testrewrite" } +func (contentRewriter) Enabled(*components.Ctx) bool { return true } +func (contentRewriter) Reformat(req *bschemas.BifrostChatRequest, _ *components.Report, _ *components.Ctx) error { + for i := range req.Input { + if req.Input[i].Role != bschemas.ChatMessageRoleAssistant || req.Input[i].Content == nil { + continue + } + for b := range req.Input[i].Content.ContentBlocks { + if t := req.Input[i].Content.ContentBlocks[b].Text; t != nil { + short := "shortened" + req.Input[i].Content.ContentBlocks[b].Text = &short + } + } + } + return nil +} + +func init() { + components.Register("testrewrite", func([]byte) (components.Component, error) { + return contentRewriter{}, nil + }) +} + +// TestDiscardedChangeIsCounted: a change the writeback layer throws away must be +// attributed to the component that made it, not silently vanish. A component that +// mutates and is then discarded used to look exactly like one that works — which is +// how #32 survived two benchmark studies. +func TestDiscardedChangeIsCounted(t *testing.T) { + cfg := pipe(t, "pipeline: [testrewrite]\n") + agg := metrics.NewAggregator() + p, _ := cfg.Build(agg) + + body := []byte(`{"model":"claude-x","messages":[ + {"role":"user","content":"go"}, + {"role":"assistant","content":[{"type":"text","text":"a long narration that the component will rewrite"},{"type":"tool_use","id":"toolu_z","name":"Bash","input":{"command":"ls"}}]} + ]}`) + + out, _ := apply.Body(context.Background(), p, store.NewMemory(store.Options{}), bschemas.Anthropic, body, "", false) + if gjson.GetBytes(out, "messages.1").Raw != gjson.GetBytes(body, "messages.1").Raw { + t.Fatalf("the unmodellable message must be kept verbatim:\n old=%s\n new=%s", + gjson.GetBytes(body, "messages.1").Raw, gjson.GetBytes(out, "messages.1").Raw) + } + snap := agg.Snapshot() + if got := snap.Components["testrewrite"].Discarded; got == 0 { + t.Fatalf("discarded change not counted: %+v", snap.Components["testrewrite"]) + } + if len(snap.TopDiscarded) == 0 || snap.TopDiscarded[0] != "testrewrite" { + t.Fatalf("top_discarded should name the component, got %v", snap.TopDiscarded) + } + // A Discarded report must not inflate Runs (it is attribution, not a second run). + if r := snap.Components["testrewrite"].Runs; r != 1 { + t.Fatalf("Runs should stay 1, got %d", r) + } +} + +// reverter always errors, so the pipeline rolls its change back. A reverted component +// must NOT be charged a discard: its change never reached the writeback layer at all. +type reverter struct{} + +func (reverter) Name() string { return "testrevert" } +func (reverter) Enabled(*components.Ctx) bool { return true } +func (reverter) Reformat(req *bschemas.BifrostChatRequest, _ *components.Report, _ *components.Ctx) error { + for i := range req.Input { + if req.Input[i].Role == bschemas.ChatMessageRoleAssistant && req.Input[i].Content != nil { + for b := range req.Input[i].Content.ContentBlocks { + if req.Input[i].Content.ContentBlocks[b].Text != nil { + s := "mutated then reverted" + req.Input[i].Content.ContentBlocks[b].Text = &s + } + } + } + } + return errors.New("deliberate failure so the pipeline reverts") +} + +func init() { + components.Register("testrevert", func([]byte) (components.Component, error) { return reverter{}, nil }) +} + +// A rolled-back component must not appear as a discard. Otherwise the counter meant to +// catch #32-class bugs becomes a false-positive generator. +func TestRevertedComponentNotChargedDiscard(t *testing.T) { + // testrewrite runs AFTER and produces the discard, so the writeback layer really does + // throw a change away at that index. Pre-fix, testrevert's ChangedIdx was recorded + // before the rollback, so it got charged for a discard caused by the other component. + cfg := pipe(t, "pipeline: [testrewrite, testrevert]\n") + agg := metrics.NewAggregator() + p, _ := cfg.Build(agg) + + body := []byte(`{"model":"claude-x","messages":[ + {"role":"user","content":"go"}, + {"role":"assistant","content":[{"type":"text","text":"narration"},{"type":"tool_use","id":"toolu_r","name":"Bash","input":{}}]} + ]}`) + apply.Body(context.Background(), p, store.NewMemory(store.Options{}), bschemas.Anthropic, body, "", false) + + cs := agg.Snapshot().Components["testrevert"] + if cs.Reverted != 1 { + t.Fatalf("expected the component to be reverted, got %+v", cs) + } + if cs.Discarded != 0 { + t.Fatalf("reverted component charged %d discards — its change never reached writeback", cs.Discarded) + } +} + +// One discarded message is charged to exactly ONE component, not to every component +// that touched it. Two components rewrite the same unmodellable message; only the last +// one's change is what the writeback layer actually threw away. +func TestDiscardChargedOnceNotPerToucher(t *testing.T) { + cfg := pipe(t, "pipeline: [testrewrite, testrewrite2]\n") + agg := metrics.NewAggregator() + p, _ := cfg.Build(agg) + + body := []byte(`{"model":"claude-x","messages":[ + {"role":"user","content":"go"}, + {"role":"assistant","content":[{"type":"text","text":"a long narration both components rewrite"},{"type":"tool_use","id":"toolu_d","name":"Bash","input":{}}]} + ]}`) + apply.Body(context.Background(), p, store.NewMemory(store.Options{}), bschemas.Anthropic, body, "", false) + + snap := agg.Snapshot() + total := snap.Components["testrewrite"].Discarded + snap.Components["testrewrite2"].Discarded + if total != 1 { + t.Fatalf("one discarded message charged %d times (testrewrite=%d testrewrite2=%d)", + total, snap.Components["testrewrite"].Discarded, snap.Components["testrewrite2"].Discarded) + } + // It must land on the LAST toucher — its change is the discarded state. + if snap.Components["testrewrite2"].Discarded != 1 { + t.Fatalf("discard should be charged to the last component to change the message, got %v", snap.TopDiscarded) + } +} + +// contentRewriter2 is a second rewriter so two components touch one message. +type contentRewriter2 struct{} + +func (contentRewriter2) Name() string { return "testrewrite2" } +func (contentRewriter2) Enabled(*components.Ctx) bool { return true } +func (contentRewriter2) Reformat(req *bschemas.BifrostChatRequest, _ *components.Report, _ *components.Ctx) error { + for i := range req.Input { + if req.Input[i].Role != bschemas.ChatMessageRoleAssistant || req.Input[i].Content == nil { + continue + } + for b := range req.Input[i].Content.ContentBlocks { + if req.Input[i].Content.ContentBlocks[b].Text != nil { + s := "even shorter" + req.Input[i].Content.ContentBlocks[b].Text = &s + } + } + } + return nil +} + +func init() { + components.Register("testrewrite2", func([]byte) (components.Component, error) { + return contentRewriter2{}, nil + }) +} diff --git a/apply/metawrite.go b/apply/metawrite.go new file mode 100644 index 0000000..37b5386 --- /dev/null +++ b/apply/metawrite.go @@ -0,0 +1,148 @@ +package apply + +import ( + "strconv" + + "github.com/tidwall/gjson" + "github.com/tidwall/sjson" +) + +// Metadata writes: the narrow exception to the losslessness guard. +// +// bifrost cannot round-trip an Anthropic assistant turn that carries a `tool_use` +// block — it drops `id`, `name` and `input` on unmarshal — so the writeback loop +// discards any change to such a message rather than splice a lossy re-marshal. +// That is correct for CONTENT. But cacheinject's only possible targets on real +// agent traffic are exactly those messages, so every breakpoint it placed was +// thrown away before the request was sent (measured: 46 applied, 0 forwarded over +// 40 captured requests — issue #32). +// +// `cache_control` is metadata, not content: it changes nothing the model reads and +// needs no message model to express. So when a component's ONLY change to a +// message is adding `cache_control` to content blocks, we write those keys at their +// exact paths on the ORIGINAL raw bytes instead of splicing a re-marshal. Nothing +// else in the message is read or rewritten, so no provider field can be dropped +// and the guard's reason to exist does not arise. +// +// Anything beyond added `cache_control` keys — a text edit, a removed key, a +// changed block — is still discarded. + +// metaWrite is one metadata key to set, as a path relative to the message and the +// raw JSON to write there. +type metaWrite struct { + path string // "content..cache_control" + raw string +} + +// metadataOnlyWrites compares a message's pre- and post-pipeline canonical +// marshals and returns the metadata writes that reproduce the change, or ok=false +// if the change is anything other than added content-block `cache_control` keys. +func metadataOnlyWrites(pre, post []byte) (writes []metaWrite, ok bool) { + preBlocks := gjson.GetBytes(pre, "content") + postBlocks := gjson.GetBytes(post, "content") + if !preBlocks.IsArray() || !postBlocks.IsArray() { + return nil, false // string content carries no block to mark + } + pb, qb := preBlocks.Array(), postBlocks.Array() + if len(pb) != len(qb) { + return nil, false // blocks added/removed — not a metadata change + } + stripped := post + for b := range qb { + p := "content." + strconv.Itoa(b) + ".cache_control" + cc := qb[b].Get("cache_control") + if !cc.Exists() || pb[b].Get("cache_control").Exists() { + continue + } + writes = append(writes, metaWrite{path: p, raw: cc.Raw}) + var err error + if stripped, err = sjson.DeleteBytes(stripped, p); err != nil { + return nil, false + } + } + if len(writes) == 0 { + return nil, false + } + // The decisive check: with the added keys removed, the post-pipeline message must + // be identical to the pre-pipeline one. Otherwise something else changed too. + if !jsonEqual(stripped, pre) { + return nil, false + } + return writes, true +} + +// applyMetaWrites sets each metadata key on the raw body at msgPath. It refuses +// (ok=false, body untouched) if the raw message's block layout does not match what +// the writes assume, so a shape the normalizer and the raw body disagree about can +// never be written to the wrong block. +func applyMetaWrites(body []byte, msgPath string, nBlocks int, writes []metaWrite) ([]byte, bool) { + blocks := gjson.GetBytes(body, msgPath+".content") + if !blocks.IsArray() || len(blocks.Array()) != nBlocks { + return body, false + } + out := body + for _, w := range writes { + full := msgPath + "." + w.path + if gjson.GetBytes(out, full).Exists() { + return body, false // caller already set it — never overwrite + } + next, err := sjson.SetRawBytes(out, full, []byte(w.raw)) + if err != nil { + return body, false + } + out = next + } + return out, true +} + +// maxWireBreakpoints is the provider's hard cap on cache_control directives per +// request. Over it, the request 400s. +const maxWireBreakpoints = 4 + +// breakpointPaths are every location a real prompt-cache breakpoint can live. The +// cap applies across all of them together. Structural (gjson path queries) for the +// same reason hasCacheBreakpoint is: a tool output whose text merely contains the +// string "cache_control" must not count. +// +// `cachePoint` is the Bedrock Converse spelling, and Bedrock places it as its OWN +// entry in the `system` and `tools` arrays — the two locations defect 2 is about. Those +// paths must be counted or the cap stays breachable on Bedrock exactly as it was on +// Anthropic (constructed: 6 on the wire, counter blind to 3 of them). +var breakpointPaths = []string{ + "system.#.cache_control", + "tools.#.cache_control", + "messages.#.cache_control", + "messages.#.content.#.cache_control", + "system.#.cachePoint", + "tools.#.cachePoint", + "messages.#.content.#.cachePoint", +} + +// wireBreakpoints counts every breakpoint the provider will see in this request. +// +// A component cannot count these for itself. `system` and `tools` never reach it at +// all, and the `tool_result` blocks this package normalizes into synthetic role=tool +// messages lose their mark on the way in (see toolMessage) — so on real Claude Code +// traffic all three of the agent's own breakpoints were invisible to it (2 in +// `system`, 1 on a `tool_result` block), and it computed 3 free slots when 1 was free: +// 6 on the wire, and a 400 (issue #32). +func wireBreakpoints(body []byte) int { + n := 0 + for _, p := range breakpointPaths { + gjson.GetBytes(body, p).ForEach(func(_, v gjson.Result) bool { + // nested arrays (content-of-messages) surface as arrays here; recurse one level + if v.IsArray() { + v.ForEach(func(_, vv gjson.Result) bool { + if vv.IsObject() { + n++ + } + return true + }) + } else if v.IsObject() { + n++ + } + return true + }) + } + return n +} diff --git a/apply/metawrite_test.go b/apply/metawrite_test.go new file mode 100644 index 0000000..67e8b6f --- /dev/null +++ b/apply/metawrite_test.go @@ -0,0 +1,103 @@ +package apply + +import ( + "testing" + + "github.com/tidwall/sjson" +) + +// The metadata-write exception must be exactly that: added cache_control keys and +// nothing else. Anything wider would put us back to splicing a lossy re-marshal +// over a message bifrost cannot round-trip. +func TestMetadataOnlyWritesRejectsNonMetadataChanges(t *testing.T) { + pre := `{"role":"assistant","content":[{"type":"text","text":"hello"},{"type":"tool_use"}]}` + cases := []struct { + name, post string + want bool + }{ + {"added cache_control", `{"role":"assistant","content":[{"type":"text","text":"hello"},{"type":"tool_use","cache_control":{"type":"ephemeral"}}]}`, true}, + {"two added", `{"role":"assistant","content":[{"type":"text","text":"hello","cache_control":{"type":"ephemeral"}},{"type":"tool_use","cache_control":{"type":"ephemeral"}}]}`, true}, + {"no change", pre, false}, + {"text edited too", `{"role":"assistant","content":[{"type":"text","text":"HELLO"},{"type":"tool_use","cache_control":{"type":"ephemeral"}}]}`, false}, + {"block removed", `{"role":"assistant","content":[{"type":"tool_use","cache_control":{"type":"ephemeral"}}]}`, false}, + {"role changed", `{"role":"user","content":[{"type":"text","text":"hello"},{"type":"tool_use","cache_control":{"type":"ephemeral"}}]}`, false}, + {"string content", `{"role":"assistant","content":"hello"}`, false}, + } + for _, tc := range cases { + if _, ok := metadataOnlyWrites([]byte(pre), []byte(tc.post)); ok != tc.want { + t.Errorf("%s: ok=%v want %v", tc.name, ok, tc.want) + } + } +} + +// applyMetaWrites must refuse when the raw body's block layout disagrees with what +// the writes assume, so a key can never land on the wrong block. +func TestApplyMetaWritesRefusesOnShapeMismatch(t *testing.T) { + body := []byte(`{"messages":[{"role":"assistant","content":[{"type":"tool_use","id":"t"}]}]}`) + w := []metaWrite{{path: "content.1.cache_control", raw: `{"type":"ephemeral"}`}} + if _, ok := applyMetaWrites(body, "messages.0", 2, w); ok { + t.Fatal("expected a refusal: the raw message has 1 block, the writes assume 2") + } + // Never overwrite a breakpoint the caller already set. + set := []byte(`{"messages":[{"role":"assistant","content":[{"type":"tool_use","cache_control":{"type":"ephemeral","ttl":"1h"}}]}]}`) + w = []metaWrite{{path: "content.0.cache_control", raw: `{"type":"ephemeral"}`}} + if _, ok := applyMetaWrites(set, "messages.0", 1, w); ok { + t.Fatal("expected a refusal: the caller's own cache_control must not be overwritten") + } +} + +// Bedrock spells the breakpoint `cachePoint` and puts it as its OWN entry in `system` +// and `tools` — the two locations defect 2 is about. Missing those paths left the cap +// breachable on Bedrock exactly as it was on Anthropic. +func TestBedrockCachePointCounted(t *testing.T) { + body := []byte(`{ + "system":[{"text":"a"},{"cachePoint":{"type":"default"}},{"text":"b"},{"cachePoint":{"type":"default"}}], + "tools":[{"toolSpec":{"name":"x"}},{"cachePoint":{"type":"default"}}], + "messages":[{"role":"user","content":[{"text":"hi"},{"cachePoint":{"type":"default"}}]}]}`) + // 2 in system + 1 in tools + 1 in message content = 4, all of them at the cap. The + // pre-fix counter saw only the content one, so it believed 3 slots were free. + if got := wireBreakpoints(body); got != 4 { + t.Fatalf("wireBreakpoints = %d, want 4 (2 system + 1 tool + 1 content); a Bedrock cap breach would go unseen", got) + } +} + +func TestBreakpointCounting(t *testing.T) { + body := []byte(`{ + "system":[{"type":"text","cache_control":{"type":"ephemeral"}},{"type":"text","cache_control":{"type":"ephemeral"}},{"type":"text"}], + "tools":[{"name":"x"},{"name":"y","cache_control":{"type":"ephemeral"}}], + "messages":[ + {"role":"user","content":"a tool output that merely mentions cache_control in its text"}, + {"role":"assistant","content":[{"type":"tool_use","cache_control":{"type":"ephemeral"}}]}, + {"role":"user","cache_control":{"type":"ephemeral"},"content":"q"} + ]}`) + if got := wireBreakpoints(body); got != 5 { + t.Errorf("wireBreakpoints = %d, want 5", got) + } +} + +// A request that arrives already over the cap is the client's problem — we forward it +// untouched. Shouting ERROR about it would blame context-guru for a request it never +// changed, so the breach check compares against the inbound count. Asserted at the +// condition level: the log call itself is not observable from here. +func TestBreachIsOursOnly(t *testing.T) { + over := []byte(`{"system":[ + {"type":"text","cache_control":{"type":"ephemeral"}},{"type":"text","cache_control":{"type":"ephemeral"}}, + {"type":"text","cache_control":{"type":"ephemeral"}},{"type":"text","cache_control":{"type":"ephemeral"}}, + {"type":"text","cache_control":{"type":"ephemeral"}}],"messages":[{"role":"user","content":"hi"}]}`) + inbound := wireBreakpoints(over) + if inbound != 5 { + t.Fatalf("fixture should carry 5 inbound breakpoints, got %d", inbound) + } + // Unchanged body: over the cap, but out == inbound, so it is not ours to report. + if out := wireBreakpoints(over); out > maxWireBreakpoints && out > inbound { + t.Fatal("a client-caused breach we did not add to must not be reported as ours") + } + // If we DID add one, it must be reported. + worse, err := sjson.SetRawBytes(over, "messages.0.cache_control", []byte(`{"type":"ephemeral"}`)) + if err != nil { + t.Fatal(err) + } + if out := wireBreakpoints(worse); !(out > maxWireBreakpoints && out > inbound) { + t.Fatalf("we added a breakpoint over the cap (%d -> %d); that must be reported", inbound, out) + } +} diff --git a/apply/prefixsplit_test.go b/apply/prefixsplit_test.go index 1982a69..80072c6 100644 --- a/apply/prefixsplit_test.go +++ b/apply/prefixsplit_test.go @@ -196,13 +196,32 @@ func TestSplitAppliedThroughBodyFull(t *testing.T) { } } -// Gated on cacheinject: a pipeline without it must leave the body byte-identical. -func TestSplitGatedOnCacheinject(t *testing.T) { +// Gated: a pipeline with neither cachesplit nor cacheinject must leave the body +// byte-identical, so `off` stays a true passthrough control for A/B runs. +func TestSplitGatedOnConfig(t *testing.T) { full, _, _ := blockWithGitTail(6000) in := sysBody(textBlock(full, true)) got, changed := runBody(t, pipeWith(t, "pipeline: [format]\n"), in, false) if changed || string(got) != string(in) { - t.Fatal("split fired without cacheinject configured") + t.Fatal("split fired with neither cachesplit nor cacheinject configured") + } +} + +// `cachesplit` alone enables the split. This is what keeps #32's preset change (dropping +// the unmeasured cacheinject) from silently disabling the measured split along with it. +func TestSplitEnabledByCachesplitAlone(t *testing.T) { + full, _, _ := blockWithGitTail(6000) + in := sysBody(textBlock(full, true)) + got, changed := runBody(t, pipeWith(t, "pipeline: [cachesplit]\n"), in, false) + if !changed { + t.Fatal("cachesplit did not enable the volatile-tail split") + } + if n := gjson.GetBytes(got, "system.#").Int(); n != 2 { + t.Fatalf("expected the system block to split into 2, got %d", n) + } + // cachesplit must add NO breakpoint of its own — it is not a placement policy. + if before, after := wireBreakpoints(in), wireBreakpoints(got); after != before { + t.Fatalf("cachesplit changed the breakpoint count %d -> %d", before, after) } } diff --git a/apply/replay_capture_test.go b/apply/replay_capture_test.go new file mode 100644 index 0000000..6de30e5 --- /dev/null +++ b/apply/replay_capture_test.go @@ -0,0 +1,118 @@ +package apply_test + +import ( + "bufio" + "context" + "encoding/json" + "os" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + "github.com/rossoctl/context-guru/apply" + "github.com/rossoctl/context-guru/store" + "github.com/tidwall/gjson" +) + +// TestReplayCapturedTrafficBreakpointsReachTheWire replays real captured Claude +// Code requests (a CONTEXT_GURU_CAPTURE jsonl: {provider, model, body}) through the +// pipeline and asserts the three things #32 is about: +// +// 1. cacheinject's breakpoints appear on the wire (before the fix: 0 of 46 did); +// 2. the total never exceeds the provider's cap of 4; +// 3. the output stays valid JSON with every tool_use provider field intact. +// +// Skipped unless CONTEXT_GURU_CAPTURE names a readable capture, so CI does not need +// the fixture. Run with: +// +// CONTEXT_GURU_CAPTURE=/path/to/capture.jsonl go test ./apply/ -run ReplayCaptured +func TestReplayCapturedTrafficBreakpointsReachTheWire(t *testing.T) { + path := os.Getenv("CONTEXT_GURU_CAPTURE") + if path == "" { + t.Skip("set CONTEXT_GURU_CAPTURE to a captured-traffic jsonl to run this") + } + f, err := os.Open(path) + if err != nil { + t.Skipf("cannot read capture %q: %v", path, err) + } + defer f.Close() + + cfg := pipe(t, "pipeline: [cacheinject]\n") + p, _ := cfg.Build(nil) + st := store.NewMemory(store.Options{}) + + sc := bufio.NewScanner(f) + sc.Buffer(make([]byte, 1<<20), 64<<20) + requests, withMarks := 0, 0 + for sc.Scan() { + var rec struct { + Provider string `json:"provider"` + Body json.RawMessage `json:"body"` + } + if json.Unmarshal(sc.Bytes(), &rec) != nil || len(rec.Body) == 0 { + continue + } + requests++ + before := wireMarks(rec.Body) + out, _ := apply.Body(context.Background(), p, st, + bschemas.ModelProvider(rec.Provider), rec.Body, "", false) + + if !gjson.ValidBytes(out) { + t.Fatalf("request %d: output is not valid JSON", requests) + } + after := wireMarks(out) + if after > 4 { + t.Fatalf("request %d: %d breakpoints on the wire, provider caps at 4", requests, after) + } + if after > before { + withMarks++ + } + // Every tool_use block must keep id/name/input — the fields bifrost drops. + gjson.GetBytes(out, `messages.#.content|@flatten`).ForEach(func(_, blk gjson.Result) bool { + if blk.Get("type").String() != "tool_use" { + return true + } + if !blk.Get("id").Exists() || !blk.Get("name").Exists() || !blk.Get("input").Exists() { + t.Fatalf("request %d: tool_use lost a provider field: %s", requests, blk.Raw) + } + return true + }) + } + if requests == 0 { + t.Skipf("capture %q held no usable requests", path) + } + // The bug: 46 breakpoints applied, 0 forwarded, across 40 real requests. On these + // captures the unfixed code reaches the wire on 1 of 19 requests (the one message + // bifrost happens to round-trip), so "nonzero" is not a gate — require a majority. + if withMarks*2 <= requests { + t.Fatalf("breakpoints reached the wire on only %d of %d captured requests — still suppressed", + withMarks, requests) + } + t.Logf("replayed %d captured requests; breakpoints reached the wire on %d", requests, withMarks) +} + +// wireMarks counts cache_control across system, tools and messages, independently of +// the implementation's own counter. +func wireMarks(body []byte) int { + n := 0 + for _, p := range []string{"system", "tools"} { + gjson.GetBytes(body, p).ForEach(func(_, v gjson.Result) bool { + if v.Get("cache_control").Exists() { + n++ + } + return true + }) + } + gjson.GetBytes(body, "messages").ForEach(func(_, m gjson.Result) bool { + if m.Get("cache_control").Exists() { + n++ + } + m.Get("content").ForEach(func(_, blk gjson.Result) bool { + if blk.Get("cache_control").Exists() { + n++ + } + return true + }) + return true + }) + return n +} diff --git a/components/component.go b/components/component.go index 34abf17..64b356c 100644 --- a/components/component.go +++ b/components/component.go @@ -123,6 +123,17 @@ type Ctx struct { // -1 = unknown/first turn/cache off ⇒ no tail restriction. Only meaningful when // CacheAware is true. MaxCachedIdx int + // ExistingBreakpoints is how many prompt-cache breakpoints the RAW request already + // carries, counted across `system`, `tools` and `messages` — which is what the + // provider's cap of four applies to. A component that spends breakpoint slots must + // budget against this number rather than what it can see in Input, for two reasons + // measured on real Claude Code traffic: 2 of its 3 breakpoints live in the + // top-level `system` array, which components never see at all, and the third sits + // on a `tool_result` block whose mark the host's own normalize step drops. Both + // were invisible, so the budget came out as 3 free slots when only 1 was free — + // enough to put 6 on the wire and take a 400 (issue #32). The host fills it from + // the raw body; 0 means "unknown, fall back to what you can see". + ExistingBreakpoints int } // TailOnly reports whether a supersession/age-based offloader may mutate the message @@ -150,6 +161,17 @@ type Report struct { Reverted bool // pipeline reverted it (error/panic/never-worse) Irreversible bool // Offload dropped content on purpose without stashing (marker_mode summary/off) Err error + // ChangedIdx are the req.Input indices this component modified, filled by the + // pipeline. The writeback layer uses them to attribute a discarded change back to + // the component that made it (see Pipeline.RecordDiscards). + ChangedIdx []int + // Discarded counts changes this component made that the WRITEBACK layer then threw + // away (bifrost could not round-trip the message, so splicing would have dropped + // provider fields). A report carrying Discarded > 0 is a follow-up attribution, not + // a fresh run — emitters must not count it as one. A component that mutates and is + // then silently discarded looked identical to one that works, which is how issue + // #32 survived two benchmark studies. + Discarded int } // Saved returns non-negative tokens saved by this component. diff --git a/components/pipeline.go b/components/pipeline.go index 7a1786d..33dad3b 100644 --- a/components/pipeline.go +++ b/components/pipeline.go @@ -2,6 +2,7 @@ package components import ( "fmt" + "reflect" "github.com/maximhq/bifrost/core/schemas" "github.com/rossoctl/context-guru/schema" @@ -116,6 +117,10 @@ func (p *Pipeline) runOne(comp Component, req *schemas.BifrostChatRequest, c *Ct rep.TokensAfter = rep.TokensBefore default: rep.TokensAfter = after + // Only a change that SURVIVED can be discarded by the writeback layer. Recording + // it in the revert branches above would charge a rolled-back component for a + // discard it never caused. + rep.ChangedIdx = changedIdx(before, req.Input) } return rep } @@ -124,6 +129,63 @@ func tokensOf(msgs []schemas.ChatMessage) int { return schema.MessagesTokens(&schemas.BifrostChatRequest{Input: msgs}) } +// changedIdx returns the indices at which a component's output differs from its +// input, so the writeback layer can attribute a discarded change to the component +// that made it. Count changes (summarize) yield nil — those go down the rebuild path, +// which never discards per-message. +// +// reflect.DeepEqual, not a marshal-and-compare: this runs per component per request +// purely for a diagnostic, so it must stay cheap. Measured on a realistic 80-message +// request, marshalling both sides cost 20.52 ms/op vs 16.56 ms (−19.3%) and 3,206 extra +// allocs. Struct equality is the same decision here — the writeback loop's own marshal +// is what actually decides whether to splice. +func changedIdx(before, after []schemas.ChatMessage) []int { + if len(before) != len(after) { + return nil + } + var out []int + for i := range after { + if !reflect.DeepEqual(before[i], after[i]) { + out = append(out, i) + } + } + return out +} + +// RecordDiscards emits one follow-up Report per component whose changes the +// writeback layer threw away, so a silently-suppressed component is visible in +// telemetry instead of looking like a working one. discarded maps req.Input index -> +// number of discarded changes at that index; hosts call this after the splice. +// +// One discarded message is charged to exactly ONE component: the LAST one that +// changed that index. Several components can touch the same message, but the +// writeback layer discards the final cumulative state, so that component's change is +// the one actually thrown away — charging every earlier toucher too would make this +// counter a false-positive generator, and its whole point is to be trustworthy enough +// to catch a #32-class bug. +func (p *Pipeline) RecordDiscards(rr *RunReport, discarded map[int]int) { + if p == nil || rr == nil || len(discarded) == 0 { + return + } + // owner[i] = index into rr.Components of the last component that changed message i. + owner := map[int]int{} + for c := range rr.Components { + for _, i := range rr.Components[c].ChangedIdx { + owner[i] = c + } + } + counts := map[int]int{} // component index -> discards charged + for i, n := range discarded { + if c, ok := owner[i]; ok { + counts[c] += n + } + } + for c, n := range counts { + d := Report{Component: rr.Components[c].Component, Kind: rr.Components[c].Kind, Discarded: n} + safeEmit(func() { p.emitter.Component(d) }) + } +} + // Has reports whether a component with this name is configured in the pipeline. // Hosts use it to gate body-level work that belongs to a component's concern but // cannot be done inside it — e.g. cacheinject's cache-prefix repair, which must diff --git a/components/reformat/cacheinject.go b/components/reformat/cacheinject.go index fe3e7e8..27b9862 100644 --- a/components/reformat/cacheinject.go +++ b/components/reformat/cacheinject.go @@ -141,15 +141,25 @@ func (ci Cacheinject) Reformat(req *schemas.BifrostChatRequest, rep *components. } applied := 0 - // Count breakpoints the caller already set: they occupy provider slots, and an - // agent that sets its own (claude-code does) is already at the optimum. - existing := 0 + // Breakpoints the caller already set occupy provider slots, and an agent that sets + // its own (claude-code does) is already at the optimum. Two separate things must + // happen with them: the positions we can SEE are dropped from `want` (never mark + // twice), and the BUDGET is computed from the host's raw-body count, which also + // sees the ones we cannot — the `system` array components never receive, and + // `tool_result` blocks whose mark apply's own normalize drops. On real + // Claude Code traffic that is all 3 of them, so counting only Input gave a budget + // of 3 free slots when 1 was free: 6 on the wire, and a 400 (issue #32). + visible := 0 for i := range req.Input { if hasBreakpoint(&req.Input[i]) { - existing++ + visible++ delete(want, i) } } + existing := visible + if c != nil && c.ExistingBreakpoints > visible { + existing = c.ExistingBreakpoints + } budget := maxBreakpoints - existing if budget <= 0 { rep.Skipped = true // no slots left; adding one would be a 400 from the provider @@ -282,3 +292,35 @@ func cacheAware(p schemas.ModelProvider) bool { return false } } + +// --------------------------------------------------------------------------- // + +func init() { components.Register("cachesplit", newCachesplit) } + +// Cachesplit is a marker component: it carries no logic of its own, and exists so a +// preset can enable the volatile-tail split (apply/prefixsplit.go) WITHOUT also +// enabling cacheinject's breakpoint placement. +// +// The two were one config entry until #32, which separated them because their evidence +// is not comparable. The split is measured: −34.1% cost and 0% → 96.7% cache hit in an +// isolated A/B, because it moves a churning env snapshot out of a hashed prefix. +// Placement has never been measured at all — until #32 its breakpoints never reached +// the provider. So the split ships on by default and placement does not. +// +// It is a Reformat that always skips: the actual rewrite is body-level (it edits the +// top-level `system` array, which components never see) and lives in `apply`, gated on +// this name being present. ponytail: a marker beats plumbing a new config flag through +// every host. +type Cachesplit struct{} + +func newCachesplit([]byte) (components.Component, error) { return Cachesplit{}, nil } + +func (Cachesplit) Name() string { return "cachesplit" } + +func (Cachesplit) Enabled(*components.Ctx) bool { return true } + +// Reformat is intentionally a no-op — see the type doc. The split happens in apply. +func (Cachesplit) Reformat(_ *schemas.BifrostChatRequest, rep *components.Report, _ *components.Ctx) error { + rep.Skipped = true + return nil +} diff --git a/components/reformat/cacheinject_test.go b/components/reformat/cacheinject_test.go index b93623f..108d2bb 100644 --- a/components/reformat/cacheinject_test.go +++ b/components/reformat/cacheinject_test.go @@ -281,3 +281,51 @@ func TestTTLConfig(t *testing.T) { t.Fatal("an unsupported ttl must be rejected, not silently accepted") } } + +// The provider's cap counts breakpoints this component cannot see: `system` and +// `tools` never reach it, and the host's normalize step drops the mark on tool_result +// blocks it rewrites. Real Claude Code traffic carries (system=2, tools=0, messages=1), so the +// true remaining budget is 1, not 3 (issue #32, defect 2). The synthetic 60-message +// shape below is exactly the probe that produced 4 message marks — 6 on the wire. +func TestBudgetCountsInvisibleBreakpoints(t *testing.T) { + msgs := convo(60) + mark(&msgs[59], nil) // the caller's own message breakpoint + + c := ctx() + c.ExistingBreakpoints = 3 // (system=2, tools=0, messages=1) — the real shape + req := &schemas.BifrostChatRequest{Provider: schemas.Anthropic, Input: msgs} + rep := &components.Report{} + if err := (Cacheinject{}).Reformat(req, rep, c); err != nil { + t.Fatalf("Reformat: %v", err) + } + got := marked(req.Input) + // The caller's own message breakpoint is one of the 3 counted, so the wire total is + // c.ExistingBreakpoints plus whatever we added on top. + added := 0 + for _, i := range got { + if i != 59 { + added++ + } + } + if wire := c.ExistingBreakpoints + added; wire > maxBreakpoints { + t.Fatalf("wire breakpoints %d > cap %d (marked %v, existing %d)", + wire, maxBreakpoints, got, c.ExistingBreakpoints) + } +} + +// Four breakpoints already on the wire leave no budget at all: adding any would be a 400. +func TestExistingBreakpointsCanExhaustTheBudget(t *testing.T) { + c := ctx() + c.ExistingBreakpoints = maxBreakpoints + req := &schemas.BifrostChatRequest{Provider: schemas.Anthropic, Input: convo(30)} + rep := &components.Report{} + if err := (Cacheinject{}).Reformat(req, rep, c); err != nil { + t.Fatalf("Reformat: %v", err) + } + if !rep.Skipped { + t.Fatal("expected a skip: the caller's own breakpoints already fill the cap") + } + if got := marked(req.Input); len(got) != 0 { + t.Fatalf("marked %v over a full budget", got) + } +} diff --git a/config/config.go b/config/config.go index 11334ae..adf6a78 100644 --- a/config/config.go +++ b/config/config.go @@ -100,31 +100,31 @@ func (c *Config) applyPreset() error { // Build time as a clear error. var presets = map[string][]string{ "off": {}, // passthrough: no components (baseline / A-B control) - "safe": {"format", "cacheinject"}, - "balanced": {"format", "dedup", "failed_run", "cmdfilter", "cacheinject"}, - "aggressive": {"format", "dedup", "failed_run", "cmdfilter", "smartcrush", "extract", "extract_llm", "cacheinject"}, - "coding": {"format", "skeleton", "cmdfilter", "cacheinject"}, - "mcp": {"format", "smartcrush", "cacheinject"}, + "safe": {"format", "cachesplit"}, + "balanced": {"format", "dedup", "failed_run", "cmdfilter", "cachesplit"}, + "aggressive": {"format", "dedup", "failed_run", "cmdfilter", "smartcrush", "extract", "extract_llm", "cachesplit"}, + "coding": {"format", "skeleton", "cmdfilter", "cachesplit"}, + "mcp": {"format", "smartcrush", "cachesplit"}, // agent: tuned for long agentic sessions (e.g. Claude Code on SWE-bench), // where the dominant cost is the transcript of tool outputs (file reads) // re-sent every turn. mask (drop old tool outputs) is the biggest lever // there — ~27% content-token savings with no task-reward loss in the // eval-containers SWE-bench sweep (see docs/RESULTS.md); extract + failed_run - // + dedup add relevance/supersession/dup wins; cacheinject keeps the prefix - // cacheable. Order: lossless first, then offload old-then-large, cache last. - "agent": {"format", "dedup", "failed_run", "mask", "extract", "extract_llm", "cacheinject"}, + // + dedup add relevance/supersession/dup wins; cachesplit keeps the shared system + // prefix cacheable. Order: lossless first, then offload old-then-large, cache last. + "agent": {"format", "dedup", "failed_run", "mask", "extract", "extract_llm", "cachesplit"}, // general: the recommended all-round pipeline, safe+effective for any agent/ // benchmark. Ordered by pipeline semantics: lossless repack first (format, toon) // so downstream token counts are honest; cheap structural offloaders next (dedup, // failed_run, cmdfilter); age-based mask; relevance-based extract; the blind // head/tail collapse as the last-resort catch-all for anything still oversized; - // cacheinject last so the cache breakpoint sits on the final bytes. Every offloader + // cachesplit last (it edits `system`, not `messages`). Every offloader // defaults to marker_mode:full (reversible via the injected expand tool) and skips // content already carrying a placeholder, so they never double-reduce. Combines the // levers that proved reward-neutral in the benchmark sweeps without stacking the // two overlapping old-context reducers (mask is the one kept; summarize // is its own preset — see docs/components.md redundancy notes). - "general": {"format", "toon", "dedup", "failed_run", "cmdfilter", "mask", "extract", "extract_llm", "collapse", "cacheinject"}, + "general": {"format", "toon", "dedup", "failed_run", "cmdfilter", "mask", "extract", "extract_llm", "collapse", "cachesplit"}, // summarize restructures the whole transcript (changes the message count) — run // it alone so no other component's in-place edits race apply's rebuild. "summarize": {"summarize"}, @@ -132,8 +132,8 @@ var presets = map[string][]string{ // recommended defaults (codesmart is the proxy default). Their tuned per-component // settings live in presetConfigs; the name-lists here keep PresetPipeline (used by // /compact?preset=) resolving them. - "codesmart": {"format", "dedup", "failed_run", "cmdfilter", "extract_llm", "extract", "cacheinject"}, - "codesafe": {"format", "dedup", "failed_run", "cmdfilter", "extract", "collapse", "cacheinject"}, + "codesmart": {"format", "dedup", "failed_run", "cmdfilter", "extract_llm", "extract", "cachesplit"}, + "codesafe": {"format", "dedup", "failed_run", "cmdfilter", "extract", "collapse", "cachesplit"}, } // presetConfigs carries FULL config docs for presets whose behavior depends on tuned @@ -143,13 +143,13 @@ var presets = map[string][]string{ // relevance-trimmer extract_llm routed to the CHEAP model (model.source: config, // nil-when-unset ⇒ it silently no-ops to deterministic — see docs), gated at 3000 // tok so most turns make no model call, ≤4 calls/req; the free deterministic extract -// catches smaller noise; cacheinject keeps the prefix warm. +// catches smaller noise; cachesplit keeps the shared system prefix warm. // - codesafe: the deterministic-only variant (NO LLM, by policy) — same structural // offloaders plus a blind collapse fallback, zero model calls. // // Component defaults are left untouched, so general/agent/aggressive are unaffected. var presetConfigs = map[string]string{ - "codesmart": `pipeline: [format, dedup, failed_run, cmdfilter, extract_llm, extract, cacheinject] + "codesmart": `pipeline: [format, dedup, failed_run, cmdfilter, extract_llm, extract, cachesplit] components: extract: min_tokens: 400 @@ -162,7 +162,7 @@ components: min_request_tokens: 3000 llm_every_n_requests: 1 llm_max_per_request: 4`, - "codesafe": `pipeline: [format, dedup, failed_run, cmdfilter, extract, collapse, cacheinject] + "codesafe": `pipeline: [format, dedup, failed_run, cmdfilter, extract, collapse, cachesplit] components: collapse: max_tokens: 3000`, diff --git a/config/config_test.go b/config/config_test.go index 4b5d127..db70da3 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -46,7 +46,7 @@ func TestRichPresetCarriesComponentConfig(t *testing.T) { if err != nil { t.Fatal(err) } - want := []string{"format", "dedup", "failed_run", "cmdfilter", "extract_llm", "extract", "cacheinject"} + want := []string{"format", "dedup", "failed_run", "cmdfilter", "extract_llm", "extract", "cachesplit"} if strings.Join(c.Pipeline, ",") != strings.Join(want, ",") { t.Fatalf("codesmart pipeline = %v, want %v", c.Pipeline, want) } @@ -108,3 +108,32 @@ func TestStoreOptionsParse(t *testing.T) { t.Fatalf("store options not parsed: %+v", c.Store) } } + +// cacheinject is deliberately absent from every default preset (#32). Its breakpoints +// only started reaching the provider with that fix, and placement has never been shown +// to help — so it must not be on by default. `cachesplit` carries the volatile-tail +// split, which IS measured, so disabling placement does not disable the split with it. +func TestNoPresetEnablesCacheinjectByDefault(t *testing.T) { + for name := range presets { + p, _ := PresetPipeline(name) + for _, c := range p { + if c == "cacheinject" { + t.Errorf("preset %q enables cacheinject; placement is unmeasured and must be opt-in", name) + } + } + } + // The split must still be on in the recommended presets, or dropping cacheinject + // would be a cost regression rather than a neutral default change. + for _, name := range []string{"general", "codesmart", "codesafe", "balanced"} { + p, _ := PresetPipeline(name) + found := false + for _, c := range p { + if c == "cachesplit" { + found = true + } + } + if !found { + t.Errorf("preset %q lost the volatile-tail split", name) + } + } +} diff --git a/docs/components.md b/docs/components.md index 7867f7d..4ad0372 100644 --- a/docs/components.md +++ b/docs/components.md @@ -11,7 +11,8 @@ messages (`role:"tool"`; for Anthropic, `tool_result` blocks normalized to that |---|---|---|---|---|---| | `format` | Reformat | nothing (compacts JSON) | n/a (lossless) | pretty-printed JSON tool output | `min_tokens` (50) | | `toon` | Reformat | nothing (re-encodes JSON arrays as TOON) | n/a (lossless) | uniform flat JSON object-arrays | `min_tokens` (50) | -| `cacheinject` | Reformat | nothing (adds `cache_control`) | n/a (lossless) | Anthropic-family requests | — | +| `cacheinject` | Reformat | nothing (adds `cache_control`) | n/a (lossless) | Anthropic-family requests; **opt-in, in no preset** — placement is unmeasured (#32) | `ttl` (5m) | +| `cachesplit` | Reformat | nothing (splits a `system` block) | n/a (lossless) | Anthropic-family requests; **in the default presets** — enables the measured volatile-tail split | — | | `skeleton` | Offload | function/method bodies | via expand | fenced ` ```lang ` code blocks | `min_tokens` (80) | | `dedup` | Offload | later byte-identical tool outputs | via expand | repeated identical outputs | `min_tokens` (100) | | `collapse` | Offload | middle of an oversized output | via expand | any large tool output (fallback) | `max_tokens` (2000), `head_lines` (20), `tail_lines` (20) | @@ -23,12 +24,12 @@ messages (`role:"tool"`; for Anthropic, `tool_result` blocks normalized to that | `mask` | Offload | older tool outputs (age-based) | via expand | more than `keep_recent` outputs | `keep_recent` (3), `min_tokens` (100), `keep_head_chars` (96) | | `summarize` | Offload (LLM) | the middle of the transcript → one summary | via expand | long trajectories | `summary_level` (regular), `keep_last` (3), `min_tokens` (500), `resummarize_tokens` (6000), `model.source`, `trigger` | -Presets (`config`): `off` `[]` · `safe` `[format, cacheinject]` · `balanced` -`[format, dedup, failed_run, cmdfilter, cacheinject]` · `aggressive` adds `smartcrush, extract` · -`coding` `[format, skeleton, cmdfilter, cacheinject]` · `mcp` `[format, smartcrush, cacheinject]` · -**`agent`** `[format, dedup, failed_run, mask, extract, cacheinject]` — for long agentic sessions; +Presets (`config`): `off` `[]` · `safe` `[format, cachesplit]` · `balanced` +`[format, dedup, failed_run, cmdfilter, cachesplit]` · `aggressive` adds `smartcrush, extract` · +`coding` `[format, skeleton, cmdfilter, cachesplit]` · `mcp` `[format, smartcrush, cachesplit]` · +**`agent`** `[format, dedup, failed_run, mask, extract, cachesplit]` — for long agentic sessions; `mask` is the biggest lever there (~27–30% content-token savings, no reward loss — see [RESULTS.md](RESULTS.md)) · -**`general`** `[format, toon, dedup, failed_run, cmdfilter, mask, extract, collapse, cacheinject]` — the +**`general`** `[format, toon, dedup, failed_run, cmdfilter, mask, extract, collapse, cachesplit]` — the recommended all-round pipeline: the reward-neutral levers of `agent` plus the situational shrinkers (`toon`/`cmdfilter`/`collapse`) that cost nothing when they don't fire. `balanced` is **not** recommended for agentic traffic — it omits `mask`, so it barely helps (6% vs 31% in the Terminal-Bench replay) · @@ -96,9 +97,14 @@ after: [2]{id,name}: output, or not smaller. ### `cacheinject` -Places an Anthropic `cache_control: {type: ephemeral}` breakpoint on the last content block of -the message just **before** the newest turn (a stable prefix boundary), so the provider KV cache -hits across turns. Adds a control directive, changes no model-visible content. +Places Anthropic `cache_control: {type: ephemeral}` breakpoints at the positions that minimise +billed input cost, so the provider KV cache is read rather than re-processed. Adds control +directives, changes no model-visible content. + +**In no preset — opt in explicitly.** Until [#32](https://github.com/rossoctl/context-guru/issues/32) +its breakpoints never reached the provider on Claude Code traffic, so the placement policy has +never been measured. The presets carry `cachesplit` instead, which enables the volatile-tail split +(measured) without the placement (not). - **Lossiness:** none. **Shines:** Anthropic/Bedrock/Vertex agents that don't self-cache (the savings lever is provider-side cache hits, invisible to `/stats` token counts). **Inert:** diff --git a/docs/components/cacheinject.md b/docs/components/cacheinject.md index 1f7aea4..a0cc616 100644 --- a/docs/components/cacheinject.md +++ b/docs/components/cacheinject.md @@ -4,6 +4,42 @@ Places Anthropic `cache_control` breakpoints at the positions that minimise billed input cost, so the provider KV cache is read rather than re-processed. +!!! danger "Every placement number below predates the fix in #32 — read this first" + Until #32, **this component's breakpoints never reached the wire on Claude Code + traffic.** Measured over 40 captured requests: **46 breakpoints applied at the + component level, 0 in the output body.** + + The cause was structural, not a tuning error. The component can only mark messages + carrying content *blocks*; on Claude Code traffic those are exclusively assistant + turns carrying `tool_use` — and bifrost drops `tool_use.id/name/input` on unmarshal, + so `apply`'s losslessness guard correctly discarded every one of those changes + rather than splice a corrupted re-marshal. A component whose only possible targets + were precisely the messages that could never be written back. + + A second, independent defect compounded it: the 4-breakpoint budget counted only + `messages`. Real traffic puts 2 of its 3 breakpoints in the top-level `system` array + and the third on a `tool_result` block — and that third mark is lost by **this + repo's own** `normalize`, which rebuilds each `tool_result` into a synthetic + `role=tool` message from text + `tool_use_id` alone (`toolMessage`), dropping + `cache_control` along with everything else it does not copy. So all three were + invisible and the component computed 3 free slots when 1 was free. On a 60-message + request it emitted 4 message marks, **6 on the wire**, which the provider rejects + with a 400. That never fired in production *only* because the first defect + suppressed the marks; fixing one without the other would have produced a live 400. + + Both are fixed (see [design.md](../design.md) — the metadata-write exception). + Breakpoints now reach the wire on **86 of 92** replayed captured requests, and the + wire total is asserted never to exceed 4. + + **Consequence for every figure on this page:** the placement rows measured a + component whose output was discarded. The `acted=0` / "placement contributes $0" + findings are still *true as recorded* — nothing reached the provider, so nothing + could have contributed — but they are **not** evidence that placement has no + headroom. That question had not been asked until #32; see + [What placement is actually worth](#what-placement-is-actually-worth) for the + first honest measurement. The volatile-tail-split figures are unaffected: that is a + body-level rewrite which never went through the discarded path. + ## How it works Placement is the solution to a cost minimisation, not a heuristic. With `R = 0.1` @@ -64,12 +100,20 @@ the cache-hit rate a live 50-task run actually billed: **Against an agent that already caches well, expect exactly 0%.** Measured on real traffic: claude-code marks its own final message on **466 of 472 requests (98.7%)**, so rule 1 is already satisfied and every candidate position this component would -choose collides with an existing breakpoint. Instrumented over a live 12-task run it -placed **zero** extra breakpoints (`runs 432, acted 0`) and was byte-identical to -baseline on the wire. +choose collides with an existing breakpoint. + +!!! warning "The `acted 0` figure below was not a measurement of placement" + The same 12-task run also reported `runs 432, acted 0` and "byte-identical to + baseline on the wire". That was read as *the policy chose to place nothing*. It was + not: the component **did** place breakpoints and the writeback layer discarded every + one before the request was sent (#32). The wire was byte-identical because nothing + reached it, not because the policy declined. Both statements — placement collides + with existing breakpoints, and nothing reached the wire — were true at once, and the + second one masked the first from ever being tested. See + [What placement is actually worth](#what-placement-is-actually-worth). -So this component's value is precisely two things, neither of which is a saving -against claude-code: +So this component's value was claimed to be precisely two things, neither of which is a +saving against claude-code: 1. It **stops v1's +5.5% regression** (see the warning below). 2. It places breakpoints for agents that do *not* mark their own tail, and it anchors @@ -77,7 +121,7 @@ against claude-code: claude-code needs. The measurable cost saving on claude-code traffic comes from the **cross-session -prefix repairs** in `apply/prefixorder.go`, which are gated on this component being +prefix repairs** in `apply/prefixsplit.go`, which are gated on this component being configured but are a different mechanism. See below. !!! warning "v1 was a regression" @@ -87,6 +131,83 @@ configured but are a different mechanism. See below. the savings as "invisible to `/stats`", which made a negative effect unfalsifiable. If you are pinning an older release, this component cost money. +## What placement is actually worth + +**Still unmeasured.** #32 made the policy reach the provider for the first time; it did +not establish what the policy is worth. One `cacheonly` vs `off` pair was attempted and +is reported here in full because it is all the evidence there is — not because it settles +anything. + +SWE-bench Verified, `aws/claude-sonnet-5`, n=1 per arm. `off`'s second trial died on a +Docker-compose error, so only `astropy-12907` completed in both arms: + +| metric | off | cacheonly | delta | +|---|--:|--:|--:| +| steps | 14 | 11 | −21.4% | +| billed cost | $0.17607 | $0.14931 | −15.2% | +| cache-read | 609,968 | 461,844 | −24.3% | +| cache-write | 8,695 | 11,062 | +27.2% | +| cache-hit | 98.59% | 96.66% | −1.93 pp | + +The −15.2% is **not a saving** — the agent took 3 fewer steps, and on this traffic cost +tracks steps at corr 0.95. Per step, cost is +7.9% and cache-write +61.9%. + +!!! warning "What this does NOT show" + **No mechanism is established for the cache-write difference, and the obvious one is + ruled out.** The tempting story — that the extra breakpoint lands *above* + claude-code's own and shortens the readable prefix — does not hold: across three + captures, **0 of 106** of our marks land above the agent's. Ours consistently sits one + message *below*, where the policy's own Rule 2 says an extra breakpoint costs exactly + zero. + + **`acted=0` does not isolate placement.** It only rules out content compaction. The + `cacheonly` arm still runs `splitVolatileTail`, which rewrites the `system` array, so + the arm is "placement + split", not "placement". A single trial per arm also cannot + separate either from the step-count nondeterminism that produced the 3-step gap. + + Treat the table as one task, once, on a contended box, with a degenerate control. The + honest summary is that placement's value **has still never been measured** — which is + why `cacheinject` is not in any default preset. + +## How the breakpoints reach the wire + +Worth knowing, because it is where this component was broken for two benchmark +studies. `cacheinject` computes *where* the breakpoints go; `apply` performs the write. + +The component can only mark a message carrying content **blocks**, and on Claude Code +traffic those are exclusively assistant turns carrying `tool_use`. bifrost drops +`tool_use.id/name/input` on unmarshal, so `apply`'s losslessness guard cannot splice a +re-marshal of such a message without corrupting it — and correctly refused to. + +The fix is not to relax that guard. `cache_control` is **metadata, not content**: it +changes nothing the model reads, so it needs no message model to express. `apply` writes +it as a targeted `sjson` key at `messages..content..cache_control` on the +**original raw bytes**, verifying first that the component's *only* change to that +message was adding `cache_control` keys. A write that reads no other field cannot drop +one. Any wider change is still discarded, and a discard now increments +`discarded_changes` for the responsible component in `/stats` — the observability gap +that let this survive unnoticed. + +## The 4-breakpoint budget is computed by the host, not the component + +The provider caps `cache_control` at **4 across `system` + `tools` + `messages` +together**, and a component sees none of the first two. Worse, `apply`'s own `normalize` +rebuilds each `tool_result` block into a synthetic `role=tool` message from text + +`tool_use_id` alone (`toolMessage`), so a `cache_control` on such a block never reaches +the component either — even some *message* breakpoints are invisible to it. + +On real Claude Code traffic that hides **all three** of the agent's own breakpoints — +measured, 1,771 of 1,794 requests carry exactly `(system=2, tools=0, messages=1)`, with +the message one sitting on a `tool_result` block. Counting only what it could see, the +component computed `budget = 4 − 0 = 4` when 1 slot was free, and on a 60-message +request emitted 4 marks: **6 on the wire, which the provider rejects with a 400.** + +So `apply` counts them structurally from the raw body and passes the total as +`Ctx.ExistingBreakpoints`; the component budgets against that. The count covers the +Bedrock `cachePoint` spelling too, including its own entries in `system` and `tools`. +`apply` also counts the output and logs an error if **we** pushed it over 4 — a request +that arrived over the cap is forwarded as-is and is not reported as ours. + ## Lossiness None. It attaches cache directives only; model-visible content is unchanged, and @@ -99,6 +220,19 @@ markable block below so the prefix is still written. ## Configuration +!!! warning "Not enabled by any preset — opt in explicitly" + Since #32, `cacheinject` is in **no** default preset. Its breakpoints only began + reaching the provider with that fix, and placement has never been shown to help, so + shipping it on by default would enable an unmeasured policy on every request. + + The presets carry **`cachesplit`** instead — a marker component that enables the + [volatile-tail split](#the-volatile-tail-split) (which *is* measured) without the + breakpoint placement. The two were one config entry until #32; separating them is + what keeps disabling placement from silently disabling the split too. + + Add `cacheinject` to a pipeline by hand to run the placement study, or when your agent + does not set its own `cache_control` (the case where the policy should help most). + ```yaml components: cacheinject: @@ -117,13 +251,23 @@ scratchpad, a re-rendered header, an injected timestamp). On OpenAI- and Gemini-shaped wires, where `cache_control` does not exist at all — placement has nothing to express and the split has nothing to gain (see below). Also -inert when four breakpoints are already present, and on string-content messages. +inert when four breakpoints are already present **anywhere in the request** (including +`system` and `tools`), and on string-content messages. + +That last case is now the common one on Claude Code traffic, and it is correct: the agent +already spends 3 of the 4 slots, so this component has exactly one to place. Before #32 +it believed it had three. ## The volatile-tail split -Enabling `cacheinject` also switches on a body-level repair in `apply/prefixsplit.go` -that no breakpoint placement can achieve, because a cache entry hashes **everything -before** its breakpoint and no position can exclude part of a single block. +Enabling **`cachesplit`** (or `cacheinject`) switches on a body-level repair in +`apply/prefixsplit.go` that no breakpoint placement can achieve, because a cache entry +hashes **everything before** its breakpoint and no position can exclude part of a single +block. + +This is the mechanism the default presets keep. It is a different lever from placement +and has its own, much stronger evidence — everything below was measured with placement +contributing `$0`, so none of it depends on the policy this page's other half describes. Claude Code appends a live environment snapshot to the **end** of its main system block: diff --git a/docs/design.md b/docs/design.md index 7aad713..3eaaf67 100644 --- a/docs/design.md +++ b/docs/design.md @@ -111,7 +111,7 @@ sequenceDiagram apply->>apply: normalize → []ChatMessage + write-back slots apply->>Pipe: Run(chat, ctx) Pipe-->>apply: mutated messages - apply->>apply: per message: unchanged → keep bytes,
changed & lossless round-trip → sjson splice + apply->>apply: per message: unchanged → keep bytes,
changed & lossless round-trip → sjson splice,
changed & metadata-only → sjson key write on raw bytes,
else discard + count apply-->>Host: rewritten body (or original, fail open) ``` @@ -124,6 +124,42 @@ Non-string tool_result content is skipped (never lose non-text). A whole-message spliced back if bifrost round-trips that message losslessly (`jsonEqual`); otherwise the change is discarded — correctness over the marginal saving. +**The metadata exception.** That guard has one deliberate hole, and it exists because the guard +alone made `cacheinject` a no-op. bifrost drops `tool_use.id/name/input` on unmarshal, so every +Anthropic assistant turn carrying a `tool_use` is non-round-trippable — and those are exactly the +only messages `cacheinject` can mark. Measured on 40 captured Claude Code requests: **46 +breakpoints applied at the component level, 0 in the output body** (issue #32). + +So `apply/metawrite.go` adds a narrow path: when a component's *only* change to a message is an +added `cache_control` key, that key is written at its exact path (`messages..content..cache_control`) +on the **original raw bytes** via `sjson`. `cache_control` is metadata, not content — it changes +nothing the model reads, so it needs no message model to express, and a targeted `sjson` write +provably cannot drop a field it never reads. The `metadataOnlyWrites` diff enforces "only that": +a text edit, a removed key, a changed block count, anything else at all, and the change is still +discarded. `applyMetaWrites` additionally refuses if the raw body's block layout disagrees with +the normalized view, so a key can never land on the wrong block, and never overwrites a +breakpoint the caller set. + +**Discards are now loud.** `Pipeline.RecordDiscards` attributes each thrown-away change back to +the component that made it (via `Report.ChangedIdx`), surfacing as `discarded_changes` per +component and `top_discarded` in `/stats`. Before this, a mutated-then-discarded component looked +byte-identical to a working Reformat — which is how #32 survived two full benchmark studies. +Attribution is deliberately conservative, because a counter meant to catch that class of bug is +worthless if it cries wolf: `ChangedIdx` is recorded only on the surviving path (a reverted +component is never charged), and one discarded message is charged to exactly ONE component — the +last one to change it, whose state is what the writeback layer actually threw away. + +**Breakpoint budgeting is a host job.** The provider caps `cache_control` at 4 across `system` + +`tools` + `messages` together, and a component sees none of the first two. Nor does it see a +`cache_control` on a `tool_result` block: `normalize` rebuilds those into synthetic `role=tool` +messages from text + `tool_use_id` alone (`toolMessage`), dropping the mark. (bifrost is not the +culprit here — it round-trips `cache_control` on `tool_result` fine.) On real Claude Code traffic +that hides all three of the agent's own breakpoints, so a component counting only what it saw +computed 4 free slots when 1 was free. `apply` counts them from the raw body (`wireBreakpoints`, +covering the Bedrock `cachePoint` spelling and its own `system`/`tools` entries) and passes the +total as `Ctx.ExistingBreakpoints`. A breach is logged only when *we* pushed the total over the +cap — an already-over-cap request is forwarded untouched and is not ours to report. + If a component changes the message *count* (none of the v1 set does), the slot map no longer aligns, so `apply` forwards the original untouched. diff --git a/docs/how-to/choose-a-preset.md b/docs/how-to/choose-a-preset.md index 3c03de0..937ccbb 100644 --- a/docs/how-to/choose-a-preset.md +++ b/docs/how-to/choose-a-preset.md @@ -32,16 +32,19 @@ per-component behavior is in [Components](../components.md). No components. Passthrough. Use it as the A/B control when you measure savings — the baseline in [Benchmarks](../RESULTS.md) is this preset. -### `safe` — `[format, cacheinject]` +### `safe` — `[format, cachesplit]` Two lossless [Reformat](../components.md#reformat-lossless) components only: compact JSON -(`format`) and an Anthropic cache breakpoint (`cacheinject`). Nothing is ever dropped, so there is -nothing to expand. +(`format`) and the Anthropic volatile-tail split (`cachesplit`). Nothing is ever dropped, so there +is nothing to expand. - **Fits:** any traffic where you want a zero-risk win and no reversibility surface. -- **Caveat:** `cacheinject`'s savings are provider-side cache hits, invisible to `/stats` token +- **Caveat:** `cachesplit`'s savings are provider-side cache hits, invisible to `/stats` token counts — it will show up under `top_passthrough`. That's expected, not dead weight. +- **Note:** breakpoint *placement* (`cacheinject`) is deliberately **not** here — it is unmeasured + and opt-in since [#32](https://github.com/rossoctl/context-guru/issues/32). `cachesplit` carries + the part with measured savings. -### `balanced` — `[format, dedup, failed_run, cmdfilter, cacheinject]` +### `balanced` — `[format, dedup, failed_run, cmdfilter, cachesplit]` The default. Adds three cheap, high-precision offloaders: exact-dup removal (`dedup`), superseded test/build runs (`failed_run`), and DSL command-log filtering (`cmdfilter`). @@ -50,7 +53,7 @@ test/build runs (`failed_run`), and DSL command-log filtering (`cmdfilter`). one. Its builtins cover pytest / npm-install / make; author more with a [custom DSL filter](custom-dsl-filter.md). -### `aggressive` — `[format, dedup, failed_run, cmdfilter, smartcrush, extract, cacheinject]` +### `aggressive` — `[format, dedup, failed_run, cmdfilter, smartcrush, extract, cachesplit]` `balanced` plus JSON-array crushing (`smartcrush`) and query-relevance projection (`extract`). - **Fits:** you want more savings and accept structural/LLM offload with expand recovery. @@ -58,7 +61,7 @@ test/build runs (`failed_run`), and DSL command-log filtering (`cmdfilter`). the default `deterministic` strategy is free. Keep the [store](recover-context.md) on so the extra offloads stay recoverable. -### `coding` — `[format, skeleton, cmdfilter, cacheinject]` +### `coding` — `[format, skeleton, cmdfilter, cachesplit]` Swaps in `skeleton`, which tree-sitter-parses fenced code blocks and replaces function bodies with `{ … }`, keeping signatures/imports/types. @@ -66,14 +69,14 @@ Swaps in `skeleton`, which tree-sitter-parses fenced code blocks and replaces fu - **Caveat:** `skeleton` is inert on unfenced file reads, unknown languages, or when the skeleton isn't smaller than the body. -### `mcp` — `[format, smartcrush, cacheinject]` +### `mcp` — `[format, smartcrush, cachesplit]` Targets homogeneous JSON arrays (list endpoints, search hits): keep `keep_first` + `keep_last` items plus any item carrying an error signal, drop the middle. - **Fits:** MCP tools and REST list endpoints returning long uniform arrays. - **Caveat:** inert on non-array output or arrays below `min_items`. -### `agent` — `[format, dedup, failed_run, mask, extract, cacheinject]` +### `agent` — `[format, dedup, failed_run, mask, extract, cachesplit]` Tuned for long agentic sessions (e.g. Claude Code on SWE-bench) where the dominant cost is the transcript of old tool outputs re-sent every turn. diff --git a/docs/results/components.md b/docs/results/components.md index 52bf94c..7adc9e9 100644 --- a/docs/results/components.md +++ b/docs/results/components.md @@ -102,8 +102,19 @@ Run: **245 acts, 34,293 cumulative / ~2.9k unique tok**, ~0.6 s total (near-zero ### 7. `cacheinject` (Reformat) Stamps an ephemeral `cache_control` breakpoint on the prefix boundary so the provider KV -cache hits across turns. No content change. Its payoff is systemic — the 97.8% cache-hit -rate the whole cache-aware design is tuned around. +cache hits across turns. No content change. + +!!! danger "This row measured a suppressed component" + On the run behind this page, `cacheinject`'s breakpoints **never reached the wire** — + 46 applied at the component level, 0 in the output body across 40 captured requests + ([#32](https://github.com/rossoctl/context-guru/issues/32)). Its only possible targets + are assistant turns carrying `tool_use`, which bifrost cannot round-trip, so the + writeback layer discarded every mark. + + So the **97.8% cache-hit rate is not attributable to this component.** That rate is + claude-code's own breakpoints, which it sets on every request and which the proxy + forwarded untouched. Fixed in #32; a re-run must re-measure this row rather than carry + the number forward. --- diff --git a/metrics/metrics.go b/metrics/metrics.go index c4b345f..5e08b94 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -49,6 +49,7 @@ func (s Slog) Component(r components.Report) { "context_engineering.tokens.after", r.TokensAfter, "context_engineering.tokens.saved", r.Saved(), "context_engineering.reverted", r.Reverted, + "context_engineering.discarded_changes", r.Discarded, "context_engineering.duration_ms", r.DurationMs, ) } @@ -96,9 +97,15 @@ type compStat struct { // OvercountRatio = Saved / SavedUnique — how many times, on average, each distinct // compaction was re-counted (the agent re-sends history verbatim every turn). ~1.0 // is honest; large values mean the cumulative figure is inflated by re-sends. - OvercountRatio float64 `json:"overcount_ratio"` - DurationMs float64 `json:"duration_ms"` // cumulative wall time this component spent (its own latency cost on the hot path) - seenKeys map[string]struct{} // content keys already counted toward SavedUnique (not serialized) + OvercountRatio float64 `json:"overcount_ratio"` + DurationMs float64 `json:"duration_ms"` // cumulative wall time this component spent (its own latency cost on the hot path) + // Discarded counts changes this component made that the WRITEBACK layer then threw + // away (bifrost could not round-trip the message, so splicing would have dropped + // provider fields). Nonzero means the component ran, mutated, and had no effect on + // the wire — which for two whole benchmark studies looked exactly like a working + // Reformat (issue #32). + Discarded int64 `json:"discarded_changes"` + seenKeys map[string]struct{} // content keys already counted toward SavedUnique (not serialized) } // NewAggregator returns an empty aggregator. @@ -112,6 +119,13 @@ func (a *Aggregator) Component(r components.Report) { cs = &compStat{} a.perComp[r.Component] = cs } + // A Discarded report is a follow-up from the writeback layer attributing thrown-away + // changes to the component that made them — not a fresh run. Count it and stop, or + // Runs would double per request. + if r.Discarded > 0 { + cs.Discarded += int64(r.Discarded) + return + } cs.Runs++ cs.Saved += int64(r.Saved()) cs.DurationMs += r.DurationMs // per-component latency cost on the hot path @@ -207,6 +221,11 @@ type Snapshot struct { // TopPassthrough names components that ran but never saved a token — dead // weight in the pipeline, candidates to drop from the config. TopPassthrough []string `json:"top_passthrough"` + // TopDiscarded names components whose changes the writeback layer threw away at + // least once — they mutated the request but (for those changes) never reached the + // wire. Any entry here needs investigating; see the per-component + // `discarded_changes` for the count. + TopDiscarded []string `json:"top_discarded"` // LLM* report the cheap (config-source) model usage the CONTEXT-GURU components // themselves incurred (e.g. extract:code's Starlark-writer calls) — the CG // components' OWN cost, separate from the agent. Priced externally. @@ -231,8 +250,11 @@ func (a *Aggregator) Snapshot() Snapshot { pct = float64(saved) / float64(a.before) * 100 } comps := make(map[string]compStat, len(a.perComp)) - var passthrough []string + var passthrough, discarded []string for k, v := range a.perComp { + if v.Discarded > 0 { + discarded = append(discarded, k) + } cs := *v if cs.SavedUnique > 0 { cs.OvercountRatio = float64(cs.Saved) / float64(cs.SavedUnique) @@ -247,6 +269,7 @@ func (a *Aggregator) Snapshot() Snapshot { } } sort.Strings(passthrough) + sort.Strings(discarded) addedAvg, upAvg, upAvgByp := 0.0, 0.0, 0.0 if a.addedSamples > 0 { addedAvg = a.addedMs / float64(a.addedSamples) @@ -261,7 +284,7 @@ func (a *Aggregator) Snapshot() Snapshot { Requests: a.requests, TokensBefore: a.before, TokensAfter: a.after, SavedTokens: saved, SavingsPct: pct, WastedTokens: a.wasted, Bounces: a.bounces, AdjustedSaved: saved - a.wasted, - Components: comps, TopPassthrough: passthrough, + Components: comps, TopPassthrough: passthrough, TopDiscarded: discarded, AddedLatencyMsAvg: addedAvg, UpstreamMsAvg: upAvg, UpstreamMsAvgBypassed: upAvgByp, } }