diff --git a/apply/apply.go b/apply/apply.go index 13e33dca..86ac6395 100644 --- a/apply/apply.go +++ b/apply/apply.go @@ -1359,7 +1359,41 @@ func rebuildCountChanged(body []byte, orig []gjson.Result, slots []slot, out []b if len(covered) != len(orig) { return body, false } + // A RETAINED TOOL MESSAGE WHOSE TEXT A COMPONENT REWROTE, which byte-matching cannot see. + // + // Anthropic has no tool role: a synthetic role=tool message is this package's internal + // representation of a tool_result content block. The loop below matches survivors by BYTES, so + // a tool message that some component rewrote (format compacting indented JSON, extract_llm + // reducing a retained output) no longer matches its pre-image, falls through to the + // "new message" branch, and is marshaled from the bifrost struct — putting `"role":"tool"` on + // the wire, which the provider rejects outright: + // + // 400 messages: Unexpected role "tool". Allowed roles are "user" or "assistant." + // + // It needs TWO components in one turn, which is why it went unseen: one to change the count so + // this rebuild runs at all, and one to rewrite a tool message the first one kept. Observed as a + // real 400 on a live session. + // + // Fixed the same way the equal-count path already handles tool text: write the new text into + // the body's tool_result block, so the rebuild only ever decides WHICH messages to keep and + // never how to serialize one. Then match those messages by tool_call_id rather than by bytes, + // since their text may now legitimately differ from the pre-image. Doing only the second half + // would emit the original bytes and silently discard the compaction. + if nb, no, ok := writeBackToolText(body, orig, slots, out); ok { + body, orig = nb, no + } used := make([]bool, len(slots)) + // toolSlotByID indexes the tool-text slots so a rewritten tool message can still be + // recognised as a survivor. Built once; the matching loop is already O(out × slots). + toolSlotByID := map[string]int{} + for k := range slots { + if slots[k].kind != anthropicToolText { + continue + } + if id := toolCallIDAt(orig, slots[k]); id != "" { + toolSlotByID[id] = k + } + } var parts [][]byte // emitted guards against emitting one body message TWICE: several normalized // messages can share a body index (an Anthropic user message with several @@ -1372,10 +1406,21 @@ func rebuildCountChanged(body []byte, orig []gjson.Result, slots []slot, out []b return body, false } matched := -1 - for k := range slots { - if !used[k] && bytes.Equal(mb, slots[k].pre) { + // A synthetic tool message is identified by its tool_call_id, not its bytes: the text + // may have been rewritten (and written back into the body just above), and the id is + // what pairing actually depends on. + if out[i].Role == bschemas.ChatMessageRoleTool && out[i].ChatToolMessage != nil && + out[i].ChatToolMessage.ToolCallID != nil { + if k, ok := toolSlotByID[*out[i].ChatToolMessage.ToolCallID]; ok && !used[k] { matched = k - break + } + } + if matched < 0 { + for k := range slots { + if !used[k] && bytes.Equal(mb, slots[k].pre) { + matched = k + break + } } } if matched < 0 { @@ -1409,6 +1454,67 @@ func rebuildCountChanged(body []byte, orig []gjson.Result, slots []slot, out []b return res, true } +// toolCallIDAt reads the tool_use_id of the tool_result block a tool-text slot points at. +// The slot path is "messages..content..content", so the block is its parent. +func toolCallIDAt(orig []gjson.Result, s slot) string { + bi, rel, ok := splitSlotPath(s.path) + if !ok || bi < 0 || bi >= len(orig) { + return "" + } + blk := strings.TrimSuffix(rel, ".content") + if blk == rel { // not a tool-text path + return "" + } + return orig[bi].Get(blk + ".tool_use_id").String() +} + +// writeBackToolText splices rewritten tool-output text into the body's tool_result blocks before +// the count-change rebuild reads them, so a retained-but-rewritten tool message can be emitted +// from body bytes (role intact) instead of marshaled from the bifrost struct (role leaked). +// +// This is deliberately the SAME shape of edit the equal-count path makes — only the block's +// `content` string changes, so the rest of the message stays byte-identical — and it is why the +// rebuild can keep the rule "decide which messages to keep, never how to serialize one". +// +// Returns ok=false when nothing needed writing, so the caller keeps its original slices and no +// body copy is made on the common path. +func writeBackToolText(body []byte, orig []gjson.Result, slots []slot, + out []bschemas.ChatMessage) ([]byte, []gjson.Result, bool) { + byID := map[string]int{} + for k := range slots { + if slots[k].kind != anthropicToolText { + continue + } + if id := toolCallIDAt(orig, slots[k]); id != "" { + byID[id] = k + } + } + var wrote bool + for i := range out { + if out[i].Role != bschemas.ChatMessageRoleTool || out[i].ChatToolMessage == nil || + out[i].ChatToolMessage.ToolCallID == nil { + continue + } + k, ok := byID[*out[i].ChatToolMessage.ToolCallID] + if !ok { + continue + } + txt := schema.MessageText(out[i]) + if txt == slots[k].preText { + continue // unchanged; the original bytes already carry it + } + nb, err := sjson.SetBytes(body, slots[k].path, txt) + if err != nil { + return nil, nil, false // fail open: leave the body alone + } + body, wrote = nb, true + } + if !wrote { + return nil, nil, false + } + return body, gjson.GetBytes(body, "messages").Array(), true +} + // splitSlotPath splits a slot path into the body message index and the remainder of // the path RELATIVE to that message: "messages.3.content.2.content" -> 3, // "content.2.content". A whole-message slot has an empty remainder. diff --git a/apply/parallel_wire_test.go b/apply/parallel_wire_test.go new file mode 100644 index 00000000..cd7dfbd3 --- /dev/null +++ b/apply/parallel_wire_test.go @@ -0,0 +1,139 @@ +package apply_test + +import ( + "context" + "encoding/json" + "strings" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + "github.com/tidwall/gjson" + + "github.com/rossoctl/context-guru/apply" + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/store" +) + +// THE SHAPE LIVE TRAFFIC CARRIES. In Anthropic's wire format a PARALLEL tool call is ONE assistant +// message carrying several tool_use blocks, answered by ONE user message carrying several +// tool_result blocks. apply normalizes that user message into several synthetic role=tool messages, +// so N normalized messages share ONE body index. +// +// This asserts tool pairing in BOTH directions on the emitted wire, because the two fail +// independently and each hid the other: +// +// - FORWARD (every tool_use is answered). This was PR #80's iter011 defect, where the body +// message holding both tool_results was dropped and the parallel call went unanswered — +// 28 of 75 live runs rejected. Fixed upstream; asserted here as a regression guard. +// - BACKWARD (every tool_result answers a call that PRECEDES it). keep_last counts messages and +// a tool result is a message, so the tail boundary could begin mid-exchange: the assistant's +// calls were summarized away while their results survived. At keep_last 2 and 3 this emitted +// [user, summary, user(tool_result pa_h, tool_result pb_h), user] — two results, no call. +// +// A forward-only check passes on that wire, which is why the direction matters. Both are provider +// rejections of the entire request, not degraded output. +func TestSummarizeNeverSplitsAToolExchange(t *testing.T) { + big := strings.Repeat("verbose parallel tool output\n", 60) + msgs := []map[string]any{ + {"role": "user", "content": "start the task"}, + } + for i := 0; i < 8; i++ { + a, b := "pa_"+string(rune('a'+i)), "pb_"+string(rune('a'+i)) + msgs = append(msgs, + map[string]any{"role": "assistant", "content": []map[string]any{ + {"type": "text", "text": "calling two"}, + {"type": "tool_use", "id": a, "name": "Read", "input": map[string]any{}}, + {"type": "tool_use", "id": b, "name": "Read", "input": map[string]any{}}, + }}, + // BOTH results in ONE user message -- Anthropic's requirement for a parallel call. + map[string]any{"role": "user", "content": []map[string]any{ + {"type": "tool_result", "tool_use_id": a, "content": big}, + {"type": "tool_result", "tool_use_id": b, "content": big}, + }}, + ) + } + msgs = append(msgs, map[string]any{"role": "user", "content": "final question"}) + body, _ := json.Marshal(map[string]any{"model": "claude-x", "messages": msgs}) + + // Preconditions, so a wire that carries no tool content at all cannot pass silently. + var sawResult, sawParallelCall, acted bool + + for _, keep := range []int{1, 2, 3, 4, 5} { + cfg := pipe(t, "pipeline: [summarize]\ncomponents:\n summarize: {keep_last: "+ + string(rune('0'+keep))+", start_from_message: 0, min_tokens: 1}\n") + p, _ := cfg.Build(nil) + out, changed := apply.BodyWithModel(context.Background(), p, + store.NewMemory(store.Options{}), bschemas.Anthropic, body, "", false, + components.ModelSpec{Incoming: stubModel{resp: "essential facts"}}) + if !changed { + continue + } + acted = true + arr := gjson.GetBytes(out, "messages").Array() + + // Ids are collected AS THE TRANSCRIPT IS WALKED, so a result can only pair with a call + // that precedes it -- the same rule schema.ToolCalls documents and the provider enforces. + declared := map[string]bool{} + for i, m := range arr { + var uses []string + m.Get("content").ForEach(func(_, blk gjson.Result) bool { + switch blk.Get("type").String() { + case "tool_use": + id := blk.Get("id").String() + uses = append(uses, id) + declared[id] = true + case "tool_result": + sawResult = true + if id := blk.Get("tool_use_id").String(); !declared[id] { + t.Errorf("keep_last=%d: wire message %d carries tool_result %q with no "+ + "preceding tool_use -- the provider rejects this", keep, i, id) + dumpWire(t, arr) + } + } + return true + }) + if len(uses) >= 2 { + sawParallelCall = true + } + if len(uses) == 0 { + continue + } + // FORWARD: the results must be in the message immediately after the call. + answered := map[string]bool{} + if i+1 < len(arr) { + arr[i+1].Get("content").ForEach(func(_, blk gjson.Result) bool { + if blk.Get("type").String() == "tool_result" { + answered[blk.Get("tool_use_id").String()] = true + } + return true + }) + } + for _, u := range uses { + if !answered[u] { + t.Errorf("keep_last=%d: wire message %d declares tool_use %q with no "+ + "tool_result immediately after -- the provider rejects this", keep, i, u) + dumpWire(t, arr) + } + } + } + } + + if !acted { + t.Fatal("summarize never acted, so no wire was checked -- the assertions are vacuous") + } + if !sawResult { + t.Fatal("no tool_result ever reached the wire, so the BACKWARD assertion never ran") + } + if !sawParallelCall { + t.Fatal("no parallel tool_use pair ever reached the wire, so the FORWARD assertion " + + "never exercised the shape this test exists for") + } +} + +func dumpWire(t *testing.T, arr []gjson.Result) { + t.Helper() + for k, mm := range arr { + t.Errorf(" [%d] role=%s content_head=%.90s", k, + mm.Get("role").String(), mm.Get("content").Raw) + } +} diff --git a/apply/toolrole_wire_test.go b/apply/toolrole_wire_test.go new file mode 100644 index 00000000..92d48746 --- /dev/null +++ b/apply/toolrole_wire_test.go @@ -0,0 +1,140 @@ +package apply_test + +import ( + "context" + "encoding/json" + "strings" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + "github.com/tidwall/gjson" + + "github.com/rossoctl/context-guru/apply" + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/store" +) + +// Anthropic has NO tool role. A synthetic role=tool message is this package's internal +// representation of a tool_result content block, and serializing one onto the wire is a hard +// provider rejection: +// +// 400 messages: Unexpected role "tool". Allowed roles are "user" or "assistant." +// +// rebuildCountChanged emits a message from its ORIGINAL body bytes only when it byte-matches its +// pre-pipeline form, and marshals it fresh otherwise -- a branch its own comment describes as being +// for "a new message (e.g. the summary)". A RETAINED tool-result message that a LATER component +// also rewrites lands in that same branch, and gets marshaled with its internal role intact. +// +// Two components in one turn are required, which is why this was never seen before: summarize +// changes the message count (so the rebuild runs at all), and a second component rewrites the text +// of a tool message that summarize kept. Observed as a real 400 on a live session. +func TestNoToolRoleOnAnthropicWireAfterCountChange(t *testing.T) { + // Indented JSON, so a reducing component actually rewrites it. Compact or prose content is + // left alone, and the test would pass while proving nothing -- the exact trap that made an + // earlier version of this test vacuous twice. + toolBody := func(n int) string { + recs := make([]map[string]any, 0, n) + for i := 0; i < n; i++ { + recs = append(recs, map[string]any{ + "ts": "2024-01-01T00:00:00Z", "path": "src/api/users.py", + "level": "INFO", "msg": "request served", "seq": i, + "detail": strings.Repeat("verbose detail text ", 6), + }) + } + b, _ := json.MarshalIndent(recs, "", " ") + return string(b) + } + + msgs := []map[string]any{{"role": "user", "content": "audit the request log"}} + for i := 0; i < 6; i++ { + id := "call_" + string(rune('a'+i)) + msgs = append(msgs, + map[string]any{"role": "assistant", "content": []map[string]any{ + {"type": "text", "text": "reading the log"}, + {"type": "tool_use", "id": id, "name": "Read", "input": map[string]any{}}, + }}, + map[string]any{"role": "user", "content": []map[string]any{ + {"type": "tool_result", "tool_use_id": id, "content": toolBody(40)}, + }}, + ) + } + msgs = append(msgs, map[string]any{"role": "user", "content": "what failed?"}) + body, _ := json.Marshal(map[string]any{"model": "claude-x", "messages": msgs}) + + // summarize changes the count; extract_llm rewrites a RETAINED tool output in the same turn. + // strategy: deterministic keeps this hermetic -- the reduction is a real rewrite of the kept + // message's bytes with no model reply to stub. + const comps = "components:\n" + + " summarize: {keep_last: 3, start_from_message: 0, min_tokens: 1}\n" + + " extract_llm: {strategy: deterministic, min_tokens: 1, economic_gate: false, " + + "allow_on_caching_backend: true}\n" + + // Both the reduced pipeline and the one this was reported against. cachesplit is not a + // spectator: it is a no-op in Reformat and the split it names happens inside THIS package, + // which rewrites the envelope before the rebuild runs and changes the rebuild's control flow + // (a declined rebuild is still forwarded when systemSplit is set). So a fix verified only + // without it is not verified for the configuration that produced the live 400. + for _, tc := range []struct{ name, pipeline string }{ + {"summarize+extract_llm", "pipeline: [summarize, extract_llm]\n"}, + {"reported: summarize+extract_llm+cachesplit", "pipeline: [summarize, extract_llm, cachesplit]\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + checkNoToolRole(t, tc.pipeline+comps, body, len(msgs)) + }) + } +} + +func checkNoToolRole(t *testing.T, yaml string, body []byte, inCount int) { + t.Helper() + cfg := pipe(t, yaml) + p, _ := cfg.Build(nil) + out, changed := apply.BodyWithModel(context.Background(), p, + store.NewMemory(store.Options{}), bschemas.Anthropic, body, "", false, + components.ModelSpec{Incoming: stubModel{resp: "essential facts"}}) + if !changed { + t.Fatal("no component acted, so the rebuild never ran -- assertion is vacuous") + } + arr := gjson.GetBytes(out, "messages").Array() + + // PRECONDITION 1: the count must actually have changed, or rebuildCountChanged never ran. + if len(arr) == inCount { + t.Fatalf("message count unchanged (%d), so the count-change rebuild never ran", len(arr)) + } + // Count both shapes in ONE pass, because they are alternatives rather than independent + // facts: a tool exchange that survived is either a well-formed tool_result block (correct) + // or a leaked role=tool message (the defect). Counting only tool_result blocks would make + // the precondition fire on the very output the assertion exists to catch -- the leaked + // message has no tool_result block to find, so "no tool content survived" and "the bug + // happened" would be indistinguishable, and the test would abort as vacuous instead of + // failing. + var toolRoleMsgs, toolResultBlocks int + for _, m := range arr { + if m.Get("role").String() == "tool" { + toolRoleMsgs++ + } + m.Get("content").ForEach(func(_, blk gjson.Result) bool { + if blk.Get("type").String() == "tool_result" { + toolResultBlocks++ + } + return true + }) + } + + // PRECONDITION 2: some tool exchange must have survived into the output, in either shape. + // If summarize swallowed them all there is no retained tool message to mis-serialize. + if toolRoleMsgs+toolResultBlocks == 0 { + t.Fatal("no tool exchange survived into the output, so no retained tool message could " + + "reach the rebuild -- assertion is vacuous") + } + + // THE ASSERTION: no message may carry the internal tool role. + if toolRoleMsgs > 0 { + t.Errorf(`%d wire message(s) have role="tool" -- Anthropic rejects the request with `+ + `"Unexpected role \"tool\". Allowed roles are \"user\" or \"assistant\{-}."`, + toolRoleMsgs) + for k, mm := range arr { + t.Errorf(" [%d] role=%s content_head=%.80s", k, + mm.Get("role").String(), mm.Get("content").Raw) + } + } +} diff --git a/components/offload/extract_llm.go b/components/offload/extract_llm.go index f8086d4b..05464a56 100644 --- a/components/offload/extract_llm.go +++ b/components/offload/extract_llm.go @@ -232,7 +232,27 @@ var staticWindows = modelinfo.DefaultStatic() // inputLimit resolves the extraction model's input-token budget. Config pin first, then the // model's own window as DATA (modelinfo's table), then a conservative default. -func (e *ExtractLLM) inputLimit(c *components.Ctx) int { +// +// effSource is the source the extraction model was ACTUALLY resolved from, which is not always +// the configured one: ModelSpec falls back from `incoming` to the static client whenever no +// incoming client could be built (the proxy returns nil when no usable credential is on the +// request). Sizing the prompt by e.modelSource instead would then hand the REQUEST model's window +// to a call that is really going to the small static model — over-estimating, on a coding agent by +// as much as 1M against 200k, which is the direction fitsModelContext calls the costly one: the +// request goes out, the upstream rejects it, and the round-trip buys nothing. Pass "" when the +// effective source is not known and the configured one is used as before. +// +// A CONFIG-PINNED client needs no correction and never reaches the effSource branch, which is worth +// stating because it looks like a second path that could resurrect the mismatch: Offload threads +// effSource only through the `model == nil` branch, so a client resolved from e.modelClient keeps +// the configured source. It is safe because modelConfig.Client() requires model.model to be +// non-empty, so e.modelName is always set whenever e.modelClient is, and the e.modelName != "" +// branch below (the static-table lookup) short-circuits before effSource or CtxWindow is consulted. +// Raised in review on #110; recorded here so the next reader need not re-derive it. +func (e *ExtractLLM) inputLimit(c *components.Ctx, effSource string) int { + if effSource == "" { + effSource = e.modelSource + } if e.modelMaxInput > 0 { return e.modelMaxInput } @@ -245,7 +265,7 @@ func (e *ExtractLLM) inputLimit(c *components.Ctx) int { // No pinned model. `source: config` means the host's separate cheap client, whose id we // never see — stay conservative. Otherwise the extraction model IS the proxied model, // and the host already resolved its window onto the Ctx. - if e.modelSource != "config" && c.CtxWindow > 0 { + if effSource != "config" && c.CtxWindow > 0 { return c.CtxWindow } return unknownModelInputLimit @@ -701,6 +721,9 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R return nil, nil } model := e.modelClient + // The source the model is ACTUALLY resolved from, which the prompt budget below must be + // sized against rather than the configured one — see inputLimit. + effSource := e.modelSource if model == nil { // ForModel, not For: `model.model` names the model to COMPACT with even when the // source is the incoming request. Without that, compaction on a coding agent runs on @@ -709,6 +732,9 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R // it. Same endpoint, same credential, cheap model. var usedSource string model, usedSource = c.Model.ForModelSource(e.modelSource, e.modelName) + if usedSource != "" { + effSource = usedSource + } // The fallback from `incoming` to the static model is a DIFFERENT credential on a // DIFFERENT endpoint, so it cannot be silent: an operator whose config says // `source: incoming` would otherwise have no way to learn that none of their calls @@ -817,7 +843,7 @@ func (e *ExtractLLM) Offload(req *bschemas.BifrostChatRequest, rep *components.R keepIDs := extract.HarvestIdentifiers(conversationContext(req, ctxRecent, e.ctxMessages), 40) // Per-call context budget (constant across this request's candidates): the extraction // model's input limit, and the prompt's fixed cost around the tool output itself. - inputLimit := e.inputLimit(c) + inputLimit := e.inputLimit(c, effSource) promptOverhead := extractPromptOverheadTokens + schema.TextTokens(goal) // 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. @@ -1046,7 +1072,29 @@ 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) - if huge := e.trigger.IsHuge(sz, c.CtxWindow); !c.CacheAware && !fires && !huge { + // 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. + // + // `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. + // + // 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 { rep.Gate("request_trigger_not_fired") continue } diff --git a/components/offload/extract_llm_ctxguard_test.go b/components/offload/extract_llm_ctxguard_test.go index 3baabe18..848f8c8d 100644 --- a/components/offload/extract_llm_ctxguard_test.go +++ b/components/offload/extract_llm_ctxguard_test.go @@ -117,20 +117,29 @@ func TestExtractLLMInputLimit(t *testing.T) { name string yaml string ctxWindow int + effSource string want int }{ - {"config pin wins", "model_max_input_tokens: 4096\nmodel:\n model: claude-haiku-4-5\n", 200_000, 4096}, - {"pinned model resolved from the table", "model:\n model: claude-haiku-4-5\n", 0, 200_000}, - {"unnameable pinned model falls back", "model:\n model: qwen3-coder-30b-local\n", 999_999, unknownModelInputLimit}, - {"incoming model uses the host-resolved window", "", 128_000, 128_000}, - {"incoming model, window unknown", "", 0, unknownModelInputLimit}, - {"source config hides the model id", "model:\n source: config\n", 128_000, unknownModelInputLimit}, + {"config pin wins", "model_max_input_tokens: 4096\nmodel:\n model: claude-haiku-4-5\n", 200_000, "", 4096}, + {"pinned model resolved from the table", "model:\n model: claude-haiku-4-5\n", 0, "", 200_000}, + {"unnameable pinned model falls back", "model:\n model: qwen3-coder-30b-local\n", 999_999, "", unknownModelInputLimit}, + {"incoming model uses the host-resolved window", "", 128_000, "", 128_000}, + {"incoming model, window unknown", "", 0, "", unknownModelInputLimit}, + {"source config hides the model id", "model:\n source: config\n", 128_000, "", unknownModelInputLimit}, + // `source: incoming` that FELL BACK to the static client. The call is going to the + // small config model, so the proxied model's window is the wrong budget — sizing the + // prompt by it over-estimates and the upstream rejects the request. + {"incoming fell back to config: window is not the request model's", + "model:\n source: incoming\n", 1_000_000, "config", unknownModelInputLimit}, + // The same config that did NOT fall back keeps using the host-resolved window. + {"incoming that stayed incoming keeps the window", + "model:\n source: incoming\n", 1_000_000, "incoming", 1_000_000}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { e := newCtxGuardComponent(t, &silentModel{}, tc.yaml) c := &components.Ctx{Ctx: context.Background(), CtxWindow: tc.ctxWindow} - if got := e.inputLimit(c); got != tc.want { + if got := e.inputLimit(c, tc.effSource); got != tc.want { t.Fatalf("inputLimit = %d, want %d", got, tc.want) } }) diff --git a/components/offload/extract_trigger_cacheaware_test.go b/components/offload/extract_trigger_cacheaware_test.go new file mode 100644 index 00000000..aec5fb1d --- /dev/null +++ b/components/offload/extract_trigger_cacheaware_test.go @@ -0,0 +1,86 @@ +package offload + +import ( + "context" + "strings" + "sync/atomic" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/store" +) + +// An operator's REQUEST-level trigger must be honored on a prompt-caching backend. +// +// The guard on this check used to read `!c.CacheAware && !fires && !huge`, so with CACHE_MODE=on +// (or any caching provider) a configured trigger.min_request_tokens had no effect at all. That is +// not a cache-awareness carve-out protecting a heuristic: Trigger's zero value fires always (see +// components/trigger.go), so `!fires` is reachable ONLY when the operator configured a request +// threshold that was not met. The condition could therefore only ever void explicit +// configuration, silently. +// +// Both arms run the SAME config against the SAME request and differ ONLY in CacheAware, which is +// what makes this an assertion about the carve-out rather than about triggers in general. +// +// The COLD SWEEP is the part of the old carve-out that carried real weight and is deliberately +// still exempt: on a cold turn the whole transcript re-bills whatever the request's size, so a +// request-size threshold answers the wrong question there and the sweep brings its own floor. +// Both arms here run warm (ColdCache unset), and the exemption is pinned separately by +// TestHousellmColdSweepActuallyFires in components/all — which is what caught the first version +// of this fix removing it. +// +// economic_gate: false is required setup, not a convenience: it also sets +// allow_on_caching_backend, without which the component disables itself entirely on the +// CacheAware arm and the arm would pass for a completely unrelated reason. +func TestExplicitRequestTriggerIsHonoredOnCachingBackends(t *testing.T) { + // One output far above any floor, in a request far below the configured request threshold. + big := strings.Repeat("2024-01-01 GET /users/42 200 12ms src/api/users.py\n", 400) + newReq := func() *bschemas.BifrostChatRequest { + return &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{ + userMsg("Find the auth timeout in src/api/users.py and fix it."), + assistantMsg("Reading the log."), + toolResultMsg(big), + userMsg("keep going"), + }} + } + + for _, cacheAware := range []bool{false, true} { + name := "cache_aware=false" + if cacheAware { + name = "cache_aware=true" + } + t.Run(name, func(t *testing.T) { + // A threshold no request in this test can meet, so `fires` is false in BOTH arms. + comp, err := newExtractLLM([]byte( + "economic_gate: false\ntrigger:\n min_request_tokens: 10000000\n")) + if err != nil { + t.Fatalf("config: %v", err) + } + e := comp.(*ExtractLLM) + model := &silentModel{} + req, rep := newReq(), components.Report{} + ctx := &components.Ctx{ + Session: "s", Ctx: context.Background(), + Store: store.NewMemory(store.Options{}), CtxWindow: 1_000_000, + CacheAware: cacheAware, MaxCachedIdx: -1, + Model: components.ModelSpec{Static: model, Incoming: model}, + } + if _, err := e.Offload(req, &rep, ctx); err != nil { + t.Fatalf("offload: %v", err) + } + // Precondition: the candidate must have REACHED this gate. Filtered earlier by the + // floor or the cached-prefix tail gate, the assertion below would be vacuous. + if rep.Gates["below_output_floor"] > 0 || rep.Gates["cached_prefix"] > 0 { + t.Fatalf("candidate never reached the request trigger; gates=%v", rep.Gates) + } + if rep.Gates["request_trigger_not_fired"] == 0 { + t.Errorf("configured min_request_tokens was ignored: expected the "+ + "request_trigger_not_fired gate, got gates=%v", rep.Gates) + } + if n := atomic.LoadInt64(&model.calls); n != 0 { + t.Errorf("model called %d time(s) despite an unmet configured request trigger", n) + } + }) + } +} diff --git a/components/offload/summarize.go b/components/offload/summarize.go index c9c8f734..6152b1b7 100644 --- a/components/offload/summarize.go +++ b/components/offload/summarize.go @@ -148,8 +148,9 @@ func (*Summarize) NeedsModel() bool { return true } func (s *Summarize) Offload(req *bschemas.BifrostChatRequest, rep *components.Report, c *components.Ctx) ([]string, error) { msgs := req.Input - // Keep msg0 (system/first) + the last keepLast; summarize the span between. - start, end := 1, len(msgs)-s.keepLast + // Keep msg0 (system/first) + the last keepLast; summarize the span between — with both + // boundaries aligned so neither cuts inside a tool exchange. See summarizeSpan. + headCount, start, end := summarizeSpan(msgs, s.keepLast) // Request-level trigger: don't summarize (an LLM call) until the transcript // is genuinely large / deep. Zero thresholds fire always (back-compat). if !s.trigger.Fires(req, c.CtxWindow) || end <= start { @@ -169,7 +170,7 @@ func (s *Summarize) Offload(req *bschemas.BifrostChatRequest, rep *components.Re // that checkpoint is still small — no LLM call, and the summary message stays // byte-identical (KV-cache stable). Roll the checkpoint forward only once the // tail grows past resummarize_tokens. - if out, keys, ok := s.tryReuse(c, msgs, start, end); ok { + if out, keys, ok := s.tryReuse(c, msgs, headCount, start, end); ok { if len(keys) == 0 { rep.Irreversible = true // reused a non-full checkpoint (nothing stashed) } @@ -221,7 +222,24 @@ func (s *Summarize) Offload(req *bschemas.BifrostChatRequest, rep *components.Re } summaryText := summaryWrapper(summary, key, mode) - summaryMsg := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleSystem} + // USER, not system. The summary is injected context, and Anthropic will not accept a + // system-role message in the middle of `messages`: system content belongs in the + // top-level `system` field, and a system role inside the array must precede an + // assistant message or end it. This component emits [msgs[0], summary, tail...], so + // when msgs[0] is itself the system prompt — the normal case — a system-role summary + // lands at index 1 and the provider rejects the whole request: + // + // 400 messages.1: role 'system' must precede an 'assistant' message or end the array + // + // Measured on live LOCA-bench traffic: every task that triggered a summarization failed + // this way, including in an arm with NO other component enabled, so it is this + // component's own output and not a pipeline interaction. It went unnoticed because + // every prior measurement replayed through /compact, which never forwards upstream and + // therefore never has the body validated by a provider. + // + // A user-role message carrying the summary is both valid and conventional — it is what + // Claude Code's own compaction does. + summaryMsg := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleUser} schema.SetMessageText(&summaryMsg, summaryText) // Checkpoint: this summary subsumes the leading span (len(span) messages from @@ -233,8 +251,14 @@ func (s *Summarize) Offload(req *bschemas.BifrostChatRequest, rep *components.Re // [msg0, summary, last-K] — reassign; apply.Body rebuilds losslessly. out := make([]bschemas.ChatMessage, 0, 2+s.keepLast) - out = append(out, msgs[0], summaryMsg) + out = append(out, msgs[:headCount]...) + out = append(out, summaryMsg) out = append(out, msgs[end:]...) + // Removing a span can orphan the tail's leading tool_result blocks; a provider rejects + // the whole request if it does. See dropOrphanedToolResults. + if repaired, n := dropOrphanedToolResults(out); n > 0 { + out = repaired + } req.Input = out if key != "" { return []string{key}, nil @@ -247,7 +271,7 @@ func (s *Summarize) Offload(req *bschemas.BifrostChatRequest, rep *components.Re // since that boundary is below resummarize_tokens. It returns the rebuilt // [msg0, priorSummary, msgs[boundary:]] and the (refreshed) stash key. No LLM // call. ok=false means "re-summarize fresh". -func (s *Summarize) tryReuse(c *components.Ctx, msgs []bschemas.ChatMessage, start, end int) ([]bschemas.ChatMessage, []string, bool) { +func (s *Summarize) tryReuse(c *components.Ctx, msgs []bschemas.ChatMessage, headCount, start, end int) ([]bschemas.ChatMessage, []string, bool) { if s.resummarizeTokens <= 0 { return nil, nil, false } @@ -274,11 +298,25 @@ func (s *Summarize) tryReuse(c *components.Ctx, msgs []bschemas.ChatMessage, sta c.Store.Put(cp.Key, b) } } - summaryMsg := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleSystem} + // USER for the same reason as the fresh-summary path above: a system role at index 1 + // is rejected by the provider. The replayed checkpoint must match that shape exactly, + // or a replayed turn would emit different bytes from the turn that created it. + summaryMsg := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleUser} schema.SetMessageText(&summaryMsg, cp.SummaryMsg) + // The replayed boundary must respect exchange atomicity exactly as the fresh path does, + // or a replayed turn emits different bytes from the turn that created it. + for boundary < len(msgs) && msgs[boundary].Role == bschemas.ChatMessageRoleTool { + boundary++ + } out := make([]bschemas.ChatMessage, 0, 2+(len(msgs)-boundary)) - out = append(out, msgs[0], summaryMsg) + out = append(out, msgs[:headCount]...) + out = append(out, summaryMsg) out = append(out, msgs[boundary:]...) + // Removing a span can orphan the tail's leading tool_result blocks; a provider rejects + // the whole request if it does. See dropOrphanedToolResults. + if repaired, n := dropOrphanedToolResults(out); n > 0 { + out = repaired + } if cp.Key != "" { return out, []string{cp.Key}, true } diff --git a/components/offload/summarize_pairing.go b/components/offload/summarize_pairing.go new file mode 100644 index 00000000..ac46a582 --- /dev/null +++ b/components/offload/summarize_pairing.go @@ -0,0 +1,117 @@ +package offload + +import ( + bschemas "github.com/maximhq/bifrost/core/schemas" +) + +// Tool-pairing repair for a component that REMOVES messages. +// +// Anthropic (and every provider with the same shape) requires each `tool_result` to answer a +// `tool_use` that appeared earlier. summarize replaces a span of the transcript with one +// summary message — [msgs[0], summary, msgs[end:]...] — and the kept tail can begin part-way +// through a tool exchange, so its leading `tool_result` blocks refer to `tool_use` blocks that +// were just deleted. The provider then rejects the entire request: +// +// 400 messages.0.content.2: unexpected `tool_use_id` found in `tool_result` blocks +// +// Measured on live LOCA-bench traffic: 5 of 12 tasks failed this way once the earlier +// system-role defect was fixed and summarize could finally act. +// +// This is an invariant for any component that deletes messages, and the reason coref does not +// need it: coref rewrites a tool message's text IN PLACE and never removes a message, so +// pairing is preserved by construction. summarize removes, so summarize must repair. +// +// The repair is deliberately one-directional — DROP orphaned results, never synthesise +// placeholder ones. A synthetic "[tool result unavailable]" would be a second lie on top of +// the summary: the summary already claims to carry that content forward, so re-asserting a +// missing result invites the model to reason about an absence that the summary is supposed to +// have described. The rig-side shim used for LOCA's own trimmer synthesises because it must +// preserve a foreign agent's history; a component summarising its own span does not. +func dropOrphanedToolResults(msgs []bschemas.ChatMessage) ([]bschemas.ChatMessage, int) { + // Every tool_use id available to answer, accumulated as we walk forward. A result may + // answer any earlier call, not only the immediately preceding message, because a summary + // may sit between the call and its result. + seen := map[string]struct{}{} + out := make([]bschemas.ChatMessage, 0, len(msgs)) + dropped := 0 + for _, m := range msgs { + if m.ChatAssistantMessage != nil { + for _, tc := range m.ChatAssistantMessage.ToolCalls { + if tc.ID != nil { + seen[*tc.ID] = struct{}{} + } + } + } + if m.Role == bschemas.ChatMessageRoleTool && m.ChatToolMessage != nil && + m.ChatToolMessage.ToolCallID != nil { + if _, ok := seen[*m.ChatToolMessage.ToolCallID]; !ok { + dropped++ + continue // orphaned: its call is gone + } + } + out = append(out, m) + } + return out, dropped +} + +// summarizeSpan picks the span to summarize so that it NEVER cuts inside a tool exchange, +// and reports how many head messages to preserve. +// +// The naive boundaries — preserve msgs[0], summarize msgs[1 : len-keepLast] — are pure +// arithmetic and know nothing about tool pairing, which produced two separate provider +// rejections on live traffic: +// +// 400 messages.N.content.M: unexpected `tool_use_id` found in `tool_result` blocks +// — the kept tail began with a tool_result whose tool_use was inside the span +// 400 messages.N: `tool_use` ids were found without `tool_result` blocks immediately after +// — msgs[0] was an assistant tool_use whose result was inside the span +// +// Both are the same mistake seen from either side, so both are fixed by one rule: **a tool +// exchange is atomic**. Review put it best — an unanswered call means the agent is still +// waiting on that tool, so summarize after it completes, not through it. +// +// Two adjustments implement that: +// +// - END is advanced forward past any tool messages the kept tail would begin with, so the +// exchange is summarized WHOLE rather than split. Advancing (rather than retreating) +// keeps the call and its result on the same side of the boundary without ever keeping +// LESS context than asked for. +// - The HEAD is dropped when msgs[0] is an assistant message carrying tool calls, because +// its results necessarily lie inside the span. msgs[0] is preserved to retain the +// conversation's identity — its system prompt or opening user turn — and an assistant +// tool-call message is neither, so nothing is lost by folding it into the summary. +// +// Returns headCount (0 or 1), start, end. A caller that gets end <= start should skip. +func summarizeSpan(msgs []bschemas.ChatMessage, keepLast int) (headCount, start, end int) { + headCount = 1 + if len(msgs) > 0 && msgs[0].Role == bschemas.ChatMessageRoleAssistant && + msgs[0].ChatAssistantMessage != nil && len(msgs[0].ChatAssistantMessage.ToolCalls) > 0 { + headCount = 0 // preserving it would leave its calls unanswered + } + start = headCount + end = len(msgs) - keepLast + if end > len(msgs) { + end = len(msgs) + } + // CLAMP BEFORE INDEXING. A transcript shorter than keep_last makes `end` NEGATIVE, and the + // loop below indexes msgs[end] — `end < len(msgs)` is trivially true for a negative index, so + // it reads msgs[-1] and panics. With the default keep_last: 3 that is any request with fewer + // than three messages, i.e. the first turn or two of EVERY session, not an edge case. + // + // The old boundary was pure arithmetic and never indexed anything, so Offload's `end <= start` + // check caught this case cleanly; adding the tool-boundary walk moved an index read in front of + // that guard. Clamping to `start` restores the short-circuit: Offload sees end <= start and + // declines, exactly as before. + // + // It was survivable rather than visible because pipeline.runOne recovers per component, so the + // panic surfaced only as verdict=reverted in the logs while summarize silently did nothing on + // short turns. Found in review, on live sessions. + if end < start { + end = start + } + // Advance past a tail that would begin mid-exchange. + for end < len(msgs) && msgs[end].Role == bschemas.ChatMessageRoleTool { + end++ + } + return headCount, start, end +} diff --git a/components/offload/summarize_role_test.go b/components/offload/summarize_role_test.go new file mode 100644 index 00000000..31081ae4 --- /dev/null +++ b/components/offload/summarize_role_test.go @@ -0,0 +1,200 @@ +package offload + +import ( + "context" + "strings" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/schema" + "github.com/rossoctl/context-guru/store" +) + +// fixedModel returns one canned summary, so the test asserts on the SHAPE of what +// summarize emits rather than on a model's wording. +type fixedModel struct{ out string } + +func (m *fixedModel) Complete(context.Context, string) (string, error) { return m.out, nil } + +// THE REGRESSION THIS GUARDS AGAINST +// +// summarize emitted its summary as a SYSTEM-role message and spliced it in as +// [msgs[0], summary, tail...]. When msgs[0] is itself the system prompt — the normal case — +// that puts a system role at index 1, and Anthropic rejects the entire request: +// +// 400 messages.1: role 'system' must precede an 'assistant' message or end the array +// +// It shipped because nothing asserted the summary's role, and because every measurement +// before this replayed through /compact, which never forwards upstream and so never had a +// body validated by a provider. It was found only when LOCA-bench ran against a real API: +// every task that triggered a summarization failed, including in an arm with no other +// component enabled. +// +// The contract is therefore: the summary must NOT be system-role, and no system-role +// message may appear anywhere except index 0. +func TestSummarizeEmitsNoSystemRoleAwayFromTheHead(t *testing.T) { + s := newSummarizeTestComponent(t, &fixedModel{out: "SUMMARY: explored the handler, 3 tests fail."}) + span := strings.Repeat("ran pytest tests/test_handler.py, 3 failures in src/mod/file.py\n", 40) + req := &bschemas.BifrostChatRequest{ + Input: []bschemas.ChatMessage{ + // index 0 is the system prompt, exactly as a real agent sends it + sysMsg("you are a coding agent"), + userMsg("Fix the failing handler in src/mod/file.py and run the tests."), + toolResultMsg(span), + toolResultMsg(span), + userMsg("keep going"), + }, + } + var rep components.Report + c := &components.Ctx{Ctx: context.Background(), Session: "s", Store: store.NewMemory(store.Options{})} + if _, err := s.Offload(req, &rep, c); err != nil { + t.Fatalf("Offload: %v", err) + } + if rep.Skipped { + t.Skip("summarize declined on this fixture; the role assertion needs it to act") + } + for i, m := range req.Input { + if m.Role == bschemas.ChatMessageRoleSystem && i != 0 { + t.Fatalf("system-role message at index %d — Anthropic rejects this "+ + "(400 messages.%d: role 'system' must precede an 'assistant' message or "+ + "end the array). Messages: %s", i, i, roleList(req.Input)) + } + } +} + +// sysMsg is a system-role message, the shape a real agent puts at index 0. +func sysMsg(text string) bschemas.ChatMessage { + m := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleSystem} + schema.SetMessageText(&m, text) + return m +} + +func roleList(msgs []bschemas.ChatMessage) string { + var b strings.Builder + for i, m := range msgs { + if i > 0 { + b.WriteString(", ") + } + b.WriteString(string(m.Role)) + } + return b.String() +} + +// THE SECOND REGRESSION, found only after the first was fixed +// +// summarize replaces a span with one summary message, so the kept tail can begin part-way +// through a tool exchange — its leading `tool_result` blocks answer `tool_use` blocks that +// were just deleted. Anthropic rejects the whole request: +// +// 400 messages.0.content.2: unexpected `tool_use_id` found in `tool_result` blocks +// +// Measured on live LOCA-bench traffic: 5 of 12 tasks failed this way once the system-role +// defect was fixed and summarize could finally act at all. +// +// This is an invariant for any component that DELETES messages. coref does not need it — +// it rewrites a tool message's text in place and never removes a message, so pairing holds +// by construction. +func TestDropOrphanedToolResults(t *testing.T) { + call := func(id string) bschemas.ChatMessage { + m := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleAssistant, + ChatAssistantMessage: &bschemas.ChatAssistantMessage{ + ToolCalls: []bschemas.ChatAssistantMessageToolCall{{ID: &id}}, + }} + return m + } + result := func(id string) bschemas.ChatMessage { + m := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleTool, + ChatToolMessage: &bschemas.ChatToolMessage{ToolCallID: &id}} + schema.SetMessageText(&m, "output for "+id) + return m + } + + // A well-formed history must pass through untouched — the repair has to be idempotent + // and must never drop a result whose call is present, even at a distance (a summary can + // legitimately sit between a call and its result). + ok := []bschemas.ChatMessage{userMsg("go"), call("t1"), result("t1"), call("t2"), result("t2")} + got, n := dropOrphanedToolResults(ok) + if n != 0 || len(got) != len(ok) { + t.Errorf("well-formed history must be unchanged, dropped %d (%d -> %d)", n, len(ok), len(got)) + } + + // The summarize shape: the span holding call("t1") was replaced by a summary, so the + // tail's result("t1") is orphaned and must go, while result("t2") stays. + orphaned := []bschemas.ChatMessage{ + userMsg("go"), + userMsg("SUMMARY: earlier work, including a call whose result follows"), + result("t1"), // its call was deleted + call("t2"), result("t2"), + } + got, n = dropOrphanedToolResults(orphaned) + if n != 1 { + t.Fatalf("expected exactly 1 orphaned result dropped, got %d", n) + } + for _, m := range got { + if m.Role == bschemas.ChatMessageRoleTool && m.ChatToolMessage != nil && + m.ChatToolMessage.ToolCallID != nil && *m.ChatToolMessage.ToolCallID == "t1" { + t.Error("orphaned tool_result for t1 survived the repair") + } + } + if len(got) != len(orphaned)-1 { + t.Errorf("repair removed %d messages, want 1", len(orphaned)-len(got)) + } +} + +// THE THIRD REGRESSION, and the one that showed the first two shared a root cause +// +// Review's question was the right one: an unanswered `tool_use` means the agent is still +// waiting on that tool, so summarize AFTER the exchange completes rather than through it. +// Both earlier pairing defects were the same mistake seen from either side, caused by +// boundaries chosen arithmetically: +// +// msgs[0] preserved while its results sit in the span -> unanswered call +// tail starting on a tool_result whose call is in span -> orphaned result +// +// summarizeSpan makes a tool exchange atomic, which fixes both without inventing content. +func TestSummarizeSpanNeverCutsInsideAToolExchange(t *testing.T) { + asst := func(id string) bschemas.ChatMessage { + return bschemas.ChatMessage{Role: bschemas.ChatMessageRoleAssistant, + ChatAssistantMessage: &bschemas.ChatAssistantMessage{ + ToolCalls: []bschemas.ChatAssistantMessageToolCall{{ID: &id}}}} + } + res := func(id string) bschemas.ChatMessage { + m := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleTool, + ChatToolMessage: &bschemas.ChatToolMessage{ToolCallID: &id}} + schema.SetMessageText(&m, "out "+id) + return m + } + + // The kept tail must never BEGIN on a tool message: end advances past the exchange. + msgs := []bschemas.ChatMessage{ + sysMsg("system"), userMsg("go"), + asst("t1"), res("t1"), asst("t2"), res("t2"), + } + for keep := 1; keep <= 4; keep++ { + headCount, start, end := summarizeSpan(msgs, keep) + if end < len(msgs) && msgs[end].Role == bschemas.ChatMessageRoleTool { + t.Errorf("keepLast=%d: tail begins on a tool message at %d — orphans its result", keep, end) + } + if start != headCount { + t.Errorf("keepLast=%d: start %d must equal headCount %d", keep, start, headCount) + } + } + + // A head that is an assistant tool-call message must NOT be preserved: its results are + // inside the span, so keeping it would leave the call unanswered. + headIsCall := []bschemas.ChatMessage{asst("t9"), res("t9"), userMsg("next"), userMsg("more")} + headCount, start, _ := summarizeSpan(headIsCall, 1) + if headCount != 0 { + t.Errorf("an assistant tool-call head must not be preserved, got headCount=%d", headCount) + } + if start != 0 { + t.Errorf("start must follow headCount, got %d", start) + } + + // A normal head (system prompt) IS preserved — the identity the head exists for. + headCount, _, _ = summarizeSpan(msgs, 2) + if headCount != 1 { + t.Errorf("a system-prompt head must be preserved, got headCount=%d", headCount) + } +} diff --git a/components/offload/summarize_shortturn_test.go b/components/offload/summarize_shortturn_test.go new file mode 100644 index 00000000..e020c949 --- /dev/null +++ b/components/offload/summarize_shortturn_test.go @@ -0,0 +1,79 @@ +package offload + +import ( + "context" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + "github.com/rossoctl/context-guru/components" + "github.com/rossoctl/context-guru/store" +) + +// A transcript SHORTER THAN keep_last must not panic. +// +// summarizeSpan computes end = len(msgs) - keepLast and then walks forward past tool messages, +// indexing msgs[end]. A negative end still satisfies `end < len(msgs)`, so the walk read msgs[-1] +// and panicked. With the default keep_last: 3 that is any request with fewer than three messages — +// the first turn or two of every session, not an edge case. +// +// It was invisible because pipeline.runOne recovers per component: the panic surfaced only as +// verdict=reverted while summarize silently did nothing on short turns. The tests written with the +// tool-atomicity fix all use fixtures deliberately long enough to summarize, so none of them +// exercised "too short to act on yet". Found in review against live sessions. +func TestSummarizeSpanDoesNotPanicOnShortTranscripts(t *testing.T) { + // The reduced case from the review: two messages, default keep_last. + msgs := []bschemas.ChatMessage{userMsg("hi"), assistantMsg("hello")} + for _, keepLast := range []int{1, 2, 3, 5, 20} { + headCount, start, end := summarizeSpan(msgs, keepLast) + if end < start { + t.Errorf("keepLast=%d: end=%d below start=%d; Offload's `end <= start` guard "+ + "expects a clamped boundary", keepLast, end, start) + } + if end > len(msgs) || start > len(msgs) || headCount < 0 { + t.Errorf("keepLast=%d: boundary out of range (head=%d start=%d end=%d len=%d)", + keepLast, headCount, start, end, len(msgs)) + } + // The whole point: for a transcript this short there is nothing to summarize, so the + // span must be empty rather than merely in-range. + if keepLast >= len(msgs) && end != start { + t.Errorf("keepLast=%d >= len(msgs)=%d: span must be empty, got start=%d end=%d", + keepLast, len(msgs), start, end) + } + } + // An empty transcript must be handled too — the head probe indexes msgs[0]. + if _, start, end := summarizeSpan(nil, 3); end != start { + t.Errorf("nil transcript: span must be empty, got start=%d end=%d", start, end) + } +} + +// End to end through Offload, since that is where the panic was actually observed: it must +// decline a short turn rather than panic, and leave the request untouched. +func TestSummarizeDeclinesShortTurnWithoutPanicking(t *testing.T) { + comp, err := newSummarize([]byte("keep_last: 3\nstart_from_message: 0\nmin_tokens: 1\n")) + if err != nil { + t.Fatalf("config: %v", err) + } + s := comp.(*Summarize) + model := &silentModel{} + req := &bschemas.BifrostChatRequest{Input: []bschemas.ChatMessage{ + userMsg("hi"), assistantMsg("hello"), + }} + rep := components.Report{} + ctx := &components.Ctx{ + Session: "s", Ctx: context.Background(), + Store: store.NewMemory(store.Options{}), CtxWindow: 1_000_000, + Model: components.ModelSpec{Static: model, Incoming: model}, + } + // No recover() here on purpose: a panic must fail this test, not be absorbed the way + // pipeline.runOne absorbs it in production. + if _, err := s.Offload(req, &rep, ctx); err != nil { + t.Fatalf("offload: %v", err) + } + if !rep.Skipped { + t.Errorf("a 2-message request must be skipped, got rep=%+v", rep) + } + if len(req.Input) != 2 { + t.Errorf("a declined turn must leave the request untouched, got %d messages", + len(req.Input)) + } +}