diff --git a/components/component.go b/components/component.go index d30771ce..b6573539 100644 --- a/components/component.go +++ b/components/component.go @@ -158,6 +158,9 @@ type Ctx struct { MaxCachedIdx int // FilterStats receives cmdfilter's per-filter ledger (which command families pay // off, and which output shapes matched nothing). nil = not recording. + // + // Read it through Stats(), never directly: in observe mode nothing is forwarded, so + // recording into an enforced-namespace field would report savings that never happened. FilterStats FilterStatsSink // ExistingBreakpoints is how many prompt-cache breakpoints the RAW request already // carries, counted across `system`, `tools` and `messages` — which is what the @@ -207,6 +210,30 @@ func (c *Ctx) TailOnly(i int) bool { return i > c.MaxCachedIdx } +// Stats returns the per-filter ledger sink, or nil in observe mode. +// +// Observe computes what compaction WOULD have done and forwards the request untouched, so +// every enforced-namespace metric must stay zero: a figure that cannot be told apart from +// a real saving is worse than no figure, because it silently inflates the product's own +// headline. The savings totals are already namespaced (potential_* / projected_*), but the +// filter ledger is not — an observe-only run was reporting real-looking `cmdfilter_families` +// and `cmdfilter_filters` entries with no mode label and no hypothetical counterpart. +// +// Gating here rather than at the call site is deliberate. A component author reaching for +// c.FilterStats has no reason to think about modes, and the next sink added to Ctx would +// reproduce the bug; an accessor makes the safe path the only convenient one. +// +// Two enforced fields are deliberately NOT suppressed in observe mode, because they are real +// rather than hypothetical: cg_added_ms_avg (a true measurement of the enforced path, which +// correctly reads ~0) and context-guru's own model spend (observe measures off-path, and that +// costs real money). Those are labelled instead — see metrics.Snapshot's observe notices. +func (c *Ctx) Stats() FilterStatsSink { + if c == nil || c.Mode == ModeObserve { + return nil + } + return c.FilterStats +} + // Report is the per-component result, modelled after lean-ctx's ToolOutput // token accounting and headroom's record_pipeline_run inputs. The pipeline // fills TokensBefore/After/DurationMs; the component fills CacheKey and may set diff --git a/components/offload/cmdfilter.go b/components/offload/cmdfilter.go index c328065b..95e88c21 100644 --- a/components/offload/cmdfilter.go +++ b/components/offload/cmdfilter.go @@ -96,13 +96,13 @@ func (f *Cmdfilter) Offload(req *schemas.BifrostChatRequest, rep *components.Rep key := selectorKey(content) filt := f.reg.Match(key) if filt == nil { - if c.FilterStats != nil { + if fs := c.Stats(); fs != nil { // The miss ledger: it turns "which filter to write next" into data // instead of guesswork (after rtk's parse_failures table). Log only the // FIRST line — the selector is multi-line, and keying the bounded ledger // on whole multi-line blobs would make almost every entry unique and // exhaust the cap on noise instead of ranking real shapes. - c.FilterStats.FilterMiss(firstLine(key)) + fs.FilterMiss(firstLine(key)) } continue } @@ -141,8 +141,8 @@ func (f *Cmdfilter) Offload(req *schemas.BifrostChatRequest, rep *components.Rep rep.Irreversible = true } schema.SetMessageText(m, newText) - if c.FilterStats != nil { - c.FilterStats.FilterAct(filt.Family(), filt.Name, stashKey, before-after) + if fs := c.Stats(); fs != nil { + fs.FilterAct(filt.Family(), filt.Name, stashKey, before-after) } changed++ } diff --git a/components/offload/cmdfilter_test.go b/components/offload/cmdfilter_test.go index d32f0cc2..d859b161 100644 --- a/components/offload/cmdfilter_test.go +++ b/components/offload/cmdfilter_test.go @@ -250,3 +250,43 @@ func TestFamilyMetricsAndSelectorMisses(t *testing.T) { t.Fatalf("expected the unmatched selector to be logged, got %v", fs.misses) } } + +// The per-filter ledger is an ENFORCED-namespace field with no potential_* counterpart, so +// an observe-mode run populating it reports savings that never happened — and a consumer +// cannot tell them apart from real ones. #31 named that as its primary correctness risk: a +// mislabelled hypothetical is worse than no number, because it inflates the product's own +// headline claim. +// +// The gate lives on Ctx.Stats() rather than at this component's two call sites, so it also +// covers the next sink added to Ctx — a component author reaching for c.FilterStats has no +// reason to think about modes. Asserting that sync DOES record proves the gate rather than a +// dead sink. +func TestObserveModeDoesNotRecordFilterStats(t *testing.T) { + const aptOut = "Reading package lists...\nSetting up git (1:2.43.0-1ubuntu7.3) ...\n" + + "Processing triggers for libc-bin (2.39-0ubuntu8.6) ...\n" + + for _, tc := range []struct { + mode components.Mode + wantRecord bool + }{{components.ModeSync, true}, {components.ModeObserve, false}} { + sink := &recordingSink{} + f := newFilterComp(t, "min_size: 1\n") + req := &schemas.BifrostChatRequest{Provider: schemas.Anthropic, + Input: []schemas.ChatMessage{cmdToolMsg(aptOut)}} + c := &components.Ctx{Ctx: context.Background(), Session: "s", + Store: store.NewMemory(store.Options{}), MaxCachedIdx: -1, + Mode: tc.mode, FilterStats: sink} + if _, err := f.Offload(req, &components.Report{}, c); err != nil { + t.Fatalf("mode=%s: %v", tc.mode, err) + } + if got := sink.acts + sink.misses; (got > 0) != tc.wantRecord { + t.Errorf("mode=%s: %d ledger events (acts=%d misses=%d), wantRecord=%v", + tc.mode, got, sink.acts, sink.misses, tc.wantRecord) + } + } +} + +type recordingSink struct{ acts, misses int } + +func (r *recordingSink) FilterAct(_, _, _ string, _ int) { r.acts++ } +func (r *recordingSink) FilterMiss(string) { r.misses++ }