diff --git a/apply/shape_validate_test.go b/apply/shape_validate_test.go new file mode 100644 index 00000000..56890761 --- /dev/null +++ b/apply/shape_validate_test.go @@ -0,0 +1,248 @@ +package apply + +import ( + "context" + "encoding/json" + "fmt" + "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 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{} + +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)) + } +} + +// 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 +// 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) { + // 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)) + 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}) + + // 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) + } + 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) + } + } + // 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") + } + // 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/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/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 new file mode 100644 index 00000000..96dc18aa --- /dev/null +++ b/schema/validate.go @@ -0,0 +1,221 @@ +package schema + +import ( + "fmt" + "strings" + + "github.com/maximhq/bifrost/core/schemas" +) + +// 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" + RuleNonEmptyContent = "non-empty-content" +) + +// 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 — 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 — 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 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 != "" { + 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 +} + +// 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. +// +// "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..2e00d672 --- /dev/null +++ b/schema/validate_test.go @@ -0,0 +1,346 @@ +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) + } +} + +// 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)) + } + } +}