apply: role=tool may leak onto the Anthropic wire when a rewritten tool message survives a count change

[amiddavid]

rebuildCountChanged decides which messages to re-emit verbatim by byte-matching each output
message against its pre-pipeline normalized form (apply/apply.go:1344: "Each output message that
byte-matches a pre-pipeline normalized message (a survivor) is emitted as its ORIGINAL body raw
bytes"). A message that does not match is marshaled fresh.

For a synthetic role=tool message that is unsafe. Anthropic has no tool role — the synthetic
message is this proxy's internal representation of a tool_result content block. So if any
component rewrites a tool message's text (format compacting indented JSON is the ordinary case,
and it rewrites hundreds per run) and the message count changes (summarize), the rewritten
message no longer byte-matches, falls through to a fresh marshal, and the internal role reaches
the wire:

400 messages: Unexpected role "tool"

The equal-count path already avoids this by writing tool-text rewrites into the body's
tool_result blocks with sjson (apply/apply.go:151: "a change rewrites only that block's
content"). The count-change path does not.

Status: plausible, NOT yet reproduced on main

Being explicit because it matters for triage: this is inferred from reading the mechanism, not
observed. It was a real, measured defect on #80 — 12 of 39 live runs failed with the error above
once an earlier fix stopped tool results being deleted, which had been masking it — and caf32d7
fixed it there by (a) writing tool-text rewrites into the body's tool_result blocks before the
rebuild, so the rebuild only decides which messages to keep and never how to serialize one, and
(b) matching synthetic tool messages by tool_call_id rather than by bytes, since their text may
legitimately differ from the pre-pipeline form.

main's apply.go has since been reworked substantially and caf32d7 does not cherry-pick — it
conflicts in three places. Hand-resolving conflicts inside the byte-losslessness machinery is
exactly what #80's own commit warned against ("a careless change risks the byte-losslessness
guarantee this package exists to provide"), so it was not attempted in #110.

Suggested order of work

1. Reproduce first. A test with an Anthropic body whose tool_result carries indented JSON
   (so format actually rewrites it — see below), a pipeline of [format, summarize] so the count
   changes, asserting no role: "tool" appears anywhere in the emitted body. Confirm it fails on
   main before changing anything.
2. Then port caf32d7's two changes against the current apply.go rather than resolving its diff.

The test trap, recorded because it cost three attempts on #80

caf32d7's regression test passed with the fix removed, twice:

• first version: the fixture's tool content was plain prose, which format leaves alone;
• second: the content was already-compact JSON, hitting format's already_compact gate.

It only became real with indented JSON, plus an assertion of the precondition — that some case
actually carried a rewritten tool_result through a count change — so it fails loudly instead of
passing vacuously. Any new reproduction should assert that precondition too.

[amiddavid]

Upgraded from plausible to confirmed, by review on #110. @OsherElhadad observed this as a real 400 from the gateway on a live Claude Code session — messages: Unexpected role "tool". Allowed roles are "user" or "assistant." — and reduced it to a deterministic repro.

Corrections to my original writeup:

• It is reproduced. I had written "inferred from reading the mechanism, not observed". No longer true: summarize with keep_last: 3 plus extract_llm, both acting on one request, driven through apply.BodyWithModel end to end — message 3 comes back as {"role":"tool",...}.
• The trigger is narrower and more precise than I described. I framed it as "a component rewrites tool text AND the count changes". The exact shape is: a retained tool-result message that a later component rewrites in the same turn. rebuildCountChanged marshals out[i] directly — bifrost's internal shape — for any message that does not byte-match its pre-image, and its own comment says that branch exists for "a new message (e.g. the summary)". A retained-but-rewritten tool message falls into the same branch and is serialized with its synthetic role: tool intact.
• fix: three general defects in summarize, extract_llm triggers and prompt sizing #110 is what makes it reachable in production. Before it, summarize acting on live Anthropic traffic was blocked by the system-role defect, so count-change plus a same-turn content rewrite never occurred against a real provider. So this is not merely pre-existing — it is pre-existing and newly live, which is why it should be picked up promptly rather than queued.

The suggested order in the issue body stands, and step 1 is now cheaper than written: the repro exists, so it only needs porting into the tree (the reviewer offered it). The test trap recorded below still applies — assert the precondition that some case actually carried a rewritten tool_result through a count change, or the test passes vacuously.

Note the reviewer's repro reaches this through extract_llm rewriting a retained output rather than through format compacting indented JSON, which is a second and probably more common route than the one I described.

[amiddavid]

Fixed in #110, now merged, by 6e503e2 plus 8fbc17b.

Both halves were needed, since either alone is wrong: the rewritten tool text is written into the body's tool_result block before the count-change rebuild reads it (the same edit the equal-count path already makes), and tool messages are then matched by tool_call_id rather than by bytes. Matching by id alone would have emitted the original bytes and silently discarded the compaction — a valid wire carrying uncompacted content, which no counter would surface.

Verified in both directions on the eval box, against apply.go at the base commit and then fixed, for the reduced pipeline and the reported one (summarize + extract_llm + cachesplit). cachesplit mattered: the split it names happens inside apply, so it rewrites the envelope before the rebuild and changes its control flow — had it shifted the slot indices, the write-back would have landed on the wrong message. It does not.

Byte-losslessness re-checked by name: TestSummarizeCountChangeLossless, TestLosslessGuardProtectsUnmodeledFields and both byte-stability tests pass.