Skip to content

fix: three general defects in summarize, extract_llm triggers and prompt sizing - #110

Merged
amiddavid merged 10 commits into
mainfrom
fix/extract-llm-general-defects
Aug 27, 2026
Merged

fix: three general defects in summarize, extract_llm triggers and prompt sizing#110
amiddavid merged 10 commits into
mainfrom
fix/extract-llm-general-defects

Conversation

@amiddavid

Copy link
Copy Markdown
Collaborator

Three product defects that occur with selection_mode unset — the per-output design main
ships today — split out of #80 so they are not blocked behind the co-reference experiment.
Branched from main (c863519), no dependency on internal/extract/bulk.go,
extract_llm_merged.go, internal/coref, or the prefix-ask machinery.

What is here

fix(summarize): the keep_last boundary could split a tool exchange. Not on #80's
list — found while checking whether #80's documented apply defect was still live. keep_last
counts messages and a tool result is a message, so the tail boundary could land between an
assistant's tool calls and the results answering them: the calls were summarized away while
their results survived, and the provider rejected the whole request. At keep_last 2, 3 and 5
the emitted wire was [user, summary, user(tool_result pa_h, tool_result pb_h), user] — two
results answering nothing.

It hid behind the direction of every existing pairing check. They all asked "is every
tool_use answered", and in this failure no call is left unanswered because the calls are not
on the wire at all. The new test asserts pairing in both directions and collects declared
ids as it walks, so a result can only pair with a call that precedes it — the rule the provider
enforces and the one schema.ToolCalls already documents.

Fixed by snapping the boundary backwards onto the declaring assistant message rather than
repairing the wire afterwards. Deleting orphaned results is the other option and it is the
dangerous one: that same repair, on a branch where Anthropic tool ids were invisible to all
pairing logic, saw every result as orphaned and deleted the lot.

fix(extract_llm): a configured request trigger was ignored on caching backends. The
check read !c.CacheAware && !fires && !huge, so under CACHE_MODE=on a configured
trigger.min_request_tokens / min_request_frac / min_messages had no effect. That term
could only ever discard operator configuration: Trigger's zero value fires always, so
!fires is reachable only when the operator set a threshold that was not met, and there is no
derived value inside fires for a cache carve-out to protect.

But it was not pure noise — 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 whole transcript re-bills whatever the request's size, and
the sweep carries its own floor. sweeping already overrides the cadence and pressure gates
for that reason; this check had never been given the same treatment, and !c.CacheAware was
standing in for it badly — CacheAware is true on warm turns too, so the stand-in also threw
the threshold away where it means exactly what the operator wrote. The term becomes
!sweeping.

fix(extract_llm): the prompt was sized by the request model's window, not the extraction
model's.
inputLimit used c.CtxWindow whenever the configured source was not config,
on the premise that "the extraction model IS the proxied model". ModelSpec falls back from
incoming to the static client whenever no incoming client could be built, so the configured
source still read incoming while the call went to the small static model — up to 1M against
200k, and in the direction fitsModelContext singles out as costly: the request goes out, the
upstream rejects it, and the round-trip buys nothing. ForModelSource already reports the
source it resolved from (the line below uses it for model_source_fell_back_to_config); the
value just was not carried as far as the budget.

Verification

Every test was verified to fail when its subject is reverted, on the eval box (Go 1.26.4,
CGO_ENABLED=1):

  • boundary snap: fails on keep_last 2, 3 and 5, passes when restored;
  • request trigger: cache_aware=false passes and cache_aware=true fails with gates=map[]
    — the trigger silently ignored — then both pass;
  • window: the fallback case fails with inputLimit = 1000000, want 32768, while the
    stayed-incoming case still passes, so the test is sensitive to the defect and not merely to
    the plumbing.

Each test also carries preconditions that fail loudly rather than passing vacuously (summarize
must have acted; a tool_result and a parallel tool_use pair must have reached the wire; the
candidate must have reached the gate under test rather than being filtered earlier).

Full suite: 27 packages, 0 failures, gofmt clean. No benchmarks were run.

Deliberately not here

Most of #80's "general fixes" have already landed on main independently, and are not
re-applied: the Anthropic tool_use id recovery (main has attachToolUse, in a more general
form — not provider-gated); the output-token ceiling, including the truncated-vs-declined
counter (DefaultMaxTokens = 4096, cheapExtractOutputTokens bound to that constant, and the
reply_truncated gate); the request-path expand restore (expand.RepairToolResults); and the
zero-resolved continuation.

Excluded as not applicable to main: #80's schema answered-tool-use fix (ValidateShape
does not exist on main — the validator is #80's own), the AllowCachedPrefix doc correction
(the field is introduced by #80), and the always-advertise commit (capture_hop.py plus an
iteration pre-registration).

One item is left open by design rather than oversight: min_tokens implying "fire on every
request". TestExplicitMinTokensStillGoverns pins that behaviour deliberately as backward
compatibility for pre-#28 configs, so separating the per-output floor from the when-to-act
decision is a breaking config change and wants a decision, not a patch. fire_on: size is now
the explicit way to ask for size-only firing, which makes the implicit coupling removable —
but on purpose, with a deprecation path.

@amiddavid

Copy link
Copy Markdown
Collaborator Author

Two follow-ups deliberately left out of this PR, recorded so they are not lost:

expand unresolved-cause counters → #111. A well-formed marker id with nothing stashed behind it is a context-guru defect (reversibility promised, not delivered), while a malformed id is just the model inventing one. Both currently collapse onto the same Unavailable placeholder with no counter, and neither wasted_tokens nor sse_expand_after_stream can go non-zero for a broken stash — a silent no-op is indistinguishable from no expand calls at all. Left out because it needs a design decision on what counts as a well-formed marker id, not a patch; options and acceptance criteria are in the issue.

min_tokens implying fire-on-every-request. Not a defect to patch: TestExplicitMinTokensStillGoverns (components/offload/extract_econ_test.go) pins it on purpose as backward compatibility for pre-#28 configs, which set min_tokens and expected size-based firing. Separating the per-output floor from the when-to-act decision is therefore a breaking config change. fire_on: size now provides the explicit way to ask for size-only firing, which makes the implicit coupling removable — but as a deprecation with a stated migration, not silently.

Also worth a look, found while testing and out of scope here: summarize emits the summary as a role: "system" message inside messages (summarize.go:247, :300) with no provider branch. Mid-conversation system messages are supported on Opus-class models but not Sonnet 5, and are required to be followed by an assistant turn or be last — this one is often followed by a user/tool message. If a gateway is normalizing it into the top-level system block instead, the cached-prefix benefit it exists for would not be materializing. Worth confirming against cache_read_input_tokens on a summarize turn.

@amiddavid

Copy link
Copy Markdown
Collaborator Author

Probed the mid-conversation role: system question against the gateway (benchmark endpoint, not Guru; identical ~3k-token cached prefix, varying only messages). Result: the shape summarize emits is a hard 400 on both claude-sonnet-5 and claude-opus-5messages.1: role 'system' must precede an 'assistant' message or end the array — while [user, system, assistant, user] succeeds and bills read=3064 / write=0.

So the cache rationale for placing the summary mid-conversation is confirmed (nothing is folded into system by the gateway), and it is not a model-support restriction — both models accept the construct. Only the placement is wrong.

The boundary snap in this PR narrows it incidentally, since snapping off a split tool exchange lands on the declaring assistant message, but does not close it: keep_last: 1 on a transcript ending in a user turn still emits [user, summary, user]. Tracked in #112 with the full probe table.

I implemented and then reverted the obvious fix (require the tail to begin with an assistant message): it over-reaches. With no assistant message near the boundary the snap collapses the span and summarize declines entirely, regressing TestSummarizeRestructures, TestSummarizeCountChangeLossless, TestSummarizeModelErrorFailsOpen and TestSummarizeReusesCheckpoint to skipped=true/calls=0. Silently not compacting is worse than the defect. #112 lays out four options and what each gives up; the placement assertion is drafted and should land with whichever is chosen.

@amiddavid

Copy link
Copy Markdown
Collaborator Author

Added 3681e1c — cherry-pick of 80e95d5 from #80, which fixes the role=system placement defect I measured above. It was written on 2026-08-21 and never reached main, which is why a binary built from feat/coref-compaction emits a user-role summary while main still emits system.

It supersedes the design discussion in #112: the answer is a user-role summary, in both the fresh-summary and checkpoint-replay paths (they must agree, or a replayed turn emits different bytes from the turn that created it). My concern that this might forfeit the cached-prefix benefit was unfounded — the benefit comes from the message's position, which does not change; only the role does.

This is now the most serious item in this PR. On main, summarize emits [msgs[0], summary, tail...], so whenever msgs[0] is the system prompt — the normal case — the summary lands at index 1 and the provider rejects the entire request. Measured on live LOCA-bench traffic: every task that triggered a summarization failed, including in an arm with no other component enabled. Summarize is effectively unusable on live Anthropic traffic on main today.

The reason it shipped is the part worth keeping, and it is recorded in the commit: every prior measurement 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 also why the apply-level tests in this PR could not have caught it: they assert on apply's output, not on what a provider accepts.

Suite after the cherry-pick: 27 packages, 0 failures, gofmt clean.

…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) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…he 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) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
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 <DAVIDA@il.ibm.com>
(cherry picked from commit 80e95d5)
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 <DAVIDA@il.ibm.com>
(cherry picked from commit 0971a32)
…cts 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 <DAVIDA@il.ibm.com>
(cherry picked from commit 2d6902d)
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) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
@amiddavid

Copy link
Copy Markdown
Collaborator Author

Swapped in your better fix, and swept #80 for anything else general. Branch force-pushed; 6 commits now.

My 066a453 is dropped in favour of fb5c460 + e7d1aa8" (cherry-picks of 0971a32and2d6902d). Yours is better on two counts: it advances the boundary FORWARD 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", where mine retreated and kept more than keep_lastasked — and it fixes a case mine did not handle at all:msgs[0]` being an assistant message carrying tool calls whose results fall inside the span, which leaves an unanswered call. Mine only addressed the tail. It also covers the checkpoint-replay path.

Kept my ee29756 as a test-only commit: it asserts pairing on the EMITTED WIRE rather than on summarize's message list, which is a different surface — rebuildCountChanged sits between them and is where a correctly-paired message list can still produce an invalid wire. Verified as a real guard: fails on origin/main at keep_last 2, 3 and 5, passes once the exchange is atomic.

Sweep results (main..feat/coref-compaction, restricted to apply/schema/expand/cheapmodel/summarize/trigger)

Taken: 80e95d5, 0971a32, 2d6902d.

Excluded — 659e7a6 (cheapmodel truncation). The fix is confined to CompletePrefixed, a merged/prefix-ask-only method, and its counters are merged_reply_truncated / merged_unparseable. main already has the general form (DefaultMaxTokens = 4096 + the reply_truncated gate). Residual, not a defect: #80 chose 16000 for CompletePrefixed while main sits at 4096, and on a thinking-capable request model 4096 can still be consumed by thinking before any text — worth a measurement, not a patch.

Excluded but tracked — caf32d7 (role=tool leak) → #113. Plausibly still live on main: rebuildCountChanged still byte-matches survivors, and the count-change path does not write tool-text rewrites into the body's tool_result blocks the way the equal-count path does. Not reproduced, and caf32d7 conflicts in three places against the reworked apply.go — hand-resolving inside the byte-losslessness machinery is what #80's own commit warned against. #113 has the suggested order (reproduce on main first, then port the two changes rather than the diff) and records the test trap that made #80's version pass vacuously twice.

Excluded as merged-specific: fcf78cd, 4ca1f13, a9d666f, d6c5231.

Suite after the swap: 27 packages, 0 failures, gofmt clean.

@OsherElhadad OsherElhadad left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the kind of PR I like reviewing least, in the best sense — three defects, each one traced back to a reasoning mistake rather than a typo, each with a test that fails on revert for the reason the prose says it will, and none of them papered over with a defensive if err != nil somewhere upstream. !c.CacheAware being a stand-in for !sweeping is a genuinely sharp catch — it's the kind of bug that looks like a deliberate carve-out until you ask what it's actually protecting, and the answer here was "nothing, ever." And the window-sizing fix is exactly the failure mode fitsModelContext exists to prevent: a budget computed against the wrong model's context, guaranteed to get the request bounced by the provider, spending nothing but the round trip.

I merged fix/extract-llm-general-defects onto current origin/main (the manager-controlled keep-alive, predictor-driven TTL, and KV-cache-suggestion work from #107/#109/#114#116) — clean, no textual conflicts, and I don't see any shared state either: nothing in that newer work reads Config, Ctx, or apply.Opts in a way that overlaps these three fixes. Full suite is green on the merge (28 packages, gofmt clean), matching what the PR body claims for the branch alone.

I also went looking for siblings of each bug, since a defect this precisely diagnosed usually isn't alone:

  • summarize is the only component that reassigns req.Input — everything else that touches an old/superseded message (failed_run, agentdiet, mask) rewrites it in place and never changes the count. So the tool-exchange-atomicity fix doesn't need a sibling anywhere else; there's nothing else that can split a tool_use/tool_result pair by deleting messages.
  • extract_llm is the only component that sizes an LLM prompt against a model other than the one the request came in onsummarize and collapse use c.CtxWindow too, but only to size the request's own budget, never a different model's. So the effSource fix doesn't need replicating either.
  • I did wonder whether a config-pinned extraction client (model.model + base_url/api_key, no source: config) could resurrect the same window mismatch through the other model-selection path — Offload only threads effSource through the if model == nil branch, so a client resolved from e.modelClient never gets a corrected source. It doesn't, though: Client() requires model.model to be non-empty, which means e.modelName is always set whenever e.modelClient is, and inputLimit's e.modelName != "" branch (the static-table lookup) short-circuits before effSource/CtxWindow ever gets consulted. Worth having asked, glad it didn't pan out.

Then I built the merged branch into a real single-tenant proxy and ran actual Claude Code sessions through it against the real IBM gateway — a pipeline of summarize + extract_llm + cachesplit against a live coding-exploration task on this repo's own source. Real numbers, no /compact replay: one turn at 46,338 input tokens came back at 4,299 after summarize alone (42,039 saved), served in 7.5s upstream with cache_read=48258. In an earlier turn of the same kind of session, extract_llm also fired on top of summarize and shaved a further 234 tokens off a retained tool output — small, because the fixture the real session happened to produce didn't hand it much to work with, but real, end to end, no synthetic traffic. CacheAware was true on every one of these turns by default (real Anthropic traffic, not a forced CACHE_MODE=on) — which is exactly the condition fix #2 is about: pre-fix, a configured trigger.min_request_tokens would have been silently discarded on every single one of these, and I confirmed it by reverting just that line and re-running TestExplicitRequestTriggerIsHonoredOnCachingBackends myself — cache_aware=true fails with gates=map[], exactly as the PR body says. Same for the system-role fix: reverted, TestSummarizeEmitsNoSystemRoleAwayFromTheHead fails with a system-role message at index 1, exactly the shape Anthropic rejects.

I couldn't get real HTTP traffic to exercise the literal IncomingStatic fallback fix #3 targets, and I want to be upfront about why rather than wave it away: in single-tenant/gateway mode, incomingModel falls back to h.serverKey(h.opts.AnthropicKey) whenever the caller's own credential is absent, and that server key is exactly what makes a local Claude Code session work at all — so Incoming is essentially never nil here. The fallback only happens for real when h.opts.Tenants != nil (hosted mode, where serverKey deliberately returns nothing so a tenant's compaction can't be billed to the operator) and the caller sent no provider credential of its own. Setting that up felt like more infrastructure than the question warranted, so instead I verified the mechanism directly: the fix's own updated table test (incoming fell back to config: window is not the request model's) passes on the merge, and reverting effSource back to plain e.modelSource reproduces the inputLimit = 1000000, want 32768 failure from the PR body verbatim. That's a real gap in what I was able to exercise live, not a dismissal of the fix — I just want you to know which parts got the real-traffic treatment and which got the unit-level one.

Two things I found while stress-testing that aren't in the diff:

A real panic, on ordinary early-session traffic, not an edge case. summarizeSpan computes end = len(msgs) - keepLast before checking whether that's sensible, then walks forward from end to skip past a tool message. With the default keep_last: 3, any request with fewer than 3 messages — i.e., turn one or two of every single session — makes end negative and msgs[end] panics with index out of range [-1]. My proxy log showed it happening on essentially every short turn in my live sessions (component=summarize ... verdict=reverted ... err="panic: runtime error: index out of range [-1]"), and it reduces to four lines:

msgs := []bschemas.ChatMessage{userMsg("hi"), assistantMsg("hello")}
summarizeSpan(msgs, 3) // panics: index out of range [-1]

It's caught by pipeline.runOne's per-component recover(), so nothing breaks for the user — but it means summarize is now panicking on a large fraction of real traffic instead of hitting the end <= start guard in Offload that used to catch this cleanly (the old arithmetic-only boundary never indexed anything before that check ran). None of the new tests catch it because every fixture in the PR is deliberately long enough to summarize; nothing exercises "too short to act on yet." A if end < start { end = start } right after computing end, before the tool-boundary loop, would restore the old short-circuit.

A pre-existing wire-shape bug that these fixes make reachable for the first time. I watched a real session get a real 400 from the gateway — messages: Unexpected role "tool". Allowed roles are "user" or "assistant." — on a turn where summarize had just acted (changing the message count) and extract_llm rewrote a retained tool output's content in the same turn. That's apply.rebuildCountChanged, not this PR's code, but I think it's worth flagging here anyway: it marshals out[i] directly — bifrost's internal shape — for any message that doesn't byte-match its pre-image, and its own comment says that branch is for "a new message (e.g. the summary)." A retained tool-result message that a later component also rewrites falls into the same branch, and a synthetic role: tool message serialized directly is exactly what Anthropic rejects. Before this PR, summarize acting on live Anthropic traffic at all was blocked by the system-role bug, so this combination — count change plus a same-turn content rewrite — never got exercised in production. I turned it into a small, deterministic repro (summarize with keep_last: 3 + extract_llm both acting on one request, apply.BodyWithModel end to end): message 3 in the output comes back as {"role":"tool",...}. Happy to share the exact test if it's useful; it's outside this diff's files so I didn't want to just paste a patch to something you didn't touch, but it seems like the natural next thing to chase given this PR is what makes it visible.

None of that changes my read of the actual diff — the three fixes are correct, precisely scoped, and each one closes a real hole with a test that means what it says. I'd just want the panic fixed before this sees real traffic, since it's not intermittent — it's turn one of nearly every session — and I'd flag the wire-shape issue to whoever owns apply.go, since it's the thing standing between "summarize finally works" and "summarize finally works, most of the time, on requests where nothing else also touches a tool message."

…nscripts

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) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…ce correction

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) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
@amiddavid

amiddavid commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks — this is a much better review than the PR deserved, and it found a real regression. Both pushed.

The panic is fixed (e9bf3a7), and you were right that it is not an edge case. Your reduced case reproduces verbatim: panic: runtime error: index out of range [-1]. The clamp goes exactly where you suggested, if end < start { end = start } before the tool-boundary walk, which restores the old short-circuit — Offload sees end <= start and declines.

Worth naming the mechanism precisely, because it explains why no test caught it: the boundary used to be pure arithmetic and indexed nothing, so end <= start in Offload was a sufficient guard. Making the exchange atomic moved an index read in FRONT of that guard, and end < len(msgs) is trivially true for a negative index. So this is a regression the atomicity fix introduced, not a latent bug it exposed — my characterisation of that change as purely additive was wrong.

Two tests, both verified to fail (by panicking) without the clamp: summarizeSpan 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 rather than being absorbed the way pipeline.runOne absorbs it in production — which is the property that let this hide as verdict=reverted in your logs.

Your config-pinned-client question is now recorded in the code (7638155). You reached the right answer — Client() requires model.model, so e.modelName is always set when e.modelClient is, and inputLimit's static-table branch short-circuits before effSource is consulted — and it took two hops to derive. Comment only, no behaviour change; the next reader shouldn't have to redo it.

The wire-shape bug is #113, and your report upgraded it from plausible to confirmed. I had opened it from reading the mechanism and labelled it unreproduced; you have a live 400 and a deterministic repro. I have corrected the issue on two points: the trigger is narrower than I wrote — a retained tool-result that a later component rewrites in the same turn, not just "a rewrite plus a count change" — and your route through extract_llm is probably more common than the format-on-indented-JSON route I described. I have also noted there that #110 is what makes it reachable in production, so it is pre-existing and newly live, which argues for picking it up promptly rather than queueing it. Please do paste the repro into #113 — it is the expensive half of the work. One caveat recorded there from #80's history: that test passed with the fix removed twice (prose content, then already-compact JSON), so it needs a precondition assertion that some case really carried a rewritten tool_result through a count change.

On the fallback you couldn't exercise live: that is the honest answer and I'd rather have it stated than papered over. Standing up hosted mode with Tenants != nil and a credential-less caller is more infrastructure than the question warrants, and the table test plus the revert-reproduction (inputLimit = 1000000, want 32768) is the right level for it.

Suite on this branch after both commits: 27 packages, 0 failures, gofmt clean — 27 rather than your 28 because this branch is based on c863519, before the #107/#109/#114#116 work you merged onto.

…hange

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) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
@amiddavid

Copy link
Copy Markdown
Collaborator Author

Pulled the wire-shape fix into this PR after all (6e503e2). On reflection my reason for excluding it was weak: "outside this diff's files" does not hold when this PR is what makes the defect reachable. Before it, summarize acting on live Anthropic traffic was blocked by the system-role bug, so shipping these fixes without this one delivers a component that finally works and then fails with a different 400 on any turn where something else also rewrites a tool message. Same shipping decision, not a follow-up.

Reproduced first, per the order in #113apply/toolrole_wire_test.go, deterministic: summarize (keep_last 3, changing the count) plus extract_llm (strategy: deterministic, rewriting a retained output) over an Anthropic transcript. Message 3 comes back {"role":"tool",...}, matching what you saw live.

Fixed with both halves, because either alone is wrong:

  • rewritten tool text is written into the body's tool_result block before the rebuild reads it — the same edit the equal-count path already makes, so only the block's content string changes and the rest of the message stays byte-identical. This is what preserves the rebuild's rule: decide which messages to keep, never how to serialize one.
  • tool messages are then matched by tool_call_id instead of bytes, since their text may now legitimately differ from the pre-image.

Matching by id alone would emit the original bytes and silently discard the compaction — a valid wire carrying uncompacted content, which no counter would surface. Writing back alone would still fail to match, so the message would still be marshaled fresh. Fail-open is preserved: a failed sjson write leaves the body untouched, and when no tool text changed the body is not copied at all.

Two things worth flagging about the test, since you know this defect's history:

My first version of it was vacuous, and its own precondition caught it. I asserted that a tool_result block survived into the output to prove there was a retained tool message to mis-serialize — but a leaked role: tool message has no tool_result block, so "nothing survived" and "the bug fired" were indistinguishable, and it aborted as vacuous on precisely the output it exists to catch. It now counts leaked role=tool messages and well-formed tool_result blocks in one pass. That is the third distinct way this test has managed to pass while proving nothing, after the prose-content and already-compact-JSON traps from #80 — worth recording as a pattern rather than three coincidences.

And since this touches the losslessness machinery, I re-checked that specifically rather than relying on the aggregate: TestSummarizeCountChangeLossless, TestLosslessGuardProtectsUnmodeledFields, TestFilterDeclarationsByteStable and TestFilterSkillIsByteStableWhenNothingMatches all pass.

So your repro is no longer needed for #113 — but if it exercises a route mine does not (yours came through extract_llm on a real session; mine is synthetic and deterministic), it is still worth having. I have left #113 open until this merges.

Suite: 27 packages, 0 failures, gofmt clean. 9 commits.

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.<i>.content.<b>.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) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
@amiddavid

Copy link
Copy Markdown
Collaborator Author

Reproduced against your actual pipeline, and the fix holds there too (8fbc17b). Good prompt to check — my repro ran only [summarize, extract_llm], and your live 400 came from [summarize, extract_llm, cachesplit].

cachesplit is not a spectator in this, which is why it was worth doing rather than assuming: it is a no-op in Reformat and the split it names happens inside apply itself, so it rewrites the envelope before the count-change rebuild reads it, and it changes that rebuild's control flow (a declined rebuild is still forwarded when systemSplit is set).

That bears on this fix specifically. writeBackToolText edits the body at slot paths (messages.<i>.content.<b>.content) and then re-reads the messages array from the edited body — so if the split had shifted those indices, the write would land on the wrong message, and the losslessness guarantee would be quietly wrong rather than loudly broken. Exactly the failure mode worth ruling out by test rather than by reading.

It holds. Verified in both directions on the eval box, with apply.go restored to the base commit and then to the fix:

apply.go pipeline result
base summarize + extract_llm FAIL — 1 message role="tool"
base summarize + extract_llm + cachesplit FAIL — 1 message role="tool"
fixed both PASS

So the reported configuration reproduces the defect and the fix covers it. The test now runs both pipelines as subtests, with the reduced one kept because it isolates the mechanism and the reported one added because it is the one that actually failed in production.

Suite: 27 packages, 0 failures, gofmt clean. 10 commits.

Still not merging: the ruleset on main sets require_extra_approval_for_unattributed_changes, and your approval is pinned to ee29756 — four commits back now, including the apply.go change to the losslessness machinery, which did not exist when you approved. All three checks are green (build-test, trivy, DCO); it needs one more approving review covering 6e503e2 and 8fbc17b in particular.

@amiddavid
amiddavid merged commit 8631c1b into main Aug 27, 2026
3 checks passed
@github-project-automation github-project-automation Bot moved this from New/ToDo to Done in Rossoctl Issue Prioritization Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants