From 7924ddf3c08a8e6c6189175b62701a80af7a09e4 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Mon, 31 Aug 2026 12:35:02 +0300 Subject: [PATCH 1/2] feat(schema): validate message SHAPE statically, so the fifth summarize defect is caught offline summarize has shipped four message-shape defects and every one was found REACTIVELY -- by a provider rejecting a live request or by a benchmark failing -- each masked by the one before it: 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] (a transcript shorter than keep_last) The first three are properties of the MESSAGE LIST alone: no provider, no model and no traffic are needed to decide them. Nothing in the tree checked them, which is why each one had to be paid for in live requests. schema.ValidateShape checks them, and the existing summarize/apply suites now assert what the component emits. WHAT IT CHECKS, and what it deliberately does not: system-position a system-role message away from index 0 must be followed by an assistant turn, or end the array -- the provider's own wording, NOT "index 0 only". The Claude Agent SDK appends a fresh system-role message inside `messages` on every turn (its `` budget line, the same messages schema.SessionHead exists for), and that traffic is ACCEPTED: apply's captured Agent-SDK fixture carries system roles at indices 1, 4 and 7. An index-0-only rule fires on ordinary live traffic and is worthless exactly where it is needed. Verified by mutation: making the rule index-0-only fails the fixture-shaped acceptance case. answered-tool-use every `tool_use` answered in 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. Checking only msgs[i+1] reports a violation on every ordinary parallel call. Verified by mutation: restricting the scan to msgs[i+1] fails the parallel-exchange acceptance cases. paired-tool-result every `tool_result` answers a `tool_use` seen earlier. The mirror of the above, and both are checked because e7d1aa8's finding was that they are one mistake seen from either side. NOT on the request hot path, deliberately. 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. It is a test-time assertion over the normalized view. NOT able to catch e9bf3a7, and the comment says so: that was a panic inside the boundary arithmetic, so there was never an output list to inspect (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. WIRED IN THREE PLACES: schema/validate_test.go the rules themselves, including the pre-fix transcript for each of the three defects components/offload/summarize_shape_test summarize's OUTPUT across keep_last 1..4 over the two transcript shapes that make the boundary arithmetic go wrong, plus the historical shapes apply/shape_validate_test.go end to end: the EMITTED WIRE, re- normalized, for a parallel-call Anthropic transcript; and real captured traffic (Anthropic tool-use + five Agent-SDK turns) as the false-positive guard VACUITY CHECKS -- each fix reverted on the eval box, the new tests re-run: 2edb9d4 summary role -> system FAIL offload + apply (system-position) e7d1aa8 head half (headCount 0->1) FAIL offload (answered-tool-use) -- the HEAD half is what answered-tool-use catches. The span-boundary half REVERTED ALONE passes, benignly masked by dropOrphanedToolResults. e7d1aa8 + fb5c460 (repair no-op too) FAIL offload + apply (paired-tool-result on both parallel ids, i.e. the exact wire parallel_wire_test records) fb5c460 repair no-op ALONE PASS -- honest result: with the exchange made atomic no orphan is ever produced, so dropOrphanedToolResults is the defensive net its own comment claims to be and nothing observable depends on it e9bf3a7 clamp removed FAIL offload (panics, as it did in production) Five mutations of the validator itself were also run, each failing the test that covers it, so no rule is decorative. Full suite on the eval box: go build ./... clean, go test ./... green, gofmt clean. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- apply/shape_validate_test.go | 175 +++++++++++++++++ components/offload/summarize_shape_test.go | 195 +++++++++++++++++++ schema/validate.go | 201 +++++++++++++++++++ schema/validate_test.go | 212 +++++++++++++++++++++ 4 files changed, 783 insertions(+) create mode 100644 apply/shape_validate_test.go create mode 100644 components/offload/summarize_shape_test.go create mode 100644 schema/validate.go create mode 100644 schema/validate_test.go diff --git a/apply/shape_validate_test.go b/apply/shape_validate_test.go new file mode 100644 index 00000000..7899a1fa --- /dev/null +++ b/apply/shape_validate_test.go @@ -0,0 +1,175 @@ +package apply + +import ( + "context" + "encoding/json" + "os" + "strings" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" + "github.com/tidwall/gjson" + + "github.com/rossoctl/context-guru/components" + _ "github.com/rossoctl/context-guru/components/all" + "github.com/rossoctl/context-guru/config" + "github.com/rossoctl/context-guru/schema" + "github.com/rossoctl/context-guru/store" +) + +// schema.ValidateShape applied where the four historical summarize defects actually surfaced: +// the message list a request is rebuilt from. Each of 2edb9d4, fb5c460, e7d1aa8 was found by a +// provider rejecting a live request; all three are properties of this list, so they are +// assertable here with no provider and no model. +// +// The list checked is the NORMALIZED view — the shape components mutate, with Anthropic +// `tool_use` blocks lifted into ToolCalls and each `tool_result` block its own synthetic +// role=tool message. Re-normalizing the EMITTED wire is what makes this an end-to-end check: +// it validates the bytes the proxy would actually send, after apply's rebuild, not the +// in-memory slice a component happened to hand back. + +// shapeModel is a canned summarizer, so these tests assert on SHAPE and never on wording. +type shapeModel struct{} + +func (shapeModel) Complete(context.Context, string) (string, error) { + return "essential facts from the earlier trajectory", nil +} + +func assertShapeValid(t *testing.T, what string, msgs []bschemas.ChatMessage) { + t.Helper() + if vs := schema.ValidateShape(msgs); len(vs) != 0 { + t.Errorf("%s: %d shape violation(s) — a provider rejects this request:\n%s", + what, len(vs), schema.FormatShapeViolations(vs, msgs)) + } +} + +// REAL CAPTURED TRAFFIC MUST PASS. This is the guard against the failure mode that would make +// the validator worthless: firing on ordinary requests. Both fixtures are real traffic through +// the proxy — an Anthropic tool-use exchange and five turns of Claude-Agent-SDK conversation +// whose per-turn `` budget messages sit at indices 1, 4 and 7 of `messages`. +// A validator that demanded "system only at index 0" (the first version did) rejects every one +// of those turns. +func TestRealCapturedTrafficIsShapeValid(t *testing.T) { + checked := 0 + check := func(name string, body []byte) { + msgs := gjson.GetBytes(body, "messages").Array() + if len(msgs) == 0 { + t.Fatalf("%s: fixture carries no messages", name) + } + norm, _ := normalize(bschemas.Anthropic, msgs) + if len(norm) == 0 { + t.Fatalf("%s: normalized to nothing", name) + } + assertShapeValid(t, name, norm) + checked++ + } + + raw, err := os.ReadFile("testdata/anthropic_tool_use.json") + if err != nil { + t.Fatal(err) + } + var fx struct { + Body json.RawMessage `json:"body"` + } + if err := json.Unmarshal(raw, &fx); err != nil { + t.Fatal(err) + } + check("anthropic_tool_use", fx.Body) + + raw, err = os.ReadFile("testdata/session_head_agentsdk.json") + if err != nil { + t.Fatal(err) + } + var turns map[string]json.RawMessage + if err := json.Unmarshal(raw, &turns); err != nil { + t.Fatal(err) + } + sawSystemAwayFromHead := false + for name, body := range turns { + norm, _ := normalize(bschemas.Anthropic, gjson.GetBytes(body, "messages").Array()) + for i, m := range norm { + if i > 0 && m.Role == bschemas.ChatMessageRoleSystem { + sawSystemAwayFromHead = true + } + } + check("agentsdk/"+name, body) + } + if !sawSystemAwayFromHead { + t.Fatal("no fixture carried a system-role message away from index 0, so the " + + "false-positive guard this test exists for never ran") + } + if checked == 0 { + t.Fatal("no fixture was validated") + } +} + +// THE END-TO-END GUARD. summarize is run over an Anthropic transcript of PARALLEL tool calls +// (one assistant message with two tool_use blocks, both results in the single user message +// that follows — the shape live traffic carries), across every keep_last that lands the span +// boundary in a different place, and the emitted wire is re-normalized and validated. +// +// This is the test the four historical defects would have failed: +// +// 2edb9d4 a system-role summary spliced in front of the kept tail -> system-position +// fb5c460 the tail beginning on a tool_result whose call was cut -> paired-tool-result +// e7d1aa8 an assistant tool-call head kept while its results were cut -> answered-tool-use +func TestSummarizeEmittedWireIsShapeValid(t *testing.T) { + big := strings.Repeat("verbose parallel tool output line\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 tools"}, + {"type": "tool_use", "id": a, "name": "Read", "input": map[string]any{}}, + {"type": "tool_use", "id": b, "name": "Read", "input": map[string]any{}}, + }}, + 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}) + + acted, sawParallelCall, sawResult := false, false, false + for _, keep := range []int{1, 2, 3, 4, 5} { + cfg, err := config.LoadBytes([]byte("pipeline: [summarize]\ncomponents:\n" + + " summarize: {keep_last: " + string(rune('0'+keep)) + + ", start_from_message: 0, min_tokens: 1}\n")) + if err != nil { + t.Fatal(err) + } + p, _ := cfg.Build(nil) + out, changed := BodyWithModel(context.Background(), p, store.NewMemory(store.Options{}), + bschemas.Anthropic, body, "", false, + components.ModelSpec{Incoming: shapeModel{}}) + if !changed { + continue + } + acted = true + norm, _ := normalize(bschemas.Anthropic, gjson.GetBytes(out, "messages").Array()) + for _, m := range norm { + if a := m.ChatAssistantMessage; a != nil && len(a.ToolCalls) >= 2 { + sawParallelCall = true + } + if m.Role == bschemas.ChatMessageRoleTool { + sawResult = true + } + } + assertShapeValid(t, "keep_last="+string(rune('0'+keep)), norm) + } + // Vacuity guards: this test is worthless if summarize never acted, and it does not + // exercise the shape it exists for unless a parallel exchange survived onto the wire. + if !acted { + t.Fatal("summarize never acted, so no wire was validated — the assertions are vacuous") + } + if !sawParallelCall { + t.Fatal("no parallel tool_use pair reached the wire, so the run-scanning half of " + + "answered-tool-use was never exercised") + } + if !sawResult { + t.Fatal("no tool_result reached the wire, so paired-tool-result was never exercised") + } +} diff --git a/components/offload/summarize_shape_test.go b/components/offload/summarize_shape_test.go new file mode 100644 index 00000000..23d9b03d --- /dev/null +++ b/components/offload/summarize_shape_test.go @@ -0,0 +1,195 @@ +package offload + +import ( + "context" + "strconv" + "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" +) + +// schema.ValidateShape over what summarize EMITS, which is the one static check that would +// have caught the shape defects this component shipped three times running: +// +// 2edb9d4 the summary emitted as a system message, spliced in front of the kept tail +// fb5c460 a tool_result left behind by the span that removed its tool_use +// e7d1aa8 a preserved assistant tool-call head whose results were inside the span +// +// Each was found only by a provider rejecting a live request, because nothing asserted the +// shape of the output and every offline measurement replayed through /compact, which never +// forwards upstream. The transcripts below are the two shapes that make the boundary +// arithmetic go wrong, run across every keep_last that puts the boundary somewhere new. + +// callMsg is an assistant turn carrying one or more tool calls — a parallel call when it +// carries several, exactly as apply.attachToolUse lifts an Anthropic assistant message. +func callMsg(ids ...string) bschemas.ChatMessage { + calls := make([]bschemas.ChatAssistantMessageToolCall, 0, len(ids)) + for i := range ids { + id := ids[i] + calls = append(calls, bschemas.ChatAssistantMessageToolCall{ + Index: uint16(i), ID: &id}) + } + return bschemas.ChatMessage{Role: bschemas.ChatMessageRoleAssistant, + ChatAssistantMessage: &bschemas.ChatAssistantMessage{ToolCalls: calls}} +} + +// bulkResult is a tool result big enough that summarize finds the span worth compressing. +func bulkResult(id string) bschemas.ChatMessage { + return toolResultMsgWithID(id, + strings.Repeat("ran pytest tests/test_"+id+".py, 3 failures in src/mod/file.go\n", 40)) +} + +func newSummarizeKeepLast(t *testing.T, keepLast int) *Summarize { + t.Helper() + c, err := newSummarize([]byte( + "keep_last: " + strconv.Itoa(keepLast) + "\nmin_tokens: 10\nresummarize_tokens: 0\n" + + "trigger:\n min_messages: 2\n min_request_tokens: 10\n")) + if err != nil { + t.Fatalf("newSummarize: %v", err) + } + s := c.(*Summarize) + s.modelClient = &fixedModel{out: "SUMMARY: explored the handler, 3 tests fail."} + return s +} + +func TestSummarizeEmitsAShapeValidTranscript(t *testing.T) { + fixtures := map[string][]bschemas.ChatMessage{ + // The ordinary shape: a system prompt at index 0, a PARALLEL tool exchange in the + // middle, a user turn at the tail. The summary lands at index 1, in front of that + // tail — the position 2edb9d4's system-role summary was rejected at — and the span + // boundary can fall between the parallel call and its two results (fb5c460). + "system head, parallel exchange": { + sysMsg("you are a coding agent"), + userMsg("Fix the failing handler in src/mod/file.go and run the tests."), + callMsg("t_a", "t_b"), + bulkResult("t_a"), bulkResult("t_b"), + userMsg("keep going"), + }, + // The shape e7d1aa8 was about: msgs[0] is itself an assistant tool-call message, so + // preserving it as the head leaves its calls unanswered once their results are + // summarized away. dropOrphanedToolResults cannot repair this direction — there is + // nothing orphaned to drop, the CALL is the thing left dangling. + "assistant tool-call head": { + callMsg("t_head"), + bulkResult("t_head"), + userMsg("Fix the failing handler and run the tests."), + userMsg("keep going"), + }, + // A serial exchange at the tail, so the boundary can land between a single call and + // its single result too. + "serial exchange at the tail": { + sysMsg("you are a coding agent"), + userMsg("Fix the failing handler and run the tests."), + bulkResult("t_old"), // an idless-history-shaped result, its call already summarized + callMsg("t_1"), bulkResult("t_1"), + userMsg("keep going"), + }, + } + + acted := 0 + for name, base := range fixtures { + for keepLast := 1; keepLast <= 4; keepLast++ { + // The fixture is deliberately re-cloned: Offload reassigns req.Input but the + // messages themselves are shared, and a leaked mutation would make the next + // keep_last mean something else. + req := &bschemas.BifrostChatRequest{Input: schema.CloneMessages(base)} + var rep components.Report + c := &components.Ctx{Ctx: context.Background(), Session: "s", + Store: store.NewMemory(store.Options{}), CtxWindow: 1_000_000} + if _, err := newSummarizeKeepLast(t, keepLast).Offload(req, &rep, c); err != nil { + t.Fatalf("%s keep_last=%d: Offload: %v", name, keepLast, err) + } + if rep.Skipped { + continue + } + acted++ + if vs := schema.ValidateShape(req.Input); len(vs) != 0 { + t.Errorf("%s keep_last=%d: summarize emitted %d shape violation(s) — a "+ + "provider rejects the whole request:\n%s", name, keepLast, len(vs), + schema.FormatShapeViolations(vs, req.Input)) + } + } + } + // The fixtures must actually be summarized, or every assertion above is vacuous. Three + // tests on this branch's ancestor passed with their fix removed for exactly this reason. + if acted < len(fixtures) { + t.Fatalf("summarize acted on only %d of %d fixture/keep_last combinations; the shape "+ + "assertions never ran on at least one fixture", acted, len(fixtures)) + } +} + +// The paired proof that the assertion above has teeth: the transcripts the PRE-FIX code +// emitted, fed to the validator directly. Each must be rejected, and each must be rejected for +// the rule that names the defect — a check that fires for the wrong reason is not a check. +func TestValidateShapeRejectsTheHistoricalSummarizeOutputs(t *testing.T) { + summary := func(role bschemas.ChatMessageRole) bschemas.ChatMessage { + m := bschemas.ChatMessage{Role: role} + schema.SetMessageText(&m, "=== History Summary ===\nthe earlier trajectory") + return m + } + cases := []struct { + name string + rule string + msgs []bschemas.ChatMessage + }{ + // 2edb9d4: [msgs[0], summary(system), tail...] with the system prompt at index 0. + {"system-role summary", schema.RuleSystemPosition, []bschemas.ChatMessage{ + sysMsg("you are a coding agent"), + summary(bschemas.ChatMessageRoleSystem), + userMsg("keep going")}}, + // fb5c460: the span took the call, the tail kept the result. + {"orphaned tool_result", schema.RulePairedToolResult, []bschemas.ChatMessage{ + sysMsg("s"), summary(bschemas.ChatMessageRoleUser), + bulkResult("t_gone"), userMsg("keep going")}}, + // e7d1aa8: the head kept the call, the span took the result. + {"unanswered tool_use head", schema.RuleAnsweredToolUse, []bschemas.ChatMessage{ + callMsg("t_head"), summary(bschemas.ChatMessageRoleUser), + userMsg("keep going")}}, + } + for _, tc := range cases { + vs := schema.ValidateShape(tc.msgs) + if len(vs) == 0 { + t.Errorf("%s: the pre-fix transcript was accepted; the validator cannot have "+ + "caught this defect", tc.name) + continue + } + found := false + for _, v := range vs { + if v.Rule == tc.rule { + found = true + } + } + if !found { + t.Errorf("%s: rejected, but not for %s:\n%s", tc.name, tc.rule, + schema.FormatShapeViolations(vs, tc.msgs)) + } + } +} + +// e9bf3a7 is the one of the four this validator CANNOT catch, and saying so is the point: it +// was a panic inside the boundary arithmetic, so there was never an output list to inspect +// (and pipeline.runOne swallowed the panic into verdict=reverted). What is assertable is the +// property that replaced it — a transcript too short to summarize comes back untouched and +// well-formed, rather than half-rewritten. +func TestSummarizeLeavesAShortTranscriptShapeValid(t *testing.T) { + base := []bschemas.ChatMessage{userMsg("hi"), assistantMsg("hello")} + for keepLast := 1; keepLast <= 20; keepLast++ { + req := &bschemas.BifrostChatRequest{Input: schema.CloneMessages(base)} + var rep components.Report + c := &components.Ctx{Ctx: context.Background(), Session: "s", + Store: store.NewMemory(store.Options{}), CtxWindow: 1_000_000} + // No recover() on purpose: a panic must fail this test, not be absorbed the way + // pipeline.runOne absorbs it in production. + if _, err := newSummarizeKeepLast(t, keepLast).Offload(req, &rep, c); err != nil { + t.Fatalf("keep_last=%d: Offload: %v", keepLast, err) + } + if vs := schema.ValidateShape(req.Input); len(vs) != 0 { + t.Errorf("keep_last=%d: a short turn came back malformed:\n%s", keepLast, + schema.FormatShapeViolations(vs, req.Input)) + } + } +} diff --git a/schema/validate.go b/schema/validate.go new file mode 100644 index 00000000..f6fe329a --- /dev/null +++ b/schema/validate.go @@ -0,0 +1,201 @@ +package schema + +import ( + "fmt" + "strings" + + "github.com/maximhq/bifrost/core/schemas" +) + +// Static validation of a message list against the provider's message-SHAPE rules — the +// rules that make a request well-formed regardless of its content. +// +// WHY THIS EXISTS. Four separate shape violations shipped in `summarize` and every one of +// them was found REACTIVELY, by a live provider rejection or a benchmark failure, each +// masked by the one before it: +// +// 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] (a short transcript; see NOT COVERED below) +// +// None was findable by the project's existing methods. No test asserted the shape of what a +// component emitted, and every offline measurement replayed through the `/compact` endpoint, +// which runs the pipeline and returns the rewritten body WITHOUT forwarding it upstream — so +// no provider ever validated it. Replay can tell you what a component removed; it is +// structurally incapable of telling you whether the result is a sendable request. +// +// The first three are checkable statically, with no provider and no model, because they are +// properties of the message list alone. That is what this is for: a component that mutates a +// message list can be asserted well-formed in a unit test, closing the blind spot without +// paying for live traffic on every change. +// +// WHERE IT RUNS. Unit tests, over the pipeline's NORMALIZED view of a transcript — the shape +// apply.normalize produces and components mutate, where an Anthropic `tool_use` block has +// been lifted into bifrost's ToolCalls and each `tool_result` block is its own synthetic +// role=tool message. It is deliberately NOT on the request hot path: it walks the whole +// transcript and allocates per tool exchange, which is not free enough to spend on every +// request, and a validator that can only fail open adds latency without adding a decision. +// +// NOT COVERED, and worth being explicit about: e9bf3a7 was a PANIC inside boundary +// arithmetic, not a malformed list. No check on the output list can see it — by the time +// there is a list to inspect, the panic has already happened (and pipeline.runOne swallowed +// it into verdict=reverted). Shape validation is not a substitute for exercising a component +// on transcripts shorter than its own thresholds. +// +// DELIBERATELY NOT A REQUEST VALIDATOR. It checks shape invariants that hold across +// providers with an Anthropic-style tool protocol; it does not check token limits, model +// names, sampling parameters, or anything content-dependent. + +// ShapeViolation is one broken invariant, phrased to point at the message that broke it. +type ShapeViolation struct { + Index int // message index, or -1 when the violation is about the list as a whole + Rule string // short, stable identifier for the invariant + Msg string // human-readable, mirroring the provider's own wording where possible +} + +func (v ShapeViolation) String() string { + if v.Index < 0 { + return fmt.Sprintf("[%s] %s", v.Rule, v.Msg) + } + return fmt.Sprintf("messages.%d [%s] %s", v.Index, v.Rule, v.Msg) +} + +// Shape rule identifiers, so a test can assert on the specific invariant it exists for +// rather than on wording. +const ( + RuleSystemPosition = "system-position" + RuleAnsweredToolUse = "answered-tool-use" + RulePairedToolResult = "paired-tool-result" +) + +// ValidateShape reports every message-shape invariant the list breaks. An empty result +// means the list is well-formed in the ways a provider enforces structurally. +// +// The invariants, and why each one exists: +// +// 1. system-position — a system-role message away from index 0 must be immediately +// followed by an assistant message, or end the array. This is the provider's own +// wording, and it is the defect that made `summarize` unusable on every call: it +// emitted [msgs[0], summary(system), tail...], so with the usual system prompt at +// index 0 a second system role landed at index 1 in front of the kept tail. +// 2. answered-tool-use — every `tool_use` must be answered by a `tool_result` in the +// messages immediately following it. Removing a span can delete the answer while +// keeping the call: preserving msgs[0] when it is an assistant tool-call message does +// exactly that. +// 3. paired-tool-result — every `tool_result` must answer a `tool_use` that appeared +// earlier. Removing a span can delete the call while keeping the answer. The mirror of +// (2), and the reason both are checked: e7d1aa8 showed they are one mistake seen from +// either side, and fixing one alone leaves the other live — which is what happened. +func ValidateShape(msgs []schemas.ChatMessage) []ShapeViolation { + var out []ShapeViolation + seenCall := map[string]bool{} + + for i := range msgs { + m := msgs[i] + + if m.Role == schemas.ChatMessageRoleSystem && i != 0 && !systemPositionOK(msgs, i) { + out = append(out, ShapeViolation{Index: i, Rule: RuleSystemPosition, + Msg: "role 'system' must precede an 'assistant' message or end the array"}) + } + + // A result must answer a call seen earlier. + if m.Role == schemas.ChatMessageRoleTool && m.ChatToolMessage != nil && + m.ChatToolMessage.ToolCallID != nil && *m.ChatToolMessage.ToolCallID != "" { + if id := *m.ChatToolMessage.ToolCallID; !seenCall[id] { + out = append(out, ShapeViolation{Index: i, Rule: RulePairedToolResult, + Msg: fmt.Sprintf("unexpected `tool_use_id` found in `tool_result` blocks: %s", id)}) + } + } + + if m.ChatAssistantMessage == nil || len(m.ChatAssistantMessage.ToolCalls) == 0 { + continue + } + var ids []string + for _, tc := range m.ChatAssistantMessage.ToolCalls { + if tc.ID != nil && *tc.ID != "" { + seenCall[*tc.ID] = true + ids = append(ids, *tc.ID) + } + } + answered := answeredInRun(msgs, i+1) + for _, id := range ids { + if !answered[id] { + out = append(out, ShapeViolation{Index: i, Rule: RuleAnsweredToolUse, + Msg: fmt.Sprintf("`tool_use` ids were found without `tool_result` blocks "+ + "immediately after: %s", id)}) + } + } + } + return out +} + +// systemPositionOK applies the provider's rule for a system role inside `messages`, which is +// narrower than "index 0 only" in one direction and wider in another. +// +// "Index 0 only" is what the first version of this check said, and it is WRONG for this +// codebase: the Claude Agent SDK appends a fresh system-role message inside `messages` on +// every turn (its `N tokens left` budget +// reminder), and that traffic is ACCEPTED by the provider. apply's captured Agent-SDK +// fixture carries system roles at indices 1, 4 and 7 of a five- and eight-message +// transcript — see schema.SessionHead, which exists because of those same messages. A +// validator that rejected them would fire on ordinary live traffic and be worthless +// precisely where it is needed, the same trap as checking only msgs[i+1] for a parallel +// call's results. +// +// So the rule is the provider's literal one: such a message must be followed by an assistant +// turn (the SDK's reminder always is, or it ends the array). The summarize defect fails it +// because the summary was spliced in FRONT of the kept tail, which begins with whatever the +// conversation was doing — a user turn or a tool exchange, not an assistant reply. +func systemPositionOK(msgs []schemas.ChatMessage, i int) bool { + if i == len(msgs)-1 { + return true // ends the array + } + return msgs[i+1].Role == schemas.ChatMessageRoleAssistant +} + +// answeredInRun collects the tool_result ids in the contiguous run of tool messages starting +// at `from`. Any other role ends the run. +// +// PARALLEL CALLS are why this scans a run rather than one message. One assistant message may +// carry several tool_use blocks, and Anthropic requires every result in the SINGLE user +// message that follows. bifrost's schema represents each result as its OWN role=tool message, +// so the wire's one user message maps to a RUN of consecutive tool messages here (see +// apply.normalize). Inspecting only msgs[i+1] therefore reported a violation on every +// ordinary parallel exchange — the second call always looked unanswered: +// +// messages.1 [answered-tool-use] ... without `tool_result` blocks immediately after: call_b +// +// which is indistinguishable from the real defect being hunted. Scanning the whole run is the +// representation-correct reading of the same invariant: the run must still be CONTIGUOUS, so +// anything else in between (a summary, a user turn) still fails. +func answeredInRun(msgs []schemas.ChatMessage, from int) map[string]bool { + answered := map[string]bool{} + for j := from; j < len(msgs); j++ { + if msgs[j].Role != schemas.ChatMessageRoleTool { + break + } + if t := msgs[j].ChatToolMessage; t != nil && t.ToolCallID != nil { + answered[*t.ToolCallID] = true + } + } + return answered +} + +// FormatShapeViolations renders violations one per line for a test failure message, with the +// list's roles appended — the roles are what makes a shape failure diagnosable. +func FormatShapeViolations(vs []ShapeViolation, msgs []schemas.ChatMessage) string { + var b strings.Builder + for _, v := range vs { + b.WriteString(v.String()) + b.WriteString("\n") + } + b.WriteString("roles: ") + for i, m := range msgs { + if i > 0 { + b.WriteString(", ") + } + fmt.Fprintf(&b, "%d:%s", i, m.Role) + } + return b.String() +} diff --git a/schema/validate_test.go b/schema/validate_test.go new file mode 100644 index 00000000..86dd1699 --- /dev/null +++ b/schema/validate_test.go @@ -0,0 +1,212 @@ +package schema + +import ( + "strings" + "testing" + + bschemas "github.com/maximhq/bifrost/core/schemas" +) + +func vsys(text string) bschemas.ChatMessage { + m := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleSystem} + SetMessageText(&m, text) + return m +} + +func vuser(text string) bschemas.ChatMessage { + m := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleUser} + SetMessageText(&m, text) + return m +} + +func vasst(text string) bschemas.ChatMessage { + m := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleAssistant} + SetMessageText(&m, text) + return m +} + +// vcall is an assistant turn carrying tool calls — one id per parallel call, exactly as +// apply.attachToolUse lifts an Anthropic assistant message's tool_use blocks. +func vcall(ids ...string) bschemas.ChatMessage { + calls := make([]bschemas.ChatAssistantMessageToolCall, 0, len(ids)) + for i := range ids { + id := ids[i] + calls = append(calls, bschemas.ChatAssistantMessageToolCall{ID: &id}) + } + return bschemas.ChatMessage{Role: bschemas.ChatMessageRoleAssistant, + ChatAssistantMessage: &bschemas.ChatAssistantMessage{ToolCalls: calls}} +} + +// vresult is one synthetic role=tool message, apply.normalize's representation of a single +// tool_result block. +func vresult(id string) bschemas.ChatMessage { + m := bschemas.ChatMessage{Role: bschemas.ChatMessageRoleTool, + ChatToolMessage: &bschemas.ChatToolMessage{ToolCallID: &id}} + SetMessageText(&m, "output for "+id) + return m +} + +// rules returns the rule ids reported, so an assertion names the invariant rather than the +// provider's wording. +func rules(vs []ShapeViolation) string { + out := make([]string, 0, len(vs)) + for _, v := range vs { + out = append(out, v.Rule) + } + return strings.Join(out, ",") +} + +func TestValidateShapeAcceptsWellFormedTranscripts(t *testing.T) { + cases := []struct { + name string + msgs []bschemas.ChatMessage + }{ + {"empty", nil}, + {"system prompt then a turn", []bschemas.ChatMessage{ + vsys("you are an agent"), vuser("go"), vasst("done")}}, + {"a serial tool exchange", []bschemas.ChatMessage{ + vsys("s"), vuser("go"), vcall("t1"), vresult("t1"), vuser("next")}}, + // The shape the first version of this check got wrong: one assistant message with two + // tool_use blocks, answered by ONE user message on the wire, which normalizes to a RUN + // of two role=tool messages. + {"a parallel tool exchange", []bschemas.ChatMessage{ + vuser("go"), vcall("pa", "pb"), vresult("pa"), vresult("pb"), vuser("next")}}, + {"four parallel calls", []bschemas.ChatMessage{ + vuser("go"), vcall("a", "b", "c", "d"), + vresult("a"), vresult("b"), vresult("c"), vresult("d")}}, + // Results may arrive in any order within the run; the provider pairs them by id. + {"parallel results out of order", []bschemas.ChatMessage{ + vuser("go"), vcall("pa", "pb"), vresult("pb"), vresult("pa")}}, + // REAL, ACCEPTED live traffic: the Claude Agent SDK appends a system-role budget + // reminder inside `messages` on every turn. apply's captured fixture carries them at + // indices 1, 4 and 7. Rejecting these would make the validator fire on ordinary + // traffic — see systemPositionOK. + {"agent-sdk per-turn system reminders", []bschemas.ChatMessage{ + vuser("task"), vsys("1000 tokens left"), + vasst("working"), + vuser("more"), vsys("900 tokens left"), + vasst("working"), + vuser("more"), vsys("800 tokens left")}}, + // An idless tool message is this repo's generic "tool output" fixture shape and carries + // no pairing claim, so it makes no assertion for the validator to break. + {"tool output with no wire id", []bschemas.ChatMessage{ + vuser("go"), {Role: bschemas.ChatMessageRoleTool}}}, + } + for _, tc := range cases { + if got := ValidateShape(tc.msgs); len(got) != 0 { + t.Errorf("%s: well-formed transcript reported %d violation(s):\n%s", + tc.name, len(got), FormatShapeViolations(got, tc.msgs)) + } + } +} + +// The shape 2edb9d4 emitted: [msgs[0], summary(SYSTEM), tail...]. With the usual system +// prompt at index 0, the summary lands at index 1 in front of the kept tail and the provider +// rejects the whole request. +func TestValidateShapeRejectsSystemRoleInFrontOfTheKeptTail(t *testing.T) { + msgs := []bschemas.ChatMessage{ + vsys("you are an agent"), + vsys("=== History Summary ===\nearlier work"), // the pre-fix summary role + vuser("keep going"), + } + got := ValidateShape(msgs) + if len(got) != 1 || got[0].Rule != RuleSystemPosition || got[0].Index != 1 { + t.Fatalf("want one system-position violation at index 1, got [%s]:\n%s", + rules(got), FormatShapeViolations(got, msgs)) + } + // The fix, and the only difference: role=user. + fixed := []bschemas.ChatMessage{msgs[0], vuser("=== History Summary ===\nearlier work"), msgs[2]} + if got := ValidateShape(fixed); len(got) != 0 { + t.Errorf("the user-role summary must be accepted, got:\n%s", FormatShapeViolations(got, fixed)) + } +} + +// The shape fb5c460 emitted: the span holding a call was replaced by the summary, so the kept +// tail begins with a tool_result whose tool_use is gone. +// +// 400 messages.0.content.2: unexpected `tool_use_id` found in `tool_result` blocks +func TestValidateShapeRejectsOrphanedToolResult(t *testing.T) { + msgs := []bschemas.ChatMessage{ + vsys("s"), + vuser("=== History Summary ===\nearlier work"), + vresult("t1"), // its call was inside the summarized span + vcall("t2"), vresult("t2"), + } + got := ValidateShape(msgs) + if len(got) != 1 || got[0].Rule != RulePairedToolResult || got[0].Index != 2 { + t.Fatalf("want one paired-tool-result violation at index 2, got [%s]:\n%s", + rules(got), FormatShapeViolations(got, msgs)) + } + if !strings.Contains(got[0].Msg, "t1") { + t.Errorf("the violation must name the unanswerable id, got %q", got[0].Msg) + } +} + +// The shape e7d1aa8 emitted from the other side: msgs[0] was an assistant tool-call message, +// preserved as the head while its results sat inside the summarized span. +// +// 400 messages.N: `tool_use` ids were found without `tool_result` blocks immediately after +func TestValidateShapeRejectsUnansweredToolUse(t *testing.T) { + msgs := []bschemas.ChatMessage{ + vcall("t9"), // preserved head; result("t9") was summarized away + vuser("=== History Summary ===\nearlier work"), + vuser("keep going"), + } + got := ValidateShape(msgs) + if len(got) != 1 || got[0].Rule != RuleAnsweredToolUse || got[0].Index != 0 { + t.Fatalf("want one answered-tool-use violation at index 0, got [%s]:\n%s", + rules(got), FormatShapeViolations(got, msgs)) + } + + // A summary spliced BETWEEN a call and its result breaks the same rule: the run of tool + // messages must be contiguous with the call, and a repair that merely keeps the result + // somewhere later in the list does not make the request sendable. + split := []bschemas.ChatMessage{ + vcall("t1"), vuser("=== History Summary ==="), vresult("t1"), + } + if got := ValidateShape(split); len(got) != 1 || got[0].Rule != RuleAnsweredToolUse { + t.Errorf("a call answered at a DISTANCE must be reported, got [%s]:\n%s", + rules(got), FormatShapeViolations(got, split)) + } + + // Only the unanswered id of a parallel call is reported — not its answered sibling. + half := []bschemas.ChatMessage{vuser("go"), vcall("pa", "pb"), vresult("pa"), vuser("next")} + got = ValidateShape(half) + if len(got) != 1 || !strings.Contains(got[0].Msg, "pb") { + t.Fatalf("want exactly one violation naming pb, got [%s]:\n%s", + rules(got), FormatShapeViolations(got, half)) + } +} + +// Both directions must be reported independently. e7d1aa8's finding was that they are one +// mistake seen from either side, and that fixing one alone leaves the other live — so a +// validator that stops at the first is the same trap again. +func TestValidateShapeReportsBothPairingDirections(t *testing.T) { + msgs := []bschemas.ChatMessage{ + vcall("head"), // unanswered call + vuser("=== Summary ==="), // + vresult("gone"), // orphaned result + vsys("mid-array system"), // followed by a user turn, not an assistant one + vuser("keep going"), // + } + got := ValidateShape(msgs) + if len(got) != 3 { + t.Fatalf("want all three violations, got [%s]:\n%s", rules(got), FormatShapeViolations(got, msgs)) + } + want := map[string]bool{RuleAnsweredToolUse: true, RulePairedToolResult: true, RuleSystemPosition: true} + for _, v := range got { + delete(want, v.Rule) + } + if len(want) != 0 { + t.Errorf("missing rules %v; got [%s]", want, rules(got)) + } +} + +func TestShapeViolationString(t *testing.T) { + if s := (ShapeViolation{Index: 3, Rule: "r", Msg: "m"}).String(); s != "messages.3 [r] m" { + t.Errorf("indexed violation rendered %q", s) + } + if s := (ShapeViolation{Index: -1, Rule: "r", Msg: "m"}).String(); s != "[r] m" { + t.Errorf("list-level violation rendered %q", s) + } +} From df08daba4cd23d313a72366c2d1b75d4ae136424 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Tue, 1 Sep 2026 10:52:23 +0300 Subject: [PATCH 2/2] fix(schema): gate system-position by dialect, check empty content, and stop the wire overclaim Review of #136 by @OsherElhadad. Four substantive findings, all addressed; the reviewer's own measurements are taken as the record and reconciled with rather than re-argued. DIALECT GATE (the one hard prerequisite). ValidateShape took no provider, and system-position is Anthropic's rule, not a property of the Anthropic-style tool protocol the docstring claimed scope over. OpenAI imposes NO positional constraint on system/developer messages, and /compact defaults to OpenAI (proxy/proxy.go:566) -- so the rule reports a violation on every OpenAI turn where the client re-injects a system message. Zero impact while nothing outside tests calls it, but `schema` is public API with a pkg.go.dev badge, so an external importer hits it unwarned, and wired to the request path it would revert those requests and silently lose their savings: a savings regression dressed as a safety check. ValidateShapeFor(provider, msgs) does the work, gating system-position on provider == Anthropic ValidateShape(msgs) the Anthropic-dialect shorthand, delegating The two PAIRING rules are protocol properties and stay ungated for every provider -- asserted explicitly, because a gate that swept them up with it would trade one wrong answer for a worse one. EMPTY CONTENT is now a rule (non-empty-content), Anthropic-gated. A blank text block is a hard 400, and ~20 SetMessageText call sites can produce one. Worth recording what the mutation actually showed: `summarize` itself is NOT one of them -- it refuses a blank summary (summarize.go:201), and making its model return "" makes it DECLINE rather than emit empty content. So the rule guards the other rewriters (cmdfilter, dedup, skeleton, textclean, ...), which have no such guard, not this component. The rule is deliberately narrow, because the property the reviewer identified as the validator's strongest -- content-shape false positives being IMPOSSIBLE rather than merely absent -- is worth more than the extra coverage a looser rule would buy. MessageText cannot see an image, a thinking block or an Anthropic tool_result payload and calls every one of them 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. THE WIRE OVERCLAIM is closed rather than narrowed, but not where the review suggested, because there it would have been vacuous. normalize() maps a legal wire and one carrying the illegal role="tool" onto the IDENTICAL normalized list, so role legality is a property of the BYTES and is now asserted on the raw body by assertWireRolesLegal. The placement matters: the leak needs TWO components in one turn (summarize to change the count so rebuildCountChanged runs, a second to rewrite a tool message summarize KEPT so it no longer byte-matches its pre-image). Under [summarize] alone every retained tool message still byte-matches and is emitted from its original bytes, so no role can leak and the predicate cannot fail. TestSummarizeEmittedWireIsShapeValid therefore 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. Verified by mutation: reverting 6e503e2 leaves [summarize] passing and fails [summarize, extract_llm] on messages.3 and messages.4 -- which is exactly the reviewer's point that the normalized round-trip destroys the evidence before the validator runs. THE HOT-PATH JUSTIFICATION WAS WRONG and is rewritten to say so. The old comment claimed the walk was "not free enough to spend on every request" and that "a check that can only fail open buys no decision". Measured (the reviewer's numbers, quoted in the doc): 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 us on a 500-message transcript is ~0.007% of a one-second provider call and less than normalize plus the tokenizer already cost per request. And fail-open IS a decision: validate the compacted body, revert on violation, forward that -- the trade this repo makes everywhere else. The real blocker was the dialect gate, which this commit removes. Still NOT wired in here; that is a separate change (a post-pipeline check in apply.Body). DOCS. The 40-line rationale rendered NOWHERE in godoc -- it sat after the imports with a blank line before the next decl, so it attached to nothing on a badged public package. Moved to schema/doc.go as the package comment, which the package previously lacked, and extended with a "NOT CHECKED, and why" list so the deliberate omissions are not mistaken for oversights: consecutive same-role must NOT be checked. summarize's own legal output is [msgs[0], summary(user), user-tail...] -- consecutive USER messages, which Anthropic accepts. An alternation rule would reject the very output this validator exists to bless. There is now a test asserting this, as a guard rail against a future reader "completing" the validator. e9bf3a7's panic no output list exists to inspect. role="tool" a bytes property; checked on the bytes instead. orphaned <> would drag a Store dependency into `schema`. first/trailing role neither is a hard rejection; a trailing assistant turn is prefill, a supported Anthropic feature. docs/design.md's schema/ row gains ValidateShapeFor/ValidateShape, per that doc's own convention of enumerating the package's exports. docs/components/summarize.md gains a "Shape invariants" section: the four 400s this component has produced, the invariants its output is now held to, and the two things that are deliberately NOT invariants. Left alone as the review advises: the near-duplicate `seen`/`seenCall` accumulation in dropOrphanedToolResults. One repairs and 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: revert 6e503e2 (apply.go) FAIL apply assertWireRolesLegal, on messages.3+4 of [summarize, extract_llm]; [summarize] alone still passes, as analyzed drop the dialect gate FAIL schema the OpenAI half of TestValidateShapeForGatesSystemPositionToAnthropic drop the non-empty-content rule FAIL schema TestValidateShapeRejects EmptyContentWithoutFalsePositives ADD an alternation rule FAIL schema 5 tests, including the new consecutive-same-role guard -- which is the demonstration that the rule must stay out Eval box: gofmt clean, go build ./... clean, go test ./... green except TestSpendSurvivesRowEviction in `dash`, which FAILS IDENTICALLY on main e88f5d8 ("month-to-date = 7, want 10") and is untouched by this change -- `dash` does not import `schema`. No production file is touched by this commit. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- apply/shape_validate_test.go | 125 +++++++++++++++++++++++++------- docs/components/summarize.md | 31 ++++++++ docs/design.md | 2 +- schema/doc.go | 76 ++++++++++++++++++++ schema/validate.go | 130 +++++++++++++++++++-------------- schema/validate_test.go | 134 +++++++++++++++++++++++++++++++++++ 6 files changed, 416 insertions(+), 82 deletions(-) create mode 100644 schema/doc.go diff --git a/apply/shape_validate_test.go b/apply/shape_validate_test.go index 7899a1fa..56890761 100644 --- a/apply/shape_validate_test.go +++ b/apply/shape_validate_test.go @@ -3,6 +3,7 @@ package apply import ( "context" "encoding/json" + "fmt" "os" "strings" "testing" @@ -25,8 +26,19 @@ import ( // The list checked is the NORMALIZED view — the shape components mutate, with Anthropic // `tool_use` blocks lifted into ToolCalls and each `tool_result` block its own synthetic // role=tool message. Re-normalizing the EMITTED wire is what makes this an end-to-end check: -// 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 starts from the bytes the proxy would actually send, after apply's rebuild, rather than +// from the in-memory slice a component happened to hand back. +// +// WHAT THE ROUND-TRIP CANNOT SEE, and why the raw-body check below exists. normalize() is +// lossy in exactly the direction that matters for one defect: it maps a legal Anthropic wire +// and a wire carrying the illegal `"role":"tool"` onto the IDENTICAL normalized list, because +// a tool_result block and a leaked role=tool message both become one synthetic role=tool +// message. So the one wire-level defect this repo has actually paid a 400 for +// (`Unexpected role "tool"`, fixed on main, recorded in apply/toolrole_wire_test.go) is +// structurally invisible to ValidateShape here — the round-trip destroys the evidence before +// the validator runs. Roles are therefore asserted on the RAW BODY, before normalization, by +// assertWireRolesLegal. Shape rules go through the validator; byte-level role legality does +// not, and cannot. // shapeModel is a canned summarizer, so these tests assert on SHAPE and never on wording. type shapeModel struct{} @@ -43,6 +55,22 @@ func assertShapeValid(t *testing.T, what string, msgs []bschemas.ChatMessage) { } } +// assertWireRolesLegal checks role legality on the RAW body, which is the only place it is +// decidable: Anthropic accepts exactly "user", "assistant" and "system" in `messages`, and +// normalize() cannot distinguish a legal wire from one carrying this package's internal +// role=tool. Three lines, no walk, and it closes the gap the comment above describes. +func assertWireRolesLegal(t *testing.T, what string, body []byte) { + t.Helper() + for i, m := range gjson.GetBytes(body, "messages").Array() { + switch r := m.Get("role").String(); r { + case "user", "assistant", "system": + default: + t.Errorf("%s: messages.%d role %q reaches the Anthropic wire — a 400 "+ + "(`Unexpected role`), and normalize() would hide it", what, i, r) + } + } +} + // REAL CAPTURED TRAFFIC MUST PASS. This is the guard against the failure mode that would make // the validator worthless: firing on ordinary requests. Both fixtures are real traffic through // the proxy — an Anthropic tool-use exchange and five turns of Claude-Agent-SDK conversation @@ -114,7 +142,20 @@ func TestRealCapturedTrafficIsShapeValid(t *testing.T) { // fb5c460 the tail beginning on a tool_result whose call was cut -> paired-tool-result // e7d1aa8 an assistant tool-call head kept while its results were cut -> answered-tool-use func TestSummarizeEmittedWireIsShapeValid(t *testing.T) { - big := strings.Repeat("verbose parallel tool output line\n", 60) + // INDENTED JSON, so the second pipeline's extract_llm performs a real rewrite of a RETAINED + // tool output. Prose or already-compact content is left alone, which is what made two + // earlier versions of the toolrole test vacuous — see apply/toolrole_wire_test.go. + rec := make([]map[string]any, 0, 40) + for i := 0; i < 40; i++ { + rec = append(rec, 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 parallel tool output ", 6), + }) + } + rb, _ := json.MarshalIndent(rec, "", " ") + big := string(rb) + 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)) @@ -133,32 +174,58 @@ func TestSummarizeEmittedWireIsShapeValid(t *testing.T) { msgs = append(msgs, map[string]any{"role": "user", "content": "final question"}) body, _ := json.Marshal(map[string]any{"model": "claude-x", "messages": msgs}) - acted, sawParallelCall, sawResult := false, false, false - for _, keep := range []int{1, 2, 3, 4, 5} { - cfg, err := config.LoadBytes([]byte("pipeline: [summarize]\ncomponents:\n" + - " summarize: {keep_last: " + string(rune('0'+keep)) + - ", start_from_message: 0, min_tokens: 1}\n")) - if err != nil { - t.Fatal(err) - } - p, _ := cfg.Build(nil) - out, changed := BodyWithModel(context.Background(), p, store.NewMemory(store.Options{}), - bschemas.Anthropic, body, "", false, - components.ModelSpec{Incoming: shapeModel{}}) - if !changed { - continue - } - acted = true - norm, _ := normalize(bschemas.Anthropic, gjson.GetBytes(out, "messages").Array()) - for _, m := range norm { - if a := m.ChatAssistantMessage; a != nil && len(a.ToolCalls) >= 2 { - sawParallelCall = true + // TWO PIPELINES, because the role predicate is only non-vacuous in the second one. The + // role="tool" leak needs TWO components in one turn: summarize to change the message count + // (so rebuildCountChanged runs at all) and a second component to rewrite a tool message + // summarize KEPT (so that message no longer byte-matches its pre-image and gets marshaled + // fresh, internal role and all). With summarize alone every retained tool message still + // byte-matches and is emitted from its original bytes, so no role can leak and the predicate + // proves nothing. Verified by mutation: reverting 6e503e2 leaves [summarize] passing and + // fails [summarize, extract_llm]. + // + // extract_llm with strategy=deterministic keeps this hermetic — a real rewrite of the kept + // message's bytes with no model reply to stub. + pipelines := []struct{ name, yaml string }{ + {"summarize", "pipeline: [summarize]\ncomponents:\n" + + " summarize: {keep_last: %d, start_from_message: 0, min_tokens: 1}\n"}, + {"summarize+extract_llm", "pipeline: [summarize, extract_llm]\ncomponents:\n" + + " summarize: {keep_last: %d, start_from_message: 0, min_tokens: 1}\n" + + " extract_llm: {strategy: deterministic, min_tokens: 1, economic_gate: false, " + + "allow_on_caching_backend: true}\n"}, + } + + acted, sawParallelCall, sawResult, sawRewritingRun := false, false, false, false + for _, pl := range pipelines { + for _, keep := range []int{1, 2, 3, 4, 5} { + what := fmt.Sprintf("%s keep_last=%d", pl.name, keep) + cfg, err := config.LoadBytes([]byte(fmt.Sprintf(pl.yaml, keep))) + if err != nil { + t.Fatal(err) } - if m.Role == bschemas.ChatMessageRoleTool { - sawResult = true + p, _ := cfg.Build(nil) + out, changed := BodyWithModel(context.Background(), p, store.NewMemory(store.Options{}), + bschemas.Anthropic, body, "", false, + components.ModelSpec{Incoming: shapeModel{}}) + if !changed { + continue } + acted = true + assertWireRolesLegal(t, what, out) + arr := gjson.GetBytes(out, "messages").Array() + if len(arr) != len(msgs) && pl.name == "summarize+extract_llm" { + sawRewritingRun = true + } + norm, _ := normalize(bschemas.Anthropic, arr) + for _, m := range norm { + if a := m.ChatAssistantMessage; a != nil && len(a.ToolCalls) >= 2 { + sawParallelCall = true + } + if m.Role == bschemas.ChatMessageRoleTool { + sawResult = true + } + } + assertShapeValid(t, what, norm) } - assertShapeValid(t, "keep_last="+string(rune('0'+keep)), norm) } // Vacuity guards: this test is worthless if summarize never acted, and it does not // exercise the shape it exists for unless a parallel exchange survived onto the wire. @@ -172,4 +239,10 @@ func TestSummarizeEmittedWireIsShapeValid(t *testing.T) { if !sawResult { t.Fatal("no tool_result reached the wire, so paired-tool-result was never exercised") } + // The role predicate is only meaningful over a count-changing run of the two-component + // pipeline; without one it is a check that cannot fail. + if !sawRewritingRun { + t.Fatal("the two-component pipeline never changed the message count, so the " + + "count-change rebuild never ran and assertWireRolesLegal cannot fail") + } } diff --git a/docs/components/summarize.md b/docs/components/summarize.md index f6edd2b0..8f1f53fe 100644 --- a/docs/components/summarize.md +++ b/docs/components/summarize.md @@ -40,6 +40,37 @@ after: [system, "=== History Summary === … … <>", uN-1, Lossy but reversible — the replaced span is stashed under the summary message's marker and recovered via `context_guru_expand` / `GET /expand`. +## Shape invariants + +Rewriting a transcript can make it *unsendable*, and this component has done so four times — +each found only when a provider returned a 400 on live traffic: + +| Symptom | Cause | +|---|---| +| `400 messages.1: role 'system' must precede an 'assistant' message or end the array` | the summary was emitted with role `system` and spliced in front of the kept tail | +| `400 … unexpected tool_use_id found in tool_result blocks` | the span boundary cut a `tool_use` while keeping its `tool_result` | +| `400 … tool_use ids were found without tool_result blocks immediately after` | the mirror: the boundary kept an assistant tool-call turn and cut its results | +| `panic: index out of range [-1]` | a transcript shorter than `keep_last` | + +So the output is held to invariants that are properties of the message list alone, checked +offline by `schema.ValidateShapeFor` (see `components/offload/summarize_shape_test.go` and +`apply/shape_validate_test.go`): + +- a system-role message away from index 0 is followed by an assistant turn, or ends the array + (Anthropic's rule; **not** "system only at index 0" — the Claude Agent SDK legitimately + re-injects a system message every turn); +- every `tool_use` is answered in the contiguous run of tool results that follows it, and every + `tool_result` answers an earlier `tool_use` — so the span boundary never splits an exchange; +- no message reaches the wire with blank content — a hard Anthropic 400. This component + already refuses a blank summary itself; the invariant is what catches a *later* component + in the same pipeline reducing a message `summarize` kept down to nothing. + +Two things are deliberately **not** invariants. **Consecutive same-role messages are legal**: +this component's own correct output is `[msgs[0], summary(user), tail…]`, i.e. consecutive user +messages, and Anthropic accepts it — an alternation rule would reject correct output. And role +legality on the wire (`role:"tool"` never reaching Anthropic) is a property of the *bytes*, not +of the normalized list, so it is asserted on the raw body instead (`apply/toolrole_wire_test.go`). + ## Configuration | Key | Default | Meaning | diff --git a/docs/design.md b/docs/design.md index 85f01251..9297ac16 100644 --- a/docs/design.md +++ b/docs/design.md @@ -15,7 +15,7 @@ infrastructure the components sit on. | `components/offload/` | lossy-reversible components: `skeleton`, `dedup`, `collapse`, `failed_run`, `cmdfilter`, `extract`, `extract_llm`, `smartcrush`, `mask`, `summarize` | | `components/dsl/` | declarative text-filter engine (wrapped by `cmdfilter`) | | `components/all/` | blank-imports every component so `init()` registrations run | -| `schema/` | helpers over bifrost's schema: token counting, deep-clone, `MessageText`/`SetMessageText`, `Rewritable`, `ToolCalls` (pairs a tool result with the call that produced it) | +| `schema/` | helpers over bifrost's schema: token counting, deep-clone, `MessageText`/`SetMessageText`, `Rewritable`, `ToolCalls` (pairs a tool result with the call that produced it), `ValidateShapeFor`/`ValidateShape` (static message-shape validation) | | `apply/` | the one place the pipeline meets a raw wire body: extract `messages` → run → byte-lossless splice | | `expand/` | reversibility: `<>` marker, the `context_guru_expand` tool def, response parsing + continuation | | `store/` | `Store` interface + in-memory TTL+LRU backend (rewind + sticky ids) | diff --git a/schema/doc.go b/schema/doc.go new file mode 100644 index 00000000..f0db8f69 --- /dev/null +++ b/schema/doc.go @@ -0,0 +1,76 @@ +// Package schema holds the helpers this project layers over bifrost's message schema: +// token counting, deep-clone, MessageText/SetMessageText, Rewritable, ToolCalls (which +// pairs a tool result with the call that produced it), SessionHead, and static +// message-SHAPE validation. +// +// # Message-shape validation +// +// ValidateShapeFor checks a message list against the provider's message-SHAPE rules — the +// rules that make a request well-formed regardless of its content. +// +// WHY THIS EXISTS. Four separate shape violations shipped in `summarize` and every one of +// them was found REACTIVELY, by a live provider rejection or a benchmark failure, each +// masked by the one before it: +// +// 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] (a short transcript; see NOT CHECKED below) +// +// None was findable by the project's existing methods. No test asserted the shape of what a +// component emitted, and every offline measurement replayed through the `/compact` endpoint, +// which runs the pipeline and returns the rewritten body WITHOUT forwarding it upstream — so +// no provider ever validated it. Replay can tell you what a component removed; it is +// structurally incapable of telling you whether the result is a sendable request. +// +// The first three are checkable statically, with no provider and no model, because they are +// properties of the message list alone. That is what this is for: a component that mutates a +// message list can be asserted well-formed in a unit test, closing the blind spot without +// paying for live traffic on every change. +// +// WHERE IT RUNS. Unit tests, over the pipeline's NORMALIZED view of a transcript — the shape +// apply.normalize produces and components mutate, where an Anthropic `tool_use` block has +// been lifted into bifrost's ToolCalls and each `tool_result` block is its own synthetic +// role=tool message. +// +// It is deliberately NOT on the request hot path, but NOT because of cost. Measured on the +// eval box: +// +// 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 is ~0.007% of a one-second provider call, and less than +// this package's own tokenizer plus apply.normalize already cost per request. Nor is +// "fail-open" a reason to stay off: fail-open IS a 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 this repo makes everywhere else. +// +// The real blocker is the DIALECT GATE, and it is why ValidateShapeFor takes a provider. +// system-position is an Anthropic rule; OpenAI imposes no positional constraint on +// system/developer messages, and `/compact` defaults to OpenAI (proxy/proxy.go:566). Wired to +// the hot path with that rule ungated, every OpenAI request whose client re-injects a system +// message mid-array would be reverted and its saving silently lost — a savings regression +// dressed as a safety check. With the gate in place the remaining work is a post-pipeline +// check in apply.Body that reverts on violation. That is a separate change. +// +// NOT CHECKED, and why — so nobody "fixes" these: +// +// - CONSECUTIVE SAME-ROLE messages. Must NOT be checked. `summarize`'s own legal output is +// [msgs[0], summary(user), user-tail...] — consecutive user messages. An alternation rule +// would reject the very output this validator exists to bless, and Anthropic accepts it. +// - e9bf3a7's PANIC inside boundary arithmetic. No check on the output list can see it: by +// the time there is a list to inspect the panic has already happened (and +// pipeline.runOne swallowed it into verdict=reverted). Shape validation is not a +// substitute for exercising a component on transcripts shorter than its own thresholds. +// - role="tool" reaching the Anthropic wire. Structurally invisible here, because +// apply.normalize maps a legal wire and one carrying the illegal role onto the IDENTICAL +// normalized list. It is a raw-BYTES property, so it is checked on the bytes, by +// apply/toolrole_wire_test.go and by the role predicate in apply/shape_validate_test.go. +// - ORPHANED `<>` markers. Deciding one needs the expansion Store, and a Store +// dependency does not belong in this package. +// - FIRST message not user, and a TRAILING assistant message. Neither is a hard provider +// rejection, and prefill (a trailing assistant turn) is a supported Anthropic feature. +// - Token limits, model names, sampling parameters, or anything content-dependent. This is +// a shape validator, not a request validator. +package schema diff --git a/schema/validate.go b/schema/validate.go index f6fe329a..96dc18aa 100644 --- a/schema/validate.go +++ b/schema/validate.go @@ -7,46 +7,6 @@ import ( "github.com/maximhq/bifrost/core/schemas" ) -// Static validation of a message list against the provider's message-SHAPE rules — the -// rules that make a request well-formed regardless of its content. -// -// WHY THIS EXISTS. Four separate shape violations shipped in `summarize` and every one of -// them was found REACTIVELY, by a live provider rejection or a benchmark failure, each -// masked by the one before it: -// -// 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] (a short transcript; see NOT COVERED below) -// -// None was findable by the project's existing methods. No test asserted the shape of what a -// component emitted, and every offline measurement replayed through the `/compact` endpoint, -// which runs the pipeline and returns the rewritten body WITHOUT forwarding it upstream — so -// no provider ever validated it. Replay can tell you what a component removed; it is -// structurally incapable of telling you whether the result is a sendable request. -// -// The first three are checkable statically, with no provider and no model, because they are -// properties of the message list alone. That is what this is for: a component that mutates a -// message list can be asserted well-formed in a unit test, closing the blind spot without -// paying for live traffic on every change. -// -// WHERE IT RUNS. Unit tests, over the pipeline's NORMALIZED view of a transcript — the shape -// apply.normalize produces and components mutate, where an Anthropic `tool_use` block has -// been lifted into bifrost's ToolCalls and each `tool_result` block is its own synthetic -// role=tool message. It is deliberately NOT on the request hot path: it walks the whole -// transcript and allocates per tool exchange, which is not free enough to spend on every -// request, and a validator that can only fail open adds latency without adding a decision. -// -// NOT COVERED, and worth being explicit about: e9bf3a7 was a PANIC inside boundary -// arithmetic, not a malformed list. No check on the output list can see it — by the time -// there is a list to inspect, the panic has already happened (and pipeline.runOne swallowed -// it into verdict=reverted). Shape validation is not a substitute for exercising a component -// on transcripts shorter than its own thresholds. -// -// DELIBERATELY NOT A REQUEST VALIDATOR. It checks shape invariants that hold across -// providers with an Anthropic-style tool protocol; it does not check token limits, model -// names, sampling parameters, or anything content-dependent. - // ShapeViolation is one broken invariant, phrased to point at the message that broke it. type ShapeViolation struct { Index int // message index, or -1 when the violation is about the list as a whole @@ -67,38 +27,81 @@ const ( RuleSystemPosition = "system-position" RuleAnsweredToolUse = "answered-tool-use" RulePairedToolResult = "paired-tool-result" + RuleNonEmptyContent = "non-empty-content" ) -// ValidateShape reports every message-shape invariant the list breaks. An empty result -// means the list is well-formed in the ways a provider enforces structurally. +// ValidateShape reports every message-shape invariant the list breaks, judged as an +// ANTHROPIC request. It is the Anthropic-dialect shorthand for ValidateShapeFor; see there +// for the invariants and for why the dialect matters. +func ValidateShape(msgs []schemas.ChatMessage) []ShapeViolation { + return ValidateShapeFor(schemas.Anthropic, msgs) +} + +// ValidateShapeFor reports every message-shape invariant the list breaks when sent to +// `provider`. An empty result means the list is well-formed in the ways that provider +// enforces structurally. +// +// THE PROVIDER ARGUMENT IS NOT DECORATION. Two of the four invariants are properties of the +// Anthropic-style tool protocol and hold for every provider that speaks it; the other two are +// Anthropic's own rules and are simply FALSE elsewhere. OpenAI imposes no positional +// constraint on system/developer messages, so checking system-position against an OpenAI +// transcript reports a violation on every turn where the client re-injects a system message — +// ordinary traffic, and `/compact` defaults to OpenAI (proxy/proxy.go:566). An ungated rule +// would therefore be a savings regression the moment this is used on the request path, which +// is why the gate is a prerequisite for that wiring rather than a nicety. // // The invariants, and why each one exists: // -// 1. system-position — a system-role message away from index 0 must be immediately -// followed by an assistant message, or end the array. This is the provider's own -// wording, and it is the defect that made `summarize` unusable on every call: it +// 1. system-position — ANTHROPIC ONLY. A system-role message away from index 0 must be +// immediately followed by an assistant message, or end the array. This is the provider's +// own wording, and it is the defect that made `summarize` unusable on every call: it // emitted [msgs[0], summary(system), tail...], so with the usual system prompt at // index 0 a second system role landed at index 1 in front of the kept tail. -// 2. answered-tool-use — every `tool_use` must be answered by a `tool_result` in the -// messages immediately following it. Removing a span can delete the answer while -// keeping the call: preserving msgs[0] when it is an assistant tool-call message does -// exactly that. -// 3. paired-tool-result — every `tool_result` must answer a `tool_use` that appeared -// earlier. Removing a span can delete the call while keeping the answer. The mirror of -// (2), and the reason both are checked: e7d1aa8 showed they are one mistake seen from -// either side, and fixing one alone leaves the other live — which is what happened. -func ValidateShape(msgs []schemas.ChatMessage) []ShapeViolation { +// 2. answered-tool-use — ALL PROVIDERS. Every `tool_use` must be answered by a +// `tool_result` in the messages immediately following it. Removing a span can delete the +// answer while keeping the call: preserving msgs[0] when it is an assistant tool-call +// message does exactly that. +// 3. paired-tool-result — ALL PROVIDERS. Every `tool_result` must answer a `tool_use` that +// appeared earlier. Removing a span can delete the call while keeping the answer. The +// mirror of (2), and the reason both are checked: e7d1aa8 showed they are one mistake +// seen from either side, and fixing one alone leaves the other live — which is what +// happened. +// 4. non-empty-content — ANTHROPIC ONLY, on the evidence available. A message that CARRIES +// content whose text is blank is a hard 400 ("text content blocks must be non-empty"), +// and this pipeline has ~20 places that can produce one: every component that rewrites a +// message does it through SetMessageText, and any of them reducing a message to the empty +// string writes `""` straight onto the wire. `summarize` itself is NOT one of them — it +// refuses a blank summary (components/offload/summarize.go:201), confirmed by mutation: +// making its model return "" makes it decline rather than emit empty content. So this rule +// guards the OTHER rewriters (cmdfilter, dedup, skeleton, textclean and the rest), which +// have no such guard. Gated to Anthropic because an Anthropic rejection is what is on +// record; widen it when another provider's 400 is. +// +// The rule is deliberately narrow in three ways, to preserve the property that makes this +// validator usable at all — content-shape false positives being impossible rather than +// merely absent. It fires only when Content is non-nil (a pure tool-call assistant message +// has nil content and is legal), only when the message carries no non-text block (Rewritable, +// so an image-only, thinking-only or tool_result-payload message is never judged by its text), +// and never on a role=tool message or an assistant message carrying ToolCalls, whose meaning +// lives in fields other than their text. +func ValidateShapeFor(provider schemas.ModelProvider, msgs []schemas.ChatMessage) []ShapeViolation { var out []ShapeViolation seenCall := map[string]bool{} for i := range msgs { m := msgs[i] - if m.Role == schemas.ChatMessageRoleSystem && i != 0 && !systemPositionOK(msgs, i) { + if provider == schemas.Anthropic && + m.Role == schemas.ChatMessageRoleSystem && i != 0 && !systemPositionOK(msgs, i) { out = append(out, ShapeViolation{Index: i, Rule: RuleSystemPosition, Msg: "role 'system' must precede an 'assistant' message or end the array"}) } + if provider == schemas.Anthropic && contentPresentButBlank(m) { + out = append(out, ShapeViolation{Index: i, Rule: RuleNonEmptyContent, + Msg: "text content blocks must be non-empty"}) + } + // A result must answer a call seen earlier. if m.Role == schemas.ChatMessageRoleTool && m.ChatToolMessage != nil && m.ChatToolMessage.ToolCallID != nil && *m.ChatToolMessage.ToolCallID != "" { @@ -130,6 +133,23 @@ func ValidateShape(msgs []schemas.ChatMessage) []ShapeViolation { return out } +// contentPresentButBlank reports whether m has content that a provider will read as empty. +// +// The guards are the whole point (see ValidateShapeFor rule 4): a message with a non-text +// block is never judged by MessageText, because MessageText cannot see an image, a thinking +// block or an Anthropic tool_result payload and would call every one of them blank. Restricting +// this to Rewritable messages keeps the "content-shape false positives are impossible" property +// intact — this rule cannot fire on any shape it does not fully understand. +func contentPresentButBlank(m schemas.ChatMessage) bool { + if m.Content == nil || m.Role == schemas.ChatMessageRoleTool { + return false + } + if a := m.ChatAssistantMessage; a != nil && len(a.ToolCalls) > 0 { + return false + } + return Rewritable(m) && strings.TrimSpace(MessageText(m)) == "" +} + // systemPositionOK applies the provider's rule for a system role inside `messages`, which is // narrower than "index 0 only" in one direction and wider in another. // diff --git a/schema/validate_test.go b/schema/validate_test.go index 86dd1699..2e00d672 100644 --- a/schema/validate_test.go +++ b/schema/validate_test.go @@ -210,3 +210,137 @@ func TestShapeViolationString(t *testing.T) { t.Errorf("list-level violation rendered %q", s) } } + +// THE DIALECT GATE. system-position is Anthropic's rule and is FALSE for OpenAI, which +// imposes no positional constraint on system/developer messages. The transcript below is the +// exact shape a Claude-Agent-SDK-style client produces when it re-injects a system message +// mid-array ahead of a user turn — ordinary traffic, and `/compact` defaults to OpenAI +// (proxy/proxy.go:566). Ungated, this rule would report a violation on every such turn, and +// on the request path that means reverting the request and silently losing its saving. +// +// The two pairing rules are protocol properties, not dialect ones, so they must keep firing +// for BOTH providers — asserted here, because a gate that swept them up with it would trade +// one wrong answer for a worse one. +func TestValidateShapeForGatesSystemPositionToAnthropic(t *testing.T) { + systemMidArray := []bschemas.ChatMessage{ + vsys("you are a helpful assistant"), + vuser("start"), + vasst("working"), + vsys("1200 tokens left"), + vuser("continue"), + } + + if got := rules(ValidateShapeFor(bschemas.Anthropic, systemMidArray)); got != RuleSystemPosition { + t.Errorf("Anthropic: want %q, got %q", RuleSystemPosition, got) + } + if vs := ValidateShapeFor(bschemas.OpenAI, systemMidArray); len(vs) != 0 { + t.Errorf("OpenAI imposes no positional constraint on system messages, but got: %s", + FormatShapeViolations(vs, systemMidArray)) + } + + // ValidateShape is the Anthropic shorthand, so it must agree with the Anthropic call. + if got := rules(ValidateShape(systemMidArray)); got != RuleSystemPosition { + t.Errorf("ValidateShape must delegate as Anthropic: want %q, got %q", RuleSystemPosition, got) + } + + // The pairing rules are provider-independent and must survive the gate. + pairing := []bschemas.ChatMessage{vuser("go"), vcall("call_a"), vuser("thanks")} + for _, p := range []bschemas.ModelProvider{bschemas.Anthropic, bschemas.OpenAI} { + if got := rules(ValidateShapeFor(p, pairing)); got != RuleAnsweredToolUse { + t.Errorf("%s: pairing rules must not be gated: want %q, got %q", + p, RuleAnsweredToolUse, got) + } + } +} + +// EMPTY CONTENT. Anthropic rejects a blank text block, and this pipeline can produce one: +// every rewriting component goes through SetMessageText, so a summarizer or extractor that +// returns "" writes it straight onto the wire. +// +// The acceptance half is the important half. MessageText cannot see an image, a thinking +// block or an Anthropic tool_result payload and reports every one of them as blank, so an +// unguarded version of this rule would fire on legal traffic — the exact failure mode that +// made the first system-position rule worthless. The guards (Rewritable, nil content, tool +// role, ToolCalls) are what keep "content-shape false positives are impossible" true. +func TestValidateShapeRejectsEmptyContentWithoutFalsePositives(t *testing.T) { + blank := func(role bschemas.ChatMessageRole, text string) bschemas.ChatMessage { + m := bschemas.ChatMessage{Role: role} + SetMessageText(&m, text) + return m + } + nonText := func(bt bschemas.ChatContentBlockType) bschemas.ChatMessage { + return bschemas.ChatMessage{Role: bschemas.ChatMessageRoleUser, + Content: &bschemas.ChatMessageContent{ + ContentBlocks: []bschemas.ChatContentBlock{{Type: bt}}}} + } + + reject := []struct { + name string + msgs []bschemas.ChatMessage + }{ + {"summary rewritten to the empty string", []bschemas.ChatMessage{ + vuser("start"), blank(bschemas.ChatMessageRoleUser, "")}}, + {"whitespace only is still empty to the provider", []bschemas.ChatMessage{ + vuser("start"), blank(bschemas.ChatMessageRoleUser, " \n\t ")}}, + {"assistant turn with no text and no tool calls", []bschemas.ChatMessage{ + vuser("start"), blank(bschemas.ChatMessageRoleAssistant, "")}}, + {"empty content block array", []bschemas.ChatMessage{vuser("start"), + {Role: bschemas.ChatMessageRoleUser, Content: &bschemas.ChatMessageContent{}}}}, + } + for _, tc := range reject { + t.Run("reject/"+tc.name, func(t *testing.T) { + if got := rules(ValidateShapeFor(bschemas.Anthropic, tc.msgs)); got != RuleNonEmptyContent { + t.Errorf("want %q, got %q", RuleNonEmptyContent, got) + } + }) + } + + accept := []struct { + name string + msgs []bschemas.ChatMessage + }{ + {"nil content on a pure tool-call assistant turn", []bschemas.ChatMessage{ + vuser("go"), vcall("call_a"), vresult("call_a")}}, + // Deliberately exempt rather than asserted legal: an assistant turn whose text is + // blank but which carries tool calls means something through the calls, and this rule + // fails open on shapes whose meaning it does not fully read. + {"blank text alongside tool calls is exempt", func() []bschemas.ChatMessage { + c := vcall("call_a") + SetMessageText(&c, "") + return []bschemas.ChatMessage{vuser("go"), c, vresult("call_a")} + }()}, + {"image-only message: MessageText is blank, the content is not", []bschemas.ChatMessage{ + vuser("look"), nonText(bschemas.ChatContentBlockTypeImage)}}, + {"thinking-only assistant turn", []bschemas.ChatMessage{vuser("go"), + {Role: bschemas.ChatMessageRoleAssistant, + Content: &bschemas.ChatMessageContent{ContentBlocks: []bschemas.ChatContentBlock{ + {Type: bschemas.ChatContentBlockType("thinking")}}}}}}, + {"tool_result payload lives outside MessageText", []bschemas.ChatMessage{ + vuser("look"), nonText(bschemas.ChatContentBlockType("tool_result"))}}, + } + for _, tc := range accept { + t.Run("accept/"+tc.name, func(t *testing.T) { + if vs := ValidateShapeFor(bschemas.Anthropic, tc.msgs); len(vs) != 0 { + t.Errorf("false positive: %s", FormatShapeViolations(vs, tc.msgs)) + } + }) + } +} + +// CONSECUTIVE SAME-ROLE MUST STAY UNCHECKED, and this test is the guard rail against a future +// reader "completing" the validator with an alternation rule. `summarize`'s own legal output +// is [msgs[0], summary(user), user-tail...] — consecutive user messages, which Anthropic +// accepts. An alternation rule would reject the very output this validator exists to bless. +func TestValidateShapeAcceptsConsecutiveSameRoleBecauseSummarizeEmitsIt(t *testing.T) { + summarizeOutput := []bschemas.ChatMessage{ + vuser("original first turn"), + vuser("[summary] essential facts from the earlier trajectory"), + vuser("the kept tail's first user turn"), + } + for _, p := range []bschemas.ModelProvider{bschemas.Anthropic, bschemas.OpenAI} { + if vs := ValidateShapeFor(p, summarizeOutput); len(vs) != 0 { + t.Errorf("%s: consecutive same-role must be accepted — it is summarize's own "+ + "legal output:\n%s", p, FormatShapeViolations(vs, summarizeOutput)) + } + } +}