From 0dea45a1bc256f3571260dc482d4381a47600b33 Mon Sep 17 00:00:00 2001 From: Osher-Elhadad Date: Mon, 10 Aug 2026 16:58:41 +0000 Subject: [PATCH] fix(cmdfilter): bound selector-miss ledger keys by size, and drop non-text blocks maxMissKeys bounds how MANY keys the ledger holds, not how big they are, and selectorKey runs on whatever the tool returned. Replaying multimodal traffic filled the top slots with base64 image payloads: [{"type":"image","source":{"type":"base64","data":"iVBOR... Two consequences. The ledger exists to answer 'which filter is worth writing next' (after rtk's parse_failures table), and 200 near-identical image blobs answer nothing while crowding out the actionable misses. And they sit in the aggregator under its lock and ship in every /stats scrape. A selector is a SHAPE, so a 120-byte prefix identifies it; the tail only makes near-identical misses occupy separate slots. Truncation cuts on a rune boundary so a key stays valid UTF-8 in the JSON payload. Non-text blocks are dropped rather than truncated: an image carries no output shape a filter could ever match, so recording it is noise by construction, not a key that happens to be too long. A truncated blob would still be a useless entry. The test asserts the bound, rune-safety, that non-text blocks are dropped, AND that real command banners survive intact -- the bound must not cost the signal it exists to rank. Signed-off-by: Osher-Elhadad --- components/offload/cmdfilter.go | 44 ++++++++++++++++++++++++++-- components/offload/cmdfilter_test.go | 38 ++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/components/offload/cmdfilter.go b/components/offload/cmdfilter.go index 95e88c2..3df8687 100644 --- a/components/offload/cmdfilter.go +++ b/components/offload/cmdfilter.go @@ -6,8 +6,10 @@ package offload import ( "crypto/sha256" "encoding/hex" + "regexp" "strconv" "strings" + "unicode/utf8" "github.com/maximhq/bifrost/core/schemas" "github.com/rossoctl/context-guru/components" @@ -102,7 +104,9 @@ func (f *Cmdfilter) Offload(req *schemas.BifrostChatRequest, rep *components.Rep // 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. - fs.FilterMiss(firstLine(key)) + if mk := firstLine(key); mk != "" { + fs.FilterMiss(mk) + } } continue } @@ -165,13 +169,49 @@ func (f *Cmdfilter) Offload(req *schemas.BifrostChatRequest, rep *components.Rep const selectorHeadLines = 6 // firstLine returns the leading line of a (possibly multi-line) selector key. +// maxMissKeyLen caps a ledger key. A selector is a SHAPE, so a short prefix identifies it; +// the tail only makes near-identical misses occupy separate slots. Chosen well above any real +// command banner and well below a payload. +const maxMissKeyLen = 120 + +// firstLine reduces a multi-line selector to one bounded ledger key. +// +// Bounding the LENGTH matters as much as the count. maxMissKeys caps how many keys the +// ledger holds, not how big they are, and selectorKey runs on whatever the tool returned — +// so on multimodal traffic the top slots filled with base64 image payloads +// (`[{"type":"image","source":{"type":"base64","data":"iVBOR…`). The ledger exists to answer +// "which filter is worth writing next"; 200 image blobs answer nothing, and they sit in the +// aggregator under its lock and ship in every /stats scrape. +// +// Non-text blocks are dropped entirely rather than truncated: an image has no output shape a +// filter could ever match, so recording it is noise by construction, not a key that is merely +// too long. func firstLine(key string) string { if i := strings.IndexByte(key, '\n'); i >= 0 { - return key[:i] + key = key[:i] + } + if notTextShape(key) { + return "" + } + if len(key) > maxMissKeyLen { + // Cut on a rune boundary so a truncated key stays valid UTF-8 in the JSON payload. + for len(key) > maxMissKeyLen { + key = key[:len(key)-1] + } + for len(key) > 0 && !utf8.ValidString(key) { + key = key[:len(key)-1] + } } return key } +// nonTextBlock matches the serialized head of a content block that carries no command output: +// an image or any other base64 payload. Anchored on the serialized JSON shape, because that is +// what selectorKey sees. +var nonTextBlock = regexp.MustCompile(`^\[?\{"type":\s*"(image|document|audio|video)"|"data":\s*"[A-Za-z0-9+/]{64,}`) + +func notTextShape(key string) bool { return nonTextBlock.MatchString(key) } + // selectorKey is the string a filter's match regex is tested against: the first few // non-empty, trimmed lines of the tool output, newline-joined. func selectorKey(content string) string { diff --git a/components/offload/cmdfilter_test.go b/components/offload/cmdfilter_test.go index d859b16..e4a1fe7 100644 --- a/components/offload/cmdfilter_test.go +++ b/components/offload/cmdfilter_test.go @@ -4,6 +4,7 @@ import ( "context" "strings" "testing" + "unicode/utf8" "github.com/maximhq/bifrost/core/schemas" "github.com/rossoctl/context-guru/components" @@ -290,3 +291,40 @@ type recordingSink struct{ acts, misses int } func (r *recordingSink) FilterAct(_, _, _ string, _ int) { r.acts++ } func (r *recordingSink) FilterMiss(string) { r.misses++ } + +// maxMissKeys bounds how MANY keys the selector-miss ledger holds, not how big they are, and +// selectorKey runs on whatever the tool returned. So on multimodal traffic the top slots filled +// with base64 image payloads: the ledger exists to answer "which filter is worth writing next", +// and 200 image blobs answer nothing while sitting in the aggregator under its lock and +// shipping in every /stats scrape. +func TestMissLedgerKeysAreBoundedAndTextOnly(t *testing.T) { + long := "docker build -t " + strings.Repeat("x", 400) + " ." + if got := firstLine(long); len(got) > maxMissKeyLen { + t.Errorf("key not bounded: %d bytes (cap %d)", len(got), maxMissKeyLen) + } else if !strings.HasPrefix(got, "docker build") { + t.Errorf("truncation lost the identifying prefix: %q", got) + } + + // A cut must not split a multi-byte rune, or the key ships as invalid UTF-8 in the payload. + if got := firstLine("build " + strings.Repeat("é", 200)); !utf8.ValidString(got) { + t.Errorf("truncated key is not valid UTF-8: %q", got) + } + + // Non-text blocks carry no output shape a filter could ever match, so they are dropped + // rather than truncated — a truncated blob is still a useless ledger entry. + for _, blob := range []string{ + `[{"type":"image","source":{"type":"base64","data":"iVBORw0KGgoAAAANSUhEUgAAAoAAAAKACAIAAACDr150AACQZUlEQVR4nO3dd3wUZf4H8O"}}]`, + `{"type": "image", "source": {"data": "` + strings.Repeat("A", 120) + `"}}`, + } { + if got := firstLine(blob); got != "" { + t.Errorf("non-text block recorded as a miss shape: %q", got) + } + } + + // A real command banner must still survive intact — the bound must not cost the signal. + for _, ok := range []string{"Reading package lists...", "> Task :app:compileDebugKotlin", "make[1]: Entering directory '/src'"} { + if got := firstLine(ok); got != ok { + t.Errorf("real selector altered: %q -> %q", ok, got) + } + } +}