feat(schema): validate message SHAPE statically, so the fifth summarize defect is caught offline

[amiddavid]

Why

summarize has shipped four message-shape defects, and every one was found reactively — a provider rejecting a live request, or a benchmark failing — each masked by the one before it:

commit	how it surfaced
2edb9d4	400 messages.1: role 'system' must precede an 'assistant' message or end the array
fb5c460	400 messages.N.content.M: unexpected `tool_use_id` found in `tool_result` blocks
e7d1aa8	400 messages.N: `tool_use` ids were found without `tool_result` blocks immediately after
e9bf3a7	panic: index out of range [-1] on a transcript shorter than keep_last

The first three are properties of the message list alone — no provider, no model, no traffic needed to decide them. Nothing in the tree checked them, which is why each had to be paid for in live requests, and nothing would have caught a fifth. schema.ValidateShape checks them, and the summarize/apply suites now assert what the component emits.

What the validator checks

• system-position — a system-role message away from index 0 must be followed by an assistant turn, or end the array.
• answered-tool-use — every tool_use is answered within the contiguous run of tool messages that follows. The run matters: one Anthropic assistant message may carry several tool_use blocks answered by ONE user message, which apply.normalize splits into one synthetic role=tool message per result, so checking only msgs[i+1] reports a violation on every ordinary parallel call.
• paired-tool-result — every tool_result answers a tool_use seen earlier. The mirror of the above; both are checked because e7d1aa8's finding was that they are one mistake seen from either side, and fixing one alone leaves the other live.

Not on the request hot path, deliberately. It walks the whole transcript and allocates per exchange — not free enough to spend on every request, and a check that can only fail open buys no decision for the latency. Putting it there is a different proposal.

It cannot catch e9bf3a7, and the code says so: that was a panic inside the boundary arithmetic, so there was never an output list to inspect (and pipeline.runOne swallowed it into verdict=reverted). What is assertable is the property that replaced it — a too-short transcript comes back untouched and well-formed.

Where it is wired

file	what it asserts
schema/validate_test.go	the rules themselves, plus the pre-fix transcript for each of the three defects
components/offload/summarize_shape_test.go	summarize's OUTPUT across keep_last 1..4 over the two transcript shapes that make the boundary arithmetic go wrong (system-prompt head + parallel exchange; assistant tool-call head), plus the three historical output shapes
apply/shape_validate_test.go	end to end: the emitted wire, re-normalized, for a parallel-call Anthropic transcript — and real captured traffic (the Anthropic tool-use fixture and five Claude-Agent-SDK turns) as the false-positive guard

Rules from the original draft that I dropped, and why

1. "a system-role message may appear only at index 0" — dropped, replaced by the provider's literal rule. It is wrong for main. The Claude Agent SDK appends a fresh system-role message inside messages on every turn (its <system-reminder><total_tokens>N tokens left</total_tokens> budget line — the same messages schema.SessionHead exists to ignore), and that traffic is accepted: apply/testdata/session_head_agentsdk.json, real captured proxy traffic, carries system roles at indices 1, 4 and 7 of five- and eight-message transcripts, each followed by an assistant turn or ending the array. An index-0-only rule fires on ordinary live traffic — the same trap as checking only msgs[i+1] for a parallel call. The rule implemented is the provider's own wording, which still rejects the 2edb9d4 shape because the summary was spliced in front of the kept tail, which begins with a user turn or a tool exchange, not an assistant reply.
2. A stricter "a tool_result must immediately follow its call" rule — not added. main's dropOrphanedToolResults explicitly documents distance-pairing as legal ("a summary can legitimately sit between the two"), so such a rule would flag main's own repaired output. Note the tension for the record: the provider requires immediacy, so a distance-paired result that dropOrphanedToolResults keeps would still be rejected upstream. It is unreachable today — summarizeSpan's atomicity walk (and the replayed boundary's identical walk) means the kept tail never begins on a tool message — so this is an observation, not a claimed live defect. answered-tool-use does enforce immediacy from the call's side.

Vacuity checks (the standing rule)

Each fix reverted on the eval box, the new tests re-run:

reverted fix	outcome
2edb9d4 summary role → system	FAIL offload + apply (system-position)
e7d1aa8 naive span boundary	FAIL offload (answered-tool-use on the assistant tool-call head, at keep_last 1 and 2)
e7d1aa8 + fb5c460 (repair also no-op'd)	FAIL offload + apply (paired-tool-result on both parallel ids — the exact wire parallel_wire_test records: [user, summary, tool_result pa_h, tool_result pb_h, user])
fb5c460 repair no-op'd alone	PASS — reported honestly: with the exchange made atomic, no orphan is ever produced, so dropOrphanedToolResults really is the defensive net its own comment claims to be, and nothing observable depends on it. It is only reachable with the atomicity fix also reverted, as above.
e9bf3a7 clamp removed	FAIL offload — TestSummarizeLeavesAShortTranscriptShapeValid panics with index out of range [-1], as production did (that test deliberately does not recover())

Five mutations of the validator itself were also run, each failing the test that covers it, so no rule is decorative:

• drop system-position → fails the system-role-summary test
• drop paired-tool-result → fails the orphaned-result test
• drop answered-tool-use → fails the unanswered-call test
• scan only msgs[i+1] instead of the tool run → fails the parallel-exchange acceptance cases (1 and 3 false positives)
• make system-position index-0-only (the original draft's rule) → fails the Agent-SDK acceptance case with 3 false positives

Build and test

Eval box (go1.26.4, CGO_ENABLED=1): gofmt clean, go build ./... clean, go test ./... green — 27 packages, 0 failures. No benchmarks run.

[OsherElhadad]

Reviewed against main e88f5d8. Build clean, gofmt clean, all new tests pass, full
go test ./... green, CI green. 783 added lines, zero deletions, zero production files touched,
no invariant at risk.

The three rules are real, not decorative. I reintroduced each of the historical summarize
defects and 4 of 6 fail the new suites:

Mutation	Caught
2edb9d4 summary role → system	FAIL [system-position], both suites
e7d1aa8 head half (headCount = 0→1)	FAIL messages.0 [answered-tool-use] … t_head
e9bf3a7 delete the span clamp	FAIL (panics, no recover() — correct, so it fails rather than being absorbed)
e7d1aa8 span half alone	pass — benignly masked, dropOrphanedToolResults repairs it
fb5c460 orphan repair → no-op, alone	pass — disclosed in the commit, with the right reason
empty summary text → empty content on the wire	pass — genuine gap, see item 4

The false-positive guard is the strongest part, and it is closed by construction rather than by
luck. Every legal-but-unusual Anthropic shape is accepted — multiple tool_use in one turn,
interleaved text+tool_use, parallel calls answered out of order, cache_control on a block,
image blocks, thinking/redacted_thinking, system as a block array, content as a bare string. The
validator reads only Role, ChatToolMessage.ToolCallID and ChatAssistantMessage.ToolCalls, and
never inspects a content block — so content-shape false positives are impossible, permanently,
not merely absent today.

Complexity is fine too: tool_use_id matching is map-based (seenCall), O(n) not O(n²), flat
at ~150 ns/msg across a 30× size range.

The findings are all in the prose, which matters more than usual here because the prose is
unusually authoritative and will be trusted.

1. "Validates the bytes the proxy would actually send" is not true for the one wire defect this repo has paid for

apply/shape_validate_test.go:26-29 claims it validates "the bytes the proxy would actually send,
after apply's rebuild, not the in-memory slice a component happened to hand back." It calls
normalize(Anthropic, …) first and validates the normalized view — and normalize maps a
legal wire and a wire carrying the illegal "role":"tool" onto the identical normalized list:

ACCEPTED | legal anthropic wire                                       | normalized roles: 0:user 1:assistant 2:tool
ACCEPTED | LEAKED role=tool (the 400 apply's toolrole fix exists for) | normalized roles: 0:user 1:assistant 2:tool

Byte-different wires, one a guaranteed 400 messages: Unexpected role "tool", same verdict. The
round-trip destroys the evidence before the validator runs.

This is not hypothetical — fix(apply): stop role="tool" reaching the Anthropic wire on a count change is on main precisely because that 400 was observed live. The defect is covered, by
apply/toolrole_wire_test.go:31, which greps the raw body (:112). That is the correct technique.
So this is an overclaim in prose, not a coverage hole — but the prose is what the next reader
trusts, and they will believe wire shape is guarded when the one wire-level defect on record is
invisible to the new validator.

Either narrow the comment, or close it in three lines, since the check is a raw-body predicate
rather than a shape walk:

// in TestSummarizeEmittedWireIsShapeValid, after obtaining `out`:
for i, m := range gjson.GetBytes(out, "messages").Array() {
    if r := m.Get("role").String(); r != "user" && r != "assistant" && r != "system" {
        t.Errorf("messages.%d: role %q reaches the Anthropic wire; normalize() would hide this", i, r)
    }
}

Related, for merge sequencing: #137's applySweepDrop → schema.SetMessageText
(components/offload/extract_sweep_drop.go:100) rewrites a retained tool message, which is the
second ingredient apply/apply.go:1400-1403 names for that leak. This PR provides zero safety
net for it — do not merge #137 on the belief that #136 guards it. The risk is low for an
independent reason (writeBackToolText matches by tool_call_id, not by component, and no shipped
preset pairs summarize with anything — config/config.go:397 is {"summarize"} alone), but a
third tc row naming extract_llm_sweep in toolrole_wire_test.go is two lines and cheaper than
re-deriving this later.

2. system-position is Anthropic-only with no dialect gate, and /compact defaults to OpenAI

ValidateShape takes no provider. The docstring disclaims scope as "invariants that hold across
providers with an Anthropic-style tool protocol" — true for the two pairing rules, but
system-position is not about the tool protocol and is simply false for OpenAI, which imposes no
positional constraint on system/developer:

REJECTED(1) | OPENAI: system mid-array before a USER turn  ([system,user,assistant,system,user])
            -> messages.3 [system-position] role 'system' must precede an 'assistant' message or end the array
proxy/proxy.go:566: provider := bschemas.OpenAI      // <- /compact's DEFAULT

Zero impact as shipped, because nothing outside tests calls it. It bites the moment it is used as
the docstring invites: an OpenAI-dialect replay over /compact reports a violation on every turn
where the client re-injects a system message, and wired to the hot path it would spuriously revert
those requests and silently lose their savings. schema is public API with a pkg.go.dev badge, so
an external importer hits it unwarned.

ValidateShapeFor(p, msgs) with ValidateShape delegating as Anthropic, and
if p == schemas.Anthropic && … on that one rule. Minimum acceptable: one docstring sentence
saying the rule is Anthropic-only. This is a hard prerequisite for any hot-path wiring.

3. The stated reason for staying off the hot path does not survive measurement

Not being wired in is honest and clearly labelled — the commit says "NOT on the request hot path,
deliberately", and the only callers are schema/validate_test.go,
apply/shape_validate_test.go:40,42, and components/offload/summarize_shape_test.go (exhaustive
grep). So "caught offline" is literally accurate.

The justification is not:

"it walks the whole transcript and allocates per exchange; that is not free enough to spend on
every request, and a check that can only fail open buys no decision for the latency."

Both halves are wrong. Measured:

BenchmarkValidateShape166-16    25543 ns/op    9495 B/op   121 allocs/op
BenchmarkValidateShape500-16    72796 ns/op   35492 B/op   357 allocs/op
BenchmarkValidateShape5000-16  801821 ns/op  307919 B/op  3508 allocs/op

73 µs on a 500-message transcript — ~0.007% of a 1 s provider call, and less than this repo
already spends on normalize plus the tokenizer per request. And a fail-open check is a
decision: invariant 1 is the decision — validate the compacted body, revert to the original on
violation, forward that. It converts a guaranteed provider 400 into a silently-lost saving, which
is the trade made everywhere else here.

Wiring it in is the obvious next step, and it is blocked on item 2, not on cost — a hot-path
system-position with no dialect gate would revert every OpenAI request that re-injects a system
message, turning a test-only asset into a savings regression. Order: dialect-gate first, then a
post-pipeline check in apply.Body that reverts on violation. Please at least fix the comment to
state the measured number and name the real blocker.

4. Coverage is 3 of 8 wire-shape defects; two omissions are right, one is a genuine gap

Defect	Caught
tool_result with no preceding tool_use	yes — paired-tool-result, names the id
tool_use with no following tool_result	yes — answered-tool-use
tool_use_id referenced after its message was dropped	yes
system-role position	yes
role:"tool" on the Anthropic wire	no — structurally invisible (item 1)
consecutive same-role	no — correctly omitted
empty content array / empty text block	no — genuine gap
first message not user; trailing assistant	no
orphaned <<cg:HASH>> marker	no — out of scope (needs the Store)

Two omissions are right and worth recording so nobody "fixes" them: consecutive same-role must
not be checked, because summarize's own legal output is [msgs[0], summary(user), user-tail…]
— consecutive user messages, so an alternation rule would reject the very output this PR blesses.
Orphaned markers would drag a Store dependency into schema.

The real gap is empty content — Anthropic 400s on it, a component rewriting a message to ""
reaches it, and I confirmed it live by mutation. ~4 lines, no walk, no alloc:

if m.Role != schemas.ChatMessageRoleTool && m.Content != nil && strings.TrimSpace(MessageText(m)) == "" &&
	(m.ChatAssistantMessage == nil || len(m.ChatAssistantMessage.ToolCalls) == 0) {
	out = append(out, ShapeViolation{Index: i, Rule: RuleNonEmptyContent, Msg: "content must be non-empty"})
}

Otherwise add a "NOT CHECKED, and why" list — the file already does exactly that for e9bf3a7, so
the omission is conspicuous given the prose quality elsewhere.

Smaller items

• The commit message's vacuity table mislabels row 2. It says e7d1aa8 naive span boundary → FAIL offload (answered-tool-use). answered-tool-use is the head half; the span boundary
  alone passes. Substance right, label points at the wrong edit — would cost the next reproducer
  time. One word.
• The 40-line rationale renders nowhere. schema/validate.go:10-48 sits after package schema
  and after the imports, separated from // ShapeViolation is… by a blank line at :49 — so godoc
  attaches it to nothing and pkg.go.dev shows none of it, on a badged public package. Move it to
  schema/doc.go as the package comment (the package has none today) or delete the blank line.
  This is the highest-value docs fix here, ahead of the two below.
• docs/design.md:18 enumerates schema's exports rather than describing it abstractly, so a
  new exported entry point is a gap by that doc's own convention. Append
  `ValidateShape` (static message-shape validation) to the schema/ row.
• docs/components/summarize.md has zero mentions of shape, invariants or 400s
  (grep -in 'shape\|invariant\|400' → empty), so a short "Shape invariants" section there is
  net-new and worth having.
• Near-duplicate, leave it: dropOrphanedToolResults (summarize_pairing.go:30) accumulates a
  seen set of call ids the same way seenCall does. Not interchangeable — one repairs, one
  reports, and the repairer deliberately lets a result answer any earlier call while the validator
  requires contiguity. If a third copy appears, that is the pair to unify.

One thing worth calling out as a credit: the commit volunteers that reverting the orphan repair
alone still passes, with the correct reason. I confirmed that independently. An honest negative
result is what makes the rest of the table believable.

Verdict

Mergeable with nits. This closes a blind spot the project paid for four times in live requests.
Nothing blocks merge. Only items 1 and 2 would I want fixed before this is wired into the request
path, and item 2 is a hard prerequisite for that.

[amiddavid]

Thanks — this is the most useful kind of review to get: you reproduced the defects, benchmarked the
thing, and every finding landed in the prose rather than in the logic. I took your measurements as
the record and reconciled with them rather than re-arguing any of them. Pushed as 5a1760b, plus a
one-line amend to fb00b51's commit message.

Item 2 — dialect gate: implemented, not disclaimed

ValidateShapeFor(provider, msgs) does the work; ValidateShape(msgs) is the Anthropic-dialect
shorthand that delegates. system-position is gated on provider == schemas.Anthropic. I agree the
docstring-only minimum was not enough — an ungated positional rule on the request path is a savings
regression wearing a safety check's clothes, and /compact defaulting to OpenAI makes that the
common case rather than the exotic one.

The two pairing rules stay ungated for every provider, and there is now an explicit assertion to
that effect, because a gate that swept them up with it would trade one wrong answer for a worse one.

Item 1 — the overclaim: closed, but not where you put it

Closed, not narrowed. assertWireRolesLegal asserts role legality on the raw body, and the
comment now says plainly that normalize() maps a legal wire and one carrying role:"tool" onto the
identical list, so shape rules go through the validator and byte-level role legality cannot.

One correction to the suggested patch, and I think it matters: placed literally in
TestSummarizeEmittedWireIsShapeValid, the predicate is vacuous. That test runs [summarize]
alone, and the leak needs two components in one turn — as 6e503e2 says. Under [summarize] every
retained tool message still byte-matches its pre-image, so rebuildCountChanged emits it from its
original bytes and no role can leak; the check cannot fail. I confirmed that by reverting 6e503e2:
[summarize] passes.

So the test now runs both [summarize] and [summarize, extract_llm], over indented JSON so the
second rewrite is real, with a vacuity guard that the two-component pipeline's message count actually
changed. With 6e503e2 reverted it fails on messages.3 and messages.4 of the two-component run —
which is your point exactly, and it also buys new shape coverage over a two-component wire that
toolrole_wire_test.go never validated.

I did not add the third tc row naming extract_llm_sweep. It is a good idea and I agree with
your reasoning, but it belongs to #137's diff — the guard should land with the code it guards, not
here where the leak path it names is not yet reachable by any shipped preset. Consider it a request
on #137. And noted clearly: this PR provides no safety net for applySweepDrop, so #137 must not
be merged on the belief that it does.

Item 3 — my justification was wrong, and the comment now says so

Both halves were wrong and I've replaced them with your numbers, quoted, plus the arithmetic: 73 µs
on a 500-message transcript, ~0.007% of a one-second provider call, less than normalize plus the
tokenizer already cost. And you're right that fail-open is a decision — validate the compacted
body, revert on violation, forward that, which is the trade made everywhere else here. The comment
now names the dialect gate as the real blocker. Not wired in this PR; the post-pipeline check in
apply.Body is a separate change, and item 2 no longer blocks it.

Item 4 — empty content: rule added, and the NOT CHECKED list with it

non-empty-content, Anthropic-gated. One thing the mutation showed that is worth recording:
summarize is not a producer. It refuses a blank summary at summarize.go:201 — making its
model return "" makes it decline rather than emit empty content. So the rule guards the ~20 other
SetMessageText call sites (cmdfilter, dedup, skeleton, textclean, …), which have no such
guard. Still worth having, just not for the component this PR is about.

I kept it narrower than your snippet, deliberately. Your strongest observation was that content-shape
false positives here are impossible rather than merely absent, and MessageText breaks that —
it cannot see an image, a thinking block or an Anthropic tool_result payload and reports all three
as blank. So the rule fires only on a Rewritable message with non-nil content, never on a
role=tool message, and never on an assistant message carrying ToolCalls. Five acceptance cases
pin that down. I'd rather the rule fail open on shapes it cannot fully read than buy coverage with
the property that makes the validator trustworthy.

The NOT CHECKED, and why list is in schema/doc.go, covering consecutive same-role, e9bf3a7's
panic, role:"tool", orphaned markers, and first/trailing role. Consecutive same-role also has a
test now — TestValidateShapeAcceptsConsecutiveSameRoleBecauseSummarizeEmitsIt — as a guard rail
against a future reader "completing" the validator. Adding an alternation rule breaks five tests,
which is the demonstration that it must stay out.

Smaller items

• Vacuity table row 2: fixed by amending fb00b51's message. Now reads head half (headCount 0->1), and says the span-boundary half reverted alone passes, benignly masked by
  dropOrphanedToolResults.
• The 40-line rationale: moved to schema/doc.go as the package comment (the package had none),
  and extended with the NOT CHECKED list and the measured benchmark numbers. Agreed this was the
  highest-value docs fix — it was rendering nowhere on a badged public package.
• docs/design.md: schema/ row now lists ValidateShapeFor/ValidateShape.
• docs/components/summarize.md: new "Shape invariants" section — the four 400s this component
  has produced, the invariants its output is held to, and the two things that are deliberately not
  invariants.
• seen/seenCall: left alone, for the reason you give. One repairs, one reports, and the
  repairer deliberately lets a result answer any earlier call while the validator requires
  contiguity.

Vacuity checks

Every new assertion verified to FAIL with the code it covers reverted, on the eval box:

Mutation	Result
revert 6e503e2 (apply/apply.go)	FAIL apply — assertWireRolesLegal on messages.3+4 of [summarize, extract_llm]; [summarize] alone still passes, as analysed above
drop the dialect gate	FAIL schema — the OpenAI half of TestValidateShapeForGatesSystemPositionToAnthropic
drop the non-empty-content rule	FAIL schema — TestValidateShapeRejectsEmptyContentWithoutFalsePositives
ADD an alternation rule	FAIL schema — 5 tests, including the new consecutive-same-role guard
shapeModel returns ""	FAIL, but on the vacuity guard, not non-empty-content — this is how I learned summarize refuses blank summaries

Eval box (Go 1.26.4, CGO_ENABLED=1): gofmt clean, go build ./... clean, go test ./... green
with one exception — TestSpendSurvivesRowEviction in dash fails with month-to-date = 7, want 10.
That failure reproduces identically on main e88f5d8 and dash does not import schema, so it
is pre-existing and unrelated. Flagging it because it is not something this branch should be blamed
for, and it probably wants its own issue.

No production file is touched by either commit on this branch.

One place I'd push back

Nothing in the review is wrong on the facts. The only thing I'd adjust is the framing of item 1's
patch as "three lines": the predicate is three lines, but making it mean something took a second
pipeline in the test, because the defect is unreachable in the configuration the suggested site runs.
That is a detail of the site rather than of the finding, and the finding was right.

[amiddavid]

Rebased onto main (dc29f22) — force-pushed 5a1760b → df08dab, so review anchors on the previous SHA are stale. No conflicts, and no functional change from the review response: this is purely the rebase.

Re-verified against main's six new commits, which matters here because this PR's suites assert on
summarize and apply output rather than on schema in isolation:

gofmt          clean
go build ./... clean
schema         ok
apply          TestSummarizeEmittedWireIsShapeValid, TestRealCapturedTrafficIsShapeValid,
               TestApplyMetaWritesRefusesOnShapeMismatch, TestWireBreakpointCapRealTrafficShape  PASS
offload        TestSummarizeEmitsAShapeValidTranscript,
               TestValidateShapeRejectsTheHistoricalSummarizeOutputs,
               TestSummarizeLeavesAShortTranscriptShapeValid  PASS
go test ./...  green, all packages

That last line is worth stating explicitly: earlier reports on this PR — including mine — said the suite
was green except dash/TestSpendSurvivesRowEviction. It is now green with no exception. That test was
never broken by anything here; every reproduction of it, mine included, ran against a tree predating
1f1ac2b, which is where the month-boundary guard landed. See #157.

mergeable_state is blocked rather than clean, which I read as branch protection rather than
anything in the diff — mergeable is true and the tree is current. A force-push dismisses a prior
approval, so this most likely wants a re-approve on df08dab.