From d88bcb6070fad5ab8d18ad25565db015189ab626 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 18 Jul 2026 23:44:27 +0000 Subject: [PATCH] Rewrite brittle validation and schema tests --- internal/core/source/source_test.go | 240 +++++++----------- .../framework/llm/schema_registry_test.go | 4 - .../modules/dnd/chunk/scenes/schema_test.go | 161 ++++++++---- .../modules/dnd/extract/spells/schema_test.go | 64 ++++- 4 files changed, 248 insertions(+), 221 deletions(-) diff --git a/internal/core/source/source_test.go b/internal/core/source/source_test.go index 0434826..a58167c 100644 --- a/internal/core/source/source_test.go +++ b/internal/core/source/source_test.go @@ -17,44 +17,39 @@ func TestValidateDocumentValid(t *testing.T) { func TestValidateDocumentNil(t *testing.T) { err := ValidateDocument(nil) - if err == nil { - t.Fatal("ValidateDocument() error = nil, want error") - } - if err.Error() != "source document must not be nil" { - t.Fatalf("ValidateDocument() error = %q", err.Error()) - } + requireErrorFragments(t, err, "source document", "nil") } func TestValidateDocumentMissingFields(t *testing.T) { tests := []struct { - name string - mutate func(*SourceDocument) - wantErr string + name string + mutate func(*SourceDocument) + fragments []string }{ { - name: "id", - mutate: func(doc *SourceDocument) { doc.ID = " \t" }, - wantErr: "source document id must not be empty", + name: "id", + mutate: func(doc *SourceDocument) { doc.ID = " \t" }, + fragments: []string{"source document id", "must not be empty"}, }, { - name: "id surrounding whitespace", - mutate: func(doc *SourceDocument) { doc.ID = " source-1 " }, - wantErr: "source document id \" source-1 \" must not contain leading or trailing whitespace", + name: "id surrounding whitespace", + mutate: func(doc *SourceDocument) { doc.ID = " source-1 " }, + fragments: []string{"source document id", "leading or trailing whitespace"}, }, { - name: "kind", - mutate: func(doc *SourceDocument) { doc.Kind = "" }, - wantErr: "source document kind must not be empty", + name: "kind", + mutate: func(doc *SourceDocument) { doc.Kind = "" }, + fragments: []string{"source document kind", "must not be empty"}, }, { - name: "format", - mutate: func(doc *SourceDocument) { doc.Format = "\n" }, - wantErr: "source document format must not be empty", + name: "format", + mutate: func(doc *SourceDocument) { doc.Format = "\n" }, + fragments: []string{"source document format", "must not be empty"}, }, { - name: "digest", - mutate: func(doc *SourceDocument) { doc.Digest = "" }, - wantErr: "source document digest must not be empty", + name: "digest", + mutate: func(doc *SourceDocument) { doc.Digest = "" }, + fragments: []string{"source document digest", "must not be empty"}, }, } @@ -65,12 +60,7 @@ func TestValidateDocumentMissingFields(t *testing.T) { err := ValidateDocument(doc) - if err == nil { - t.Fatal("ValidateDocument() error = nil, want error") - } - if err.Error() != tt.wantErr { - t.Fatalf("ValidateDocument() error = %q, want %q", err.Error(), tt.wantErr) - } + requireErrorFragments(t, err, tt.fragments...) }) } } @@ -81,34 +71,29 @@ func TestValidateDocumentEmptyUnits(t *testing.T) { err := ValidateDocument(doc) - if err == nil { - t.Fatal("ValidateDocument() error = nil, want error") - } - if err.Error() != "source document units must not be empty" { - t.Fatalf("ValidateDocument() error = %q", err.Error()) - } + requireErrorFragments(t, err, "source document units", "must not be empty") } func TestValidateDocumentMissingUnitFields(t *testing.T) { tests := []struct { - name string - mutate func(*SourceDocument) - wantErr string + name string + mutate func(*SourceDocument) + fragments []string }{ { - name: "id", - mutate: func(doc *SourceDocument) { doc.Units[1].ID = 0 }, - wantErr: "source unit[1].id must be positive", + name: "id", + mutate: func(doc *SourceDocument) { doc.Units[1].ID = 0 }, + fragments: []string{"source unit[1].id", "must be positive"}, }, { - name: "kind", - mutate: func(doc *SourceDocument) { doc.Units[1].Kind = " " }, - wantErr: "source unit[1].kind must not be empty", + name: "kind", + mutate: func(doc *SourceDocument) { doc.Units[1].Kind = " " }, + fragments: []string{"source unit[1].kind", "must not be empty"}, }, { - name: "text", - mutate: func(doc *SourceDocument) { doc.Units[1].Text = "\n\t" }, - wantErr: "source unit[1].text must not be empty", + name: "text", + mutate: func(doc *SourceDocument) { doc.Units[1].Text = "\n\t" }, + fragments: []string{"source unit[1].text", "must not be empty"}, }, } @@ -119,12 +104,7 @@ func TestValidateDocumentMissingUnitFields(t *testing.T) { err := ValidateDocument(doc) - if err == nil { - t.Fatal("ValidateDocument() error = nil, want error") - } - if err.Error() != tt.wantErr { - t.Fatalf("ValidateDocument() error = %q, want %q", err.Error(), tt.wantErr) - } + requireErrorFragments(t, err, tt.fragments...) }) } } @@ -135,45 +115,27 @@ func TestValidateDocumentDuplicateUnitIDs(t *testing.T) { err := ValidateDocument(doc) - if err == nil { - t.Fatal("ValidateDocument() error = nil, want error") - } - if err.Error() != "source unit id 1 is duplicated" { - t.Fatalf("ValidateDocument() error = %q", err.Error()) - } + requireErrorFragments(t, err, "source unit id 1", "duplicated") } func TestValidateDocumentUnitReferences(t *testing.T) { tests := []struct { - name string - mutate func(*SourceDocument) - wantErr string + name string + mutate func(*SourceDocument) + fragments []string }{ { - name: "missing", - mutate: func(doc *SourceDocument) { doc.Units[0].Ref = SourceRef{} }, - wantErr: "source unit[0].ref: source ref source_id must not be empty", + name: "nested reference context", + mutate: func(doc *SourceDocument) { doc.Units[0].Ref.SourceID = "source-2" }, + fragments: []string{"source unit[0].ref", "source_id", "does not match"}, }, { - name: "foreign source", - mutate: func(doc *SourceDocument) { doc.Units[0].Ref.SourceID = "source-2" }, - wantErr: "source unit[0].ref: source ref source_id \"source-2\" does not match document id \"source-1\"", - }, - { - name: "non-self range", + name: "document unit self-reference", mutate: func(doc *SourceDocument) { doc.Units[0].Ref.StartUnitID = 2 doc.Units[0].Ref.EndUnitID = 2 }, - wantErr: "source unit[0].ref must identify source unit id 1", - }, - { - name: "reversed range", - mutate: func(doc *SourceDocument) { - doc.Units[0].Ref.StartUnitID = 2 - doc.Units[0].Ref.EndUnitID = 1 - }, - wantErr: "source unit[0].ref: source ref start_unit_id 2 appears after end_unit_id 1", + fragments: []string{"source unit[0].ref", "must identify source unit id 1"}, }, } @@ -183,12 +145,7 @@ func TestValidateDocumentUnitReferences(t *testing.T) { tt.mutate(doc) err := ValidateDocument(doc) - if err == nil { - t.Fatal("ValidateDocument() error = nil, want unit reference error") - } - if err.Error() != tt.wantErr { - t.Fatalf("ValidateDocument() error = %q, want %q", err.Error(), tt.wantErr) - } + requireErrorFragments(t, err, tt.fragments...) }) } } @@ -294,59 +251,51 @@ func TestValidateRefValid(t *testing.T) { } } -func TestValidateRefSourceIDMismatch(t *testing.T) { - doc := validDocument() - ref := SourceRef{ - SourceID: "source-2", - StartUnitID: 1, - EndUnitID: 2, - } - - err := ValidateRef(doc, ref) - - if err == nil { - t.Fatal("ValidateRef() error = nil, want error") - } - if err.Error() != "source ref source_id \"source-2\" does not match document id \"source-1\"" { - t.Fatalf("ValidateRef() error = %q", err.Error()) - } -} - -func TestValidateRefMissingUnitIDs(t *testing.T) { +func TestValidateRefRejectsMalformedReferences(t *testing.T) { tests := []struct { - name string - ref SourceRef - wantErr string + name string + ref SourceRef + fragments []string }{ { - name: "missing source id", - ref: SourceRef{StartUnitID: 1, EndUnitID: 2}, - wantErr: "source ref source_id must not be empty", + name: "missing source id", + ref: SourceRef{StartUnitID: 1, EndUnitID: 2}, + fragments: []string{"source_id", "must not be empty"}, }, { - name: "source id surrounding whitespace", - ref: SourceRef{SourceID: " source-1 ", StartUnitID: 1, EndUnitID: 2}, - wantErr: "source ref source_id \" source-1 \" must not contain leading or trailing whitespace", + name: "source id surrounding whitespace", + ref: SourceRef{SourceID: " source-1 ", StartUnitID: 1, EndUnitID: 2}, + fragments: []string{"source_id", "leading or trailing whitespace"}, }, { - name: "missing start id", - ref: SourceRef{SourceID: "source-1", EndUnitID: 2}, - wantErr: "source ref start_unit_id must be positive", + name: "missing start id", + ref: SourceRef{SourceID: "source-1", EndUnitID: 2}, + fragments: []string{"start_unit_id", "must be positive"}, }, { - name: "missing end id", - ref: SourceRef{SourceID: "source-1", StartUnitID: 1}, - wantErr: "source ref end_unit_id must be positive", + name: "missing end id", + ref: SourceRef{SourceID: "source-1", StartUnitID: 1}, + fragments: []string{"end_unit_id", "must be positive"}, }, { - name: "unknown start id", - ref: SourceRef{SourceID: "source-1", StartUnitID: 9, EndUnitID: 2}, - wantErr: "source ref start_unit_id 9 was not found", + name: "unknown start id", + ref: SourceRef{SourceID: "source-1", StartUnitID: 9, EndUnitID: 2}, + fragments: []string{"start_unit_id", "was not found"}, }, { - name: "unknown end id", - ref: SourceRef{SourceID: "source-1", StartUnitID: 1, EndUnitID: 9}, - wantErr: "source ref end_unit_id 9 was not found", + name: "unknown end id", + ref: SourceRef{SourceID: "source-1", StartUnitID: 1, EndUnitID: 9}, + fragments: []string{"end_unit_id", "was not found"}, + }, + { + name: "source id mismatch", + ref: SourceRef{SourceID: "source-2", StartUnitID: 1, EndUnitID: 2}, + fragments: []string{"source_id", "does not match"}, + }, + { + name: "reversed unit order", + ref: SourceRef{SourceID: "source-1", StartUnitID: 2, EndUnitID: 1}, + fragments: []string{"start_unit_id", "appears after"}, }, } @@ -354,34 +303,11 @@ func TestValidateRefMissingUnitIDs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := ValidateRef(validDocument(), tt.ref) - if err == nil { - t.Fatal("ValidateRef() error = nil, want error") - } - if err.Error() != tt.wantErr { - t.Fatalf("ValidateRef() error = %q, want %q", err.Error(), tt.wantErr) - } + requireErrorFragments(t, err, tt.fragments...) }) } } -func TestValidateRefReversedUnitOrder(t *testing.T) { - doc := validDocument() - ref := SourceRef{ - SourceID: "source-1", - StartUnitID: 2, - EndUnitID: 1, - } - - err := ValidateRef(doc, ref) - - if err == nil { - t.Fatal("ValidateRef() error = nil, want error") - } - if !strings.Contains(err.Error(), "appears after") { - t.Fatalf("ValidateRef() error = %q, want reversed order error", err.Error()) - } -} - func TestUnitIndex(t *testing.T) { doc := validDocument() @@ -424,3 +350,15 @@ func validDocument() *SourceDocument { }, } } + +func requireErrorFragments(t *testing.T, err error, fragments ...string) { + t.Helper() + if err == nil { + t.Fatalf("error = nil, want fragments %q", fragments) + } + for _, fragment := range fragments { + if !strings.Contains(err.Error(), fragment) { + t.Fatalf("error = %q, want fragment %q", err.Error(), fragment) + } + } +} diff --git a/internal/framework/llm/schema_registry_test.go b/internal/framework/llm/schema_registry_test.go index 076b8bc..f647fd4 100644 --- a/internal/framework/llm/schema_registry_test.go +++ b/internal/framework/llm/schema_registry_test.go @@ -56,10 +56,6 @@ func TestMustLookupResponseSchemaPanicsForUnknownKey(t *testing.T) { func TestRegisteredResponseSchemasSortedByKey(t *testing.T) { schemas := RegisteredResponseSchemas() - if len(schemas) != 2 { - t.Fatalf("expected two schemas, got %d", len(schemas)) - } - keys := make([]string, len(schemas)) seen := make(map[ResponseSchemaKey]bool, len(schemas)) for i, schema := range schemas { diff --git a/internal/modules/dnd/chunk/scenes/schema_test.go b/internal/modules/dnd/chunk/scenes/schema_test.go index 5cd9cd1..d23f4f0 100644 --- a/internal/modules/dnd/chunk/scenes/schema_test.go +++ b/internal/modules/dnd/chunk/scenes/schema_test.go @@ -1,9 +1,13 @@ package scenes import ( + "bytes" "encoding/json" + "fmt" "strings" "testing" + + "github.com/santhosh-tekuri/jsonschema/v6" ) func TestLoadResponseSchemaForScenes(t *testing.T) { @@ -31,62 +35,83 @@ func TestLoadResponseSchemaForScenes(t *testing.T) { } } -func TestResponseSchemaShapeUsesSourceUnitBoundaries(t *testing.T) { +func TestResponseSchemaValidatesSceneResponses(t *testing.T) { schema, err := loadResponseSchema() if err != nil { t.Fatalf("loadResponseSchema() error = %v, want nil", err) } - var decoded map[string]any - if err := json.Unmarshal(schema.JSONSchema, &decoded); err != nil { - t.Fatalf("Unmarshal() error = %v, want nil", err) + valid := validSceneSchemaResponse() + validJSON, err := json.Marshal(valid) + if err != nil { + t.Fatalf("Marshal(validSceneSchemaResponse()) error = %v, want nil", err) } - if decoded["$id"] != ResponseSchemaID { - t.Fatalf("$id = %#v, want %q", decoded["$id"], ResponseSchemaID) - } - if decoded["additionalProperties"] != false { - t.Fatalf("additionalProperties = %#v, want false", decoded["additionalProperties"]) + if err := validateJSONSchema(validJSON, schema.JSONSchema); err != nil { + t.Fatalf("valid scene response rejected: %v", err) } - properties := decoded["properties"].(map[string]any) - if _, ok := properties["artifact_type"]; ok { - t.Fatal("schema includes artifact_type, want only scene response fields") - } - if _, ok := properties["session_scope"]; ok { - t.Fatal("schema includes session_scope, want no session wrapper") + tests := []struct { + name string + mutate func(map[string]any) + }{ + { + name: "obsolete segment boundaries", + mutate: func(response map[string]any) { + scene := response["scenes"].([]any)[0].(map[string]any) + scene["start_segment_id"] = 1 + scene["end_segment_id"] = 2 + }, + }, + { + name: "non-positive start unit", + mutate: func(response map[string]any) { + response["scenes"].([]any)[0].(map[string]any)["start_unit_id"] = 0 + }, + }, + { + name: "non-positive end unit", + mutate: func(response map[string]any) { + response["scenes"].([]any)[0].(map[string]any)["end_unit_id"] = 0 + }, + }, + { + name: "invalid primary mode", + mutate: func(response map[string]any) { + response["scenes"].([]any)[0].(map[string]any)["primary_mode"] = "Unknown" + }, + }, + { + name: "invalid boundary confidence", + mutate: func(response map[string]any) { + response["scenes"].([]any)[0].(map[string]any)["boundary_confidence"] = "Unknown" + }, + }, + { + name: "empty boundary caveat", + mutate: func(response map[string]any) { + response["boundary_caveats"] = []any{""} + }, + }, + { + name: "unknown property", + mutate: func(response map[string]any) { + response["unexpected"] = true + }, + }, } - sceneProperties := properties["scenes"].(map[string]any)["items"].(map[string]any)["properties"].(map[string]any) - for _, field := range []string{"scene_id", "start_segment_id", "end_segment_id"} { - if _, ok := sceneProperties[field]; ok { - t.Fatalf("scene schema includes old field %q", field) - } - } - for _, field := range []string{"start_unit_id", "end_unit_id"} { - property := sceneProperties[field].(map[string]any) - if property["type"] != "integer" { - t.Fatalf("%s type = %#v, want integer", field, property["type"]) - } - if property["minimum"] != float64(1) { - t.Fatalf("%s minimum = %#v, want 1", field, property["minimum"]) - } - } - - modeEnum := sceneProperties["primary_mode"].(map[string]any)["enum"].([]any) - if !sameStrings(modeEnum, []string{"Recap", "Discussion", "Combat", "Narrative"}) { - t.Fatalf("primary_mode enum = %#v, want Recap/Discussion/Combat/Narrative", modeEnum) - } - confidenceEnum := sceneProperties["boundary_confidence"].(map[string]any)["enum"].([]any) - if !sameStrings(confidenceEnum, []string{"High", "Medium", "Low"}) { - t.Fatalf("boundary_confidence enum = %#v, want High/Medium/Low", confidenceEnum) - } - - boundaryCaveatItems := decoded["properties"].(map[string]any)["boundary_caveats"].(map[string]any)["items"].(map[string]any) - if boundaryCaveatItems["type"] != "string" { - t.Fatalf("boundary_caveats.items.type = %#v, want string", boundaryCaveatItems["type"]) - } - if boundaryCaveatItems["minLength"] != float64(1) { - t.Fatalf("boundary_caveats.items.minLength = %#v, want 1", boundaryCaveatItems["minLength"]) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + response := validSceneSchemaResponse() + tt.mutate(response) + content, err := json.Marshal(response) + if err != nil { + t.Fatalf("Marshal() error = %v, want nil", err) + } + if err := validateJSONSchema(content, schema.JSONSchema); err == nil { + t.Fatal("validateJSONSchema() error = nil, want rejected response") + } + }) } } @@ -138,14 +163,40 @@ func TestResponseSchemaJSONIsMutationSafe(t *testing.T) { } } -func sameStrings(got []any, want []string) bool { - if len(got) != len(want) { - return false +func validSceneSchemaResponse() map[string]any { + return map[string]any{ + "scenes": []any{ + map[string]any{ + "start_unit_id": 1, + "end_unit_id": 3, + "short_title": "Ambush", + "primary_mode": "Combat", + "main_participants": []any{"Aria"}, + "summary": "The party fights.", + "boundary_note": "Combat starts and resolves.", + "boundary_confidence": "High", + }, + }, + "boundary_caveats": []any{}, } - for i := range want { - if got[i] != want[i] { - return false - } - } - return true +} + +func validateJSONSchema(instanceContent, schemaContent []byte) error { + instance, err := jsonschema.UnmarshalJSON(bytes.NewReader(instanceContent)) + if err != nil { + return fmt.Errorf("parse instance: %w", err) + } + schemaDocument, err := jsonschema.UnmarshalJSON(bytes.NewReader(schemaContent)) + if err != nil { + return fmt.Errorf("parse schema: %w", err) + } + compiler := jsonschema.NewCompiler() + if err := compiler.AddResource("schema.json", schemaDocument); err != nil { + return fmt.Errorf("load schema: %w", err) + } + schema, err := compiler.Compile("schema.json") + if err != nil { + return fmt.Errorf("compile schema: %w", err) + } + return schema.Validate(instance) } diff --git a/internal/modules/dnd/extract/spells/schema_test.go b/internal/modules/dnd/extract/spells/schema_test.go index 4e6e36c..98589cc 100644 --- a/internal/modules/dnd/extract/spells/schema_test.go +++ b/internal/modules/dnd/extract/spells/schema_test.go @@ -5,6 +5,8 @@ import ( "encoding/json" "strings" "testing" + + "github.com/santhosh-tekuri/jsonschema/v6" ) func TestLoadResponseSchemaUsesExtractorOwnedLLMSchema(t *testing.T) { @@ -19,19 +21,23 @@ func TestLoadResponseSchemaUsesExtractorOwnedLLMSchema(t *testing.T) { t.Fatalf("schema metadata = %#v, want valid hashed JSON", schema) } - var decoded map[string]any - if err := json.Unmarshal(schema.JSONSchema, &decoded); err != nil { - t.Fatalf("Unmarshal(schema.JSONSchema) error = %v", err) + valid := validSpellsResponse() + validJSON, err := json.Marshal(valid) + if err != nil { + t.Fatalf("Marshal(validSpellsResponse()) error = %v, want nil", err) } - if decoded["$id"] != "notarius.dnd.spells.llm" { - t.Fatalf("LLM schema $id = %#v, want extractor transport schema", decoded["$id"]) + if err := validateJSONSchema(validJSON, schema.JSONSchema); err != nil { + t.Fatalf("valid private spells response rejected: %v", err) } - properties := decoded["properties"].(map[string]any) - spellCastProperties := properties["spell_casts"].(map[string]any)["items"].(map[string]any)["properties"].(map[string]any) - sourceRefItems := spellCastProperties["source_refs"].(map[string]any)["items"].(map[string]any) - sourceRefProperties := sourceRefItems["properties"].(map[string]any) - if _, ok := sourceRefProperties["source_id"]; ok { - t.Fatalf("LLM source ref schema contains canonical source_id: %#v", sourceRefProperties) + + withCanonicalSourceID := validSpellsResponse() + withCanonicalSourceID["spell_casts"].([]any)[0].(map[string]any)["source_refs"].([]any)[0].(map[string]any)["source_id"] = "session-alpha" + content, err := json.Marshal(withCanonicalSourceID) + if err != nil { + t.Fatalf("Marshal(response with source_id) error = %v, want nil", err) + } + if err := validateJSONSchema(content, schema.JSONSchema); err == nil { + t.Fatal("validateJSONSchema() error = nil, want canonical source_id rejected") } } @@ -60,3 +66,39 @@ func TestResponseSchemaDiagnosticsOmitRawSchema(t *testing.T) { t.Fatalf("diagnostics include raw schema: %#v", diagnostics) } } + +func validSpellsResponse() map[string]any { + return map[string]any{ + "spell_casts": []any{ + map[string]any{ + "caster": "Aria", + "spell": "Cure Wounds", + "effect": "The wounds close.", + "narrative_description": "Aria casts Cure Wounds.", + "source_refs": []any{ + map[string]any{"start_unit_id": 1, "end_unit_id": 2}, + }, + }, + }, + } +} + +func validateJSONSchema(instanceContent, schemaContent []byte) error { + instance, err := jsonschema.UnmarshalJSON(bytes.NewReader(instanceContent)) + if err != nil { + return err + } + schemaDocument, err := jsonschema.UnmarshalJSON(bytes.NewReader(schemaContent)) + if err != nil { + return err + } + compiler := jsonschema.NewCompiler() + if err := compiler.AddResource("schema.json", schemaDocument); err != nil { + return err + } + schema, err := compiler.Compile("schema.json") + if err != nil { + return err + } + return schema.Validate(instance) +}