diff --git a/apply/apply.go b/apply/apply.go index 86ac6395..d68efac5 100644 --- a/apply/apply.go +++ b/apply/apply.go @@ -113,6 +113,23 @@ func logDecisions(lg *slog.Logger, rr *components.RunReport) { if len(rep.Gates) > 0 { attrs = append(attrs, "gates", formatGates(rep.Gates)) } + // EVENTS TOO, or splitting the histogram silently blinded this line. + // + // Everything a component recorded used to land in Gates, so one field carried it all. After + // the split (#121) a component whose counters are all EVENTS logged no counter information + // whatsoever — and the component most affected is the one that only records successes when it + // works. Observed live: the turn that adjudicated twelve outputs, removed twelve and saved + // 33,340 tokens logged `verdict=acted saved=33340` and not one counter, because all eleven + // names it raised are events. That is the exact diagnosis this line exists to provide, + // missing precisely when the component succeeded. + // + // Rendered as one `name=n name=n` STRING for the same reason gates are: an attribute key is + // checked against the credential-name denylist, so a future event called `no_auth` would have + // its count replaced by «redacted». As a value it is scrubbed as content, where a short + // integer after `=` matches nothing. + if len(rep.Events) > 0 { + attrs = append(attrs, "events", formatGates(rep.Events)) + } if rep.Irreversible { attrs = append(attrs, "irreversible", true) } @@ -452,6 +469,10 @@ func BodyOpts(ctx context.Context, pipe *components.Pipeline, st store.Store, o nowMs := o.nowMs() coldCache := false idleMs := int64(0) + // ttlMs is the cache lifetime the cold decision below derives, carried onto the Ctx so a + // component can act BEFORE expiry rather than only after it. 0 when the cache-aware path did not + // run, which reads as "unknown" to every consumer. + ttlMs := int64(0) maxCachedIdx := -1 if cacheAware && !bypass { // Messages present on the previous turn of this session are already committed @@ -521,6 +542,11 @@ func BodyOpts(ctx context.Context, pipe *components.Pipeline, st store.Store, o ttl = a } coldCache = cacheIsCold(prevAt, nowMs, ttl) + // The SAME ttl the cold decision used, carried onto the Ctx. A component that wants to + // act BEFORE expiry rather than after needs the lifetime, not just the verdict, and + // re-deriving it there would be a second read of one fact — which is how the cold + // decision and the dashboard came to disagree once already (see ttlTier). + ttlMs = ttl.Milliseconds() if prevAt > 0 && nowMs > prevAt { idleMs = nowMs - prevAt } @@ -583,6 +609,8 @@ func BodyOpts(ctx context.Context, pipe *components.Pipeline, st store.Store, o // provider's cap of four counts them all (issue #32, defect 2). ExistingBreakpoints: bps.Total(), Mode: mode, + PrefixAsk: o.PrefixAsk, + CacheTTLMs: ttlMs, // Set BEFORE the run, so cachesplit's own report is right at the source and every // consumer of it agrees. Amending the report afterwards fixed the dashboard and // left /stats and the Prometheus component counters still saying "skipped", diff --git a/apply/logevents_test.go b/apply/logevents_test.go new file mode 100644 index 00000000..a5538726 --- /dev/null +++ b/apply/logevents_test.go @@ -0,0 +1,60 @@ +package apply + +import ( + "bytes" + "log/slog" + "strings" + "testing" + + "github.com/rossoctl/context-guru/components" +) + +// A COMPONENT WHOSE COUNTERS ARE ALL EVENTS MUST STILL LOG THEM. +// +// Splitting Report.Gates into Gates and Events (#121) silently blinded this line: it rendered only +// Gates, so a component that records successes rather than refusals logged no counter information at +// all. Observed live on the worst possible turn — the one that adjudicated twelve outputs, removed +// twelve and saved 33,340 tokens logged `verdict=acted saved=33340` and nothing else, because every +// name it raised was an event. That is the diagnosis this line exists to provide, absent exactly when +// the component worked. +// +// The fixture is deliberately events-ONLY. A report carrying both would pass even with the events +// branch removed, since the gates field would still appear. +func TestDecisionLogCarriesEventsNotOnlyGates(t *testing.T) { + var buf bytes.Buffer + lg := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + + var eventsOnly components.Report + eventsOnly.Component, eventsOnly.Kind = "extract_llm_sweep", "offload" + eventsOnly.TokensBefore, eventsOnly.TokensAfter = 34317, 977 + eventsOnly.EventN("sweep_offered", 12) + eventsOnly.EventN("sweep_dropped", 12) + eventsOnly.Event("sweep_prefix_cache_read_ok") + + logDecisions(lg, &components.RunReport{Components: []components.Report{eventsOnly}}) + out := buf.String() + + // Precondition: the line was emitted at all, or the assertions below pass on an empty buffer. + if !strings.Contains(out, "extract_llm_sweep") { + t.Fatalf("no component line was logged, so the assertion is vacuous: %q", out) + } + for _, want := range []string{"sweep_offered=12", "sweep_dropped=12", "sweep_prefix_cache_read_ok=1"} { + if !strings.Contains(out, want) { + t.Errorf("the decision line does not carry %q — a component that records only "+ + "successes logs no counters, which is the diagnosis this line exists for: %s", + want, out) + } + } + // Gates and events must be distinguishable in the output, not merged into one field: they answer + // opposite questions and a reader cannot tell a refusal from a success otherwise. + var both components.Report + both.Component, both.Kind = "extract_llm", "offload" + both.GateN("below_output_floor", 11) + both.Event("reapplied_same_session") + buf.Reset() + logDecisions(lg, &components.RunReport{Components: []components.Report{both}}) + line := buf.String() + if !strings.Contains(line, "gates=") || !strings.Contains(line, "events=") { + t.Errorf("declines and successes must appear as separate fields; got %s", line) + } +} diff --git a/apply/opts.go b/apply/opts.go index bbe1234a..d000d57b 100644 --- a/apply/opts.go +++ b/apply/opts.go @@ -66,6 +66,11 @@ type Opts struct { // upgrade entirely rather than defaulting, so a host that forgets to resolve its // configuration asks for nothing instead of asking on every request. HeadTTLMinTokens int + // PrefixAsk, when set, lets a component put a question to the request's own model with the + // previous turn's SENT body as the cached prefix. See components.PrefixAsker. nil => a component + // that wants one gets none and decides for itself; today's behaviour for every caller that does + // not set it. + PrefixAsk components.PrefixAsker // Tracker, when set, owns the per-session cached-prefix boundary. Supplying it also // removes the concurrent-turn race in the legacy read-then-deferred-write of prevLen. // nil => the legacy store-backed path, unchanged for library callers and /compact. diff --git a/apply/sweep_variants_test.go b/apply/sweep_variants_test.go index eeac3de0..a7685ade 100644 --- a/apply/sweep_variants_test.go +++ b/apply/sweep_variants_test.go @@ -18,14 +18,13 @@ mode: sync `}, // Osher's production document, verbatim. {name: "prod-osher", yaml: ` -pipeline: [format, toon, dedup, cmdfilter, extract_llm, extract, cachesplit] +pipeline: [format, toon, dedup, cmdfilter, extract_llm, extract_llm_sweep, extract, cachesplit] components: extract: min_tokens: 400 extract_llm: aggressiveness: medium allow_on_caching_backend: false - cold_cache: {enabled: true, min_tokens: 1000} context: recent context_messages: 7 fire_on: pressure @@ -34,15 +33,17 @@ components: llm_max_per_session: 80 min_tokens: 1000 model: {model: claude-haiku-4-5, source: incoming} - per_output: false strategy: code trigger: {min_request_tokens: 3000} + extract_llm_sweep: + min_tokens: 1000 + model: {model: claude-haiku-4-5, source: incoming} mode: sync `}, // The hypothesis: let it act on warm cached turns, fire on size, and let it see the // file reads that AUTO would skip. {name: "warm-tail", yaml: ` -pipeline: [format, toon, dedup, cmdfilter, extract_llm, extract, cachesplit] +pipeline: [format, toon, dedup, cmdfilter, extract_llm, extract_llm_sweep, extract, cachesplit] components: extract: min_tokens: 400 @@ -50,7 +51,6 @@ components: aggressiveness: medium allow_on_caching_backend: true skip_file_reads: false - cold_cache: {enabled: true, min_tokens: 1000} context: recent context_messages: 7 fire_on: size @@ -58,12 +58,11 @@ components: llm_max_per_request: 3 llm_max_per_session: 40 model: {model: claude-haiku-4-5, source: incoming} - per_output: true strategy: code mode: sync `}, {name: "warm-tail-800", yaml: ` -pipeline: [format, toon, dedup, cmdfilter, extract_llm, extract, cachesplit] +pipeline: [format, toon, dedup, cmdfilter, extract_llm, extract_llm_sweep, extract, cachesplit] components: extract: min_tokens: 400 @@ -71,19 +70,17 @@ components: aggressiveness: medium allow_on_caching_backend: true skip_file_reads: false - cold_cache: {enabled: true, min_tokens: 1000} context: recent fire_on: size min_tokens: 800 llm_max_per_request: 4 llm_max_per_session: 60 model: {model: claude-haiku-4-5, source: incoming} - per_output: true strategy: code mode: sync `}, {name: "warm-tail-high", yaml: ` -pipeline: [format, toon, dedup, cmdfilter, extract_llm, extract, cachesplit] +pipeline: [format, toon, dedup, cmdfilter, extract_llm, extract_llm_sweep, extract, cachesplit] components: extract: min_tokens: 400 @@ -91,32 +88,28 @@ components: aggressiveness: high allow_on_caching_backend: true skip_file_reads: false - cold_cache: {enabled: true, min_tokens: 1000} context: recent fire_on: size min_tokens: 1500 llm_max_per_request: 3 llm_max_per_session: 40 model: {model: claude-haiku-4-5, source: incoming} - per_output: true strategy: code mode: sync `}, // Deterministic-only extraction on the same trigger: the free comparison arm. If this // gets close to the LLM arms, the LLM calls are not buying much. {name: "det-strategy", yaml: ` -pipeline: [format, toon, dedup, cmdfilter, extract_llm, extract, cachesplit] +pipeline: [format, toon, dedup, cmdfilter, extract_llm, extract_llm_sweep, extract, cachesplit] components: extract: min_tokens: 400 extract_llm: allow_on_caching_backend: true skip_file_reads: false - cold_cache: {enabled: true, min_tokens: 1000} fire_on: size min_tokens: 1500 strategy: deterministic - per_output: true mode: sync `}, } diff --git a/components/all/xglobal_test.go b/components/all/xglobal_test.go index d9d7fcf2..4545e727 100644 --- a/components/all/xglobal_test.go +++ b/components/all/xglobal_test.go @@ -196,122 +196,47 @@ func TestGlobalCacheHitIsNotSplicedAtDepth(t *testing.T) { } } -// housellmExtractLLM returns the extract_llm block EXACTLY as the housellm preset ships it, -// so the guard above tests the shipped configuration instead of a transcription of it. -func housellmExtractLLM(t *testing.T) string { +// housellmBlock returns one component's block EXACTLY as the housellm preset ships it, so the +// guards here test the shipped configuration instead of a transcription of it. +func housellmBlock(t *testing.T, name string) string { t.Helper() cfg, err := config.LoadBytes([]byte("preset: housellm\n")) if err != nil { t.Fatalf("load housellm preset: %v", err) } - node, ok := cfg.Components["extract_llm"] + node, ok := cfg.Components[name] if !ok { - t.Fatal("housellm preset no longer configures extract_llm; this guard is testing nothing") + t.Fatalf("housellm preset no longer configures %s; this guard is testing nothing", name) } raw, err := yaml.Marshal(&node) if err != nil { - t.Fatalf("marshal extract_llm block: %v", err) + t.Fatalf("marshal %s block: %v", name, err) } return string(raw) } -// TestHousellmColdSweepActuallyFires is the other half of -// TestDefaultConfigsSpendOnlyOnTheUncachedTail replaces -// TestNoDefaultConfigRunsExtractLLMOnCachingBackend, whose premise this change deliberately -// reverses. That test asserted a default config makes NO call on a caching backend even for a -// candidate its own comment identified as being in the tail. The reason it could assert that -// was a mis-pricing, not a measurement: savedTokenValue reported `cached: true` for the whole -// request, so a tail candidate — content being written INTO the cache on this very turn, at -// 1.25x fresh — was valued at the cache-READ rate, 12.5x too low. The ~30,500-token -// break-even quoted alongside it is explicitly the CACHED break-even. Together they read as -// "extraction cannot pay on a caching backend", which is true at depth and was never -// established for the tail. -// -// So the decision this pins is now positional, which is the honest form of it: -// -// at DEPTH — inside the live cached prefix — a default must still not spend. Removing -// cached content saves the read rate and forces a suffix re-write on top. -// in the TAIL, a default MAY spend, and must, when the economics pass. -// -// What is NOT relaxed is the economic gate itself: see -// TestTheTailIsStillGatedOnItsOwnEconomics, which is the other half of this and the reason -// this is a re-pricing rather than an opening of the floodgates. -func TestDefaultConfigsSpendOnlyOnTheUncachedTail(t *testing.T) { - filter := "data = json.decode(INPUT)\nOUTPUT = json.encode([r for r in data if \"keep\" in r[\"name\"]])\n" - pad := strings.Repeat("padding ", 30_000) // ~240k tokens: economics pass on their own - body := `[{"id":1,"name":"keep this ` + pad + `"},{"id":2,"name":"drop this ` + pad + `"}]` - - cfgs := map[string]string{ - "defaults": "strategy: code\nmodel:\n source: config\n", - "codesmart": "strategy: code\nmodel:\n source: config\nmin_tokens: 3000\n" + - "trigger:\n min_request_tokens: 3000\nllm_every_n_requests: 1\nllm_max_per_request: 4\n", - // Read from config.presetConfigs rather than copied, because a copy cannot catch the - // drift this test exists to catch. - "housellm": housellmExtractLLM(t), - } - // The tool output sits at index 1, so MaxCachedIdx 1 puts it inside the cached prefix and - // MaxCachedIdx 0 leaves it in the tail. One number is the whole difference. - for _, pos := range []struct { - name string - maxCachedIdx int - wantCall bool - }{ - {"at depth, inside the cached prefix", 1, false}, - {"in the uncached tail", 0, true}, - } { - for name, cfg := range cfgs { - t.Run(pos.name+"/"+name, func(t *testing.T) { - off := newComp(t, "extract_llm", cfg) - cm := &countingModel{resp: filter} - req := &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{ - userMsg("find the keep records"), toolMsg(body), - }} - c := &components.Ctx{Ctx: context.Background(), Session: "s1", - Store: store.NewMemory(store.Options{}), Model: components.ModelSpec{Static: cm}, - // 75k, not 1M, and the number is measured rather than picked: this request is - // 60,026 tokens (schema.MessagesTokens). With no explicit min_tokens the - // `defaults` config decides on context PRESSURE, and against a 1M window - // 60k is 0.06 — far under the 0.25 bar — so it declined for a reason with - // nothing to do with caching, and the depth case passed without exercising - // anything. 75k puts pressure at 0.80, past the 0.60 bar that fires on - // pressure alone, so position is the only variable left. - CacheAware: true, MaxCachedIdx: pos.maxCachedIdx, CtxWindow: 75_000} - var rep components.Report - if _, err := off.Offload(req, &rep, c); err != nil { - t.Fatal(err) - } - if got := cm.calls > 0; got != pos.wantCall { - if pos.wantCall { - t.Fatalf("a 240k-token candidate in the UNCACHED tail must be worth a "+ - "call — it is billed at the cache-write rate, not the read rate. "+ - "calls=%d gates=%v", cm.calls, rep.Gates) - } - t.Fatalf("a default config must NOT spend on content inside the live cached "+ - "prefix (read-rate saving, plus a forced suffix re-write), calls=%d", cm.calls) - } - }) - } - } -} +func housellmExtractLLM(t *testing.T) string { return housellmBlock(t, "extract_llm") } -// TestHousellmColdSweepActuallyFires is the other half of -// TestNoDefaultConfigRunsExtractLLMOnCachingBackend, and it exists because the preset -// shipped for a day in a state where BOTH halves were silent. +// TestHousellmSweepActuallyFires is the other half of +// TestDefaultConfigsSpendOnlyOnTheUncachedTail. // -// Every extraction call this service has ever made was a cold one, so cold_cache.min_tokens -// is the single knob deciding whether extract_llm does anything at all. It was 3000, and at -// 3000 production recorded `below_output_floor` on all 36 sweeping turns and zero -// extractions across 3,437 requests — the component was configured into a no-op while -// looking fully enabled. A candidate of ~1,500 tokens is the size that regression turned -// away, so that is what this asserts on: the preset, not a copy of it, must call the model -// on a cold turn. +// Every extraction call this service has ever made was on a turn whose cache had gone, so the sweep's +// min_tokens is the single knob deciding whether the compaction-model pass does anything at all. It was +// 3000, and at 3000 production recorded `below_output_floor` on all 36 sweeping turns and zero +// extractions across 3,437 requests — the component was configured into a no-op while looking fully +// enabled. A candidate of ~1,500 tokens is the size that regression turned away, so that is what this +// asserts on: the preset, not a copy of it, must ask on a turn inside the pre-expiry window. // -// Raising the preset's cold floor above ~1,500 fails this; re-adding -// allow_on_caching_backend fails the warm guard above. The pair pins the economics from -// both sides. -func TestHousellmColdSweepActuallyFires(t *testing.T) { - off := newComp(t, "extract_llm", housellmExtractLLM(t)) - // ~1,500 tokens of the noise the sweep is meant to reduce, with a filterable shape. +// It drives the sweep through a PREFIX ASKER rather than a Model, because that is the only way the +// component reaches a model at all: it asks the REQUEST's own model over that model's prompt cache. A +// stubbed Model would leave it declining with sweep_no_asker, which is the "configured into a no-op" +// failure this guard exists to catch, one layer down. +// +// Raising the preset's floor above ~1,500 fails this; re-adding allow_on_caching_backend fails the +// warm guard above. The pair pins the economics from both sides. +func TestHousellmSweepActuallyFires(t *testing.T) { + off := newComp(t, "extract_llm_sweep", housellmBlock(t, "extract_llm_sweep")) + // ~1,500 tokens of the noise the sweep is meant to remove. body := `[` for i := 0; i < 120; i++ { if i > 0 { @@ -320,24 +245,53 @@ func TestHousellmColdSweepActuallyFires(t *testing.T) { body += `{"id":` + strconv.Itoa(i) + `,"name":"record ` + strings.Repeat("payload ", 6) + `"}` } body += `]` - cm := &countingModel{resp: "data = json.decode(INPUT)\nOUTPUT = json.encode(data[:2])\n"} + asker := &stubAsker{reply: `[{"i":0,"needed_by":"none","quote":"","verdict":"drop"}]`, cacheRead: 19595} req := &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{ - userMsg("summarize the records"), toolMsg(body), + userMsg("summarize the records"), }} - // ColdCache is what makes this a sweep: the prefix TTL has expired, so the whole - // transcript is about to be re-billed at the write rate and savedTokenValue reports - // cached:false — which is why the warm guard's decline does not apply here. - c := &components.Ctx{Ctx: context.Background(), Session: "cold1", - Store: store.NewMemory(store.Options{}), Model: components.ModelSpec{Static: cm}, - CacheAware: true, ColdCache: true, MaxCachedIdx: -1, CtxWindow: 1_000_000} + // ENOUGH CANDIDATES TO CLEAR THE PRESET'S INVENTORY FLOOR, which is part of what this guard pins. + // A single candidate is the per-output shape the design refutes at 6% live-kept, and the shipped + // component now declines it rather than asking — so a one-output fixture would assert that the + // preset does something it is deliberately unwilling to do, and would read as "configured into a + // no-op" for the wrong reason. Ten is the shipped default, so this fixture is the smallest + // transcript the preset will actually act on. + for i := 0; i < 10; i++ { + req.Input = append(req.Input, toolMsg(body)) + } + // INSIDE THE PRE-EXPIRY WINDOW: the cache still exists (idle below the TTL) and is within a minute + // of expiring, which is where the ask can still read it and what it invalidates is nearly + // worthless. A bare `ephemeral` mark buys the 5-minute TTL this uses. + c := &components.Ctx{Ctx: context.Background(), Session: "prexp1", + Store: store.NewMemory(store.Options{}), CacheAware: true, MaxCachedIdx: -1, + CtxWindow: 1_000_000, ColdCache: false, + IdleMs: 4 * 60 * 1000, CacheTTLMs: 5 * 60 * 1000, PrefixAsk: asker} var rep components.Report if _, err := off.Offload(req, &rep, c); err != nil { t.Fatal(err) } - if cm.calls == 0 { - t.Fatalf("the housellm preset made NO model call on a cold turn with a ~1.5k-token "+ + if asker.calls == 0 { + t.Fatalf("the housellm preset made NO ask in the pre-expiry window with a ~1.5k-token "+ "candidate — the component is configured into a no-op. gates=%v", rep.Gates) } + // AND IT ACTED ON THE VERDICT. An ask that removes nothing is the same no-op from the operator's + // side, and it is what a floor regression one layer down would look like. + if rep.Events["sweep_dropped"] == 0 { + t.Fatalf("the sweep asked and removed nothing. gates=%v", rep.Gates) + } +} + +// stubAsker is a components.PrefixAsker that answers without a provider, reporting the cache read the +// sweep gates on. A read of zero would make it decline, so a test that wants it to act must say the +// read happened. +type stubAsker struct { + reply string + cacheRead int + calls int +} + +func (s *stubAsker) Ask(_ context.Context, _, _ string) (string, components.PrefixUsage, error) { + s.calls++ + return s.reply, components.PrefixUsage{CacheRead: s.cacheRead, Fresh: 40, Output: 60}, nil } // TestHousellmDoesNotAttemptTheTailBelowBreakEven pins the floor that makes the warm/tail diff --git a/components/component.go b/components/component.go index e50081fe..954c4d10 100644 --- a/components/component.go +++ b/components/component.go @@ -92,6 +92,78 @@ type Remodeler interface { AsModel(id string) Model } +// Budgeter is an optional interface on a Model: the same endpoint, the same credential and the same +// model id, with a larger REPLY budget. +// +// It exists because a truncated reply is the worst outcome available — full price, zero result — and +// it is indistinguishable from a model declining to act. MEASURED (`659e7a6`): the batched +// adjudication arm had 24 of 34 replies unparseable, and the cause was the client's default output +// budget, not the prompt. A verdict array over a dozen items, each carrying an obligation label and a +// verbatim quote, is simply long; a request model running adaptive thinking spends part of the budget +// before emitting any text at all. The array was cut mid-flight, the parse failed, and the caller +// changed nothing — misread as "the model declined" for three iterations. +// +// Why an interface rather than raising cheapmodel.DefaultMaxTokens: the budget a caller needs is a +// property of what it ASKS FOR, not of the endpoint. A one-tool-output compaction reply and a +// twelve-verdict array are different lengths, and raising the shared default would move both. +// +// Output tokens bill as generated, not as budgeted, so a raised ceiling costs nothing until used. +// Optional, and callers must fall back to the client as-is: a Model implementation that does not +// support it still works, it just keeps its own budget. +type Budgeter interface { + WithMaxTokens(n int) Model +} + +// PrefixUsage reports what one PrefixAsk cost, straight from the provider's usage block. It exists +// because the whole point of a prefix ask is the cache READ, and a cache read that silently is not +// happening looks identical to one that is — except on the bill. +// +// RETURNED rather than merely recorded, and that is the load-bearing part of this type. A caller +// whose entire justification is the cache read has to be able to gate on whether the read happened, +// which a metrics counter cannot support. +type PrefixUsage struct { + CacheRead int + CacheWrite int + Fresh int + Output int +} + +// PrefixAsker completes `ask` as a trailing user message appended to the EXACT body this session +// sent upstream on the PREVIOUS turn. +// +// WHY THE PREVIOUS TURN'S SENT BODY AND NOT THE INCOMING ONE. The provider's prompt cache was +// populated by what context-guru emitted, which is the COMPACTED form. The incoming body is +// uncompacted, so it diverges from the cached bytes at the first thing any component removed, and +// everything after that point is a miss. Appending to the bytes actually sent is the only +// construction that reliably reads the cache — measured at 19,595 read against 0 created. +// +// THE CONSEQUENCE, stated here rather than left to be rediscovered: the ask sees the transcript as of +// the PREVIOUS turn, so the newest tool output is invisible to it. That is acceptable for the +// judgement this serves — the missing part is tail content, which has had no turns in which to be +// superseded and would be kept anyway — and it has the side benefit of keeping a large model call off +// the agent's critical path. +// +// nil when the host cannot support it: no stashed body for this session yet, the first turn, a +// non-Anthropic route, or the feature switched off. A caller must decide for itself what nil means; +// see extract_llm_sweep, which DECLINES rather than falling back, because the fallback is the cost the +// mechanism exists to avoid. +type PrefixAsker interface { + Ask(ctx context.Context, session, ask string) (reply string, usage PrefixUsage, err error) +} + +// ErrNoPrefix is what Ask returns on the FIRST turn of a session: nothing has been forwarded yet, so +// there is no cached prefix to append to. +// +// Declared here rather than in the host so a component can tell it apart from a transport failure +// without string matching. The distinction is worth a sentinel because the two mean opposite things to +// an operator: "there was nothing to read yet", which every session does once and which needs no +// attention, against "the read failed", which does. +var ErrNoPrefix = errNoPrefix{} + +type errNoPrefix struct{} + +func (errNoPrefix) Error() string { return "no stashed prefix for this session" } + // ModelSpec carries the LLM clients a NeedsModel component may use, resolved per // request by the host adapter. Incoming is the proxied request's own model + // credentials (nil when unavailable, e.g. the AuthBridge host); Static is a @@ -254,6 +326,20 @@ type Ctx struct { // -1 = unknown/first turn/cache off ⇒ no tail restriction. Only meaningful when // CacheAware is true. MaxCachedIdx int + // PrefixAsk, when non-nil, lets a component put a question to the request's own model with the + // previous turn's SENT body as the prefix, so the provider reads its prompt cache instead of + // being re-sent the transcript. See PrefixAsker for why that body and not the incoming one. + PrefixAsk PrefixAsker + // CacheTTLMs is how long this request's prompt cache is assumed to live, in milliseconds, as + // DERIVED from the request rather than assumed: for the Anthropic family the body declares it + // (a bare `ephemeral` mark is 5 minutes, an explicit `ttl: "1h"` is an hour), widened to the + // longest lifetime this prefix has ever asked for. 0 when unknown. + // + // Carried alongside IdleMs and ColdCache so a component can reason about where in the cache's + // LIFETIME this turn falls, not merely whether the entry is already gone. extract_llm_sweep + // needs exactly that: a prefix ask must read a cache that still EXISTS, while rewriting deep + // history wants one that is nearly worthless — which is a window before expiry, not after it. + CacheTTLMs int64 // FilterStats receives cmdfilter's per-filter ledger (which command families pay // off, and which output shapes matched nothing). nil = not recording. // @@ -413,6 +499,21 @@ type Report struct { // sat at zero on a whole workload without anyone being able to say which case each // was in. Filled by the component via Gate(); rolled up into /stats per component. Gates map[string]int + // Events counts, per named event, things this component DID rather than declined — a cache + // hit replayed, a candidate reached at depth, an output removed, an inventory offered. + // + // Separate from Gates because they were one map and the name lied. Everything in Gates is + // exported as `cg_component_gate_declines_total`, so `reapplied_same_session` (a cache HIT) + // and `sweep_dropped` (a removal that WORKED) were being counted as declines: anyone summing + // that series to gauge whether the pipeline was doing anything got the wrong SIGN, because the + // more a component succeeded the higher its "declines" climbed. Splitting the map rather than + // classifying names in the exporter puts the judgement at the call site, where the author knows + // which one it is, instead of in a lookup table that goes stale the next time a gate is added. + // + // A name must not appear in both maps for one component: that would mean the component cannot + // say whether the thing succeeded or was refused, and TestGatesAndEventsAreDisjoint pins it. + // Filled via Event()/EventN(); rolled up into /stats beside Gates. + Events map[string]int // Calls records each LLM call this component made on this request. Empty for every // deterministic component; one entry per model call for the two that make them. // @@ -510,6 +611,53 @@ func (r *Report) Gate(name string) { r.Gates[name]++ } +// GateN records n at once, for a gate whose subject is a COUNT rather than a single candidate. +// +// It exists because a per-candidate loop cannot express "this many were OFFERED". The distinction is +// not cosmetic: a live batched-adjudication arm reported 2.80 verdicts per call and that was read as +// the batch size, when it counted what the model chose to ANSWER rather than what it was SHOWN. The +// truncation counter was firing on 43 of 162 calls at the same time, which is arithmetically +// impossible for batches of 2.8 — the resolution being that the model silently omitted labels. +// Without a way to count the offer, "the batch is starved" and "the model answered for a third of the +// batch" are the same number, and the first reading cost three iterations. +func (r *Report) GateN(name string, n int) { + if r == nil || n <= 0 { + return + } + if r.Gates == nil { + r.Gates = map[string]int{} + } + r.Gates[name] += n +} + +// Event records that the component DID the named thing once — a replay served, a candidate reached +// at depth, an output removed. The counterpart to Gate, and the distinction is the whole reason both +// exist: a decline and a success exported under one metric name called "declines" produced a series +// whose value rose as the component worked better. See Report.Events. +// +// Same stability rule as Gate: the names are read off /stats and scraped by label. +func (r *Report) Event(name string) { + if r == nil { + return + } + if r.Events == nil { + r.Events = map[string]int{} + } + r.Events[name]++ +} + +// EventN records n at once, for an event whose subject is a COUNT rather than a single candidate — +// how many were offered, how many were removed. GateN's rationale applies unchanged. +func (r *Report) EventN(name string, n int) { + if r == nil || n <= 0 { + return + } + if r.Events == nil { + r.Events = map[string]int{} + } + r.Events[name] += n +} + // Saved returns non-negative tokens saved by this component. func (r Report) Saved() int { if r.TokensAfter > r.TokensBefore { diff --git a/components/gates_events_test.go b/components/gates_events_test.go new file mode 100644 index 00000000..5805f46f --- /dev/null +++ b/components/gates_events_test.go @@ -0,0 +1,49 @@ +package components + +import "testing" + +// A NAME MUST NOT BE BOTH A DECLINE AND AN EVENT for one component. +// +// The two maps answer opposite questions — did this component turn a candidate away, or did it do +// something — and they are exported as two Prometheus series. A name in both means the component +// cannot say which happened, and a consumer summing either series double-counts it. +// +// This is the invariant that replaces the old arrangement, where everything landed in Gates and was +// exported under `cg_component_gate_declines_total`. A cache hit (`reapplied_same_session`) and a +// removal that worked (`sweep_dropped`) were counted as declines there, so the series ROSE as a +// component worked better and anyone reading it to judge pipeline effectiveness got the wrong sign. +func TestGatesAndEventsAreDisjoint(t *testing.T) { + var r Report + r.Gate("below_output_floor") + r.GateN("cached_prefix", 3) + r.Event("sweep_dropped") + r.EventN("sweep_offered", 12) + + if len(r.Gates) != 2 || len(r.Events) != 2 { + t.Fatalf("gates=%v events=%v: both maps must fill independently", r.Gates, r.Events) + } + for name := range r.Gates { + if _, both := r.Events[name]; both { + t.Errorf("%q is recorded as BOTH a decline and an event; a consumer summing either "+ + "series counts it twice, and the component cannot say which happened", name) + } + } + if r.Gates["cached_prefix"] != 3 || r.Events["sweep_offered"] != 12 { + t.Errorf("the N variants must add rather than overwrite: gates=%v events=%v", r.Gates, r.Events) + } +} + +// Event must be nil-safe and must not count a non-positive N, matching Gate/GateN exactly. A +// component holding a nil Report is the ordinary case for an emitter that was not wired. +func TestEventMatchesGateOnEdgeCases(t *testing.T) { + var nilRep *Report + nilRep.Event("x") // must not panic + nilRep.EventN("y", 1) // must not panic + + var r Report + r.EventN("zero", 0) + r.EventN("negative", -5) + if len(r.Events) != 0 { + t.Errorf("a non-positive N must record nothing, got %v", r.Events) + } +} diff --git a/components/offload/extract_cold_test.go b/components/offload/extract_cold_test.go index fb317d0d..0e9432be 100644 --- a/components/offload/extract_cold_test.go +++ b/components/offload/extract_cold_test.go @@ -1,106 +1,23 @@ package offload import ( - "context" - "strconv" "strings" - "sync" - "sync/atomic" "testing" - "time" - bschemas "github.com/maximhq/bifrost/core/schemas" "github.com/rossoctl/context-guru/components" - "github.com/rossoctl/context-guru/store" ) -// The cold-cache sweep. Measured motivation, on this deployment over 1.4 days: turns whose -// prompt cache had expired were 219 of 5,596 requests (4%) but $360 of $1,173 of spend -// (31%) — $1.64 each against $0.144 for a warm turn — because all 56.7M of their tokens -// billed as cache_creation. The shipped pipeline saved 0.015% of that. +// The cold sweep left this component. It is `extract_llm_sweep` now, and the behaviour that used to +// live here — reaching depth on a cold turn, the sweep's own floor and cap, min_idle_seconds, the +// context mode, not drawing on the hot path's session budget — is tested against that component in +// extract_sweep_test.go, where it is exercised through the shape that was actually measured good +// (batched adjudication) rather than through a compaction pass pointed at deep history. // -// Two things are true only on such a turn, and both are load-bearing below: -// - rewriting deep history is free (there is no live cached prefix to invalidate); -// - a removed token is worth the cache-WRITE rate, 12.5x its warm-turn value. - -// coldReq builds a transcript whose BIG tool output sits at depth (index 1), well before -// the cached-prefix boundary, so only a sweep can reach it. -func coldReq() *bschemas.BifrostChatRequest { - return &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{ - userMsg("Find the auth timeout in src/api/users.py and fix it."), - toolResultMsg(strings.Repeat("2024-01-01 GET /users/42 200 12ms src/api/users.py\n", 700)), - assistantMsg("Read the file next."), - toolResultMsg(strings.Repeat("filler line to grow the transcript\n", 50)), - userMsg("keep going"), - }} -} - -func coldCtx(session string, cold bool, idleMs int64, model components.Model) *components.Ctx { - return &components.Ctx{ - Session: session, Ctx: context.Background(), - Store: store.NewMemory(store.Options{}), CtxWindow: 1_000_000, - // CacheAware with the boundary AFTER the big output: index 1 is inside the cached - // prefix, so the tail gate blocks it on a warm turn. - CacheAware: true, MaxCachedIdx: 3, - ColdCache: cold, IdleMs: idleMs, - Model: components.ModelSpec{Static: model, Incoming: model}, - } -} - -// The core of the feature: on a cold turn the sweep reaches a tool output at DEPTH that the -// tail gate blocks on a warm turn. Without the sweep, the warm arm records cached_prefix and -// makes no call — which is correct there and is what leaves the cold turn's money on the -// table today. +// Two things stay here, because neither is about the sweep component: // -// PROVEN TO FAIL WITHOUT THE CHANGE: with `!sweeping` removed from the tail-gate condition, -// the cold subtest reports calls=0 and gates map[cached_prefix:2 cached_prefix_above_floor:1]. -func TestColdSweepReachesDepthThatAWarmTurnCannot(t *testing.T) { - for _, tc := range []struct { - name string - yaml string - cold bool - wantCall bool - wantGate string - }{ - { - name: "warm turn: the tail gate protects the cached prefix", - yaml: "cold_cache:\n enabled: true\nstrategy: code\neconomic_gate: false\n", - cold: false, - wantCall: false, - wantGate: "cached_prefix", - }, - { - name: "cold turn: the prefix is gone, so depth is fair game", - yaml: "cold_cache:\n enabled: true\nstrategy: code\neconomic_gate: false\n", - cold: true, - wantCall: true, - }, - { - name: "cold turn with the sweep disabled changes nothing", - yaml: "strategy: code\neconomic_gate: false\n", - cold: true, - wantCall: false, - wantGate: "cached_prefix", - }, - } { - t.Run(tc.name, func(t *testing.T) { - model := &silentModel{} - e := newSizeComponent(t, model, tc.yaml) - rep := &components.Report{} - c := coldCtx("cold-"+tc.name, tc.cold, 3_600_000, model) - if _, err := e.Offload(coldReq(), rep, c); err != nil { - t.Fatalf("Offload must fail open: %v", err) - } - calls := atomic.LoadInt64(&model.calls) - if tc.wantCall != (calls > 0) { - t.Fatalf("calls=%d, wantCall=%v (gates: %v)", calls, tc.wantCall, rep.Gates) - } - if tc.wantGate != "" && rep.Gates[tc.wantGate] == 0 { - t.Fatalf("expected the %s gate, got %v", tc.wantGate, rep.Gates) - } - }) - } -} +// - the PRICING of a cold turn, which extract_llm still sees: it can run on a cold turn like any +// other, it just no longer treats one specially. +// - the refusal of the keys that moved, which is a property of THIS component's config surface. // A cold turn's tokens are re-billed as cache CREATION at 1.25x fresh, so a removed token is // worth 12.5x its warm-turn value. Getting this backwards is what would make the gate @@ -126,253 +43,53 @@ func TestColdTurnPricesTokensAtTheWriteRate(t *testing.T) { } } -// min_idle_seconds may only RAISE the bar. The TTL check is the correctness condition — -// whether the cache is actually gone — and this is extra caution on top of it. -func TestColdSweepMinIdleRaisesTheBar(t *testing.T) { - e := newSizeComponent(t, &silentModel{}, - "cold_cache:\n enabled: true\n min_idle_seconds: 1800\nstrategy: code\n") - x := e +// THE KEYS THAT MOVED MUST SAY SO. Breaking existing configs is deliberate — there is one deployment +// and it is migrated by hand — but a removed key that produces "field not found" reads as a typo +// rather than as a relocation, and `cold_cache: {enabled: true}` silently accepted would read as "the +// sweep is on" while nothing swept. That is the most expensive available misreading of this config: +// the sweep exists for the turns measured at 4% of requests and 31% of spend. +func TestKeysThatMovedToTheSweepAreRefusedByName(t *testing.T) { for _, tc := range []struct { - name string - cold bool - idleMs int64 - want bool + key, yaml, wants string }{ - {"cold but only 10 minutes idle", true, 600_000, false}, - {"cold and an hour idle", true, 3_600_000, true}, - {"warm, however long idle", false, 7_200_000, false}, + {"per_output", "per_output: false\n", "warm/tail pass"}, + {"cold_cache", "cold_cache:\n enabled: true\n", "extract_llm_sweep"}, + {"cold_cache.min_tokens", "cold_cache:\n min_tokens: 800\n", "min_tokens"}, + {"cold_cache.max_calls", "cold_cache:\n max_calls: 2\n", "max_calls"}, } { - t.Run(tc.name, func(t *testing.T) { - got := x.sweepThisRequest(coldCtx("x", tc.cold, tc.idleMs, nil)) - if got != tc.want { - t.Fatalf("sweepThisRequest = %v, want %v", got, tc.want) + t.Run(tc.key, func(t *testing.T) { + _, err := newExtractLLM([]byte(tc.yaml)) + if err == nil { + t.Fatalf("%s was accepted; the operator would believe it still does something", tc.key) + } + // It must name the REPLACEMENT, not merely reject the key. A generic yaml + // "field not found" is what this test exists to rule out. + if !strings.Contains(err.Error(), "extract_llm_sweep") { + t.Errorf("error does not name the component the key moved to: %v", err) + } + if !strings.Contains(err.Error(), tc.wants) { + t.Errorf("error does not say what %s becomes (want mention of %q): %v", + tc.key, tc.wants, err) } }) } } -// THE SWEEP MUST NOT FORCE `context: full`. It used to, and that one line was the largest -// single cost in this component: `full` renders the whole request (measured 138,596 context -// tokens on a 138,341-token request), once per candidate, so the break-even removal at k=4 -// was 113,286 tokens — more than the transcript holds — against 6,833 under `recent`. -// -// The original justification was that a full transcript is needed to judge what an old -// message may lose. It was tested and it is the keep-list, not the context, that carries -// that: a full-transcript context took acceptance from 3/4 to 0/6 because every unique token -// in the noise became a required identifier, and HarvestIdentifiers now reads ctxRecent -// explicitly, so the two concerns are separate. Measured on bench/cold.jsonl (8 requests, -// coldness verified by cache_read=0), `full` spent $0.0387 on a 36,686-token prompt to -// remove 0 tokens. -// -// So the configured mode governs on a sweep exactly as it does on a warm turn, and an -// operator who wants the old behaviour writes `context: full`. -func TestColdSweepHonoursTheConfiguredContextMode(t *testing.T) { - goalOnly := newSizeComponent(t, &silentModel{}, - "context: goal\ncold_cache:\n enabled: true\nstrategy: code\n") - if warm, swept := goalOnly.extractionContext(ctxReq(), false), goalOnly.extractionContext(ctxReq(), true); warm != swept { - t.Fatalf("sweep overrode context: goal (%d bytes warm, %d bytes swept)", len(warm), len(swept)) - } - if s := goalOnly.extractionContext(ctxReq(), true); strings.Contains(s, "file body line") { - t.Fatal("sweep included tool output under context: goal, so it is still rendering the transcript") +// And the component must still BUILD with an empty config: the split removed keys, it did not add a +// required one. A config error here would take the whole pipeline down at boot. +func TestExtractLLMStillBuildsWithNoConfig(t *testing.T) { + c, err := newExtractLLM(nil) + if err != nil { + t.Fatalf("empty config must build: %v", err) } - // `full` still means full, on the sweep and off it — the escape hatch has to work. - full := newSizeComponent(t, &silentModel{}, - "context: full\ncold_cache:\n enabled: true\nstrategy: code\n") - if s := full.extractionContext(ctxReq(), true); !strings.Contains(s, "file body line") { - t.Fatal("context: full excluded tool output, so the escape hatch no longer renders the transcript") - } -} - -// The two paths are switched independently, so a sweep must not drain the hot path's -// per-session allowance (or the reverse) depending on which happened to fire first. -func TestSweepDoesNotConsumeTheSessionBudget(t *testing.T) { - model := &silentModel{} - e := newSizeComponent(t, model, "fire_on: size\nmin_tokens: 500\nstrategy: code\n"+ - "economic_gate: false\nllm_max_per_session: 1\ncold_cache:\n enabled: true\n") - - // One cold sweep first. - rep := &components.Report{} - if _, err := e.Offload(coldReq(), rep, coldCtx("shared", true, 3_600_000, model)); err != nil { - t.Fatal(err) - } - afterSweep := atomic.LoadInt64(&model.calls) - if afterSweep == 0 { - t.Fatalf("the sweep made no call (gates: %v)", rep.Gates) - } - - // Then a warm turn on the SAME session, with distinct content so no cache answers it. - rep2 := &components.Report{} - req := coldReq() - req.Input[1] = toolResultMsg(strings.Repeat("distinct warm output line\n", 700) + "x") - c := coldCtx("shared", false, 0, model) - c.MaxCachedIdx = 0 // put the big output in the tail so only the budget can stop it - if _, err := e.Offload(req, rep2, c); err != nil { - t.Fatal(err) - } - if atomic.LoadInt64(&model.calls) == afterSweep { - t.Fatalf("the warm turn made no call after a sweep: the sweep consumed the "+ - "per-session budget it is not supposed to touch (gates: %v)", rep2.Gates) - } -} - -// A component configured to do nothing at all is a configuration error, not a silent no-op -// sitting in someone's pipeline looking enabled. -func TestPerOutputFalseWithoutColdSweepIsRejected(t *testing.T) { - if _, err := newExtractLLM([]byte("per_output: false\n")); err == nil { - t.Fatal("per_output: false with cold_cache disabled was accepted") + if _, ok := c.(*ExtractLLM); !ok { + t.Fatal("constructor returned the wrong type") } - if _, err := newExtractLLM([]byte("per_output: false\ncold_cache:\n enabled: true\n")); err != nil { - t.Fatalf("sweep-only configuration rejected: %v", err) + // The keys that remain must be untouched by the removal. + e := c.(*ExtractLLM) + if e.minTokens != 300 || e.strategy != "code" { + t.Errorf("the surviving defaults moved: min_tokens=%d strategy=%q", e.minTokens, e.strategy) } } -// per_output: false must leave the hot path alone while still sweeping. This is the -// configuration the deployment is expected to use first, since the sweep is the half whose -// economics are unambiguous. -func TestSweepOnlyConfigurationSkipsWarmTurns(t *testing.T) { - model := &silentModel{} - e := newSizeComponent(t, model, "per_output: false\nstrategy: code\neconomic_gate: false\n"+ - "cold_cache:\n enabled: true\n") - - rep := &components.Report{} - c := coldCtx("sweep-only", false, 0, model) - c.MaxCachedIdx = 0 // the big output is in the tail: nothing but per_output can stop it - if _, err := e.Offload(coldReq(), rep, c); err != nil { - t.Fatal(err) - } - if got := atomic.LoadInt64(&model.calls); got != 0 { - t.Fatalf("a warm turn made %d call(s) with per_output: false", got) - } - if rep.Gates["per_output_disabled"] == 0 { - t.Fatalf("no per_output_disabled gate, so the skip is undiagnosable: %v", rep.Gates) - } - - rep2 := &components.Report{} - if _, err := e.Offload(coldReq(), rep2, coldCtx("sweep-only", true, 3_600_000, model)); err != nil { - t.Fatal(err) - } - if atomic.LoadInt64(&model.calls) == 0 { - t.Fatalf("the cold turn made no call either, so the component does nothing at all: %v", - rep2.Gates) - } -} - -// The sweep's own cap bounds one turn's calls, and the refusal is countable. -func TestColdSweepCapIsItsOwn(t *testing.T) { - model := &silentModel{} - e := newSizeComponent(t, model, "strategy: code\neconomic_gate: false\n"+ - "llm_max_per_request: 1\ncold_cache:\n enabled: true\n max_calls: 2\n min_tokens: 100\n") - - req := &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{ - userMsg("Find the auth timeout and fix it."), - }} - for i := 0; i < 5; i++ { - req.Input = append(req.Input, - toolResultMsg(strings.Repeat("log line for candidate "+strconv.Itoa(i)+"\n", 200))) - } - req.Input = append(req.Input, userMsg("keep going")) - - rep := &components.Report{} - if _, err := e.Offload(req, rep, coldCtx("cap", true, 3_600_000, model)); err != nil { - t.Fatal(err) - } - // The sweep cap governs, NOT llm_max_per_request (which is 1). - if got := atomic.LoadInt64(&model.calls); got != 2 { - t.Fatalf("made %d calls, want the sweep's cap of 2 (gates: %v)", got, rep.Gates) - } - if rep.Gates["over_cold_sweep_cap"] == 0 { - t.Fatalf("the cap dropped candidates with no gate recorded: %v", rep.Gates) - } - if rep.Gates["over_per_request_cap"] != 0 { - t.Fatal("the hot path's per-request cap was applied to a sweep") - } -} - -// overlapModel records whether any two calls were ever in flight at once, and how many ran. -type overlapModel struct { - mu sync.Mutex - inFlight int - overlaps int - calls int -} - -func (m *overlapModel) Complete(context.Context, string) (string, error) { - m.mu.Lock() - m.calls++ - m.inFlight++ - if m.inFlight > 1 { - m.overlaps++ - } - m.mu.Unlock() - time.Sleep(5 * time.Millisecond) // wide enough for a sibling to arrive if one is coming - m.mu.Lock() - m.inFlight-- - m.mu.Unlock() - return "", nil -} - -// THE CACHE WRITE HAS TO BE EARNED BEFORE ANYTHING CAN READ IT. `CacheContext = len(cands) > 1` -// moves the conversation context into a cacheable system block, but cheapmodel.claimCacheWrite -// deliberately withholds the breakpoint from CONCURRENT siblings — a cache entry that is only -// ever written costs 1.25x fresh and buys nothing. So with llmConcurrency = 4 the first call -// took the write slot and calls 2..4 sent no mark and paid plain fresh input for the identical -// context. Measured on production: five haiku calls on ONE request each sent ~138,000 prompt -// tokens with cache_read = 0 AND cache_write = 0. -// -// The sweep therefore issues its first call ALONE, then the rest concurrently. At T = 180k, -// k = 4 that moves break-even removal from 198,620 tokens to 79,088. -// -// The warm per-output path stays fully concurrent: serializing costs a whole gateway queue -// round (~2-4 s p50, tail 12-16 s — latency here is queue time, not prompt size), which is -// worth paying only on a turn whose entire transcript is being re-billed at 1.25x fresh. -func TestSweepEarnsTheContextCacheWriteBeforeReadingIt(t *testing.T) { - // Two big outputs at depth, so the sweep has k >= 2 and CacheContext is on. The salt keeps - // the two subtests' content DISTINCT: the extraction result cache and the seen-content - // ledger are process-wide, so reusing bytes makes the second subtest answer from state and - // make no calls at all. - req := func(salt string) *bschemas.BifrostChatRequest { - return &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{ - userMsg("Find the auth timeout in src/api/users.py and fix it."), - toolResultMsg(strings.Repeat("2024-01-01 GET /users/42 200 12ms src/api/users.py "+salt+"\n", 700)), - assistantMsg("Now the second log."), - toolResultMsg(strings.Repeat("2024-01-02 POST /login 500 31ms src/api/auth.py "+salt+"\n", 700)), - userMsg("keep going"), - }} - } - run := func(t *testing.T, yaml string, cold bool) *overlapModel { - t.Helper() - m := &overlapModel{} - e := newSizeComponent(t, m, yaml) - c := coldCtx("earn-"+t.Name(), cold, 3_600_000, m) - c.MaxCachedIdx = 0 // both outputs in the tail, so only the sweep/gate can stop them - rep := &components.Report{} - if _, err := e.Offload(req(t.Name()), rep, c); err != nil { - t.Fatal(err) - } - if m.calls < 2 { - t.Fatalf("fixture made %d calls, need >= 2 for the write/read split to exist (gates: %v)", - m.calls, rep.Gates) - } - return m - } - - t.Run("sweep serializes the writer", func(t *testing.T) { - m := run(t, "per_output: false\nstrategy: code\neconomic_gate: false\n"+ - "cold_cache:\n enabled: true\n min_tokens: 500\n", true) - // The FIRST call must have run alone. With k=2 that means no overlap at all; with - // more, the readers may overlap each other but never the writer. - if m.overlaps > m.calls-2 { - t.Fatalf("%d overlapping calls across %d: the first call did not run alone, so "+ - "the readers cannot have read what nothing had written yet", m.overlaps, m.calls) - } - }) - - t.Run("warm per-output path stays concurrent", func(t *testing.T) { - m := run(t, "fire_on: size\nmin_tokens: 500\nstrategy: code\neconomic_gate: false\n"+ - "cold_cache:\n enabled: true\n", false) - if m.overlaps == 0 { - t.Fatalf("no overlap across %d warm calls: the hot path was serialized too, and it "+ - "pays a whole gateway queue round (~2-4s p50) for a fraction of a cent", m.calls) - } - }) -} +var _ = components.Report{} // keep the import honest if the pricing test above is ever trimmed diff --git a/components/offload/extract_context_test.go b/components/offload/extract_context_test.go index 9bd930e9..87674311 100644 --- a/components/offload/extract_context_test.go +++ b/components/offload/extract_context_test.go @@ -159,7 +159,7 @@ func TestContextFallsBackRatherThanInventing(t *testing.T) { t.Fatal(err) } x := e.(*ExtractLLM) - if schema.TextTokens(x.extractionContext(ctxReq(), false)) == 0 { + if schema.TextTokens(x.extractionContext(ctxReq())) == 0 { t.Fatal("the component's own context renderer returned nothing to count") } } @@ -201,7 +201,7 @@ func TestKeepIdsNeverComeFromToolOutput(t *testing.T) { } x := e.(*ExtractLLM) // The prompt's context may include the payload (that is what `full` is for)... - if mode == ctxFull && !strings.Contains(x.extractionContext(req, false), "zz9plural") { + if mode == ctxFull && !strings.Contains(x.extractionContext(req), "zz9plural") { t.Fatal("full context should carry the tool output") } // ...but the keep-list must not be derived from it. diff --git a/components/offload/extract_improve_test.go b/components/offload/extract_improve_test.go index 889744cf..5159c789 100644 --- a/components/offload/extract_improve_test.go +++ b/components/offload/extract_improve_test.go @@ -6,6 +6,7 @@ import ( "math" "strings" "testing" + "time" "github.com/rossoctl/context-guru/components" "github.com/rossoctl/context-guru/internal/cheapmodel" @@ -189,27 +190,32 @@ func headLine(s string) string { return s } -// A sweep must have a bound. Production made 27 calls on one request against a tenant cap of -// 2, spent $0.229 and added 76.6 s to a turn — the sweep does not draw on the hot path's caps, -// so its own default was the only brake and it was "unlimited". -func TestColdSweepIsBoundedByDefault(t *testing.T) { - c, err := newExtractLLM([]byte("per_output: false\ncold_cache:\n enabled: true\n")) +// THE SWEEP'S SPEND BOUND IS NO LONGER A CALL CAP, and the change is worth recording because the +// number that motivated one is still in production's logs: a single sweep once made 27 model calls, +// spent $0.229 and added 76.6 s to a turn whose upstream took 33.5 s, against a tenant cap of 2. +// +// That shape is gone. The sweep makes exactly ONE call per firing turn — an ask over the transcript +// already in the request model's prompt cache, shipping an inventory rather than any output content — +// so there is nothing left for a per-call cap to bound. What bounds it now is WHEN it fires: the +// pre-expiry window, which is at most once per cache lifetime per session. +func TestSweepMakesOneCallSoNeedsNoCallCap(t *testing.T) { + c, err := newExtractSweep(nil) if err != nil { t.Fatal(err) } - e := c.(*ExtractLLM) - if e.cold.MaxCalls <= 0 || e.cold.MaxCalls > llmConcurrency { - t.Fatalf("default sweep cap = %d, want a bound at or below one concurrency round (%d)", - e.cold.MaxCalls, llmConcurrency) + e := c.(*ExtractSweep) + // The window is the brake, and it must be bounded and non-zero: zero would never fire, and an + // unbounded one would fire on every warm turn and invalidate live prefixes. + if e.preExpiry <= 0 { + t.Fatalf("the pre-expiry window is %v, so the sweep can never fire", e.preExpiry) } - // An operator can still opt out, explicitly. - uc, err := newExtractLLM([]byte("per_output: false\ncold_cache:\n enabled: true\n max_calls: -1\n")) - if err != nil { - t.Fatal(err) + if e.preExpiry > 5*time.Minute { + t.Errorf("the default window is %v, which exceeds a bare ephemeral mark's whole 5-minute "+ + "lifetime — it would fire on every turn of a session", e.preExpiry) } - un := uc.(*ExtractLLM) - if un.cold.MaxCalls != 0 { - t.Fatalf("max_calls: -1 must mean unlimited, got %d", un.cold.MaxCalls) + // And a call cap must not have quietly come back: it would be a knob that cannot bind. + if _, err := newExtractSweep([]byte("max_calls: 4\n")); err == nil { + t.Error("max_calls was accepted; one call per turn means it can never bind") } } diff --git a/components/offload/extract_llm.go b/components/offload/extract_llm.go index 05464a56..d7715152 100644 --- a/components/offload/extract_llm.go +++ b/components/offload/extract_llm.go @@ -140,8 +140,6 @@ type ExtractLLM struct { ctxMode contextMode ctxMessages int maxChars int - perOutput bool - cold coldCacheConfig // minTokensSet records whether the operator pinned min_tokens / trigger explicitly. // When they did, their threshold governs (backward compatibility). When they did not, @@ -293,48 +291,6 @@ func fitsModelContext(bodyTok, overheadTok, limit int) bool { return est+cheapExtractOutputTokens+cheapExtractSlack <= limit } -// coldCacheConfig is the whole-transcript sweep. -// -// Why it exists, and why it is not just "extraction with a bigger budget": on a turn whose -// prompt cache has expired, the provider re-bills the ENTIRE transcript as cache creation -// at 1.25x the fresh rate. Measured on this deployment over 1.4 days, those turns were 4% -// of requests and 31% of spend ($360 of $1,173, ~$1.64 per turn against $0.144 warm), and -// the shipped pipeline saved 0.015% of it. Two things are true only on that turn: removing -// a token is worth 12.5x what it is worth on a warm turn, and rewriting deep history is -// free because there is no live cached prefix left to invalidate. So the sweep is not -// aggression, it is taking the one window where the arithmetic is overwhelmingly in favour. -type coldCacheConfig struct { - Enabled bool `yaml:"enabled"` - // MinTokens is the per-output floor for the sweep (0 = 1000). Lower than the hot path's, - // because on this turn every candidate is being re-billed at the write rate anyway. - MinTokens int `yaml:"min_tokens"` - // MinIdleSeconds demands MORE idle time than the provider TTL implies (0 = just the - // TTL). Raises the bar, never lowers it: the TTL check is the correctness condition and - // this is only extra caution. - MinIdleSeconds int `yaml:"min_idle_seconds"` - // MaxCalls caps model calls for one sweep (0 = defaultColdMaxCalls; -1 = unlimited). - // - // It used to default to unlimited, on the reasoning that a sweep runs once per idle gap - // on a turn that is already expensive. MEASURED, that reasoning was wrong in the way - // unbounded spend paths usually are: one production request made 27 calls against a - // tenant whose llm_max_per_request was 2, spent $0.229 and added 76.6 s to a turn whose - // upstream took 33.5 s — context-guru was 2.3x slower than the model it was saving money - // on. The sweep deliberately does not draw on the hot path's caps (see the comment at the - // cap site), so this is the ONLY brake it has, and an unbounded default meant it had none. - // One concurrency round is the natural bound: past it the calls serialize and the latency - // grows multiplicatively for a linear gain. - MaxCalls int `yaml:"max_calls"` -} - -// defaultColdFloor is the sweep's per-output floor when none is configured. -const defaultColdFloor = 1000 - -// defaultColdMaxCalls bounds one sweep when the operator names no cap. It is llmConcurrency -// so a sweep costs ONE round of calls: the (k+1)th call cannot start until one of the first k -// returns, and at a 7.1 s median that is where a sweep starts costing more wall clock than -// the turn it is shortening. -const defaultColdMaxCalls = llmConcurrency - type extractLLMConfig struct { MinTokens int `yaml:"min_tokens"` Strategy string `yaml:"strategy"` // code (default) | single | rlm | auto @@ -355,13 +311,6 @@ type extractLLMConfig struct { // is why the only brakes left are MinTokens, LLMMaxPerReq and LLMMaxPerSess. Set // those before setting this. FireOn string `yaml:"fire_on"` - // PerOutput enables the HOT-PATH pass: reduce individual tool outputs as they arrive. - // Unset = true (today's behaviour). Set false to run only the cold-cache sweep below, - // which is a different economic proposition and deserves its own switch. - PerOutput *bool `yaml:"per_output"` - // ColdCache configures the whole-transcript sweep on a turn whose prompt cache has - // expired. Off by default. - ColdCache coldCacheConfig `yaml:"cold_cache"` // Context selects how much conversation the extraction prompt carries: // goal | recent (default) | full. See contextMode. Context string `yaml:"context"` @@ -422,6 +371,21 @@ type extractLLMConfig struct { SkipFileReads *bool `yaml:"skip_file_reads"` } +// movedToSweep names the keys the cold-sweep split took out of this component, and where each one +// went. They are refused rather than ignored: `cold_cache: {enabled: true}` silently accepted would +// read as "the sweep is on" while nothing swept, which is the most expensive possible misreading of +// this config — the sweep exists for the turns that are 4% of requests and 31% of spend. +var movedToSweep = []struct { + key, why string +}{ + {"per_output", "this component now IS the warm/tail pass, so there is nothing to switch off; " + + "remove the key. The cold sweep is a separate component in the pipeline"}, + {"cold_cache", "the whole-transcript sweep is now the `extract_llm_sweep` component. " + + "cold_cache.min_tokens becomes its min_tokens, cold_cache.min_idle_seconds its " + + "min_idle_seconds, and cold_cache.max_calls its max_calls (which now bounds BATCH calls); " + + "cold_cache.enabled becomes the component's presence in the pipeline"}, +} + func newExtractLLM(raw []byte) (components.Component, error) { cfg := extractLLMConfig{MinTokens: 300, Strategy: "code"} // Detect whether the operator pinned a threshold BEFORE defaults are applied: the @@ -442,6 +406,21 @@ func newExtractLLM(raw []byte) (components.Component, error) { (probe.Trigger.MinRequestTokens != nil || probe.Trigger.MinOutputTokens != nil)) } } + // KEYS THAT MOVED TO extract_llm_sweep, refused BEFORE components.Decode's KnownFields + // rejects them with a generic yaml message. Breaking existing configs is deliberate — there is + // one deployment and it is migrated by hand — but a removed key must say where it went, or the + // operator reads "field not found" and concludes the key was a typo rather than relocated. + if len(raw) > 0 { + var probe map[string]yaml.Node + if err := yaml.Unmarshal(raw, &probe); err == nil { + for _, m := range movedToSweep { + if _, present := probe[m.key]; present { + return nil, fmt.Errorf("extract_llm: %s has moved to the extract_llm_sweep "+ + "component: %s", m.key, m.why) + } + } + } + } if err := components.Decode(raw, &cfg); err != nil { return nil, err } @@ -464,23 +443,6 @@ func newExtractLLM(raw []byte) (components.Component, error) { if err != nil { return nil, fmt.Errorf("extract_llm: %w", err) } - perOutput := true - if cfg.PerOutput != nil { - perOutput = *cfg.PerOutput - } - if cfg.ColdCache.MinTokens <= 0 { - cfg.ColdCache.MinTokens = defaultColdFloor - } - switch { - case cfg.ColdCache.MaxCalls == 0: - cfg.ColdCache.MaxCalls = defaultColdMaxCalls - case cfg.ColdCache.MaxCalls < 0: - cfg.ColdCache.MaxCalls = 0 // an explicit opt-out of the bound - } - if !perOutput && !cfg.ColdCache.Enabled { - return nil, fmt.Errorf("extract_llm: per_output: false with cold_cache disabled " + - "leaves the component with nothing to do; remove it from the pipeline instead") - } fireOnSize := false switch cfg.FireOn { case "", "pressure": @@ -511,7 +473,6 @@ func newExtractLLM(raw []byte) (components.Component, error) { llmEveryN: cfg.LLMEveryN, llmMaxPerReq: cfg.LLMMaxPerReq, llmMaxPerSess: cfg.LLMMaxPerSess, fireOnSize: fireOnSize, aggro: aggro, ctxMode: ctxMode, ctxMessages: cfg.ContextMessages, maxChars: cfg.MaxChars, - perOutput: perOutput, cold: cfg.ColdCache, skipFileReads: cfg.SkipFileReads, llmSeen: map[string]int{}, llmSpent: map[string]int{}, minTokensSet: explicit, gate: gate, allowCached: allowCached, @@ -534,20 +495,6 @@ func (e *ExtractLLM) noteRequestSize(session string, tokens int) int { func (*ExtractLLM) Name() string { return "extract_llm" } func (*ExtractLLM) Enabled(*components.Ctx) bool { return true } -// sweepThisRequest reports whether this turn gets the whole-transcript sweep: the operator -// enabled it, the provider's cache has certainly expired, and any extra idle requirement is -// met. Everything it unlocks (rewriting at depth, pricing at the write rate) is only correct -// when the cache really is gone, so all three must hold. -func (e *ExtractLLM) sweepThisRequest(c *components.Ctx) bool { - if !e.cold.Enabled || c == nil || !c.ColdCache { - return false - } - if e.cold.MinIdleSeconds > 0 && c.IdleMs < int64(e.cold.MinIdleSeconds)*1000 { - return false - } - return true -} - // extractionContext renders the conversation the extraction prompt will carry, in the // configured mode. One method so every caller (and every test) agrees on what the model is // told — the prompt's relevance judgement rests entirely on this. @@ -571,7 +518,7 @@ func (e *ExtractLLM) sweepThisRequest(c *components.Ctx) bool { // // An operator who wants the old behaviour writes `context: full`, which now means what it // says on every turn instead of being imposed on one. -func (e *ExtractLLM) extractionContext(req *bschemas.BifrostChatRequest, _ bool) string { +func (e *ExtractLLM) extractionContext(req *bschemas.BifrostChatRequest) string { return conversationContext(req, e.ctxMode, e.ctxMessages) } @@ -705,15 +652,8 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // Resolved once: the candidate loop below tests it per tool output, and it is a // handler call rather than a field read. dbg := debugExtractLLM(c) - sweeping := e.sweepThisRequest(c) - if !e.perOutput && !sweeping { - // per_output: false — this component is here only for the cold sweep, and this is a - // warm turn. Frozen replays below still run: they are free and they are what keeps - // the prefix byte-stable. - rep.Gate("per_output_disabled") - } fires := e.trigger.Fires(req, c.CtxWindow) - goal := e.extractionContext(req, sweeping) + goal := e.extractionContext(req) query := keywords(goal) if len(query) == 0 { rep.Gate("no_goal_keywords") @@ -741,13 +681,11 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // went there. This gate is what makes an authentication failure attributable to the // credential that was actually presented. if model != nil && usedSource != "" && e.modelSource != "config" && usedSource == "config" { - rep.Gate("model_source_fell_back_to_config") + rep.Event("model_source_fell_back_to_config") } } - // Per-session cadence: on throttled steps drop the model (skip this request). The sweep - // is exempt — it happens at most once per idle gap, which is its own throttle, and - // skipping it means paying the full re-billing of the transcript instead. - if model != nil && !sweeping && fires && !e.llmAllowedThisRequest(c.Session) { + // Per-session cadence: on throttled steps drop the model (skip this request). + if model != nil && fires && !e.llmAllowedThisRequest(c.Session) { model = nil } // Derived trigger (#28 E): context pressure + growth rate replace a hand-tuned @@ -762,15 +700,9 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R if c.CtxWindow <= 0 { pressureFires, triggerReason = fires, "context window unknown; absolute trigger only" } - if model != nil && !pressureFires && !sweeping { + if model != nil && !pressureFires { model = nil // no model call this request; frozen reapplications still run below } - if model != nil && !e.perOutput && !sweeping { - model = nil // cold-sweep-only configuration, and this is a warm turn - } - if sweeping { - triggerReason = "cold cache: prompt cache expired, whole transcript re-billed" - } metrics.RecordExtractionReason(triggerReason) floor := e.outputFloor(c.CtxWindow) @@ -781,12 +713,6 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R floor = pf } } - if sweeping { - // The sweep's own floor. Every candidate on this turn is being re-billed at the - // cache-write rate whatever we do, so the bar for "worth a call" is genuinely lower - // than on a warm turn. - floor = e.cold.MinTokens - } // Gate inputs shared by every candidate this request. // // pricing is the extraction model's REAL rates where the host could resolve them. The @@ -848,39 +774,6 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // The same prompt, for the COST model rather than the window check: callCost adds the // static preamble itself, so it must be given only the variable part. goalOverhead := promptOverheadTokens + schema.TextTokens(goal) - // MODEL ESCALATION, sweep only. The sweep sends the whole transcript as context, so the - // prompt's fixed part alone can exceed a small extraction model's window — and then - // fitsModelContext correctly declines every candidate and the sweep silently does - // nothing on exactly the largest, most expensive transcripts. When that happens, fall - // back to the model the AGENT is using: it demonstrably holds this conversation, since - // it is about to be sent the same one. - escalated := false - if sweeping && model != nil && !fitsModelContext(0, promptOverhead, inputLimit) { - if inc := c.Model.For("incoming"); inc != nil && c.CtxWindow > inputLimit { - model, inputLimit, escalated = inc, c.CtxWindow, true - // The call is now going to a DIFFERENT model, so the two things derived from - // which model it is must be re-derived. Leaving them meant an escalated call was - // recorded under the pinned cheap model's id and priced at its rates — the exact - // ~3x understatement the pricing block above exists to remove, reintroduced on - // the most expensive calls the component makes. - if !c.SelfRates.Zero() { - pricing = cheapmodel.Pricing{ - InputPerMTok: c.SelfRates.Input * 1_000_000, - OutputPerMTok: c.SelfRates.Output * 1_000_000, - CacheReadPerMTok: c.SelfRates.CacheRead * 1_000_000, - CacheWritePerMTok: c.SelfRates.CacheWrite * 1_000_000, - } - } - if c.ModelName != "" { - callModel = c.ModelName - } - if dbg { - logging.From(c.Ctx).Debug("cg.extract_llm.escalate", - "reason", "transcript exceeds the extraction model's window", - "overhead_tokens", promptOverhead, "window", c.CtxWindow) - } - } - } tools := toolIndices(req) var keys []string changed := 0 @@ -940,7 +833,7 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // A cold sweep is the exception in the other direction: nothing is cached, file reads are // the largest mass in a coding transcript, and every token of them is being re-billed at // the cache-write rate — so AUTO reduces them there. - skipFR := c.CacheAware && !sweeping + skipFR := c.CacheAware if e.skipFileReads != nil { skipFR = *e.skipFileReads } @@ -984,7 +877,7 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R } apply(i, content, cached.Projected, cached.Summary) dbgReapply++ - rep.Gate("reapplied_same_session") + rep.Event("reapplied_same_session") continue } // A NEW compaction, on the UNCACHED region only (cache-safe): when cache-aware that @@ -1008,7 +901,7 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // new cache entry whatever we do, and a message at depth is exactly as free to // rewrite as one in the tail. This is the only place that restriction is lifted, and // only because the condition it protects against is provably absent. - if c.CacheAware && !sweeping && !c.TailOnly(i) { + if c.CacheAware && !c.TailOnly(i) { dbgTail++ if sz >= floor { dbgBigTailBlocked++ // a large output we skipped ONLY because it's not in the tail @@ -1055,7 +948,7 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R putResult(c, id, cached.Projected, cached.Summary) apply(i, content, cached.Projected, cached.Summary) dbgReapply++ - rep.Gate("reapplied_cross_session") + rep.Event("reapplied_cross_session") continue } } @@ -1072,29 +965,18 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // of the few parts of this component that unambiguously pays. A metric that argues for // optimizing something already working is worse than no metric. metrics.RecordExtractionCacheLookup(false) - // The operator's REQUEST-level trigger, now honored on WARM caching turns. - // - // This condition used to carry `!c.CacheAware`, and that spelling was too broad. What - // legitimately bypasses a request-size threshold is a COLD SWEEP: on a cold turn the - // whole transcript re-bills at the cache-write rate whatever the request's size, so the - // request-level threshold answers the wrong question and the sweep brings its own floor - // (e.cold.MinTokens, set above). That is why `sweeping` already overrides the cadence - // gate and the pressure gate — this check simply had not been given the same treatment. + // The operator's REQUEST-level trigger, honored on every turn this component sees. // - // `c.CacheAware` is true on warm caching turns as well as cold ones, so the old spelling - // also discarded the threshold on every warm turn, where it means exactly what the - // operator wrote. And it could ONLY discard operator configuration: Trigger's zero value - // fires always (see components/trigger.go — "a zero field is no constraint"), so `!fires` - // is reachable only when min_request_tokens / min_request_frac / min_messages was set and - // not met. There is no derived value in `fires` for a cache carve-out to protect; the - // derived pressure trigger is separate and gates the model earlier via shouldFire. + // This condition used to carry a cold-sweep carve-out, and before that `!c.CacheAware`. + // Both are gone: the sweep is its own component now, and Trigger's zero value fires always + // (see components/trigger.go — "a zero field is no constraint"), so `!fires` is reachable + // only when min_request_tokens / min_request_frac / min_messages was set and not met. There + // is no derived value here for a carve-out to protect; the derived pressure trigger is + // separate and gates the model earlier via shouldFire. // - // Found by the housellm cold-sweep preset test, which fails if `sweeping` is dropped - // here — the sweep is the part of the old carve-out that was carrying real weight. - // - // IsHuge still overrides, unchanged: a single output that large is worth a call whatever - // the request-level threshold says. - if huge := e.trigger.IsHuge(sz, c.CtxWindow); !fires && !huge && !sweeping { + // IsHuge still overrides: a single output that large is worth a call whatever the + // request-level threshold says. + if huge := e.trigger.IsHuge(sz, c.CtxWindow); !fires && !huge { rep.Gate("request_trigger_not_fired") continue } @@ -1225,33 +1107,23 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R "cacheAware", c.CacheAware, "maxCachedIdx", c.MaxCachedIdx, "floor", floor, "nInput", len(req.Input)) } - // Caps. The sweep is bounded by its OWN cap and does not draw on the hot path's - // per-request or per-session allowance: the two paths are switched independently and - // have opposite economics, so letting a sweep drain the session budget would silently - // disable the hot path (or the reverse) depending on which fired first. - if sweeping { - if e.cold.MaxCalls > 0 && len(cands) > e.cold.MaxCalls { - for k := e.cold.MaxCalls; k < len(cands); k++ { - rep.Gate("over_cold_sweep_cap") - } - cands = cands[:e.cold.MaxCalls] + // Caps. extract_llm_sweep is bounded by its OWN cap and does not draw on these: the two + // paths are switched independently and have opposite economics, so a shared budget would + // silently disable one depending on which fired first. + if e.llmMaxPerReq > 0 && len(cands) > e.llmMaxPerReq { + for k := e.llmMaxPerReq; k < len(cands); k++ { + rep.Gate("over_per_request_cap") } - } else { - if e.llmMaxPerReq > 0 && len(cands) > e.llmMaxPerReq { - for k := e.llmMaxPerReq; k < len(cands); k++ { - rep.Gate("over_per_request_cap") - } - cands = cands[:e.llmMaxPerReq] // cap model calls per request - } - // Then the session's own allowance. Reserved here, after the per-request cap, - // because every surviving candidate becomes exactly one model call in phase 2 - // below — so the reservation is the spend. - if n := e.reserveSessionBudget(c.Session, len(cands)); n < len(cands) { - for k := n; k < len(cands); k++ { - rep.Gate("over_per_session_cap") - } - cands = cands[:n] + cands = cands[:e.llmMaxPerReq] // cap model calls per request + } + // Then the session's own allowance. Reserved here, after the per-request cap, because + // every surviving candidate becomes exactly one model call in phase 2 below — so the + // reservation is the spend. + if n := e.reserveSessionBudget(c.Session, len(cands)); n < len(cands) { + for k := n; k < len(cands); k++ { + rep.Gate("over_per_session_cap") } + cands = cands[:n] } // Phase 2 (parallel): the candidate compactions are independent. A focused per-output @@ -1267,30 +1139,33 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // costs 1.25x fresh — so paying for it would be a 25% loss. Decided here because this // is the only place the final candidate count is known (the caps above trim it). extCfg.CacheContext = len(cands) > 1 - // AND THE WRITE HAS TO BE EARNED BEFORE IT CAN BE READ. Setting CacheContext was not - // enough: cheapmodel.claimCacheWrite deliberately suppresses the breakpoint on - // CONCURRENT siblings (a cache entry only ever written is worse than no breakpoint), - // so with llmConcurrency=4 the first call took the write slot and calls 2-4 sent no - // mark and paid plain fresh input for the same context — the saving the flag exists - // for was never collected. Measured on production: five haiku calls on ONE request - // each sent ~138,000 prompt tokens with cache_read=0 AND cache_write=0. - // - // So run the first call alone, then the rest concurrently: one writer, then readers. - // At T=180k, k=4 that moves break-even removal from 198,620 tokens to 79,088. - // - // IT COSTS WALL CLOCK AND THE TRADE IS DELIBERATE. Per-call latency here is gateway - // QUEUE time, not prompt size (measured: an 8-token call has a 1,812 ms p50 floor and - // is not faster than an 8k-token one), so serializing one call adds roughly one whole - // queue round — ~2-4 s p50, and the tail reaches 12-16 s. We pay it only where the - // money is overwhelming: k >= 2 on a turn whose entire transcript is being re-billed - // at 1.25x fresh. On the warm per-output path the extra second buys a fraction of a - // cent, so it stays fully concurrent. - serialFirst := extCfg.CacheContext && sweeping + // THE ONE-WRITER-THEN-READERS ORDERING WENT WITH THE SWEEP. cheapmodel.claimCacheWrite + // suppresses the breakpoint on CONCURRENT siblings (a cache entry only ever written is + // worse than no breakpoint), so with llmConcurrency=4 the first call takes the write slot + // and calls 2-4 send no mark. Serializing the first call fixes that but costs a whole + // gateway queue round — ~2-4 s p50, tail 12-16 s. It was paid only where the money is + // overwhelming: a turn whose entire transcript re-bills at 1.25x fresh. On this warm + // per-output path the extra second buys a fraction of a cent, so it stays fully concurrent + // and the flag above is best-effort. type outT struct{ projected, summary string } out := make([]outT, len(cands)) // One record per call, written to its own slot so the goroutines need no lock (a // Report is copied by value across this codebase and cannot carry one). calls := make([]components.ModelCall, len(cands)) + // AND THE SAME RULE FOR GATES, which it did not previously get (#119). + // + // The two gates raised inside runCall — deduped_inflight_extraction and reply_truncated — + // were calling rep.Gate from the goroutines. Report.Gates is a plain map with no lock, for + // exactly the reason the comment above gives, so two concurrent raises are a data race on + // a Go map. That is NOT a wrong counter and it is NOT a recoverable panic: the runtime + // aborts the process with `fatal error: concurrent map writes`, which in a proxy means the + // whole process dies rather than one component failing open. The dedup gate is the + // reachable one — singleflight releases every follower of a shared key at the same instant, + // so N identical candidates in one request give N-1 simultaneous raises. + // + // Same discipline as the records: each call appends its own gate names to its own slot, and + // the serial phase below raises them. + gateNames := make([][]string, len(cands)) sem := make(chan struct{}, llmConcurrency) var wg sync.WaitGroup runCall := func(k int) { @@ -1340,7 +1215,7 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R if !executed { // A concurrent request derived this exact result; take it and charge nothing. inflightDeduped.Add(1) - rep.Gate("deduped_inflight_extraction") + gateNames[k] = append(gateNames[k], "deduped_inflight_extraction") out[k] = outT{projected: res, summary: sum} return } @@ -1351,7 +1226,7 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R calls[k] = components.ModelCall{ Component: rep.Component, Model: callModel, Strategy: strategy, Aggressiveness: string(e.aggro), - Cold: sweeping, Escalated: escalated, + Cold: c.ColdCache, CandidateTokens: before, LatencyMs: latency, PromptTokens: inTok, CompletionTokens: outTok, CacheRead: cr, CacheWrite: cw, @@ -1368,7 +1243,7 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // real session: 26.8s and ~$0.08 for a reply cut off at 2048 tokens. if outTok >= int64(cheapExtractOutputTokens) { calls[k].GateReason = "reply truncated at the output cap: " + calls[k].GateReason - rep.Gate("reply_truncated") + gateNames[k] = append(gateNames[k], "reply_truncated") atomic.AddInt64(&llmTruncated, 1) } // CLASSIFY THE SILENT FAILURE — and classify it INDEPENDENTLY of whether @@ -1430,12 +1305,7 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // counted (above) and still brake exploration via slowCallMs, which is the // latency-aware layer that SHOULD react to a slow server. } - first := 0 - if serialFirst { - runCall(0) // the writer; its release() marks the prefix as present - first = 1 - } - for k := first; k < len(cands); k++ { + for k := 0; k < len(cands); k++ { wg.Add(1) go func(k int) { defer wg.Done() @@ -1450,6 +1320,11 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // reporting that zero value put a phantom `cand=0 saved=0 $0.00` row in the ledger, // inflating the call count with work that by definition did not happen. for k := range calls { + // The gates first, and unconditionally: a single-flight FOLLOWER returns before + // filling its ModelCall slot, and its gate is precisely the one that says so. + for _, g := range gateNames[k] { + rep.Gate(g) + } if calls[k].Component != "" { rep.Calls = append(rep.Calls, calls[k]) } @@ -1468,13 +1343,6 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // recoverable. One key per decision (#40) — projected text and summary travel // together, so a replay can never emit half a decision. putResult(c, cands[k].id, out[k].projected, out[k].summary) - // Not published globally when the call escalated to the agent's model: the global - // key is built from e.modelName, so a result derived by a DIFFERENT model would - // be served to other sessions under the configured model's key. Session-scoped - // reuse (the replay that keeps this session's prefix stable) is unaffected. - if escalated { - continue - } if !e.rewrite || effectiveMode(c, e.mode) == markerFull { putResultGlobal(c, extract.ResultKey(cands[k].id, e.modelName, extCfg), out[k].projected, out[k].summary) @@ -1491,8 +1359,6 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R func init() { f := []components.Field{ - {Key: "per_output", Type: components.FieldBool, Default: true, - Hint: "The HOT-PATH pass: reduce individual tool outputs as they arrive. With this off and cold_cache.enabled off the component has nothing to do and refuses to build — take it out of the pipeline instead."}, {Key: "fire_on", Type: components.FieldEnum, Default: "pressure", Options: []string{"pressure", "size"}, Hint: "What decides a request is worth a model call. pressure = the derived context-pressure trigger. size = fire whenever any candidate clears min_tokens, which ALSO demotes the economic gate and the caching-backend guard to advisory — a deliberate licence to spend, so set the caps first."}, {Key: "min_tokens", Type: components.FieldInt, Default: 300, Min: 1, @@ -1525,14 +1391,6 @@ func init() { {Key: "model_max_input_tokens", Type: components.FieldInt, Hint: "Pin the EXTRACTION model's input budget, for a model id the static table cannot name (a self-hosted id, or a gateway alias). Unset = resolved per model."}, markerModeField(), - {Key: "cold_cache.enabled", Type: components.FieldBool, - Hint: "The whole-transcript sweep on a turn whose prompt cache has EXPIRED. Measured here: those turns were 4% of requests and 31% of spend, and removing a token on one is worth 12.5x what it is worth on a warm turn."}, - {Key: "cold_cache.min_tokens", Type: components.FieldInt, Default: defaultColdFloor, Min: 1, - Hint: "Per-output floor for the sweep. Lower than the hot path's, because on that turn every candidate is being re-billed at the write rate anyway."}, - {Key: "cold_cache.min_idle_seconds", Type: components.FieldInt, - Hint: "Demand MORE idle time than the provider TTL implies (0 = just the TTL). Raises the bar, never lowers it."}, - {Key: "cold_cache.max_calls", Type: components.FieldInt, - Hint: "Cap model calls for one sweep. 0 = unlimited, which is the default because the sweep runs once per idle gap on a turn that is already expensive."}, } f = append(f, modelFields("model")...) components.RegisterFields("extract_llm", extractLLMConfig{}, append(f, components.TriggerFields("trigger")...)) diff --git a/components/offload/extract_llm_gaterace_test.go b/components/offload/extract_llm_gaterace_test.go new file mode 100644 index 00000000..06b72d29 --- /dev/null +++ b/components/offload/extract_llm_gaterace_test.go @@ -0,0 +1,72 @@ +package offload + +import ( + "context" + "strings" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/store" +) + +// #119: extract_llm raised two gates from inside its per-call goroutines. +// +// components.Report is copied by value throughout this codebase and its Gates map therefore +// carries no lock — the file says so itself, at the declaration of the per-slot ModelCall records +// that exist for exactly this reason. Two concurrent raises are a data race on a Go map, and the +// runtime's response is not a wrong counter and not a recoverable panic: it is +// `fatal error: concurrent map writes`, which aborts the PROCESS. In a proxy that means every +// in-flight request of every session dies, rather than one component failing open — the severity +// that makes this worth its own fix rather than a note. +// +// The reachable path is the single-flight follower. extractInflight collapses identical content +// into one call and releases every waiter at the same instant, so N byte-identical candidates in +// one request produce N-1 simultaneous `deduped_inflight_extraction` raises. +// +// Under `-race` the reversion is caught deterministically. Without it the abort is +// timing-dependent, so the count assertion is what holds in the plain suite: a lost gate is the +// quiet form of the same defect, and `deduped_inflight_extraction` is the one counter that says a +// call was avoided rather than made. +func TestConcurrentCallsDoNotRaceOnTheGateHistogram(t *testing.T) { + // Byte-identical bodies, so all four share one extraction key and three become followers. + // Distinct enough from every other fixture in this package that the process-wide + // extractInflight group cannot collide with another test. + body := strings.Repeat("gaterace fixture line for issue 119, identical across candidates\n", 400) + req := &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{ + userMsg("Summarise what these four identical outputs say."), + }} + const identical = 4 + for i := 0; i < identical; i++ { + req.Input = append(req.Input, toolResultMsg(body)) + } + + model := &silentModel{} + e := newCtxGuardComponent(t, model, "") + rep := &components.Report{} + c := &components.Ctx{ + Session: "gaterace", Ctx: context.Background(), + Store: store.NewMemory(store.Options{}), CtxWindow: 1_000_000, + // Caching off, so the tail gate lets every candidate through and all four reach the + // concurrent phase. + CacheAware: false, MaxCachedIdx: -1, + } + if _, err := e.Offload(req, rep, c); err != nil { + t.Fatalf("Offload must fail open: %v", err) + } + + // PRECONDITION: the concurrent phase ran with more than one goroutine in it. If the + // candidates never reached phase 2 — a floor, the trigger, the economic gate — then no two + // raises were ever concurrent and this test proves nothing about the race. The dedup gate + // firing is the proof, because only a follower can raise it. + got := rep.Gates["deduped_inflight_extraction"] + if got == 0 { + t.Fatalf("no single-flight follower ran, so no two gate raises were concurrent "+ + "(gates: %v) — the race was never exercised", rep.Gates) + } + if want := identical - 1; got != want { + t.Errorf("deduped_inflight_extraction = %d, want %d: a follower's gate was lost, which is "+ + "the quiet form of the same unsynchronised write (gates: %v)", got, want, rep.Gates) + } +} diff --git a/components/offload/extract_sweep.go b/components/offload/extract_sweep.go new file mode 100644 index 00000000..00b991dc --- /dev/null +++ b/components/offload/extract_sweep.go @@ -0,0 +1,966 @@ +package offload + +import ( + "context" + "errors" + "fmt" + "log/slog" + "sort" + "strings" + "sync/atomic" + "time" + + bschemas "github.com/maximhq/bifrost/core/schemas" + + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/expand" + "github.com/rossoctl/context-guru/internal/cheapmodel" + "github.com/rossoctl/context-guru/internal/extract" + "github.com/rossoctl/context-guru/internal/logging" + "github.com/rossoctl/context-guru/metrics" + "github.com/rossoctl/context-guru/schema" + "gopkg.in/yaml.v3" +) + +func init() { components.Register("extract_llm_sweep", newExtractSweep) } + +// ExtractSweep is the SWEEP ADJUDICATOR: it asks the request's OWN model, over the transcript that +// model already has in its prompt cache, which tool outputs are spent — and removes those, leaving a +// shape descriptor plus a recoverable marker. It never rewrites anything and it never copies output +// content into a prompt. +// +// WHY IT IS A SEPARATE COMPONENT FROM extract_llm. The two situations want different operations. On a +// warm turn extract_llm works the uncached tail: the output is recent, the agent may still want most +// of it, and a smaller version of it is more useful than none of it. The sweep works DEEP HISTORY, +// where rewriting is the wrong operation on either branch of the only question that matters — deep +// history is either still load-bearing, in which case rewriting corrupts content the model has +// already reasoned about, or it is spent, in which case the answer is to remove it. +// +// WHY IT ASKS THE REQUEST'S MODEL OVER THE CACHE, in one call, shipping an inventory: +// +// Need is relevance MINUS what has already been captured elsewhere, and that second term lives in +// the LATER TURNS. A judgement shown only the candidate cannot see them. +// Verbatim quoting — the only signal that says whether the model is inventing — degraded to 20.8% +// on the cheap model at bulk batch sizes, against 0 of 59 on the request model. +// Appending a trailing user message to a byte-identical prefix read 19,595 tokens from cache and +// created 0, so the whole transcript is affordable exactly once: as a cache read. +// +// See internal/extract/adjudicate.go for the full evidence and components.PrefixAsker for the +// construction. +// +// WHAT IT NEVER DOES. It selects no compaction strategy, produces no rewritten text, and there is no +// reply field a model could return content through. `strategy`, `rewrite`, `aggressiveness` and +// `max_chars` are therefore not merely defaulted differently here, they are meaningless, and writing +// one is a config error rather than a silently ignored key (see newExtractSweep). +type ExtractSweep struct { + minTokens int + // minInventory is the fewest candidates worth asking about. See defaultMinInventory. + minInventory int + // blockFallback refuses the content-copying fallback. See extractSweepConfig.BlockFallback. + blockFallback bool + // preExpiry is how long before the prompt cache's believed expiry the sweep may fire. See + // sweeping() for why the window is where it is, and why its WIDTH is the one number here that + // no measurement settles. + preExpiry time.Duration + + mode markerMode +} + +// extractSweepConfig is the sweep's whole surface. +// +// Note what is absent and why. There is no `model` block: the ask goes to the REQUEST's own model by +// construction, because only that model's cache holds the transcript — naming another would read a +// different namespace and pay fresh for everything. There is no `context` / `context_messages`: the +// conversation IS the prefix, so there is nothing to choose how much of to re-send. There is no +// `max_calls`: there is exactly one ask per turn, bounded by maxAskItems rather than by a call count +// — and that bound is not configurable, because it follows from the reply budget rather than from a +// deployment's preference (see the cap in Offload, and #132 for the coverage question it leaves open). +// And there is no `economic_gate`: the gate prices a +// per-output cheap-model call against an expected saving, and this is one cached read for the whole +// transcript, so its arithmetic does not describe this component at all — the brakes here are the +// floor below and the verified cache read. +type extractSweepConfig struct { + // MinTokens is the per-output floor (0 = defaultSweepFloor). Candidates below it are not worth + // naming in the inventory: each line is paid fresh, and a small output's removal cannot repay + // the marker it leaves behind. + MinTokens int `yaml:"min_tokens"` + // MinInventory is the fewest candidates worth asking about (0 = defaultMinInventory). Below it + // the sweep declines entirely rather than asking, because the model's judgement at small + // inventory sizes is measured poor — see the floor check in Offload for the figures. + MinInventory int `yaml:"min_inventory"` + // PreExpirySeconds is the width of the pre-expiry window (0 = defaultPreExpiry). + PreExpirySeconds int `yaml:"pre_expiry_seconds"` + // BlockFallback refuses the fallback path: when the prefix ask cannot read the cache, decline + // instead of asking again with the output content copied into the prompt. + // + // OFF by default, which is a deliberate choice between two real costs. Falling back keeps the + // component working on a session's FIRST turn and whenever an entry has gone — treating "no + // prefix" as "no verdicts" would disable it there and read, in the counters, as a model that + // declined to act. But the fallback pays fresh for content the cached path reads for a tenth of + // the price, which is where this component's predecessor lost money. Default on the side of + // working; switch it off where the bill matters more than the yield. Counted either way. + BlockFallback bool `yaml:"block_fallback"` + // MarkerMode is how a removed output is referenced. `full`, the default, is the only mode that + // keeps the removal recoverable. + MarkerMode string `yaml:"marker_mode"` +} + +// defaultSweepFloor is the per-output floor when none is configured. Carried over from the cold_cache +// block this component replaced: at 3000 the shipped preset produced ZERO extractions across 3,437 +// production requests, with `below_output_floor` refusing every candidate on all 36 sweeping turns. +const defaultSweepFloor = 1000 + +// defaultMinInventory is the fewest candidates this component will ask about. Ten, because that is +// where `cc1aa9f` measured the model becoming willing to act CORRECTLY: at batch 3-6 it dropped a +// genuinely-spent output 2 times in 4, at batch 10 it dropped it 4 in 4 and cleared 100% of +// genuinely-spent candidates. Below that the mechanism is not a timid version of itself, it is +// answering the question the selection experiment refuted at 6% live-kept. +const defaultMinInventory = 10 + +// maxAskItems bounds how many candidates one ask may carry. Not configurable: it is a property of the +// reply budget and the model's transport limit, not of a deployment's taste, and an operator raising +// it would be trading a partial sweep for no sweep at all. See the cap in Offload for the arithmetic. +const maxAskItems = 12 + +// defaultPreExpiry is the pre-expiry window's width when none is configured. +// +// IT IS AN ASSUMPTION, AND THE ONLY UNMEASURED NUMBER IN THIS COMPONENT. One minute is +// apply.coldMargin, which is the single figure in this codebase with a stated purpose for clock +// uncertainty around cache expiry: the gap between when a turn was recorded here and when the +// provider last touched the entry. A window one margin wide therefore sits inside the interval where +// our clock and the provider's are believed to agree to within that margin. +// +// What is NOT known is the yield/cost trade-off of widening it. A wider window fires on more turns +// and invalidates prefixes with more remaining TTL; a narrower one fires rarely. Nothing measures +// either side, so this is deliberately narrow and configurable rather than tuned. +const defaultPreExpiry = time.Minute + +// sweepBannedKeys are the compaction knobs that have no meaning for an adjudicator, and the reason +// each one does not apply. They are refused rather than ignored: a silently accepted `rewrite: false` +// would read as "verified deletion-only is on" when nothing is being rewritten in the first place, +// and an operator migrating an older config by hand has no other way to find out. +// +// Detected on a SEPARATE probe struct rather than as fields of extractSweepConfig, because a field +// there would have to be declared to the settings form (components/all's field contract), which +// would put a knob on the page whose only behaviour is to fail. +var sweepBannedKeys = []struct { + key, why string +}{ + {"strategy", "an adjudicator selects no compaction strategy — it returns a verdict, not a program"}, + {"rewrite", "nothing is rewritten, so there is no rewrite to validate; the output is kept verbatim or removed"}, + {"aggressiveness", "there is no compaction target to teach: the only question asked is whether the output is spent"}, + {"max_chars", "no projection window exists — a dropped output leaves a shape descriptor, not a truncation"}, + // THE MODEL IS NOT A FREE CHOICE HERE, and this is the one place that asymmetry with extract_llm + // is visible, so it is spelled out rather than left to look like an oversight. + // + // extract_llm may compact with any model, because its prompt carries the output it is compacting: + // any model can read it. This component's prompt carries an INVENTORY, and the outputs are read + // from the prompt cache of the model being asked. Only the REQUEST's model has that cache. So + // `source: config` — a separate cheap model — is not a cheaper configuration of this component, it + // is a broken one: the ask would read nothing and degrade to paying fresh for the whole + // transcript, which is precisely the cost that made the predecessor lose money. + // + // Refused rather than accepted-and-corrected, because an operator who wrote it meant something, + // and silently substituting a different model is how a configuration comes to disagree with the + // bill. + {"model", "the ask goes to the REQUEST's own model by construction: only that model's prompt cache " + + "holds the transcript the inventory refers to. `source: config` is incoherent here rather than " + + "merely suboptimal — a separate cheap model has no such cache, so the ask would read nothing " + + "and pay fresh for the entire transcript. extract_llm's model IS a free choice because its " + + "prompt carries the output itself"}, + {"context", "the conversation IS the cached prefix, so there is no amount of it to choose to re-send"}, + {"context_messages", "the conversation IS the cached prefix; see `context`"}, + {"max_calls", "one call adjudicates every candidate, because nothing is copied per candidate"}, + {"economic_gate", "the gate prices a per-output cheap-model call; this is one cached read for the whole " + + "transcript, so its arithmetic does not describe this component"}, +} + +func newExtractSweep(raw []byte) (components.Component, error) { + // The banned keys FIRST, before components.Decode's KnownFields rejects them with a generic + // yaml message. The whole point is that the error names the reason. + if len(raw) > 0 { + var probe map[string]yaml.Node + if err := yaml.Unmarshal(raw, &probe); err == nil { + for _, b := range sweepBannedKeys { + if _, present := probe[b.key]; present { + return nil, fmt.Errorf("extract_llm_sweep: %s does not apply here: %s", b.key, b.why) + } + } + } + } + cfg := extractSweepConfig{} + if err := components.Decode(raw, &cfg); err != nil { + return nil, err + } + if cfg.MinTokens <= 0 { + cfg.MinTokens = defaultSweepFloor + } + if cfg.MinInventory <= 0 { + cfg.MinInventory = defaultMinInventory + } + pre := defaultPreExpiry + if cfg.PreExpirySeconds > 0 { + pre = time.Duration(cfg.PreExpirySeconds) * time.Second + } + return &ExtractSweep{ + minTokens: cfg.MinTokens, minInventory: cfg.MinInventory, + preExpiry: pre, mode: parseMarkerMode(cfg.MarkerMode), + blockFallback: cfg.BlockFallback, + }, nil +} + +func (*ExtractSweep) Name() string { return "extract_llm_sweep" } +func (*ExtractSweep) Enabled(*components.Ctx) bool { return true } + +// sweeping reports whether this turn falls in the PRE-EXPIRY WINDOW: the prompt cache still exists, +// and it is close enough to expiring that invalidating it costs little. +// +// THIS IS THE RESOLUTION OF A CONTRADICTION, and it is the whole reason the trigger is not the cold +// gate it started as. The two halves of this component want opposite cache states: +// +// the ASK needs a WARM cache — a prefix ask reads an entry that must still exist, or the call pays +// fresh for the whole transcript, which is the cost the design exists to avoid; +// the REMOVAL wants a COLD cache — rewriting deep history invalidates a live prefix and forces a +// cache-write of the whole suffix at 1.25x fresh. +// +// Both are cheap in the window where the entry still exists but has little life left: the ask still +// reads it, and what the removal invalidates is nearly worthless. So the trigger is +// `0 < remaining <= preExpiry`, where remaining is the cache's believed lifetime minus this session's +// idle time. +// +// THE TTL IS DERIVED, NOT ASSUMED. Ctx.CacheTTLMs is the same figure apply's cold decision uses, read +// out of the request itself: a bare `ephemeral` mark is 5 minutes, an explicit `ttl: "1h"` is an hour, +// widened to the longest lifetime this prefix has ever asked for. 0 means the cache-aware path did not +// run, i.e. unknown, and unknown must not fire — a window computed from a guessed TTL would invalidate +// live prefixes on exactly the deployments whose TTL we could not read. +// +// !ColdCache is redundant against `remaining > 0` and kept anyway: it is apply's own verdict, computed +// with its clock-skew margin, and one cheap agreement check costs nothing next to a wrongly +// invalidated prefix. +func (e *ExtractSweep) sweeping(c *components.Ctx) bool { + if c == nil || c.ColdCache || c.CacheTTLMs <= 0 || c.IdleMs <= 0 { + return false + } + remaining := time.Duration(c.CacheTTLMs-c.IdleMs) * time.Millisecond + return remaining > 0 && remaining <= e.preExpiry +} + +// sweepUnusableSamples bounds how many unparseable replies get logged in full. Process-wide, because +// the question it answers — what is the model actually emitting? — is answered by the first few. +// +// Six rounds of the predecessor's failures were diagnosed by inferring a cause from gate counters, +// and every inference was at least partly wrong. A counter can say THAT a reply was unusable; only +// the text says WHY. +var sweepUnusableSamples atomic.Int64 + +// maxSweepUnusableSamples bounds it in count as well as length, because a systematic failure would +// otherwise flood the log with transcript content lifted out of the replies. +const maxSweepUnusableSamples = 5 + +func (e *ExtractSweep) Offload(req *bschemas.BifrostChatRequest, rep *components.Report, c *components.Ctx) ([]string, error) { + sweeping := e.sweeping(c) + if !sweeping { + // NOT a return. The frozen replays below still run, and they are the reason a sweep's saving + // survives past the turn that earned it: without them a later turn would re-send every + // removed output verbatim, undoing the removal AND breaking the byte-stability of the prefix + // the provider is caching. + rep.Gate("not_in_pre_expiry_window") + } + + val := savedTokenValue(c) + var cands []sweepCand + var keys []string + changed := 0 + // eligible counts candidates that cleared every gate this component knows about. Compared with the + // inventory's size below, to catch a pre-filter that thinned it -- see the comment at the append + // site for why that is the failure worth a tripwire. + eligible := 0 + + // Phase 1 (serial): replay frozen decisions at any depth, and collect the candidates to name in + // the inventory. + for _, i := range toolIndices(req) { + msg := &req.Input[i] + if !schema.Rewritable(*msg) { + rep.Gate("non_text_blocks") + continue + } + content := schema.MessageText(*msg) + if content == "" || expand.HasPlaceholder(content) { + rep.Gate("empty_or_marker_present") + continue + } + id := extract.ContentKey(content) + // If the agent recently EXPANDED this content, leave it verbatim — removing it again would + // just trigger another expand. + if isKeptVerbatim(c, id) { + rep.Gate("kept_verbatim_after_expand") + continue + } + // SAME-SESSION REPLAY, and it bypasses the depth gate legitimately: this session already + // sent these exact bytes on an earlier turn, so the provider's cached prefix holds the + // REMOVED form and replaying it is byte-identical. + // + // The stored value is the descriptor, which sweepDescriptor derives from the content alone. + // That is what makes the replay safe in the sense TailOnlyCold's doc requires: the DECISION + // came from a model, but the REPLACEMENT is a pure function of (content, config), so a replay + // can never emit different bytes than the turn that decided it. + if cached, hit := getResult(c, id); hit { + metrics.RecordExtractionCacheLookup(true) + if saved := schema.TextTokens(content) - schema.TextTokens(cached.Projected); saved > 0 { + metrics.RecordExtractionValue(float64(saved) * val.repeatPerToken) + } + if k, ok := applySweepDrop(c, rep, e.mode, msg, content); ok { + changed++ + if k != "" { + keys = append(keys, k) + } + rep.Event("reapplied_same_session") + } + continue + } + if !sweeping { + // Outside the window no NEW decision is taken; the replays above already ran, which is + // all such a turn has to do. + continue + } + metrics.RecordExtractionCacheLookup(false) + if schema.TextTokens(content) < e.minTokens { + rep.Gate("below_output_floor") + continue + } + // NO DEPTH RESTRICTION. Candidates are the ENTIRE transcript, which is what this component + // is for, and the sweep window is the whole justification. + // + // The tail gate exists for the WARM-turn compactor: rewriting a message inside a live cached + // prefix invalidates everything after it, so extract_llm must confine itself to the uncached + // tail. This component's premise is the opposite — it ACCEPTS that invalidation, because it + // only ever runs on a prefix with almost no TTL left. Refusing depth here does not make the + // sweep conservative, it makes it pointless: the candidates would be exactly the messages the + // prefix ask CANNOT SEE, since the ask reads the previous turn's sent body (everything up to + // the cached boundary) while the tail is everything past it. Disjoint by construction. + // + // That is not hypothetical. It shipped, and live verification found the model judging outputs + // it had never read: one turn kept two outputs citing "Reply with the word ACK only." as the + // obligation for each — a real transcript string, so no fabrication counter fired — and + // another DROPPED an output having seen only `begins: # ledger_b` and a token count. See #122. + // + // It is also what made the inventory degenerate. Candidates confined to the tail means one + // candidate on an ordinary agent turn, which is the per-output shape `4ca1f13` records as + // refuted at 6% live-kept — reached by default, and acted on. + // + // WHY THIS NEEDS NO MEASUREMENT OF EARLY INVALIDATION, which is the objection the previous + // version of this comment raised against itself. The cost of invalidating early is bounded by + // the window width, not by the TTL: inside the window the prefix has at most `window` left to + // live, so at most that much cache value is being given up, and the window is deliberately + // small (one minute against a 5-minute TTL by default). The trigger is what buys the + // permission — that is the entire reason it exists — so keying the permission on ColdCache + // instead of on the window withdrew it exactly where it had just been paid for. + // + // Counted positively rather than as a refusal: `sweep_candidate_at_depth` says the component + // is genuinely reaching past the cached boundary. It going to zero is the signal that this + // regressed again, which a `cached_prefix` refusal counter could not distinguish from + // "nothing was deep this turn". + if !c.TailOnly(i) { + rep.Event("sweep_candidate_at_depth") + } + // EVERY CANDIDATE PAST THIS POINT MUST REACH THE INVENTORY, and sweep_inventory_thinned below + // is the tripwire for the day one does not. + // + // `4ca1f13`'s real defect was a per-candidate PRE-FILTER sitting exactly here. + // prefix_still_referenced removed 149,681 candidates and left about one per request, which + // silently turned a bulk adjudication arm into the per-output shape refuted at 6% live-kept -- + // and the arm reported itself as bulk throughout. It was self-defeating twice over: it starved + // the comparison, and it meant the model only ever saw what the index had ALREADY judged spent, + // which destroys the veto on the index's blind spot that the mechanism exists to provide. + // + // `main` has no such thinner, so `eligible` and the inventory size are equal by construction + // and this counter cannot fire today. THAT IS THE POINT: PR #80 rebases onto this branch and + // brings index-driven candidate selection with it, and a filter added between this line and the + // append below trips the counter on its first request. If you are adding one, the index's + // verdict belongs in the prompt as EVIDENCE for the model to weigh + // (extract.AdjudicationItem.Evidence), never as a gate that pre-decides the answer. + eligible++ + // The wire's own tool-call id, which apply.normalize sets on every synthetic tool message it + // lifts out of an Anthropic tool_result block. Read here rather than reconstructed, because a + // reconstructed anchor is exactly the defect #123 records. + toolID := "" + if msg.ChatToolMessage != nil && msg.ChatToolMessage.ToolCallID != nil { + toolID = *msg.ChatToolMessage.ToolCallID + } + cands = append(cands, sweepCand{i: i, content: content, id: id, toolID: toolID}) + } + // WHAT WAS SHOWN, counted apart from what was answered. A per-candidate loop cannot express "this + // many were OFFERED", and the distinction is not cosmetic: a live arm reported 2.80 verdicts per + // call and that was read as the batch size, when it counted what the model chose to ANSWER rather + // than what it was SHOWN. Without this, "the inventory is starved" and "the model answered for a + // third of it" are the same number. + // CAP THE ASK, because an uncapped one risks losing every verdict rather than some. + // + // The reply carries one verdict per candidate, each with a VERBATIM transcript quote, and the + // budget is PrefixAskMaxTokens (16,000). Live: a 12-candidate ask produced a 7,191-token reply — + // about 600 tokens per verdict once the model's reasoning is included — so roughly 26 candidates + // exhausts the budget. Past that the reply truncates, and truncation is ALL-OR-NOTHING: the array + // never closes, nothing parses, and every verdict in it is discarded. A 50-candidate transcript + // would therefore sweep nothing at all, having paid for the call. + // + // Twelve, and the two independent arguments agree on it, which is the only reason to trust a + // number here. Reply-budget arithmetic says ~26 is the ceiling and something well inside it is + // prudent. And `cc1aa9f` measured quote fidelity degrading with size — 4 of 37 quotes non-verbatim + // at 16 against 0 of 16 at 10 — so 12 was already the conservative end of the transport limit. + // That fidelity measurement was taken when content was copied into the prompt, which it no longer + // is, so it does not straightforwardly transfer; it is cited as corroboration, not as proof. + // + // LARGEST FIRST, so the cap keeps the candidates worth the most. And what is left over is + // COUNTED: a component that silently swept 12 of 50 while reporting success would be the same + // class of defect as the starved inventory this file already guards against. + // + // What this does NOT do is make a second ask to cover the remainder. That is a real coverage gap + // on a transcript-heavy session and it needs a measurement — whether N asks over one transcript + // beat one ask, and at what cost — so it is tracked rather than guessed at. See #132. + // Counted BEFORE the cap, and the thinning tripwire measured against this rather than against the + // post-cap length. The cap is a deliberate ceiling; sweep_inventory_thinned exists to catch a + // pre-filter quietly starving the comparison (`4ca1f13`), and letting the cap trip it would turn + // that alarm into noise on exactly the transcripts where it should be loudest. + assembled := len(cands) + if len(cands) > maxAskItems { + sort.SliceStable(cands, func(i, j int) bool { + return schema.TextTokens(cands[i].content) > schema.TextTokens(cands[j].content) + }) + rep.GateN("sweep_over_ask_cap", len(cands)-maxAskItems) + cands = cands[:maxAskItems] + } + rep.EventN("sweep_offered", len(cands)) + if eligible > assembled { + rep.EventN("sweep_inventory_thinned", eligible-assembled) + } + // DO NOT ASK AT ALL BELOW THE INVENTORY FLOOR. The yield of this mechanism is a property of how + // many candidates the model compares, and the numbers are not close: + // + // shown 1 output 6% live-kept on haiku, 14% on sonnet — both inside the + // drop-everything null model's error bar (8,105 recorded decisions) + // shown ~15 58% live-kept, at the LOWEST cost per output + // batch 3-6 dropped a genuinely-spent output 2 times in 4 + // batch 10 dropped it 4 in 4, and cleared 100% of genuinely-spent candidates + // + // So `cc1aa9f`'s conclusion — "small batches do not make it wrong, they make it UNWILLING TO + // ACT" — has a corollary this component needs: below about ten, the model is not merely timid, + // it is answering a question the measurements say it answers badly, and a `drop` from it is a + // guess. Declining is strictly better than asking, because a wrong keep costs one turn's tokens + // and a wrong drop costs content the agent still needs. + // + // Ten is the measured inflection above, not a round number. Configurable because a deployment + // whose transcripts are shorter may prefer to trade the yield away entirely rather than act on + // small inventories. + // + // Counted, because a component that declines is indistinguishable from one that is broken unless + // the decline is recorded — the failure mode that hid the `economic_gate: false` blind spot in + // this same component and three vacuous trim tests before it. + if len(cands) < e.minInventory { + rep.GateN("sweep_inventory_below_min", len(cands)) + return keys, nil + } + + // Phase 2: ONE ASK for every candidate. Not a batch and not a call per output — nothing is + // copied per candidate, so there is nothing to divide. + if len(cands) > 0 && sweeping { + drop, call := e.adjudicate(req, c, rep, cands) + for _, g := range call.gates { + rep.Gate(g) + } + for _, ev := range call.events { + rep.Event(ev) + } + if call.rec.Component != "" { + rep.Calls = append(rep.Calls, call.rec) + } + // Phase 3 (serial): freeze + splice. + for _, k := range drop { + desc := sweepDescriptor(cands[k].content) + // Freeze the decision so every later turn replays it byte-for-byte from the same-session + // path above, at any depth. Session-scoped only: unlike a compaction, a drop is a + // judgement about THIS transcript's obligations, so it must never be served to another + // session whose agent may still need the output. + putResult(c, cands[k].id, desc, "") + if key, ok := applySweepDrop(c, rep, e.mode, &req.Input[cands[k].i], cands[k].content); ok { + changed++ + if key != "" { + keys = append(keys, key) + } + } + } + } + + if changed == 0 { + rep.Skipped = true + } + return keys, nil +} + +// sweepCand is one candidate the sweep collected. A package-level type because adjudicate needs to +// resolve a model-supplied LABEL back to content, and that mapping must stay on our side of the wire. +type sweepCand struct { + i int + content string + // id is the CONTENT KEY (extract.ContentKey): the store/stash key and the result-cache key. It is + // ours, and it appears nowhere in the transcript. + id string + // toolID is the wire's own tool-call id, lifted from the normalized message. It is the only string + // here that also occurs in the transcript the model reads, so it is the only one that can serve as + // a locating anchor — which is why it is a SEPARATE field rather than a reuse of `id`. + // + // They were conflated once, and the effect was worse than omitting the anchor: the inventory + // announced "tool_use id 300c312d1492952219bfb1c4" while the real id in that transcript was + // `toolu_d2`, so the contract told the model to locate content by a key that cannot be found + // anywhere. See #123. Empty when the dialect carries no id, in which case nothing is claimed. + toolID string +} + +// sweepResult is one adjudication's outcome, carried back to the SERIAL phase. +// +// The gate names travel as data rather than being raised where they are decided, because +// components.Report is copied by value across this codebase and its Gates map therefore carries no +// lock. Raising a gate off the serial path is not a slightly-wrong counter, it is +// `fatal error: concurrent map writes` — which is how #119 was found. Kept even though this path now +// makes ONE call: the discipline is what stops the next concurrent thing here from reintroducing it. +type sweepResult struct { + gates []string + // events are the names that go to Report.Events rather than Report.Gates: work PERFORMED or + // neutral observation, as against a candidate turned away. Carried separately for the same + // reason gates are carried at all — the raise happens on the serial path — and split for the + // reason Report.Events exists: exported under a metric named "declines", a success made the + // series climb as the component worked better. + events []string + rec components.ModelCall +} + +func (r *sweepResult) gate(name string) { r.gates = append(r.gates, name) } +func (r *sweepResult) event(name string) { r.events = append(r.events, name) } + +// adjudicate makes ONE prefix ask about every candidate and returns the labels it authorised +// dropping. +// +// EVERY FAILURE PATH RESOLVES TOWARD KEEP -- no asker, no stashed prefix, a transport error, a cache +// read that did not happen, an unparseable reply, an unusable verdict, a drop that contradicts a named +// obligation, a verdict for something we did not offer. A wrong keep costs tokens on one turn; a wrong +// drop is a silent permanent loss the agent does not notice and cannot ask about. The two errors are +// not comparable, so this does not treat them symmetrically. +func (e *ExtractSweep) adjudicate(req *bschemas.BifrostChatRequest, c *components.Ctx, + rep *components.Report, cands []sweepCand) ([]int, sweepResult) { + + var r sweepResult + // A SINGLE-CANDIDATE ASK IS THE REFUTED SHAPE WEARING A NEW NAME, so it is counted rather than + // silently accepted. Shown one output, a model simply drops it: 6% live-kept on haiku and 14% on + // sonnet, both inside the drop-everything null model's error bar. The ask still proceeds — a + // transcript can legitimately have one candidate above the floor — but a workload where this + // fires routinely has an upstream filter starving the inventory, which is the failure that cost + // three iterations (4ca1f13). + if len(cands) < 2 { + r.event("sweep_inventory_of_one") + } + + items := make([]extract.AdjudicationItem, 0, len(cands)) + for k := range cands { + items = append(items, extract.AdjudicationItem{ + Label: k, + ID: cands[k].toolID, // the wire's id, not our content key — see #123 + SizeTokens: schema.TextTokens(cands[k].content), + Head: extract.HeadLine(cands[k].content, extract.AdjudicationHeadChars), + }) + } + // The transcript, flattened, so a claimed obligation quote is VERIFIED against what the agent was + // actually told rather than trusted. This is the only remaining signal that the model is + // inventing, because nothing else it returns is content. + // + // Built from the INCOMING request, while the ask reads the PREVIOUS turn's sent body. The two + // differ, and in the safe direction: the incoming transcript is a superset in content (nothing + // removed) and one turn newer, so a quote the model took from the cached prefix is still findable + // here. A quote it invented is still not. + flat := flattenTranscript(req) + + ctx, cancel := context.WithTimeout(c.Ctx, llmCallTimeout) + defer cancel() + var before int + for _, it := range items { + before += it.SizeTokens + } + start := time.Now() + var ( + reply string + usage components.PrefixUsage + err error + // fellBack records that the expensive path has already run, so the cache-read check below does + // not fire a second time for the same call. Without it a failed ask both fell back AND then + // reported a zero cache read, double-counting one event as two. + fellBack bool + ) + if c.PrefixAsk == nil { + // No asker at all: a non-Anthropic route, or no incoming client. Not a failure of the ask — + // there was nothing to ask through — so it takes the same fork as a missed read. + r.gate("sweep_no_asker") + if e.blockFallback { + r.gate("sweep_fallback_blocked") + r.rec.Rejection = "no prefix asker on this route and block_fallback is set" + return nil, r + } + if reply, err = e.fallbackAsk(ctx, req, c, &r, items, cands); err != nil { + return nil, r + } + fellBack = true + } else { + reply, usage, err = c.PrefixAsk.Ask(ctx, c.Session, extract.BuildPrefixAsk(items)) + } + latency := float64(time.Since(start).Milliseconds()) + metrics.RecordExtractionCall(latency) + // COST, which this record carried as $0.00 forever. It never set CostUSD at all, so the per-call + // ledger the dashboard shows for this component reported zero on every firing — measured live at + // $0.00 against real cache reads of 449,304 and 449,376 tokens and real completion tokens, while + // the request-level rollup (proxy/dashcapture.go, cg_llm_cost_usd) had the true $0.0940 and + // $0.1652. Two recorded totals disagreeing, one of them structurally zero, is worse than either + // alone: a component whose whole justification is cost looked free. + // + // Priced from the REQUEST's model, not a cheap-model card, because that is what this component + // calls by construction — and from the same rates the request-level figure uses, so the two agree + // rather than being two independent guesses. c.SelfRates is the model the request came in on; + // falling back to the env card keeps a figure when the host supplies no rates, the same + // convention extract_llm.pricingFor uses. + pricing := cheapmodel.PricingFromEnv() + if !c.SelfRates.Zero() { + pricing = ratesPricing(c.SelfRates) + } + r.rec = components.ModelCall{ + Component: rep.Component, Model: c.ModelName, Strategy: "prefix_ask", + CandidateTokens: before, LatencyMs: latency, + PromptTokens: int64(usage.Fresh), CompletionTokens: int64(usage.Output), + CacheRead: int64(usage.CacheRead), CacheWrite: int64(usage.CacheWrite), + CostUSD: pricing.Cost(int64(usage.Fresh), int64(usage.Output), + int64(usage.CacheWrite), int64(usage.CacheRead)), + GateReason: "pre-expiry window: the cache still exists and is nearly worthless", + } + if ctx.Err() != nil { + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + atomic.AddInt64(&llmTimeouts, 1) + } else { + atomic.AddInt64(&llmErrors, 1) + } + } + if !fellBack && err != nil { + // A first turn has no stashed prefix, which arrives here as an error. Counted separately from + // a transport failure: one is "there was nothing to read yet", which every session does once + // and which needs no attention, and the other is "the read failed", which does. + if errors.Is(err, components.ErrNoPrefix) { + r.gate("sweep_no_prefix") + } else { + r.gate("sweep_ask_failed") + } + if e.blockFallback { + r.gate("sweep_fallback_blocked") + r.rec.Rejection = "prefix ask failed and block_fallback is set: " + err.Error() + return nil, r + } + if reply, err = e.fallbackAsk(ctx, req, c, &r, items, cands); err != nil { + return nil, r + } + fellBack = true + } + // THE CACHE READ IS THE MECHANISM'S WHOLE JUSTIFICATION, so a read that did not happen is always + // COUNTED — a silent miss is what hid this class of problem before, and it looks identical to a + // working call except on the bill. + // + // What happens next is a choice between two real costs, and the default is to keep working: + // + // FALL BACK (default). Ask again with the outputs copied into the prompt. That pays fresh for + // content the cached path reads for a tenth of the price, and shows the model a TRUNCATED view of + // each output — but it keeps the component alive on a session's first turn and whenever an entry + // has gone. Treating "no prefix" as "no verdicts" would disable it there and read, in the + // counters, as a model that declined to act. + // DECLINE (`block_fallback: true`). Forgo the yield rather than pay for it. The right choice + // where the bill matters more than the removal, and the honest one to reach for if + // sweep_prefix_cache_read_ZERO turns out to be common. + // + // Note what neither mode can do: prevent the fresh read that already happened on THIS call. The + // counter is what tells an operator the window is mistimed. + if !fellBack && usage.CacheRead == 0 { + r.gate("sweep_prefix_cache_read_ZERO") + if e.blockFallback { + r.gate("sweep_fallback_blocked") + r.rec.Rejection = "the prefix ask read nothing from cache and block_fallback is set; " + + "declining rather than paying again for a full-price transcript read" + return nil, r + } + if reply, err = e.fallbackAsk(ctx, req, c, &r, items, cands); err != nil { + return nil, r + } + fellBack = true + } else if !fellBack { + r.event("sweep_prefix_cache_read_ok") + } + for range items { + r.event("sweep_adjudicated") + } + + verdicts, parsed := extract.ParseVerdicts(reply) + if !parsed { + // TRUNCATION IS NOT JUNK, and the two need opposite fixes -- raise the budget versus fix the + // prompt -- so one name for both hid a 70%-of-calls failure behind a label that reads as "the + // prompt is wrong". + if extract.ReplyWasTruncated(reply) { + r.gate("sweep_reply_truncated") + } else { + r.gate("sweep_unparseable") + } + if sweepUnusableSamples.Add(1) <= maxSweepUnusableSamples { + head := reply + if len(head) > 500 { + head = head[:500] + } + slog.Warn("cg.sweep.unusable_reply", "reply_len", len(reply), + "offered", len(items), "head", head) + } + r.rec.Rejection = "reply did not parse; every output kept verbatim" + return nil, r + } + if len(verdicts) == 0 { + // A well-formed EMPTY array: the model read the inventory and kept all of it. The contract + // explicitly invites that, so it must not be filed as a failure -- that conflation is what + // made "the model declined to act" and "the model was never successfully asked" the same + // number for three iterations (4ca1f13). + // SPLIT BY PATH, because a keep-all means different things on each and averaging them hides + // the more interesting one. The fallback has no transcript, so it cannot see that a task + // closed and resolves toward keep structurally -- measured at 12 of 12 kept where the prefix + // ask dropped 12 of 12 on the same content (#125). Without this split a run's numbers read as + // "the component sometimes acts and sometimes does not", when the real variable is whether + // the cache read happened. It is also how the goal-ordering fix in sweepIntent gets checked + // against real traffic rather than argued about. + if fellBack { + r.gate("sweep_fallback_kept_everything") + } else { + r.gate("sweep_kept_everything") + } + r.rec.Rejection = "adjudicated: keep everything" + return nil, r + } + + var drop []int + seen := map[int]bool{} + var removed int + for _, v := range verdicts { + if v.Label < 0 || v.Label >= len(cands) { + // A verdict for something we did not offer. NEVER acted on: the label is how a decision is + // keyed to an output, so a wrong label is a decision about an unknown message — and + // indexing on it would panic rather than merely act wrongly. + // + // WHAT THIS CANNOT CATCH is a label that is IN RANGE but wrong: a verdict meant for output + // 4 arriving as output 5 removes the wrong content and looks perfectly valid from here, and + // nothing downstream can detect it either. That is the failure the tool_use id in the + // inventory exists to PREVENT rather than to detect -- an exact anchor between the line and + // the content makes mis-keying less likely in the first place, which is the only defence + // available against a plausible-but-wrong label. + r.gate("sweep_verdict_unknown_label") + continue + } + if seen[v.Label] { + r.gate("sweep_verdict_duplicate_label") + continue + } + seen[v.Label] = true + content := cands[v.Label].content + a := extract.Judge(v, flat) + if a.QuoteFabricated { + r.gate("sweep_quote_fabricated") + } + if a.CriterionMissing { + r.gate("sweep_criterion_missing") + } + if a.VerdictUnusable { + r.gate("sweep_verdict_unusable") + } + // The refusal is counted INSTEAD of a keep, not alongside it. Both leave the output verbatim, + // but "the model judged this still needed" and "the model tried to remove something it had + // just said was needed" are different events, and folding the second into the keep total is + // what would make the alertable one invisible in the ratio an operator actually looks at. + if a.RefusedObligation { + r.gate("sweep_drop_refused_obligation") + continue + } + if !a.Drop { + r.gate("sweep_kept") + continue + } + sz := schema.TextTokens(content) + after := schema.TextTokens(sweepDescriptor(content)) + if after >= sz { + // The never-worse check also lives in applySweepDrop, marker included. This one is here + // so a decision phase 3 will refuse is not counted as a removal. + r.gate("sweep_drop_would_not_shrink") + continue + } + r.event("sweep_dropped") + drop = append(drop, v.Label) + removed += sz - after + metrics.RecordExtractionSaving(sz - after) + metrics.RecordExtractionValue(float64(sz-after) * savedTokenValue(c).perToken) + } + // An output named in the inventory that no verdict mentioned is UNJUDGED, and it must not look + // like a keep: 4ca1f13 found a live arm where the model silently omitted labels and the missing + // answers were invisible, so "the inventory is starved" and "the model answered for a third of + // it" were the same number. + for _, it := range items { + if !seen[it.Label] { + r.gate("sweep_verdict_missing") + } + } + if removed > 0 { + r.rec.Accepted = true + r.rec.SavedTokens = removed + } else { + r.rec.Rejection = "adjudicated: nothing was spent" + } + if debugExtractLLM(c) { + logging.From(c.Ctx).Debug("cg.sweep.ask", "offered", len(items), + "verdicts", len(verdicts), "dropped", len(drop), "candidate_tokens", before, + "removed_tokens", removed, "cache_read", usage.CacheRead, "fresh", usage.Fresh) + } + return drop, r +} + +// fallbackAsk is the EXPENSIVE path: a self-contained completion carrying a bounded sample of every +// candidate, for when the prefix ask could not read the cache. +// +// It goes to the REQUEST's own model, the same one the prefix ask would have addressed. Not a cheap +// one: the measurement that chose this model is about faithful quoting, not about caching — verbatim +// quoting degraded to 20.8% on the cheap model against 0 of 59 on the request model — and a fabricated +// quote is the only remaining signal that the model is inventing. That reason survives the loss of the +// cache read intact, so the fallback must not quietly downgrade the judge as well as the prompt. +// +// The reply budget is raised through components.Budgeter where the client supports it, for the same +// reason the prefix ask raises it: one reply carries a verdict for every candidate. +func (e *ExtractSweep) fallbackAsk(ctx context.Context, req *bschemas.BifrostChatRequest, + c *components.Ctx, r *sweepResult, items []extract.AdjudicationItem, + cands []sweepCand) (string, error) { + model := c.Model.For("incoming") + if model == nil { + r.gate("sweep_fallback_no_model") + r.rec.Rejection = "the prefix ask could not read the cache and no request model is available" + return "", errNoFallbackModel + } + if b, ok := model.(components.Budgeter); ok { + if m := b.WithMaxTokens(cheapmodel.PrefixAskMaxTokens); m != nil { + model = m + } + } + // The samples are attached HERE and nowhere else, which is what keeps content off the prefix-ask + // path by construction rather than by care. + withSamples := make([]extract.AdjudicationItem, len(items)) + for i, it := range items { + it.Sample = extract.ClipSample(cands[it.Label].content, extract.FallbackSampleChars) + withSamples[i] = it + } + r.event("sweep_fallback_used") + reply, err := model.Complete(ctx, extract.BuildFallbackAsk(sweepIntent(req), withSamples)) + if err != nil { + r.gate("sweep_fallback_failed") + r.rec.Rejection = "fallback completion failed: " + err.Error() + return "", err + } + return reply, nil +} + +// sweepIntent renders the conversation's intent for a SPENT-NESS judgement, which wants it ordered +// differently from every other component's relevance question. +// +// conversationGoal joins firstUser, lastAsst, lastUser in that order, unlabelled. That is right for +// extract_llm, which asks "is this output relevant to the task" — the opening instruction IS the +// task. It is wrong here, and measurably so. This component asks whether an output is SPENT, and the +// opening instruction describes what the session set out to do, which is precisely what may now be +// finished. Leading with it makes everything look needed. +// +// MEASURED, on two near-identical transcripts with twelve candidates each: the prefix ask dropped +// 12 of 12, while the fallback — same content, goal-string only — kept 12 of 12 and cited the +// original read instruction as the obligation for every one. See #125. The fallback has no +// transcript by construction, so it cannot see that the task closed; the goal string is the only +// place that can tell it. +// +// So: same three parts, ordered current-FIRST and LABELLED, with the original instruction explicitly +// marked as possibly already satisfied. The parts map onto the contract's own criteria — (a) the +// current step, (b) an unfinished user instruction, (c) a next step the agent stated — rather than +// arriving as one undifferentiated blob the model has to guess the structure of. +// +// The original instruction is kept rather than dropped, deliberately: criterion (b) is an unfinished +// USER instruction, and a standing "…and summarise all of them at the end" lives in exactly that +// message. Removing it would trade a bias toward keeping for a bias toward dropping, which is the +// direction that loses content the agent still needs. +func sweepIntent(req *bschemas.BifrostChatRequest) string { + var firstUser, lastUser, lastAsst string + for i := range req.Input { + if req.Input[i].Role == bschemas.ChatMessageRoleUser { + firstUser = strings.TrimSpace(schema.MessageText(req.Input[i])) + break + } + } + for i := len(req.Input) - 1; i >= 0; i-- { + switch req.Input[i].Role { + case bschemas.ChatMessageRoleUser: + if lastUser == "" { + lastUser = strings.TrimSpace(schema.MessageText(req.Input[i])) + } + case bschemas.ChatMessageRoleAssistant: + if lastAsst == "" { + lastAsst = strings.TrimSpace(schema.MessageText(req.Input[i])) + } + } + if lastUser != "" && lastAsst != "" { + break + } + } + var b strings.Builder + add := func(label, text string) { + if text == "" { + return + } + b.WriteString(label) + b.WriteString("\n") + b.WriteString(text) + b.WriteString("\n\n") + } + add("MOST RECENT USER TURN — this is the step the agent is on now:", lastUser) + add("THE AGENT'S OWN LAST STATEMENT — any next step it named is an obligation:", lastAsst) + // Last, and flagged. Its position in the prompt is the fix. + if firstUser != "" && firstUser != lastUser { + add("THE SESSION'S ORIGINAL INSTRUCTION — MAY ALREADY BE SATISFIED; treat it as an "+ + "obligation only if some part of it is still outstanding:", firstUser) + } + return clipRunes(strings.TrimSpace(b.String()), goalCap) +} + +// errNoFallbackModel is returned when the fallback has nowhere to go. Its own type so the caller can +// distinguish "we chose not to" from "we could not". +var errNoFallbackModel = errors.New("no request model for the sweep fallback") + +// flattenTranscript renders the agent's own text as one string, for verifying an obligation quote. +// Every text block of every message, tool results included: an obligation can be created by a user +// instruction, by the agent's own stated next step, or by something a tool told it. +func flattenTranscript(req *bschemas.BifrostChatRequest) string { + if req == nil { + return "" + } + var b strings.Builder + for i := range req.Input { + b.WriteString(schema.MessageText(req.Input[i])) + b.WriteByte('\n') + } + return b.String() +} + +func init() { + components.RegisterFields("extract_llm_sweep", extractSweepConfig{}, []components.Field{ + {Key: "min_tokens", Type: components.FieldInt, Default: defaultSweepFloor, Min: 1, + Hint: "Per-output floor for naming a candidate in the inventory. Every line is paid fresh, and a small output's removal cannot repay the marker it leaves behind. At 3000 the shipped preset produced ZERO extractions across 3,437 production requests."}, + {Key: "min_inventory", Type: components.FieldInt, Default: defaultMinInventory, Min: 1, + Hint: "Fewest candidates worth asking about; below it the sweep declines without asking. The model's judgement is a function of how many candidates it COMPARES, and the numbers are far apart: shown one output it scored 6% live-kept on haiku and 14% on sonnet, both inside the drop-everything null model's error bar, while ~15 together reached 58% at the lowest cost per output. At batch 3-6 it dropped a genuinely-spent output 2 times in 4; at 10, 4 in 4. Below the floor a removal is a guess, and a wrong removal costs content the agent still needs while a wrong keep costs one turn's tokens. Lower it only to trade that asymmetry away deliberately."}, + {Key: "pre_expiry_seconds", Type: components.FieldInt, Default: int(defaultPreExpiry / time.Second), + Hint: "How long before the prompt cache's believed expiry the sweep may fire. The window is where BOTH halves are cheap: the ask still reads a live cache, and the prefix it invalidates has little life left. The TTL itself is read from the request, never assumed. This WIDTH is the component's one unmeasured number — wider fires more often and invalidates more remaining TTL, narrower fires rarely, and nothing measures either side."}, + {Key: "block_fallback", Type: components.FieldBool, + Hint: "Decline instead of falling back when the prefix ask could not read the cache. Unset = FALSE: the fallback asks again with a bounded sample of each output copied into the prompt, which keeps the component working on a session's first turn and whenever a cache entry has gone — but pays fresh for content the cached path reads for a tenth of the price. Set true where the bill matters more than the removal. The miss is counted either way."}, + markerModeField(), + }) +} diff --git a/components/offload/extract_sweep_depth_test.go b/components/offload/extract_sweep_depth_test.go new file mode 100644 index 00000000..7dbfaadf --- /dev/null +++ b/components/offload/extract_sweep_depth_test.go @@ -0,0 +1,175 @@ +package offload + +import ( + "strconv" + "strings" + "sync/atomic" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/internal/extract" + "github.com/rossoctl/context-guru/schema" + "github.com/rossoctl/context-guru/store" +) + +// toolResultMsgWithID is toolResultMsg carrying the wire's own tool-call id, which apply.normalize +// sets on every synthetic tool message it lifts out of an Anthropic tool_result block. The plain +// helper leaves it nil, which is why no existing fixture could catch #123. +func toolResultMsgWithID(id, text string) bschemas.ChatMessage { + m := toolResultMsg(text) + callID := id + m.ChatToolMessage = &bschemas.ChatToolMessage{ToolCallID: &callID} + return m +} + +// deepCandidates builds n tool outputs each carrying a distinct wire id. +func deepCandidates(n int) *bschemas.BifrostChatRequest { + req := &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{userMsg("do the thing")}} + for i := 0; i < n; i++ { + req.Input = append(req.Input, toolResultMsgWithID("toolu_wire_"+strconv.Itoa(i), + strings.Repeat("candidate "+strconv.Itoa(i)+" distinct line\n", 900))) + } + return req +} + +// CANDIDATES ARE THE ENTIRE TRANSCRIPT, INCLUDING WHAT IS INSIDE THE CACHED PREFIX. +// +// This is the test whose absence hid #122. Every other sweep test runs with MaxCachedIdx: -1, which +// disables the tail gate altogether, so none of them could see that the gate was refusing every +// candidate the prefix ask can actually read. +// +// The defect: the depth permission was keyed on the cache having ALREADY expired (TailOnlyCold's +// optIn && c.ColdCache), which the pre-expiry window makes false by construction. So candidates +// collapsed to the uncached tail, while the ask reads the previous turn's sent body — everything up +// to the boundary. Disjoint sets: the model was asked to judge outputs it had never read, and live +// traffic showed it dropping one having seen only a 90-char head and a token count. +// +// A real MaxCachedIdx is therefore the whole point of this fixture. With the boundary at 4, messages +// 1..4 sit INSIDE the cached prefix and must still be offered. +func TestSweepOffersCandidatesInsideTheCachedPrefix(t *testing.T) { + req := deepCandidates(12) + asker := &fakeAsker{reply: `[]`, cacheRead: 19595} + e := newSweep(t, "") + c := preExpiryCtx("s-depth", asker, store.NewMemory(store.Options{})) + // The boundary the regression tripped over: a live cached prefix covering most of the transcript. + c.MaxCachedIdx = 8 + rep := components.Report{} + if _, err := e.Offload(req, &rep, c); err != nil { + t.Fatalf("offload: %v", err) + } + + // Precondition: the ask must have happened, or every assertion below is vacuous. + if got := rep.Events["sweep_offered"]; got != 12 { + t.Fatalf("sweep_offered = %d, want 12: candidates inside the cached prefix were refused, "+ + "which is #122 — the ask reads that region and the tail gate was excluding it; gates=%v", + got, rep.Gates) + } + // The refusal counter from the regression must not fire at all. + if got := rep.Gates["cached_prefix"]; got != 0 { + t.Errorf("cached_prefix = %d, want 0: the sweep accepts prefix invalidation by construction, "+ + "so depth is not a refusal reason for it", got) + } + // And the positive counter must say the component genuinely reached past the boundary. Its going + // to zero is the signal that this regressed again, which a refusal counter could not distinguish + // from "nothing was deep this turn". + if got := rep.Events["sweep_candidate_at_depth"]; got == 0 { + t.Errorf("sweep_candidate_at_depth = 0 with MaxCachedIdx=%d over %d messages: nothing was "+ + "counted as deep, so either the boundary is not being read or the fixture is wrong; "+ + "gates=%v", c.MaxCachedIdx, len(req.Input), rep.Gates) + } +} + +// THE INVENTORY FLOOR: below it, do not ask at all. +// +// The yield of this mechanism is a property of how many candidates the model compares. Shown one +// output a model scored 6% live-kept on haiku and 14% on sonnet, both inside the drop-everything +// null model's error bar; at batch 3-6 it dropped a genuinely-spent output 2 times in 4, and at +// batch 10 it dropped it 4 in 4. Below the floor a `drop` is a guess, and a wrong drop is a silent +// permanent loss while a wrong keep costs one turn's tokens — so declining is strictly better than +// asking. +// +// Both arms share one config and differ only in how many candidates exist, so this asserts the floor +// rather than something general about small transcripts. +func TestSweepDeclinesBelowTheInventoryFloor(t *testing.T) { + for _, tc := range []struct { + name string + n int + wantAsked bool + }{ + {"below the floor", 4, false}, + {"at the floor", 10, true}, + } { + t.Run(tc.name, func(t *testing.T) { + asker := &fakeAsker{reply: `[]`, cacheRead: 19595} + e := newSweep(t, "") // min_inventory defaults to 10 + c := preExpiryCtx("s-floor-"+tc.name, asker, store.NewMemory(store.Options{})) + rep := components.Report{} + if _, err := e.Offload(deepCandidates(tc.n), &rep, c); err != nil { + t.Fatalf("offload: %v", err) + } + asked := atomic.LoadInt64(&asker.calls) > 0 + if asked != tc.wantAsked { + t.Errorf("%d candidates: asked=%v, want %v; gates=%v", + tc.n, asked, tc.wantAsked, rep.Gates) + } + // The decline must be COUNTED. A component that declines is indistinguishable from one + // that is broken unless it says so — the failure mode that hid this component's own + // economic_gate blind spot and three vacuous trim tests before it. + if !tc.wantAsked { + if got := rep.Gates["sweep_inventory_below_min"]; got != tc.n { + t.Errorf("sweep_inventory_below_min = %d, want %d: a silent decline is "+ + "indistinguishable from a broken component; gates=%v", got, tc.n, rep.Gates) + } + } else if rep.Gates["sweep_inventory_below_min"] != 0 { + t.Errorf("the floor fired at %d candidates; gates=%v", tc.n, rep.Gates) + } + }) + } +} + +// THE ANCHOR MUST BE THE WIRE'S ID, ASSERTED BY PROVENANCE RATHER THAN BY RENDERING. +// +// #123: the inventory announced `tool_use id 300c312d1492952219bfb1c4` — extract.ContentKey, our own +// store key — while the real id in that transcript was `toolu_d2`. The contract tells the model the +// id is "shown only so you can find the output in the conversation above", so shipping a string that +// appears nowhere in the conversation is worse than omitting it: it directs the model to look up a +// key that cannot be found. +// +// The previous test could not catch it because it hard-coded `ID: "toolu_abc123"` into an +// AdjudicationItem and asserted only that BuildPrefixAsk rendered whatever was in the field. That +// passes on any string. This one starts from a REQUEST and checks what the component chose to ship, +// which is where the substitution happened. +func TestSweepShipsTheWiresToolCallIDNotTheContentKey(t *testing.T) { + req := deepCandidates(12) + asker := &fakeAsker{reply: `[]`, cacheRead: 19595} + e := newSweep(t, "") + rep := components.Report{} + if _, err := e.Offload(req, &rep, + preExpiryCtx("s-anchor", asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatalf("offload: %v", err) + } + ask := asker.ask() + if ask == "" { + t.Fatal("no ask was recorded, so the assertion is vacuous") + } + // Every wire id must be present: it is the anchor, and the model is told it can be found above. + for i := 0; i < 12; i++ { + want := "toolu_wire_" + strconv.Itoa(i) + if !strings.Contains(ask, want) { + t.Errorf("the ask does not carry the wire id %q, so the locating anchor names nothing "+ + "the model can find in the transcript", want) + } + } + // And our own content key must NOT be: shipping it is #123 exactly. Derived the same way the + // component does, so this cannot drift from the implementation. + for i := range req.Input { + if req.Input[i].Role != bschemas.ChatMessageRoleTool { + continue + } + if key := extract.ContentKey(schema.MessageText(req.Input[i])); strings.Contains(ask, key) { + t.Errorf("the ask carries the CONTENT KEY %q, which appears nowhere in the transcript "+ + "— that is #123: the anchor directs the model to a key it cannot find", key) + } + } +} diff --git a/components/offload/extract_sweep_drop.go b/components/offload/extract_sweep_drop.go new file mode 100644 index 00000000..a6f2810f --- /dev/null +++ b/components/offload/extract_sweep_drop.go @@ -0,0 +1,102 @@ +package offload + +import ( + "encoding/json" + "fmt" + "strings" + + bschemas "github.com/maximhq/bifrost/core/schemas" + + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/expand" + "github.com/rossoctl/context-guru/schema" +) + +// THE DROP PATH for the cold-sweep adjudicator: what a dropped tool output leaves behind, and how +// it stays recoverable. +// +// Two properties are load-bearing here, and each is a test. +// +// THE RESIDUE TRANSPORTS NOTHING. It is computed from the output's SHAPE — class, size, line and +// record counts — by this code, and it contains no byte of the output itself. That is stricter than +// the merged design's residue, which fell back to a 96-character head peek for unstructured content. +// A head peek is our code copying rather than the model copying, so it is not the failure cc1aa9f +// was about, but it is still content in the request that nothing verified, and the whole point of an +// adjudicator over a compactor is that no content moves. It also does not help where it looks like +// it should: for a record set the first rows say nothing about whether the field you want is in +// there, which is the argument the merged design already accepted for structured content and then +// declined to apply to the rest. +// +// THE DROP IS REVERSIBLE, AND THE MODEL IS NOT TOLD SO. A full marker stashes the original and +// `expand` restores it. A drop advertised as reversible that is not would be a worse defect than no +// drop at all, so it is verified end to end rather than assumed from the fact that commitMark was +// called. Note the asymmetry with the prompt, which never mentions recoverability: measured, +// reassuring the model that removals stay recoverable produced 91% removal at 6% live-kept, so the +// operator gets the safety net and the model does not get to hear about it. + +// sweepDescriptor renders the shape residue for a dropped output. +// +// It answers "what was here", never "what did it say". contentClass supplies the kind from the same +// head-sniffing regexes the economic gate is calibrated on, so the descriptor and the gate cannot +// disagree about what a candidate is. +func sweepDescriptor(content string) string { + kind := "tool output" + if name, _, ok := contentClass(content); ok { + kind = name + } + lines := strings.Count(content, "\n") + 1 + shape := fmt.Sprintf("%s, %d lines, %d tokens", kind, lines, schema.TextTokens(content)) + if n, ok := recordCount(content); ok { + shape = fmt.Sprintf("%s, %d records, %d lines, %d tokens", kind, n, lines, + schema.TextTokens(content)) + } + return "[context-guru removed a spent tool output — " + shape + "]" +} + +// recordCount counts the top-level elements of a JSON array, which is the one "how much was here" +// figure a line count cannot supply: a multi-megabyte API result is routinely a SINGLE line, and +// "1 line" is a useless thing to tell an agent about 200 records. +// +// Only a top-level array, and only when it parses. A partial or streaming payload gets the line +// count alone rather than a guess, because the descriptor's whole value is that every number in it +// is true. +func recordCount(content string) (int, bool) { + s := strings.TrimSpace(content) + if !strings.HasPrefix(s, "[") { + return 0, false + } + var rows []json.RawMessage + if err := json.Unmarshal([]byte(s), &rows); err != nil { + return 0, false + } + return len(rows), true +} + +// applySweepDrop replaces one adjudicated-spent tool output with its shape descriptor plus the +// marker, stashing the original so `expand` can restore it. It reports the store key it wrote (empty +// in the degraded marker modes) and whether the message was changed at all. +// +// Serial by contract, like extract_llm's own splice: the store write and the message mutation are +// not concurrency-safe. +// +// It goes through tryMark rather than writing the text directly, which is what makes the +// never-worse check MARKER-INCLUSIVE. The descriptor is small, but the marker plus its recovery hint +// is not free, and the pipeline's aggregate guard is per-request rather than per-message — so +// without this a drop just above the floor could grow the message it was meant to shrink. +func applySweepDrop(c *components.Ctx, rep *components.Report, mode markerMode, + msg *bschemas.ChatMessage, content string) (key string, ok bool) { + desc := sweepDescriptor(content) + hint := " [full output: call " + expand.ToolName + "]" + newText, key, eff, ok := tryMark(c, mode, content, hint, func(tok string) string { + if tok == "" { + return desc + } + return desc + "\n" + tok + }) + if !ok { + return "", false + } + commitMark(c, rep, eff, key, content) + schema.SetMessageText(msg, newText) + return key, true +} diff --git a/components/offload/extract_sweep_drop_test.go b/components/offload/extract_sweep_drop_test.go new file mode 100644 index 00000000..22d5722b --- /dev/null +++ b/components/offload/extract_sweep_drop_test.go @@ -0,0 +1,140 @@ +package offload + +import ( + "strings" + "testing" + + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/expand" + "github.com/rossoctl/context-guru/schema" + "github.com/rossoctl/context-guru/store" +) + +// sweepFixture is a spent-looking tool output whose every word is distinctive, so invariant 6 can +// assert that NONE of them reached the descriptor. Long enough that a drop is a real reduction. +const sweepFixture = `KUBERNETES_NAMESPACE=quarantine-zebra +deployment/ingress-flamingo READY 3/3 RESTARTS 0 +deployment/ledger-armadillo READY 1/1 RESTARTS 17 +SECRET_TOKEN_kalamazoo_74119 rotated at 2026-08-14T09:31:02Z +warning: pod ledger-armadillo evicted, reason=MemoryPressure +` + `filler-line-that-is-here-only-to-clear-the-never-worse-check +` + +// INVARIANT 5. A dropped output stays recoverable: the marker is written, the original is stashed, +// and expand resolves it BYTE-FOR-BYTE. A drop advertised as reversible that is not would be a worse +// defect than no drop at all. +func TestDroppedOutputStaysRecoverable(t *testing.T) { + st := store.NewMemory(store.Options{}) + rep := &components.Report{} + c := &components.Ctx{Session: "s", Store: st} + msg := tool(sweepFixture) + + key, ok := applySweepDrop(c, rep, markerFull, &msg, sweepFixture) + // PRECONDITION: the drop actually happened. Without this the assertions below are vacuous — + // a helper that returned early and changed nothing would leave the original in place, and + // "recoverable" is trivially true of content that was never removed. + if !ok { + t.Fatal("the drop was refused, so nothing under test ran") + } + got := schema.MessageText(msg) + if got == sweepFixture { + t.Fatal("the message was not rewritten, so no recovery path was exercised") + } + if key == "" { + t.Fatal("no store key: the original was never stashed") + } + if rep.Irreversible { + t.Fatal("a full-marker drop must not be recorded as irreversible") + } + + keys := expand.ParseMarkers(got) + if len(keys) != 1 { + t.Fatalf("expected exactly one resolvable marker, got %d in %q", len(keys), got) + } + orig, resolved := expand.Resolve(st, keys[0]) + if !resolved { + t.Fatal("the marker did not resolve — the drop would be unrecoverable") + } + if orig != sweepFixture { + t.Fatalf("round-trip is not byte-for-byte:\n want %q\n got %q", sweepFixture, orig) + } + // And it must be strictly smaller, marker included, or the drop cost more than it saved. + if schema.TextTokens(got) >= schema.TextTokens(sweepFixture) { + t.Errorf("drop did not shrink the message: %d tokens from %d", + schema.TextTokens(got), schema.TextTokens(sweepFixture)) + } +} + +// A store that cannot persist must not leave an unresolvable marker behind: the drop degrades to a +// markerless one and records the deliberate lossy removal, so the pipeline keeps it rather than +// reverting it. +func TestDroppedOutputWithoutAPersistingStoreLeavesNoDanglingMarker(t *testing.T) { + rep := &components.Report{} + c := &components.Ctx{Session: "s", Store: store.Nop{}} + msg := tool(sweepFixture) + key, ok := applySweepDrop(c, rep, markerFull, &msg, sweepFixture) + if !ok { + t.Fatal("the drop was refused, so nothing under test ran") + } + got := schema.MessageText(msg) + if key != "" || strings.Contains(got, "< 0 { + b.WriteString(",") + } + b.WriteString(`{"n":777}`) + } + b.WriteString("]") + desc := sweepDescriptor(b.String()) + if !strings.Contains(desc, "200 records") { + t.Fatalf("a 200-record single-line array must report its record count, got %q", desc) + } +} diff --git a/components/offload/extract_sweep_intent_test.go b/components/offload/extract_sweep_intent_test.go new file mode 100644 index 00000000..be5524d6 --- /dev/null +++ b/components/offload/extract_sweep_intent_test.go @@ -0,0 +1,126 @@ +package offload + +import ( + "strings" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/store" +) + +// THE FALLBACK'S GOAL MUST LEAD WITH THE CURRENT STEP, NOT THE OPENING INSTRUCTION. +// +// The fallback has no transcript by construction, so the goal string is the only thing that can tell +// it the task closed. conversationGoal leads with the FIRST user message, which for a spent-ness +// judgement is actively misleading: it describes what the session set out to do, i.e. exactly what +// may now be finished. +// +// MEASURED (#125): two near-identical transcripts, twelve candidates each — the prefix ask dropped +// 12 of 12, the fallback kept 12 of 12 and cited the original read instruction as the obligation for +// every one. +// +// The original instruction is still included, because criterion (b) is an unfinished USER +// instruction and a standing "…and summarise them at the end" lives in that message. What changes is +// its POSITION and that it is flagged as possibly satisfied. Dropping it would trade a bias toward +// keeping for a bias toward dropping, which is the direction that loses content. +func TestSweepIntentLeadsWithTheCurrentStepNotTheOpeningInstruction(t *testing.T) { + const opening = "Read every CSV under data/ and tell me the row count in each" + const latest = "thanks, that is all I needed on the counts" + const asstLast = "I will now write the summary to report.md" + req := &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{ + userMsg(opening), + toolResultMsg(strings.Repeat("data/a.csv 1200 rows\n", 400)), + assistantMsg(asstLast), + userMsg(latest), + }} + + got := sweepIntent(req) + + // All three parts must be present: dropping the opening instruction would lose criterion (b). + for _, want := range []string{opening, latest, asstLast} { + if !strings.Contains(got, want) { + t.Fatalf("intent lost a part it needs: %q missing from\n%s", want, got) + } + } + // THE FIX: the current step precedes the opening instruction. This is the assertion that fails if + // the ordering regresses to conversationGoal's. + if strings.Index(got, latest) > strings.Index(got, opening) { + t.Errorf("the opening instruction precedes the current step, which is the ordering that "+ + "made the fallback keep everything:\n%s", got) + } + // And the opening instruction must be FLAGGED, or its position alone still leaves the model to + // guess whether it is outstanding. + if !strings.Contains(got, "MAY ALREADY BE SATISFIED") { + t.Errorf("the opening instruction is not marked as possibly satisfied, so the model has no "+ + "way to tell a standing obligation from a finished one:\n%s", got) + } + // Each part must be labelled with which criterion it serves; an unlabelled blob is what the model + // had to infer structure from before. + for _, label := range []string{"MOST RECENT USER TURN", "THE AGENT'S OWN LAST STATEMENT"} { + if !strings.Contains(got, label) { + t.Errorf("part %q is unlabelled:\n%s", label, got) + } + } +} + +// THE FALLBACK'S KEEP-ALL IS COUNTED APART FROM THE PREFIX ASK'S. +// +// Only the prefix-ask half was asserted before. The split is the whole point: a keep-all means +// different things on each path, and the fallback's is the one worth watching — it has no transcript, +// so it resolves toward keep structurally (#125). Averaged together, a run reads as "the component +// sometimes acts" when the real variable is whether the cache read happened. +// +// The two arms differ ONLY in whether the asker reports a cache read, so this asserts the path split +// rather than something general about keep-alls. +func TestSweepCountsAFallbackKeepAllApartFromAPrefixAskKeepAll(t *testing.T) { + for _, tc := range []struct { + name string + cacheRead int + wantGate string + notGate string + }{ + {"prefix ask", 19595, "sweep_kept_everything", "sweep_fallback_kept_everything"}, + {"fallback", 0, "sweep_fallback_kept_everything", "sweep_kept_everything"}, + } { + t.Run(tc.name, func(t *testing.T) { + // An empty array is the contract's own way of saying "keep everything", on both paths. + asker := &fakeAsker{reply: `[]`, cacheRead: tc.cacheRead} + prev := fallbackModel.reply + fallbackModel.reply = `[]` + defer func() { fallbackModel.reply = prev }() + + e := newSweepSmall(t, "") + rep := &components.Report{} + if _, err := e.Offload(sweepReq(), rep, + preExpiryCtx("s-keepall-"+tc.name, asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + if rep.Gates[tc.wantGate] == 0 { + t.Errorf("%s path did not raise %q; gates=%v", tc.name, tc.wantGate, rep.Gates) + } + if rep.Gates[tc.notGate] != 0 { + t.Errorf("%s path raised the OTHER path's counter %q; the two must not be averaged; "+ + "gates=%v", tc.name, tc.notGate, rep.Gates) + } + }) + } +} + +// A transcript whose only user turn is the opening one must not repeat it twice under two labels — +// that would read as two independent obligations pointing at the same text. +func TestSweepIntentDoesNotDuplicateASingleUserTurn(t *testing.T) { + const only = "summarize the log" + req := &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{ + userMsg(only), + toolResultMsg(strings.Repeat("line\n", 400)), + }} + got := sweepIntent(req) + if n := strings.Count(got, only); n != 1 { + t.Errorf("the single user turn appears %d times; it must appear once:\n%s", n, got) + } + if strings.Contains(got, "MAY ALREADY BE SATISFIED") { + t.Errorf("the only user turn is also the current step, so it must not be flagged as "+ + "possibly satisfied:\n%s", got) + } +} diff --git a/components/offload/extract_sweep_test.go b/components/offload/extract_sweep_test.go new file mode 100644 index 00000000..fdc3826c --- /dev/null +++ b/components/offload/extract_sweep_test.go @@ -0,0 +1,834 @@ +package offload + +import ( + "context" + "errors" + "fmt" + "regexp" + "strconv" + "strings" + "sync/atomic" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/expand" + "github.com/rossoctl/context-guru/schema" + "github.com/rossoctl/context-guru/store" +) + +// fakeAsker stands in for the host's PrefixAsker. It records what was asked and what session it was +// asked for, and reports the cache read the caller gates on. +type fakeAsker struct { + reply string + cacheRead int + err error + calls int64 + lastAsk atomic.Value + lastSess atomic.Value +} + +func (f *fakeAsker) Ask(_ context.Context, session, ask string) (string, components.PrefixUsage, error) { + atomic.AddInt64(&f.calls, 1) + f.lastAsk.Store(ask) + f.lastSess.Store(session) + if f.err != nil { + return "", components.PrefixUsage{}, f.err + } + return f.reply, components.PrefixUsage{CacheRead: f.cacheRead, Fresh: 40, Output: 90}, nil +} + +func (f *fakeAsker) ask() string { s, _ := f.lastAsk.Load().(string); return s } +func (f *fakeAsker) sess() string { s, _ := f.lastSess.Load().(string); return s } + +// labelAsker answers for every label the inventory names, so a test exercises the whole reply rather +// than only the first candidate — the blind spot that let a batch-of-one arm pass for bulk. +type labelAsker struct { + fakeAsker + verdict, needed, quote string +} + +var askLabelRe = regexp.MustCompile(`\[(\d+)\] `) + +func (m *labelAsker) Ask(ctx context.Context, session, ask string) (string, components.PrefixUsage, error) { + labels := askLabelRe.FindAllStringSubmatch(ask, -1) + parts := make([]string, 0, len(labels)) + for _, l := range labels { + parts = append(parts, `{"i":`+l[1]+`,"needed_by":"`+m.needed+ + `","quote":"`+m.quote+`","verdict":"`+m.verdict+`"}`) + } + m.fakeAsker.reply = "[" + strings.Join(parts, ",") + "]" + return m.fakeAsker.Ask(ctx, session, ask) +} + +// newSweep builds the component through its registered constructor, so the config surface under test +// is the real one. The floor is above the filler outputs in sweepReq, so exactly the intended +// candidates reach the inventory. +func newSweep(t *testing.T, extraYAML string) *ExtractSweep { + t.Helper() + c, err := newExtractSweep([]byte("min_tokens: 2000\n" + extraYAML)) + if err != nil { + t.Fatalf("newExtractSweep: %v", err) + } + return c.(*ExtractSweep) +} + +// newSweepSmall is newSweep with the inventory floor lowered to 1. +// +// For the tests whose subject is PER-CANDIDATE accounting — this counter fired once, that verdict was +// ignored, this removal replayed — and which therefore assert exact counts against sweepReq's single +// above-floor output. Under the default floor of 10 those inventories decline before any ask, so the +// tests would assert an impossibility. +// +// It is a deliberate opt-in rather than the default for these, because the config it selects is one +// the measurements refute: at an inventory of one a model scored 6% live-kept, inside the +// drop-everything null model's error bar. A test may run there to count something precisely; a +// deployment should not. The floor's own behaviour is covered against the real default in +// TestSweepDeclinesBelowTheInventoryFloor, and the default is exercised end to end in +// TestSweepOffersCandidatesInsideTheCachedPrefix. +func newSweepSmall(t *testing.T, extraYAML string) *ExtractSweep { + t.Helper() + return newSweep(t, "min_inventory: 1\n"+extraYAML) +} + +// sweepReq puts a BIG tool output in the tail, and states an obligation the refusal test quotes back. +func sweepReq() *bschemas.BifrostChatRequest { + req := &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{ + userMsg("Find the auth timeout in src/api/users.py and fix it."), + toolResultMsg(strings.Repeat("2024-01-01 GET /users/42 200 12ms src/api/users.py\n", 700)), + assistantMsg("Next I will patch the timeout in src/api/users.py."), + toolResultMsg(strings.Repeat("filler line to grow the transcript\n", 50)), + userMsg("keep going"), + }} + return req +} + +// sweepReqStocked is sweepReq with enough further candidates to clear the inventory floor, for the +// tests whose subject is that the component ACTS — an ask happening, a removal applying, the fallback +// firing. Those cannot run on sweepReq alone any more: it carries a single above-floor output, and an +// inventory of one is the per-output shape `4ca1f13` records as refuted at 6% live-kept, which the +// floor now declines by design. +// +// Kept SEPARATE from sweepReq rather than folded into it, because several tests assert exact +// per-candidate counts against that fixture's precise shape and stocking it shifts every one of them. +// A test asserting "one candidate was adjudicated" and a test asserting "the component acted at all" +// want different fixtures, and conflating them is how a fixture change becomes a four-test cascade. +// +// Appended AFTER the original five, so index 1 remains the big output and index 0's obligation text is +// unchanged — the refusal test quotes both back. +func sweepReqStocked() *bschemas.BifrostChatRequest { + req := sweepReq() + for i := 0; i < defaultMinInventory; i++ { + req.Input = append(req.Input, toolResultMsgWithID("toolu_fixture_"+strconv.Itoa(i), + strings.Repeat("record "+strconv.Itoa(i)+" of the audit log\n", 900))) + } + return req +} + +// manyCandidates builds a transcript of n distinct tool outputs, all above the floor. +func manyCandidates(n int) *bschemas.BifrostChatRequest { + req := &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{userMsg("do the thing")}} + for i := 0; i < n; i++ { + req.Input = append(req.Input, + toolResultMsg(strings.Repeat("candidate "+strconv.Itoa(i)+" distinct line\n", 900))) + } + return req +} + +// preExpiryCtx puts the turn INSIDE the pre-expiry window: the cache still exists (idle < ttl) and it +// is within preExpiry of expiring. ttl 5 minutes is what a bare `ephemeral` mark buys. +func preExpiryCtx(session string, asker components.PrefixAsker, st store.Store) *components.Ctx { + return &components.Ctx{ + Session: session, Ctx: context.Background(), Store: st, CtxWindow: 1_000_000, + ModelName: "claude-sonnet-5", + // No depth restriction, so the candidates reach the inventory: the tail gate is exercised + // separately below. + CacheAware: true, MaxCachedIdx: -1, + ColdCache: false, IdleMs: 4 * 60 * 1000, CacheTTLMs: 5 * 60 * 1000, + PrefixAsk: asker, + // A request model, so the FALLBACK path has somewhere to go. Without it a missed cache read + // would decline for lack of a model rather than for the reason under test. + Model: components.ModelSpec{Incoming: fallbackModel, Static: fallbackModel}, + } +} + +// fallbackModel answers the self-contained fallback prompt, and records what it was shown so a test +// can assert that the expensive path really is the one that carries content. +var fallbackModel = &recordingModel{reply: `[{"i":0,"needed_by":"none","quote":"","verdict":"keep"}]`} + +type recordingModel struct { + reply string + calls int64 + prompt atomic.Value +} + +func (m *recordingModel) Complete(_ context.Context, prompt string) (string, error) { + atomic.AddInt64(&m.calls, 1) + m.prompt.Store(prompt) + return m.reply, nil +} + +func (m *recordingModel) lastPrompt() string { s, _ := m.prompt.Load().(string); return s } + +// The whole point: inside the window the sweep asks ONE question over the cached transcript and removes +// what the model says is spent, leaving a recoverable marker. +func TestSweepRemovesSpentOutputsFromOneCachedAsk(t *testing.T) { + asker := &labelAsker{verdict: "drop", needed: "none"} + asker.cacheRead = 19595 + e := newSweepSmall(t, "") + req := sweepReqStocked() + original := schema.MessageText(req.Input[1]) + st := store.NewMemory(store.Options{}) + rep := &components.Report{} + if _, err := e.Offload(req, rep, preExpiryCtx("s", asker, st)); err != nil { + t.Fatalf("Offload must fail open: %v", err) + } + // PRECONDITION: exactly one ask, and it acted. Without this every assertion below is vacuous — a + // component that never ran leaves the transcript exactly as a correct keep does. + if n := atomic.LoadInt64(&asker.calls); n != 1 { + t.Fatalf("expected ONE prefix ask, got %d (gates: %v)", n, rep.Gates) + } + if rep.Events["sweep_dropped"] == 0 { + t.Fatalf("no removal was recorded, so nothing under test ran (gates: %v)", rep.Gates) + } + got := schema.MessageText(req.Input[1]) + if got == original { + t.Fatal("the spent output was not removed") + } + if !strings.Contains(got, "context-guru removed a spent tool output") { + t.Errorf("no shape descriptor left in place: %q", got) + } + marks := expand.ParseMarkers(got) + if len(marks) != 1 { + t.Fatalf("expected one resolvable marker, got %d in %q", len(marks), got) + } + if back, ok := expand.Resolve(st, marks[0]); !ok || back != original { + t.Fatalf("the removal is not recoverable: ok=%v byte-identical=%v", ok, back == original) + } + // The ask went out under the SCOPED session id, which is what the host keys its stash by. + if asker.sess() != "s" { + t.Errorf("the ask was made for session %q, not the request's", asker.sess()) + } + // And the verified read is recorded, because it is the mechanism's whole justification. + if rep.Events["sweep_prefix_cache_read_ok"] != 1 { + t.Errorf("the cache read was not recorded (gates: %v)", rep.Gates) + } +} + +// ONE CALL FOR EVERY CANDIDATE. There is no batching any more: nothing is copied per candidate, so +// there is nothing to divide. Twenty candidates must be one ask naming twenty labels. +func TestSweepAsksOnceForEveryCandidate(t *testing.T) { + const n = 20 + asker := &labelAsker{verdict: "keep", needed: "none"} + asker.cacheRead = 19595 + e := newSweepSmall(t, "") + rep := &components.Report{} + if _, err := e.Offload(manyCandidates(n), rep, + preExpiryCtx("s", asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + if got := atomic.LoadInt64(&asker.calls); got != 1 { + t.Fatalf("%d candidates took %d asks; the shape is ONE ask over the cached transcript", n, got) + } + // UP TO THE CAP, and the remainder counted rather than dropped silently. The cap exists because + // the reply carries a verbatim quote per verdict against a fixed budget, and truncation discards + // the WHOLE array — so an uncapped ask on a large transcript sweeps nothing at all. See + // maxAskItems. What this test still pins is the shape: ONE ask, not one call per output. + if got := rep.Events["sweep_adjudicated"]; got != maxAskItems { + t.Fatalf("sweep_adjudicated = %d, want %d (the ask cap) — the inventory was starved before "+ + "assembly (gates: %v)", got, maxAskItems, rep.Gates) + } + if got := rep.Gates["sweep_over_ask_cap"]; got != n-maxAskItems { + t.Errorf("sweep_over_ask_cap = %d, want %d: what the cap left unasked must be counted, or a "+ + "component that swept %d of %d would report plain success", got, n-maxAskItems, + maxAskItems, n) + } + if rep.Events["sweep_inventory_of_one"] != 0 { + t.Errorf("an inventory of %d was recorded as one (gates: %v)", n, rep.Gates) + } + // The cap must NOT trip the starvation tripwire: that alarm is for a pre-filter quietly thinning + // the comparison, and a deliberate ceiling firing it would make it noise on exactly the + // transcripts where it should be loudest. + if got := rep.Events["sweep_inventory_thinned"]; got != 0 { + t.Errorf("the ask cap tripped sweep_inventory_thinned (%d); that counter is for an upstream "+ + "filter, not for this ceiling", got) + } + // Every candidate the ask CARRIES must be named in it, and the cap keeps the largest. + ask := asker.ask() + named := 0 + for i := 0; i < n; i++ { + if strings.Contains(ask, "["+strconv.Itoa(i)+"] ") { + named++ + } + } + if named != maxAskItems { + t.Errorf("the one ask named %d candidates, want %d", named, maxAskItems) + } +} + +// A MISSED CACHE READ FALLS BACK BY DEFAULT, AND IS COUNTED EITHER WAY. +// +// The count is the part that is not optional: the cache read is the mechanism's whole justification, and +// a silent miss looks identical to a working call except on the bill. What happens next is a choice +// between two real costs, and the default keeps the component working — treating "no prefix" as "no +// verdicts" would disable it on every session's first turn and read as a model that declined to act. +func TestSweepFallsBackWhenTheCacheReadDidNotHappen(t *testing.T) { + asker := &fakeAsker{reply: `[{"i":0,"needed_by":"none","quote":"","verdict":"drop"}]`, cacheRead: 0} + before := atomic.LoadInt64(&fallbackModel.calls) + e := newSweepSmall(t, "") + rep := &components.Report{} + if _, err := e.Offload(sweepReqStocked(), rep, + preExpiryCtx("s", asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + // PRECONDITION: the prefix ask happened, so this is about the READ and not about never asking. + if n := atomic.LoadInt64(&asker.calls); n != 1 { + t.Fatalf("expected one prefix ask, got %d (gates: %v)", n, rep.Gates) + } + if rep.Gates["sweep_prefix_cache_read_ZERO"] != 1 { + t.Fatalf("a zero cache read was not counted; the mechanism's failure would be invisible "+ + "(gates: %v)", rep.Gates) + } + if rep.Events["sweep_fallback_used"] != 1 { + t.Fatalf("the default did not fall back, so the component stops working on a first turn "+ + "(gates: %v)", rep.Gates) + } + if rep.Gates["sweep_fallback_blocked"] != 0 { + t.Errorf("the fallback was blocked without block_fallback being set (gates: %v)", rep.Gates) + } + if n := atomic.LoadInt64(&fallbackModel.calls) - before; n != 1 { + t.Fatalf("the fallback made %d completions, want 1", n) + } + // THE FALLBACK IS THE PATH THAT CARRIES CONTENT, and that is exactly what makes it expensive. + // Asserted so the two paths cannot quietly converge: if the prefix ask ever started shipping + // samples, this is the only place the difference is visible. + if p := fallbackModel.lastPrompt(); !strings.Contains(p, "content:") { + t.Errorf("the fallback prompt carries no output content, so the model cannot judge: %.200q", p) + } +} + +// STRICT MODE forgoes the yield rather than paying for it. The right choice where the bill matters more +// than the removal, and the honest one to reach for if the zero-read counter turns out to be common. +func TestBlockFallbackDeclinesInsteadOfPaying(t *testing.T) { + asker := &fakeAsker{reply: `[{"i":0,"needed_by":"none","quote":"","verdict":"drop"}]`, cacheRead: 0} + before := atomic.LoadInt64(&fallbackModel.calls) + e := newSweepSmall(t, "block_fallback: true\n") + req := sweepReq() + original := schema.MessageText(req.Input[1]) + rep := &components.Report{} + if _, err := e.Offload(req, rep, preExpiryCtx("s", asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + if n := atomic.LoadInt64(&asker.calls); n != 1 { + t.Fatalf("expected one prefix ask, got %d (gates: %v)", n, rep.Gates) + } + // COUNTED IN THIS MODE TOO. That is the part that does not depend on the switch. + if rep.Gates["sweep_prefix_cache_read_ZERO"] != 1 { + t.Fatalf("strict mode did not count the missed read (gates: %v)", rep.Gates) + } + if rep.Gates["sweep_fallback_blocked"] != 1 { + t.Fatalf("block_fallback did not refuse the fallback (gates: %v)", rep.Gates) + } + if n := atomic.LoadInt64(&fallbackModel.calls) - before; n != 0 { + t.Fatalf("strict mode still paid for %d fallback completions", n) + } + if rep.Events["sweep_dropped"] != 0 { + t.Fatalf("strict mode acted on a full-price read (gates: %v)", rep.Gates) + } + if schema.MessageText(req.Input[1]) != original { + t.Fatal("content was removed in a mode that declined to act") + } +} + +// No asker at all — a non-Anthropic route, or no incoming client. Counted, and it takes the same fork +// as a missed read: fall back by default, decline under block_fallback. +func TestSweepFallsBackWithNoAsker(t *testing.T) { + before := atomic.LoadInt64(&fallbackModel.calls) + e := newSweepSmall(t, "") + rep := &components.Report{} + if _, err := e.Offload(sweepReq(), rep, + preExpiryCtx("s", nil, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + if rep.Gates["sweep_no_asker"] != 1 { + t.Fatalf("a missing asker was not counted (gates: %v)", rep.Gates) + } + if rep.Events["sweep_fallback_used"] != 1 { + t.Fatalf("no asker did not fall back (gates: %v)", rep.Gates) + } + if n := atomic.LoadInt64(&fallbackModel.calls) - before; n != 1 { + t.Fatalf("the fallback made %d completions, want 1", n) + } + + // And under block_fallback it declines instead. + before = atomic.LoadInt64(&fallbackModel.calls) + strict := newSweepSmall(t, "block_fallback: true\n") + rep2 := &components.Report{} + if _, err := strict.Offload(sweepReq(), rep2, + preExpiryCtx("s", nil, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + if rep2.Gates["sweep_fallback_blocked"] != 1 { + t.Fatalf("block_fallback did not refuse the no-asker fallback (gates: %v)", rep2.Gates) + } + if n := atomic.LoadInt64(&fallbackModel.calls) - before; n != 0 { + t.Fatalf("strict mode still paid for %d completions with no asker", n) + } +} + +// THE MODEL IS NOT A FREE CHOICE HERE, and the asymmetry with extract_llm is the point. This component +// reads the outputs from the prompt cache of the model it asks, and only the REQUEST's model has that +// cache — so `source: config` is incoherent rather than merely suboptimal, and must be refused with a +// reason rather than silently corrected. +func TestSweepRefusesAModelBlockAndSaysWhy(t *testing.T) { + _, err := newExtractSweep([]byte("model:\n source: config\n")) + if err == nil { + t.Fatal("a model block was accepted; a separate cheap model has no cache to read") + } + for _, want := range []string{"model", "source: config", "incoherent", "extract_llm"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the error does not mention %q, so the constraint reads as an oversight: %v", + want, err) + } + } +} + +// The first turn of a session has no stashed prefix. That is ordinary, happens once per session, and +// must be counted apart from a transport failure — one needs no attention and the other does. +func TestSweepCountsAMissingPrefixApartFromAFailure(t *testing.T) { + for _, tc := range []struct { + name, gate string + err error + }{ + {"first turn, nothing stashed", "sweep_no_prefix", components.ErrNoPrefix}, + {"the read itself failed", "sweep_ask_failed", errors.New("upstream 500")}, + } { + t.Run(tc.name, func(t *testing.T) { + asker := &fakeAsker{err: tc.err} + e := newSweepSmall(t, "") + rep := &components.Report{} + if _, err := e.Offload(sweepReq(), rep, + preExpiryCtx("s", asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + if atomic.LoadInt64(&asker.calls) != 1 { + t.Fatalf("the asker was not called, so nothing under test ran (gates: %v)", rep.Gates) + } + if rep.Gates[tc.gate] != 1 { + t.Fatalf("%s was not counted as %s (gates: %v)", tc.name, tc.gate, rep.Gates) + } + // And it falls back rather than skipping, which is what keeps the component alive on a + // session's first turn. + if rep.Events["sweep_fallback_used"] != 1 { + t.Errorf("%s did not fall back (gates: %v)", tc.name, rep.Gates) + } + }) + } +} + +// THE PRE-EXPIRY WINDOW. The ask needs a cache that still EXISTS; the removal wants one that is nearly +// worthless. Both hold only in the window before expiry, and this pins every edge of it. +func TestSweepFiresOnlyInThePreExpiryWindow(t *testing.T) { + for _, tc := range []struct { + name string + idleMs int64 + ttlMs int64 + cold bool + wantFire bool + }{ + {"inside the window", 4 * 60 * 1000, 5 * 60 * 1000, false, true}, + {"right at the early edge", 4 * 60 * 1000, 5 * 60 * 1000, false, true}, + {"too early: plenty of TTL left", 60 * 1000, 5 * 60 * 1000, false, false}, + {"too late: already expired", 6 * 60 * 1000, 5 * 60 * 1000, false, false}, + {"exactly at expiry", 5 * 60 * 1000, 5 * 60 * 1000, false, false}, + {"apply already called it cold", 4 * 60 * 1000, 5 * 60 * 1000, true, false}, + {"TTL unknown", 4 * 60 * 1000, 0, false, false}, + {"no previous turn on record", 0, 5 * 60 * 1000, false, false}, + {"a 1h TTL, one minute from expiry", 59*60*1000 + 30*1000, 60 * 60 * 1000, false, true}, + {"a 1h TTL, half an hour in", 30 * 60 * 1000, 60 * 60 * 1000, false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + e := newSweepSmall(t, "") + c := &components.Ctx{ + IdleMs: tc.idleMs, CacheTTLMs: tc.ttlMs, ColdCache: tc.cold, + } + if got := e.sweeping(c); got != tc.wantFire { + t.Fatalf("sweeping = %v, want %v (idle=%dms ttl=%dms cold=%v)", + got, tc.wantFire, tc.idleMs, tc.ttlMs, tc.cold) + } + }) + } +} + +// Outside the window nothing is asked and nothing is touched. +func TestSweepDoesNothingOutsideTheWindow(t *testing.T) { + asker := &labelAsker{verdict: "drop", needed: "none"} + asker.cacheRead = 19595 + e := newSweepSmall(t, "") + req := sweepReq() + original := schema.MessageText(req.Input[1]) + c := preExpiryCtx("s", asker, store.NewMemory(store.Options{})) + c.IdleMs = 30 * 1000 // plenty of TTL left + rep := &components.Report{} + if _, err := e.Offload(req, rep, c); err != nil { + t.Fatal(err) + } + if n := atomic.LoadInt64(&asker.calls); n != 0 { + t.Fatalf("a turn outside the window made %d asks", n) + } + if rep.Gates["not_in_pre_expiry_window"] == 0 { + t.Errorf("a turn outside the window must say why it did nothing (gates: %v)", rep.Gates) + } + if schema.MessageText(req.Input[1]) != original { + t.Fatal("a turn outside the window modified a message") + } +} + +// The window's width is configurable, and widening it moves the early edge. +func TestPreExpirySecondsWidensTheWindow(t *testing.T) { + narrow := newSweepSmall(t, "") + wide := newSweepSmall(t, "pre_expiry_seconds: 180\n") + // Three minutes idle on a five-minute TTL: two minutes remaining. + c := &components.Ctx{IdleMs: 3 * 60 * 1000, CacheTTLMs: 5 * 60 * 1000} + if narrow.sweeping(c) { + t.Error("the default one-minute window fired with two minutes of TTL left") + } + if !wide.sweeping(c) { + t.Error("a three-minute window did not fire with two minutes of TTL left") + } +} + +// A removal decided in the window MUST be replayed on every later turn. Without that, the next turn +// re-sends the removed output verbatim — the saving evaporates AND the prefix the provider is caching +// stops being byte-stable, which costs more than the sweep saved. +func TestSweepReplaysItsRemovalOnLaterTurns(t *testing.T) { + asker := &labelAsker{verdict: "drop", needed: "none"} + asker.cacheRead = 19595 + e := newSweepSmall(t, "") + st := store.NewMemory(store.Options{}) + + first := sweepReq() + original := schema.MessageText(first.Input[1]) + rep1 := &components.Report{} + if _, err := e.Offload(first, rep1, preExpiryCtx("sess", asker, st)); err != nil { + t.Fatal(err) + } + if rep1.Events["sweep_dropped"] == 0 { + t.Fatalf("the first turn did not remove anything, so there is nothing to replay (gates: %v)", + rep1.Gates) + } + windowText := schema.MessageText(first.Input[1]) + + later := sweepReq() // the same transcript again, on a turn outside the window + c := preExpiryCtx("sess", asker, st) + c.IdleMs = 5 * 1000 + rep2 := &components.Report{} + if _, err := e.Offload(later, rep2, c); err != nil { + t.Fatal(err) + } + if n := atomic.LoadInt64(&asker.calls); n != 1 { + t.Fatalf("the later turn asked again: %d asks total", n) + } + if rep2.Events["reapplied_same_session"] != 1 { + t.Fatalf("the later turn did not replay the frozen removal (gates: %v)", rep2.Gates) + } + got := schema.MessageText(later.Input[1]) + if got == original { + t.Fatal("the later turn re-sent the removed output verbatim; the saving is gone") + } + if got != windowText { + t.Fatalf("the replay is not byte-identical, so the cached prefix churns:\n first %q\n later %q", + windowText, got) + } +} + +// A verdict naming a label the inventory never offered must never be acted on: the label is how a +// decision is keyed to an output, so acting on a wrong one removes the wrong content — and indexing on +// it would panic rather than merely act wrongly. +func TestSweepIgnoresAVerdictForAnUnofferedLabel(t *testing.T) { + asker := &fakeAsker{reply: `[{"i":99,"needed_by":"none","quote":"","verdict":"drop"}]`, cacheRead: 19595} + e := newSweepSmall(t, "") + req := sweepReq() + original := schema.MessageText(req.Input[1]) + rep := &components.Report{} + if _, err := e.Offload(req, rep, preExpiryCtx("s", asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + if rep.Events["sweep_adjudicated"] == 0 { + t.Fatalf("no ask was made, so nothing under test ran (gates: %v)", rep.Gates) + } + if rep.Gates["sweep_verdict_unknown_label"] != 1 { + t.Fatalf("a verdict for an unoffered label was not counted (gates: %v)", rep.Gates) + } + if rep.Events["sweep_dropped"] != 0 { + t.Fatalf("a verdict for an unoffered label was ACTED ON (gates: %v)", rep.Gates) + } + if schema.MessageText(req.Input[1]) != original { + t.Fatal("content was removed on a verdict that named no offered output") + } + if rep.Gates["sweep_verdict_missing"] != 1 { + t.Errorf("the unjudged output was not counted (gates: %v)", rep.Gates) + } +} + +// A well-formed EMPTY array is the model keeping everything, which the contract invites. It must not be +// filed as a failure — that conflation made "declined to act" and "was never asked" the same number. +func TestSweepCountsKeepEverythingSeparatelyFromAFailure(t *testing.T) { + keepAll := &fakeAsker{reply: "[]", cacheRead: 19595} + e := newSweepSmall(t, "") + req := sweepReq() + original := schema.MessageText(req.Input[1]) + rep := &components.Report{} + if _, err := e.Offload(req, rep, preExpiryCtx("s", keepAll, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + if rep.Events["sweep_adjudicated"] == 0 { + t.Fatalf("no ask was made (gates: %v)", rep.Gates) + } + if rep.Gates["sweep_kept_everything"] != 1 { + t.Fatalf("a deliberate keep-all was not counted as one (gates: %v)", rep.Gates) + } + if rep.Gates["sweep_unparseable"] != 0 || rep.Gates["sweep_reply_truncated"] != 0 { + t.Errorf("a deliberate keep-all was filed as a failure (gates: %v)", rep.Gates) + } + if schema.MessageText(req.Input[1]) != original { + t.Error("keep-all removed something") + } + + // A TRUNCATED reply is a failure, and a different one from malformed junk. + cut := &fakeAsker{reply: `[{"i":0,"needed_by":"none","quote":"partial`, cacheRead: 19595} + e2 := newSweepSmall(t, "") + rep2 := &components.Report{} + if _, err := e2.Offload(sweepReq(), rep2, + preExpiryCtx("s2", cut, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + if rep2.Gates["sweep_reply_truncated"] != 1 { + t.Fatalf("a cut-off array was not counted as truncation (gates: %v)", rep2.Gates) + } + if rep2.Gates["sweep_unparseable"] != 0 { + t.Errorf("truncation was filed as a format failure; the two need opposite fixes (gates: %v)", + rep2.Gates) + } +} + +// The refusal, reaching all the way through the component: a drop naming an outstanding obligation +// leaves the output in place and raises the alertable counter. +func TestSweepRefusesADropThatNamesAnObligation(t *testing.T) { + asker := &labelAsker{verdict: "drop", needed: "c", + quote: "Next I will patch the timeout in src/api/users.py."} + asker.cacheRead = 19595 + e := newSweepSmall(t, "") + req := sweepReq() + original := schema.MessageText(req.Input[1]) + rep := &components.Report{} + if _, err := e.Offload(req, rep, preExpiryCtx("s", asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + if rep.Events["sweep_adjudicated"] == 0 { + t.Fatalf("no ask was made, so the refusal was never exercised (gates: %v)", rep.Gates) + } + if rep.Gates["sweep_drop_refused_obligation"] == 0 { + t.Fatalf("the refusal was not counted (gates: %v)", rep.Gates) + } + if rep.Events["sweep_dropped"] != 0 { + t.Fatalf("the contradictory drop was PERFORMED (gates: %v)", rep.Gates) + } + if schema.MessageText(req.Input[1]) != original { + t.Fatal("the output was removed despite naming an outstanding obligation") + } + // The quote IS in the transcript, so the fabrication counter must stay quiet. + if rep.Gates["sweep_quote_fabricated"] != 0 { + t.Errorf("a verbatim transcript quote was counted as fabricated (gates: %v)", rep.Gates) + } +} + +// A single-candidate inventory is the refuted shape wearing a new name, so it is COUNTED. Shown one +// output, a model simply drops it: 6% live-kept, inside the null model's error bar. +// +// UNREACHABLE UNDER THE DEFAULT CONFIG, which is why this test lowers the floor explicitly. With +// min_inventory at its default of 10 an inventory of one declines before any ask, so this counter can +// only fire where an operator has deliberately lowered the floor into the range the measurements +// refute. That is the counter's remaining job: not "this happens sometimes" but "you asked for this, +// and here is how often it is costing you". Keeping the test on the default would assert an +// impossibility; keeping the counter without the floor was what let the shape ship. +func TestSweepCountsAnInventoryOfOne(t *testing.T) { + asker := &labelAsker{verdict: "keep", needed: "a", quote: "Find the auth timeout"} + asker.cacheRead = 19595 + e := newSweepSmall(t, "") + rep := &components.Report{} + if _, err := e.Offload(sweepReq(), rep, + preExpiryCtx("s", asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + if rep.Events["sweep_adjudicated"] != 1 { + t.Fatalf("expected one candidate adjudicated, got %d (gates: %v)", + rep.Events["sweep_adjudicated"], rep.Gates) + } + if rep.Events["sweep_inventory_of_one"] != 1 { + t.Fatalf("an inventory of one was not counted; a starved inventory would be invisible "+ + "(gates: %v)", rep.Gates) + } +} + +// THE STARVATION TRIPWIRE, and it is a guard for a rebase rather than for today's code. +// +// `4ca1f13`'s real defect was a per-candidate PRE-FILTER at the gathering site: prefix_still_referenced +// removed 149,681 candidates and left about one per request, silently turning a bulk adjudication arm +// into the per-output shape refuted at 6% live-kept. PR #80 rebases index-driven candidate selection +// onto this branch and can recreate exactly that. +// +// On `main` nothing thins the list, so the counter must read ZERO — a tripwire that fires today would +// be noise, and one that cannot fire when something IS thinning would be useless. Both directions are +// asserted, the second by thinning the list on purpose. +func TestSweepCountsAThinnedInventory(t *testing.T) { + const n = 8 + asker := &labelAsker{verdict: "keep", needed: "none"} + asker.cacheRead = 19595 + e := newSweepSmall(t, "") + rep := &components.Report{} + if _, err := e.Offload(manyCandidates(n), rep, + preExpiryCtx("s", asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + // PRECONDITION: candidates really reached the inventory, or "nothing was thinned" is vacuous. + if rep.Events["sweep_offered"] != n { + t.Fatalf("sweep_offered = %d, want %d (gates: %v)", rep.Events["sweep_offered"], n, rep.Gates) + } + if rep.Events["sweep_inventory_thinned"] != 0 { + t.Errorf("nothing thins the list on main, so the tripwire must be silent (gates: %v)", rep.Gates) + } + + // And it MUST fire when something does thin it. Asserted on the arithmetic the component uses, + // because there is no pre-filter here to install: this is the comparison a rebase would trip. + var probe components.Report + eligible, offered := n, 1 // what prefix_still_referenced did, in miniature + if eligible > offered { + probe.EventN("sweep_inventory_thinned", eligible-offered) + } + if probe.Events["sweep_inventory_thinned"] != n-1 { + t.Fatalf("the tripwire's arithmetic does not count a thinned inventory: %v", probe.Events) + } +} + +// THE COUNTER CONTRACT, component end. These names are what an operator's dashboard query and alert +// rule are written against. The other end is pinned in proxy/sweep_counters_test.go. +func TestSweepRaisesTheContractedCounterNames(t *testing.T) { + const obligation = "Next I will patch the timeout in src/api/users.py." + for _, tc := range []struct { + name string + asker *labelAsker + want []string + notWant []string + }{ + {"a spent output", &labelAsker{verdict: "drop", needed: "none"}, + []string{"sweep_adjudicated", "sweep_dropped", "sweep_prefix_cache_read_ok"}, nil}, + {"an output still needed", &labelAsker{verdict: "keep", needed: "a", quote: obligation}, + []string{"sweep_adjudicated", "sweep_kept"}, []string{"sweep_quote_fabricated"}}, + {"a drop contradicting an obligation", &labelAsker{verdict: "drop", needed: "a", quote: obligation}, + []string{"sweep_drop_refused_obligation"}, []string{"sweep_dropped"}}, + {"an invented obligation", &labelAsker{verdict: "keep", needed: "a", quote: "rewrite it in Rust"}, + []string{"sweep_quote_fabricated"}, nil}, + {"an unanswered criterion", &labelAsker{verdict: "drop", needed: ""}, + []string{"sweep_criterion_missing"}, nil}, + } { + t.Run(tc.name, func(t *testing.T) { + tc.asker.cacheRead = 19595 + e := newSweepSmall(t, "") + rep := &components.Report{} + if _, err := e.Offload(sweepReq(), rep, + preExpiryCtx("s", tc.asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + // PRECONDITION: an adjudication happened. A component that never asked raises no + // counters, and every assertion would then fail for the wrong reason. + if rep.Events["sweep_adjudicated"] != 1 { + t.Fatalf("no adjudication was counted: %v", rep.Gates) + } + // Looked up across BOTH histograms on purpose. What this test pins is that the + // component raises the contracted NAME; which of the two series it lands in is a + // different contract, pinned at the exporter end in proxy/sweep_counters_test.go, and + // the no-name-in-both invariant is pinned by components.TestGatesAndEventsAreDisjoint. + // Asserting the series here as well would make one fixture fail for two unrelated + // reasons. + raised := func(name string) int { return rep.Gates[name] + rep.Events[name] } + both := func() string { return fmt.Sprintf("gates=%v events=%v", rep.Gates, rep.Events) } + for _, want := range tc.want { + if raised(want) == 0 { + t.Errorf("the component did not raise %q; got %s", want, both()) + } + } + for _, no := range tc.notWant { + if raised(no) != 0 { + t.Errorf("the component raised %q when it should not; got %s", no, both()) + } + } + }) + } +} + +// The knobs that have no meaning here must be REFUSED with a reason, not silently ignored. An operator +// migrating an older config by hand has no other way to learn that `max_calls` now means nothing. +func TestSweepRejectsKeysThatDoNotApply(t *testing.T) { + for _, tc := range []struct{ key, yaml string }{ + {"strategy", "strategy: code\n"}, + {"rewrite", "rewrite: false\n"}, + {"aggressiveness", "aggressiveness: high\n"}, + {"max_chars", "max_chars: 8000\n"}, + {"model", "model:\n model: claude-haiku-4-5\n"}, + {"context", "context: full\n"}, + {"context_messages", "context_messages: 7\n"}, + {"max_calls", "max_calls: 4\n"}, + {"economic_gate", "economic_gate: true\n"}, + } { + t.Run(tc.key, func(t *testing.T) { + _, err := newExtractSweep([]byte(tc.yaml)) + if err == nil { + t.Fatalf("%s was accepted; the key would silently do nothing", tc.key) + } + if !strings.Contains(err.Error(), tc.key) { + t.Errorf("error does not name the key: %v", err) + } + if !strings.Contains(err.Error(), "does not apply here") { + t.Errorf("error does not say why the key cannot apply: %v", err) + } + }) + } +} + +// The ask the component actually sends must be the inventory contract, and must carry no output body. +func TestSweepSendsTheInventoryAndNotTheOutputs(t *testing.T) { + asker := &labelAsker{verdict: "keep", needed: "none"} + asker.cacheRead = 19595 + e := newSweepSmall(t, "") + req := sweepReq() + body := schema.MessageText(req.Input[1]) + rep := &components.Report{} + if _, err := e.Offload(req, rep, preExpiryCtx("s", asker, store.NewMemory(store.Options{}))); err != nil { + t.Fatal(err) + } + ask := asker.ask() + if ask == "" { + t.Fatal("no ask was sent, so there is nothing to assert about it") + } + for _, want := range []string{"keep|drop", `"i":