From afdf21109e606276aef7062630007e83060aad87 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Wed, 26 Aug 2026 17:22:09 +0300 Subject: [PATCH 01/10] fix(extract_llm): honor a configured request trigger on warm caching turns The request-level trigger check read if huge := e.trigger.IsHuge(sz, c.CtxWindow); !c.CacheAware && !fires && !huge { so with CACHE_MODE=on, or against any prompt-caching provider, a configured trigger.min_request_tokens / min_request_frac / min_messages had no effect whatsoever. That condition could only ever discard operator configuration. Trigger's zero value fires always -- "a zero field is no constraint", components/trigger.go -- so `!fires` is reachable ONLY when the operator set one of those thresholds and the request did not meet it. There is no derived or heuristic value inside `fires` for a cache-awareness carve-out to be protecting; the derived pressure trigger is a separate variable that gates the model earlier, in shouldFire. So the only reachable effect of the `!c.CacheAware` term was to void an explicit setting, without a counter or a gate to say so. But `!c.CacheAware` was not pure noise, and the first version of this fix -- deleting the term outright -- broke the housellm cold-sweep preset test. What legitimately bypasses a request-SIZE threshold is a COLD SWEEP: on a cold turn the entire transcript re-bills at the cache-write rate however small the request is, so the request-level threshold answers the wrong question, and the sweep already carries its own floor (cold_cache.min_tokens). `sweeping` overrides the cadence gate and the pressure gate for exactly that reason; this check had simply never been given the same treatment, and `!c.CacheAware` was standing in for it badly. CacheAware is true on warm caching turns too, so the stand-in also threw the threshold away on every warm turn, where it means precisely what the operator wrote. So the term becomes `!sweeping`, which is what was meant: the sweep stays exempt, warm turns honor the configuration. IsHuge still overrides either way. The new test runs one config against one request and varies ONLY CacheAware, so it cannot pass by saying something general about triggers, and it asserts the candidate actually reached this gate rather than being filtered earlier by the floor or the cached-prefix tail gate. Setting economic_gate: false is required setup rather than convenience: it also sets allow_on_caching_backend, without which the component disables itself on the CacheAware arm and that arm would go green for an unrelated reason. Verified by neutralising the fix: cache_aware=false passes and cache_aware=true fails with gates=map[] -- the trigger silently ignored -- then both pass when it is restored. The cold-sweep exemption stays pinned by TestHousellmColdSweepActuallyFires. Full suite: 27 packages, 0 failures, gofmt clean. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- components/offload/extract_llm.go | 24 +++++- .../extract_trigger_cacheaware_test.go | 86 +++++++++++++++++++ 2 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 components/offload/extract_trigger_cacheaware_test.go diff --git a/components/offload/extract_llm.go b/components/offload/extract_llm.go index f8086d4b..ca5019f2 100644 --- a/components/offload/extract_llm.go +++ b/components/offload/extract_llm.go @@ -1046,7 +1046,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_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) + } + }) + } +} From 75e196ae21bc50e22045be11657c14a100f9a040 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Wed, 26 Aug 2026 17:26:29 +0300 Subject: [PATCH 02/10] fix(extract_llm): size the prompt by the model actually called, not the configured one inputLimit derived the extraction model's input budget from c.CtxWindow -- the REQUEST model's window -- whenever the configured source was not `config`: if e.modelSource != "config" && c.CtxWindow > 0 { return c.CtxWindow } with the comment "otherwise the extraction model IS the proxied model". That premise does not hold. ModelSpec falls back from `incoming` to the static client whenever no incoming client could be built, which the proxy returns when the request carries no usable credential. The configured source still reads `incoming`, so the budget still came from the proxied model's window, while the call itself was going to the small static model. The size of the mistake is the gap between those two windows: on a coding agent, up to 1M against 200k. And it errs in the direction fitsModelContext's own comment singles out as the costly one -- over-estimating puts a request on the wire the upstream rejects, so the round-trip and the slot in the turn's wall clock are spent for nothing, every turn, rather than one compaction being skipped. Nothing new had to be detected to fix this. ForModelSource already returns the source it actually resolved from, and the line below already uses it to report model_source_fell_back_to_config -- for exactly the same reason, that the fallback crosses to a different credential on a different endpoint and must not be silent. The value was simply not carried as far as the budget, so it is now hoisted as effSource and passed in. inputLimit treats "" as "use the configured source", so no caller is forced to know. Two new cases pin both directions, since one alone would not distinguish this fix from "never trust CtxWindow": a `source: incoming` that FELL BACK gets the conservative default, and a `source: incoming` that stayed incoming keeps the host-resolved window. Verified by restoring the old expression: the fallback case fails with inputLimit = 1000000, want 32768, and the stayed-incoming case still passes -- so the test is sensitive to the defect and not merely to the plumbing. Full suite: 27 packages, 0 failures, gofmt clean. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- components/offload/extract_llm.go | 24 ++++++++++++++++--- .../offload/extract_llm_ctxguard_test.go | 23 ++++++++++++------ 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/components/offload/extract_llm.go b/components/offload/extract_llm.go index ca5019f2..c702a59f 100644 --- a/components/offload/extract_llm.go +++ b/components/offload/extract_llm.go @@ -232,7 +232,19 @@ 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. +func (e *ExtractLLM) inputLimit(c *components.Ctx, effSource string) int { + if effSource == "" { + effSource = e.modelSource + } if e.modelMaxInput > 0 { return e.modelMaxInput } @@ -245,7 +257,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 +713,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 +724,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 +835,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. 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) } }) From 2edb9d4a840404bcf8e819c1b5c29c521c22374f Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Fri, 21 Aug 2026 05:59:41 +0300 Subject: [PATCH 03/10] fix(summarize): emit the summary as a user message, not system summarize is unusable on live Anthropic traffic. It emits its summary as a SYSTEM-role message and splices 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 the provider rejects the entire request: 400 messages.1: role 'system' must precede an 'assistant' message or end the array System content belongs in the top-level `system` field; a system role inside `messages` must precede an assistant message or end the array. At index 1, followed by the kept tail, it does neither. Found by running LOCA-bench against a real API: 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, not a pipeline interaction. Both code paths are fixed, the fresh-summary one and the checkpoint-replay one; they must agree or a replayed turn would emit different bytes from the turn that created it. A user-role message carrying the summary is valid and conventional -- it is what Claude Code's own compaction does. WHY IT SHIPPED, which matters more than the fix: - Nothing asserted the summary's role. The existing tests reference ChatMessageRoleSystem only for the INPUT system prompt at index 0. - Every measurement in this branch replayed through /compact, which runs the pipeline and returns the rewritten body WITHOUT forwarding upstream. A body no provider ever validates cannot fail schema validation. That is a structural blind spot in replay-based measurement, not a one-off oversight. Adds summarize_role_test.go asserting no system-role message appears anywhere except index 0, verified as a real guard by temporarily restoring the old role and watching it fail. Consequences for results already recorded: iter002's deferral figure (72% fewer summarizations) came from pipelines containing this component, measured via /compact, so the malformed bodies were never rejected -- the mechanism stands but the number must be re-earned. iter004's and iter005's task errors are all explained by this defect. Those pages are already flagged; iter005 will be written up against this cause. Full suite passes. Signed-off-by: DAVID AMID (cherry picked from commit 80e95d58776afac272bb55f05ee26f367141fc1b) --- components/offload/summarize.go | 24 ++++++- components/offload/summarize_role_test.go | 82 +++++++++++++++++++++++ 2 files changed, 104 insertions(+), 2 deletions(-) create mode 100644 components/offload/summarize_role_test.go diff --git a/components/offload/summarize.go b/components/offload/summarize.go index c9c8f734..78e019e9 100644 --- a/components/offload/summarize.go +++ b/components/offload/summarize.go @@ -221,7 +221,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 @@ -274,7 +291,10 @@ 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) out := make([]bschemas.ChatMessage, 0, 2+(len(msgs)-boundary)) out = append(out, msgs[0], summaryMsg) diff --git a/components/offload/summarize_role_test.go b/components/offload/summarize_role_test.go new file mode 100644 index 00000000..c4299ade --- /dev/null +++ b/components/offload/summarize_role_test.go @@ -0,0 +1,82 @@ +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() +} From fb5c46032e0d69c4e647c1b6704b49259711a7cc Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Fri, 21 Aug 2026 06:21:05 +0300 Subject: [PATCH 04/10] fix(summarize): drop tool_results orphaned by the span it removes Second schema defect in summarize, found only after the first (system-role summary, 80e95d5) was fixed and the component could finally act on live traffic. summarize replaces a span with one summary message -- [msgs[0], summary, msgs[end:]...] -- so the kept tail can begin part-way through a tool exchange. Its leading tool_result blocks then answer tool_use blocks that were just deleted, and the provider 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, worse than the 3 the system-role defect caused, because fixing that one let summarize act more often. dropOrphanedToolResults walks forward accumulating available tool_use ids and drops any tool_result whose call is not among them. Wired into both splice sites (fresh summary and checkpoint replay), which must agree or a replayed turn would emit different bytes from the turn that created it. Two design choices worth stating: - A result may answer a call at a DISTANCE, not only in the immediately preceding message, because a summary can legitimately sit between the two. So the check is "was this id ever called", not "was it called last". - The repair is one-directional: it DROPS orphaned results and never synthesises placeholders. 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 the summary is supposed to have described. The rig-side shim used for LOCA's own trimmer does synthesise, because it must preserve a foreign agent's history; a component summarising its own span need not. This is an invariant for any component that DELETES messages, and the reason coref never needed it: coref rewrites a tool message's text in place and never removes a message, so pairing holds by construction. Test covers both halves -- a well-formed history passes through untouched (idempotence, and no dropping of distant-but-valid results) and the summarize shape drops exactly the orphan. Full suite passes. Signed-off-by: DAVID AMID (cherry picked from commit 0971a32b2e4967300836027004bfd653bf22d4ee) --- components/offload/summarize.go | 10 ++++ components/offload/summarize_pairing.go | 55 ++++++++++++++++++++ components/offload/summarize_role_test.go | 61 +++++++++++++++++++++++ 3 files changed, 126 insertions(+) create mode 100644 components/offload/summarize_pairing.go diff --git a/components/offload/summarize.go b/components/offload/summarize.go index 78e019e9..edc0af8a 100644 --- a/components/offload/summarize.go +++ b/components/offload/summarize.go @@ -252,6 +252,11 @@ func (s *Summarize) Offload(req *bschemas.BifrostChatRequest, rep *components.Re out := make([]bschemas.ChatMessage, 0, 2+s.keepLast) out = append(out, msgs[0], 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 @@ -299,6 +304,11 @@ func (s *Summarize) tryReuse(c *components.Ctx, msgs []bschemas.ChatMessage, sta out := make([]bschemas.ChatMessage, 0, 2+(len(msgs)-boundary)) out = append(out, msgs[0], 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..8a476784 --- /dev/null +++ b/components/offload/summarize_pairing.go @@ -0,0 +1,55 @@ +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 +} diff --git a/components/offload/summarize_role_test.go b/components/offload/summarize_role_test.go index c4299ade..a13f94ed 100644 --- a/components/offload/summarize_role_test.go +++ b/components/offload/summarize_role_test.go @@ -80,3 +80,64 @@ func roleList(msgs []bschemas.ChatMessage) string { } 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)) + } +} From e7d1aa8d3735665c2e4c756a47d51547150c63c0 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Fri, 21 Aug 2026 09:05:38 +0300 Subject: [PATCH 05/10] fix(summarize): make a tool exchange atomic, fixing both pairing defects at the root Review reframed defect 3 correctly: an unanswered `tool_use` means the agent is still WAITING on that tool, so summarize after the exchange completes rather than through it. That dissolves the problem instead of patching it, and it turns out both pairing defects were the same mistake seen from either side. The boundaries were pure arithmetic -- preserve msgs[0], summarize msgs[1 : len-keepLast] -- and knew nothing about tool pairing: msgs[0] preserved while its results sit in the span -> unanswered call tail beginning on a tool_result whose call is in span -> orphaned result summarizeSpan now enforces one rule, a tool exchange is atomic: - END advances forward past any tool messages the kept tail would begin with, so the exchange is summarized whole. Advancing rather than retreating keeps call and result on the same side without ever keeping less context than the caller 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 folding it into the summary loses nothing. Applied to both paths, fresh summary and checkpoint replay, including advancing the replayed boundary the same way; if they disagreed, a replayed turn would emit different bytes from the turn that created it. This needs no synthetic content, which is what I wanted and could not justify when fixing defect 2. dropOrphanedToolResults stays as a defensive net rather than the primary mechanism. Test covers all three cases across keepLast 1..4: the tail never begins on a tool message, an assistant tool-call head is not preserved, and a normal system-prompt head still is. Full suite passes. Signed-off-by: DAVID AMID (cherry picked from commit 2d6902deb2a61dbf38d8b4dc0bb53f2c8c83f3b2) --- components/offload/summarize.go | 20 +++++--- components/offload/summarize_pairing.go | 46 ++++++++++++++++++ components/offload/summarize_role_test.go | 57 +++++++++++++++++++++++ 3 files changed, 117 insertions(+), 6 deletions(-) diff --git a/components/offload/summarize.go b/components/offload/summarize.go index edc0af8a..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) } @@ -250,7 +251,8 @@ 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. @@ -269,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 } @@ -301,8 +303,14 @@ func (s *Summarize) tryReuse(c *components.Ctx, msgs []bschemas.ChatMessage, sta // 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. diff --git a/components/offload/summarize_pairing.go b/components/offload/summarize_pairing.go index 8a476784..04987a13 100644 --- a/components/offload/summarize_pairing.go +++ b/components/offload/summarize_pairing.go @@ -53,3 +53,49 @@ func dropOrphanedToolResults(msgs []bschemas.ChatMessage) ([]bschemas.ChatMessag } 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) + } + // 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 index a13f94ed..31081ae4 100644 --- a/components/offload/summarize_role_test.go +++ b/components/offload/summarize_role_test.go @@ -141,3 +141,60 @@ func TestDropOrphanedToolResults(t *testing.T) { 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) + } +} From ee29756a6b068889e81d4ab0648d931031b56fae Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Wed, 26 Aug 2026 18:56:22 +0300 Subject: [PATCH 06/10] test(apply): assert tool pairing on the EMITTED WIRE, in both directions The pairing fixes either side of this are asserted at the message-list level, on summarize's own output. This asserts the same invariants on the bytes apply actually emits, which is a different thing: the count-change rebuild sits between the two, and it is where a normalized message list that pairs correctly can still produce a wire that does not (several normalized messages share one body index for an Anthropic parallel call). Both directions, because they fail independently and each hid the other: FORWARD every tool_use is answered in the message immediately after it. BACKWARD every tool_result answers a call that PRECEDES it. A forward-only check is what let the orphaned-result defect sit unnoticed: when the calls are summarized away, NO call is left unanswered, so the forward direction reads clean while the request is invalid. Declared ids are therefore collected as the transcript is walked, so a result can only pair with an earlier call -- the rule the provider enforces and the one schema.ToolCalls already documents. Three preconditions fail loudly rather than letting the test pass on an empty wire: summarize must have acted, a tool_result must have reached the wire, and a parallel tool_use pair must have reached it. Verified as a real guard against the base defect: on origin/main it fails at keep_last 2, 3 and 5 with "wire message 2 carries tool_result pa_h with no preceding tool_use", and passes once the exchange is made atomic. Full suite: 27 packages, 0 failures. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- apply/parallel_wire_test.go | 139 ++++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 apply/parallel_wire_test.go 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) + } +} From e9bf3a70120278e2faa81205b8faf561a602e89f Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Thu, 27 Aug 2026 13:07:21 +0300 Subject: [PATCH 07/10] fix(summarize): clamp the span boundary before indexing, on short transcripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit summarizeSpan computes end = len(msgs) - keepLast and then walks forward past any tool message the kept tail would begin with. A transcript shorter than keep_last makes `end` NEGATIVE, and `end < len(msgs)` is trivially true for a negative index, so the walk read msgs[-1]: panic: runtime error: index out of range [-1] With the default keep_last: 3 that is any request carrying fewer than three messages — the first turn or two of EVERY session. Reduced case: msgs := []bschemas.ChatMessage{userMsg("hi"), assistantMsg("hello")} summarizeSpan(msgs, 3) This is a regression introduced by making the tool exchange atomic. The boundary used to be pure arithmetic and indexed nothing, so Offload's `end <= start` check caught the short- transcript case cleanly; adding the tool-boundary walk moved an index read in FRONT of that guard. Clamping end up to start restores the short-circuit — Offload sees end <= start and declines, exactly as before. It was survivable rather than visible, which is why no test caught it: pipeline.runOne recovers per component, so the panic surfaced only as verdict=reverted in the logs while summarize silently did nothing on short turns. And every fixture written with the atomicity fix is deliberately long enough to summarize, so nothing exercised "too short to act on yet". Found in review by @OsherElhadad, against live Claude Code sessions through a real proxy, where it fired on essentially every short turn. Two tests, both verified to fail without the clamp (they panic): the reduced summarizeSpan case across keepLast 1..20 plus a nil transcript, asserting the span comes back empty rather than merely in range; and an end-to-end Offload case asserting a 2-message request is skipped and left untouched. The second deliberately does not recover(), so a panic fails the test instead of being absorbed the way production absorbs it. Full suite: 27 packages, 0 failures, gofmt clean (this branch's base; the review reports 28 on a merge with a newer main). Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- components/offload/summarize_pairing.go | 16 ++++ .../offload/summarize_shortturn_test.go | 79 +++++++++++++++++++ 2 files changed, 95 insertions(+) create mode 100644 components/offload/summarize_shortturn_test.go diff --git a/components/offload/summarize_pairing.go b/components/offload/summarize_pairing.go index 04987a13..ac46a582 100644 --- a/components/offload/summarize_pairing.go +++ b/components/offload/summarize_pairing.go @@ -93,6 +93,22 @@ func summarizeSpan(msgs []bschemas.ChatMessage, keepLast int) (headCount, start, 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++ 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)) + } +} From 7638155638bf6058ec3d0dab88ebb52c7d675191 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Thu, 27 Aug 2026 13:08:12 +0300 Subject: [PATCH 08/10] docs(extract_llm): record why a config-pinned client needs no effSource correction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #110 asked whether a config-pinned extraction client (model.model with its own base_url/api_key, no source: config) could resurrect the window mismatch through the OTHER model-selection path, since Offload threads effSource only through the `model == nil` branch and a client resolved from e.modelClient therefore keeps the configured source. It cannot, and the reason is two hops away from the code that would have to be wrong: modelConfig.Client() requires model.model to be non-empty, so e.modelName is always set whenever e.modelClient is, and inputLimit's `e.modelName != ""` branch — the static-table lookup — short-circuits before effSource or CtxWindow is ever consulted. Comment only, no behaviour change. Recorded because the question is a reasonable one to ask of this code and the answer took a reviewer real work to derive; the next reader should not have to repeat it. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- components/offload/extract_llm.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/components/offload/extract_llm.go b/components/offload/extract_llm.go index c702a59f..05464a56 100644 --- a/components/offload/extract_llm.go +++ b/components/offload/extract_llm.go @@ -241,6 +241,14 @@ var staticWindows = modelinfo.DefaultStatic() // 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 From 6e503e2fdf86a95524b3b3114b71cc268b1e96bf Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Thu, 27 Aug 2026 14:34:43 +0300 Subject: [PATCH 09/10] fix(apply): stop role="tool" reaching the Anthropic wire on a count change 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 is a hard rejection of the whole request: 400 messages: Unexpected role "tool". Allowed roles are "user" or "assistant." rebuildCountChanged matches survivors by BYTES and marshals anything unmatched from the bifrost struct -- a branch its own comment describes as being for "a new message (e.g. the summary)". A RETAINED tool message whose text a later component rewrote no longer matches its pre-image, so it landed in that branch and went out with its internal role intact. It takes TWO components in one turn, which is why it stayed hidden: one to change the message count so this rebuild runs at all, and another to rewrite a tool message the first one kept. On a live session: summarize acted, extract_llm reduced a retained output, and the gateway returned the 400 above. Reduced to a deterministic case here -- summarize keep_last 3 plus extract_llm over an Anthropic transcript -- where message 3 came back as {"role":"tool",...}. Fixed with the two halves together, because either alone is wrong: * the rewritten text is written into the body's tool_result block BEFORE the rebuild reads it, the same edit the equal-count path already makes (only the block's `content` string changes, so the rest of the message stays byte-identical). This is what lets the rebuild keep its rule: decide WHICH messages to keep, never how to serialize one. * tool messages are then matched by tool_call_id rather than by bytes, since their text may now legitimately differ from the pre-image, and the id is what pairing depends on anyway. Matching by id alone would emit the ORIGINAL bytes and silently discard the compaction -- a correct wire carrying uncompacted content, which no counter would show. Writing back alone would still not match, so the message would still be marshaled fresh. Hence both. Fail-open is preserved throughout: a failed sjson write leaves the body untouched, and when no tool text changed the body is not copied at all, so the common path is unchanged. WHY THIS BELONGS WITH THESE FIXES rather than after them. The defect is older than this branch, but it was UNREACHABLE in production: summarize acting on live Anthropic traffic was blocked by the system-role defect fixed earlier in this same PR. Landing those fixes without this one ships a component that finally works, and immediately fails with a different 400 on any turn where something else also rewrites a tool message. The test is verified to fail without the fix, and its preconditions are the point rather than decoration -- an earlier version of this test on another branch passed twice with the fix removed, once because the fixture's tool content was prose (which nothing rewrites) and once because it was already-compact JSON (which hits the already_compact gate). This one uses INDENTED JSON so a reduction really happens, asserts the message count actually changed so the count-change rebuild really ran, and counts leaked role=tool messages and well-formed tool_result blocks in one pass -- because a leaked message has no tool_result block to find, so counting only blocks would report "no tool content survived" on exactly the output the assertion exists to catch, and abort as vacuous instead of failing. Byte-losslessness re-checked: TestSummarizeCountChangeLossless, TestLosslessGuardProtectsUnmodeledFields and both byte-stability tests pass. Reported in review by @OsherElhadad, who observed the live 400 and reduced it. See #113. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- apply/apply.go | 112 ++++++++++++++++++++++++++++++++- apply/toolrole_wire_test.go | 122 ++++++++++++++++++++++++++++++++++++ 2 files changed, 231 insertions(+), 3 deletions(-) create mode 100644 apply/toolrole_wire_test.go 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/toolrole_wire_test.go b/apply/toolrole_wire_test.go new file mode 100644 index 00000000..81a0490b --- /dev/null +++ b/apply/toolrole_wire_test.go @@ -0,0 +1,122 @@ +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. + cfg := pipe(t, "pipeline: [summarize, extract_llm]\ncomponents:\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") + 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("neither 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) == len(msgs) { + 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) + } + } +} From 8fbc17b3102f574118b43d1c422a735d8d95b7f9 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Thu, 27 Aug 2026 15:48:38 +0300 Subject: [PATCH 10/10] test(apply): cover the reported pipeline, not just the reduced one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The role=tool reproduction ran only [summarize, extract_llm]. The live 400 was produced by [summarize, extract_llm, cachesplit], and cachesplit is not a spectator here: it is a no-op in Reformat, and the split it names happens inside THIS package. So it rewrites the envelope BEFORE the count-change rebuild reads it, and it changes the rebuild's control flow — a declined rebuild is still forwarded when systemSplit is set. That matters for the fix specifically, not just for fidelity. writeBackToolText edits the body at slot paths (messages..content..content) and then re-reads the messages array from the edited body; if the split had shifted those indices, the write would land on the wrong message and the guarantee would be silently wrong rather than loudly broken. A fix verified only without cachesplit is not verified for the configuration that produced the failure. It holds. Verified in both directions, on the eval box, against apply.go restored to the base commit and then to the fix: UNFIXED summarize+extract_llm FAIL 1 message with role="tool" UNFIXED summarize+extract_llm+cachesplit FAIL 1 message with role="tool" FIXED both PASS So the reported pipeline reproduces the defect and the fix covers it. Full suite: 27 packages, 0 failures, gofmt clean. Pipeline reported in review by @OsherElhadad. See #113. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- apply/toolrole_wire_test.go | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/apply/toolrole_wire_test.go b/apply/toolrole_wire_test.go index 81a0490b..92d48746 100644 --- a/apply/toolrole_wire_test.go +++ b/apply/toolrole_wire_test.go @@ -64,22 +64,40 @@ func TestNoToolRoleOnAnthropicWireAfterCountChange(t *testing.T) { // 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. - cfg := pipe(t, "pipeline: [summarize, extract_llm]\ncomponents:\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") + 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("neither component acted, so the rebuild never ran -- assertion is vacuous") + 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) == len(msgs) { + 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