Improve D&D validation reliability

internal/modules/dnd/validate/spells/catalog/validator.go

@@ -5,19 +5,19 @@ import (
 	"fmt"
 	"strings"
 
+	"gitea.maximumdirect.net/eric/notarius/internal/core/source"
 	"gitea.maximumdirect.net/eric/notarius/internal/framework/contracts"
 	"gitea.maximumdirect.net/eric/notarius/internal/framework/pipeline"
 	"gitea.maximumdirect.net/eric/notarius/internal/modules/dnd"
+	"gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/shared/diagnostics"
 	spellcatalog "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/spells/catalog"
 	spellshape "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/validate/spells/shape"
 )
 
 const (
-	Key                   = "extract/dnd/spells/catalog"
-	ReasonCode            = "unknown_spell"
-	maxIssues             = 20
-	maxDisplayedNameRunes = 128
-	maxMessageBytes       = 4096
+	Key        = "extract/dnd/spells/catalog"
+	ReasonCode = "unknown_spell"
+	policy     = "dnd.spells.validator.catalog.v2"
 )
 
 type Options struct{}
@@ -54,7 +54,10 @@ func (v *Validator) CheckpointFingerprints() []pipeline.CheckpointFingerprint {
 	if v == nil {
 		return nil
 	}
-	return []pipeline.CheckpointFingerprint{{Name: "effective_catalog", Value: v.catalog.Digest()}}
+	return []pipeline.CheckpointFingerprint{
+		{Name: "policy", Value: policy},
+		{Name: "effective_catalog", Value: v.catalog.Digest()},
+	}
 }
 
 func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationRequest[dnd.SpellList]) (contracts.ValidationResult, error) {
@@ -69,7 +72,7 @@ func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationReq
 			continue
 		}
 		if _, ok := v.catalog.Lookup(name); !ok {
-			unknown = append(unknown, unknownSpell{index: index, name: name})
+			unknown = append(unknown, unknownSpell{index: index, name: name, caster: spell.Caster, sourceRefs: spell.SourceRefs})
 		}
 	}
 	if len(unknown) == 0 {
@@ -79,42 +82,29 @@ func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationReq
 }
 
 type unknownSpell struct {
-	index int
-	name  string
+	index      int
+	name       string
+	caster     string
+	sourceRefs []source.SourceRef
 }
 
 func rejection(unknown []unknownSpell) contracts.ValidationResult {
-	limit := len(unknown)
-	if limit > maxIssues {
-		limit = maxIssues
+	issues := make([]string, 0, len(unknown))
+	var corrections diagnostics.Corrections
+	for _, item := range unknown {
+		issues = append(issues, fmt.Sprintf("spell_casts[%d].spell %s", item.index, diagnostics.Quote(item.name)))
+		corrections.Add(
+			"recognized-spell",
+			"Use a recognized D&D spell name supported by the transcript, or omit the record when the evidence does not establish a spell cast.",
+			fmt.Sprintf("Affected spell %s by caster %s %s.", diagnostics.Quote(item.name), diagnostics.Quote(strings.TrimSpace(item.caster)), diagnostics.SourceRange(item.sourceRefs)),
+		)
 	}
-	issues := make([]string, 0, limit)
-	for _, item := range unknown[:limit] {
-		issue := fmt.Sprintf("spell_casts[%d].spell %q", item.index, truncateDisplayedName(item.name))
-		candidate := rejectionMessage(append(issues, issue), len(unknown)-len(issues)-1)
-		if len(candidate) > maxMessageBytes {
-			break
-		}
-		issues = append(issues, issue)
+	return contracts.ValidationResult{
+		Approved:           false,
+		ReasonCode:         ReasonCode,
+		Message:            diagnostics.Aggregate("unknown spell names", issues),
+		CorrectionGuidance: corrections.Guidance("Correct every unrecognized spell and return the complete replacement spell-cast list"),
 	}
-	message := rejectionMessage(issues, len(unknown)-len(issues))
-	return contracts.ValidationResult{Approved: false, ReasonCode: ReasonCode, Message: message, CorrectionGuidance: "Use recognized D&D spell names supported by the supplied transcript evidence, and omit any candidate that is not a spell cast."}
-}
-
-func truncateDisplayedName(name string) string {
-	runes := []rune(name)
-	if len(runes) <= maxDisplayedNameRunes {
-		return name
-	}
-	return string(runes[:maxDisplayedNameRunes-1]) + "…"
-}
-
-func rejectionMessage(issues []string, omitted int) string {
-	message := "unknown spell names: " + strings.Join(issues, ", ")
-	if omitted > 0 {
-		message += fmt.Sprintf("; %d additional issue(s) omitted", omitted)
-	}
-	return message
 }
 
 func Spec() pipeline.ValidatorSpec {

internal/modules/dnd/validate/spells/catalog/validator_test.go

@@ -2,11 +2,9 @@ package catalog
 
 import (
 	"context"
-	"fmt"
 	"reflect"
 	"strings"
 	"testing"
-	"unicode/utf8"
 
 	"gitea.maximumdirect.net/eric/notarius/internal/core/source"
 	"gitea.maximumdirect.net/eric/notarius/internal/framework/contracts"
@@ -36,7 +34,7 @@ func TestValidatorCheckpointFingerprintUsesEffectiveCatalogDigest(t *testing.T)
 		t.Fatal(err)
 	}
 	fingerprints := validator.CheckpointFingerprints()
-	if len(fingerprints) != 1 || fingerprints[0].Name != "effective_catalog" || fingerprints[0].Value != validator.catalog.Digest() {
+	if len(fingerprints) != 2 || fingerprints[0].Name != "policy" || fingerprints[0].Value != policy || fingerprints[1].Name != "effective_catalog" || fingerprints[1].Value != validator.catalog.Digest() {
 		t.Fatalf("checkpoint fingerprints = %#v, want effective catalog digest %q", fingerprints, validator.catalog.Digest())
 	}
 }
@@ -60,59 +58,13 @@ func TestValidatorRejectsMultipleUnknownCastsInStableOrder(t *testing.T) {
 	if want := `spell_casts[0].spell "Unknown First", spell_casts[2].spell "Unknown Second"`; !strings.Contains(result.Message, want) {
 		t.Fatalf("message = %q, want %q", result.Message, want)
 	}
-}
-
-func TestValidatorBoundsUnknownCastMessage(t *testing.T) {
-	value := dnd.SpellList{SpellCasts: make([]dnd.SpellCast, 22)}
-	for index := range value.SpellCasts {
-		value.SpellCasts[index] = validCast(fmt.Sprintf("Unknown Spell %02d", index))
+	for _, want := range []string{"Unknown First", "Unknown Second", "source unit 1", "complete replacement"} {
+		if !strings.Contains(result.CorrectionGuidance, want) {
+			t.Fatalf("CorrectionGuidance = %q, want %q", result.CorrectionGuidance, want)
+		}
 	}
-	validator, err := New(Options{})
-	if err != nil {
-		t.Fatalf("New() error = %v, want nil", err)
-	}
-	result, err := validator.Validate(context.Background(), validationRequest(value))
-	if err != nil {
-		t.Fatalf("Validate() error = %v, want nil", err)
-	}
-	if result.Approved || result.ReasonCode != ReasonCode {
-		t.Fatalf("Validate() = %#v, want unknown-spell rejection", result)
-	}
-	if got := strings.Count(result.Message, "spell_casts["); got != maxIssues {
-		t.Fatalf("message includes %d issues, want %d: %q", got, maxIssues, result.Message)
-	}
-	if !strings.Contains(result.Message, "2 additional issue(s) omitted") || strings.Contains(result.Message, "Unknown Spell 21") {
-		t.Fatalf("message = %q, want bounded diagnostics", result.Message)
-	}
-}
-
-func TestValidatorBoundsUnknownSpellNamesAndTotalMessage(t *testing.T) {
-	longName := strings.Repeat("火", maxDisplayedNameRunes+100) + "\n\t"
-	value := dnd.SpellList{SpellCasts: make([]dnd.SpellCast, maxIssues)}
-	for index := range value.SpellCasts {
-		value.SpellCasts[index] = validCast(fmt.Sprintf("%s-%d", longName, index))
-	}
-	validator, err := New(Options{})
-	if err != nil {
-		t.Fatal(err)
-	}
-	result, err := validator.Validate(context.Background(), validationRequest(value))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if result.Approved || len([]byte(result.Message)) > maxMessageBytes || !strings.Contains(result.Message, "…") {
-		t.Fatalf("message length/content = %d/%q, want bounded message with truncation", len([]byte(result.Message)), result.Message)
-	}
-	if !strings.Contains(result.Message, "additional issue(s) omitted") {
-		t.Fatalf("message = %q, want byte-budget omitted count", result.Message)
-	}
-	displayed := strings.Count(result.Message, "spell_casts[")
-	wantOmitted := fmt.Sprintf("%d additional issue(s) omitted", len(value.SpellCasts)-displayed)
-	if !strings.Contains(result.Message, wantOmitted) {
-		t.Fatalf("message = %q, want omitted count %q", result.Message, wantOmitted)
-	}
-	if !utf8.ValidString(result.Message) {
-		t.Fatal("bounded message is not valid UTF-8")
+	if strings.Contains(result.CorrectionGuidance, "spell_casts[") || strings.Contains(result.CorrectionGuidance, ReasonCode) {
+		t.Fatalf("CorrectionGuidance exposed internal diagnostics: %q", result.CorrectionGuidance)
 	}
 }
 

internal/modules/dnd/validate/spells/shape/validator.go

@@ -8,12 +8,13 @@ import (
 	"gitea.maximumdirect.net/eric/notarius/internal/framework/contracts"
 	"gitea.maximumdirect.net/eric/notarius/internal/framework/pipeline"
 	"gitea.maximumdirect.net/eric/notarius/internal/modules/dnd"
+	"gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/shared/diagnostics"
 )
 
 const (
 	Key        = "extract/dnd/spells/shape"
 	ReasonCode = "invalid_spell_shape"
-	policy     = "dnd.spells.validator.shape.v1"
+	policy     = "dnd.spells.validator.shape.v2"
 )
 
 type Options struct{}
@@ -31,28 +32,69 @@ func (v *Validator) CheckpointFingerprints() []pipeline.CheckpointFingerprint {
 	return []pipeline.CheckpointFingerprint{{Name: "policy", Value: policy}}
 }
 func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationRequest[dnd.SpellList]) (contracts.ValidationResult, error) {
-	if err := Validate(req.Value); err != nil {
-		return rejection(err.Error()), nil
+	assessment := assess(req.Value)
+	if len(assessment.issues) != 0 {
+		return rejection(assessment.message(), assessment.corrections.Guidance("Correct every rejected spell cast and return the complete replacement list")), nil
 	}
 	return contracts.ValidationResult{Approved: true}, nil
 }
 
 func Validate(value dnd.SpellList) error {
+	assessment := assess(value)
+	if len(assessment.issues) == 0 {
+		return nil
+	}
+	return fmt.Errorf("%s", assessment.message())
+}
+
+type validationAssessment struct {
+	issues      []string
+	corrections diagnostics.Corrections
+}
+
+func assess(value dnd.SpellList) validationAssessment {
+	var assessment validationAssessment
 	if value.SpellCasts == nil {
-		return fmt.Errorf("spell_casts must be present")
+		assessment.issues = append(assessment.issues, "spell_casts must be present")
+		assessment.corrections.Add("list", "Return a `spell_casts` array; use an empty array when the transcript establishes no spell casts.", "")
+		return assessment
 	}
 	for index, spell := range value.SpellCasts {
+		context := spellContext(spell)
 		if strings.TrimSpace(spell.Caster) == "" {
-			return fmt.Errorf("spell_casts[%d].caster must not be empty", index)
+			assessment.issues = append(assessment.issues, fmt.Sprintf("spell_casts[%d].caster must not be empty", index))
+			assessment.corrections.Add("caster", "Provide the contextual caster name for every spell cast.", context)
 		}
 		if strings.TrimSpace(spell.Spell) == "" {
-			return fmt.Errorf("spell_casts[%d].spell must not be empty", index)
+			assessment.issues = append(assessment.issues, fmt.Sprintf("spell_casts[%d].spell must not be empty", index))
+			assessment.corrections.Add("spell", "Provide the transcript-supported spell name for every spell cast.", context)
 		}
 		if len(spell.SourceRefs) == 0 {
-			return fmt.Errorf("spell_casts[%d].source_refs must not be empty", index)
+			assessment.issues = append(assessment.issues, fmt.Sprintf("spell_casts[%d].source_refs must not be empty", index))
+			assessment.corrections.Add("source-refs", "Provide at least one transcript source range that directly supports every spell cast.", context)
 		}
 	}
-	return nil
+	return assessment
+}
+
+func (assessment validationAssessment) message() string {
+	return diagnostics.Aggregate("invalid spell shape", assessment.issues)
+}
+
+func spellContext(spell dnd.SpellCast) string {
+	name := strings.TrimSpace(spell.Spell)
+	if name == "" {
+		name = "blank spell name"
+	} else {
+		name = "spell " + diagnostics.Quote(name)
+	}
+	caster := strings.TrimSpace(spell.Caster)
+	if caster == "" {
+		caster = "blank caster name"
+	} else {
+		caster = "caster " + diagnostics.Quote(caster)
+	}
+	return "Affected cast: " + name + " with " + caster + " " + diagnostics.SourceRange(spell.SourceRefs) + "."
 }
 
 func Spec() pipeline.ValidatorSpec {
@@ -74,6 +116,6 @@ func DecodeOptions(options map[string]any) (Options, error) {
 	return Options{}, nil
 }
 func validateOptions(options map[string]any) error { _, err := DecodeOptions(options); return err }
-func rejection(message string) contracts.ValidationResult {
-	return contracts.ValidationResult{Approved: false, ReasonCode: ReasonCode, Message: message, CorrectionGuidance: "Return a complete spell-cast list with a nonblank spell name and caster plus valid source references for every cast."}
+func rejection(message, guidance string) contracts.ValidationResult {
+	return contracts.ValidationResult{Approved: false, ReasonCode: ReasonCode, Message: message, CorrectionGuidance: guidance}
 }

internal/modules/dnd/validate/spells/shape/validator_test.go

@@ -2,6 +2,7 @@ package shape
 
 import (
 	"context"
+	"strings"
 	"testing"
 
 	"gitea.maximumdirect.net/eric/notarius/internal/core/source"
@@ -54,6 +55,30 @@ func TestValidatorRejectsMissingRequiredSpellFields(t *testing.T) {
 	}
 }
 
+func TestValidatorReportsAllSpellDefectsWithContextualGuidance(t *testing.T) {
+	value := dnd.SpellList{SpellCasts: []dnd.SpellCast{
+		{Caster: "", Spell: "Fire Bolt", SourceRefs: refsAt(4)},
+		{Caster: "Aria", Spell: "", SourceRefs: nil},
+	}}
+	result, err := New(Options{}).Validate(context.Background(), requestWithValue(value))
+	if err != nil || result.Approved {
+		t.Fatalf("Validate() = %#v, %v; want rejection", result, err)
+	}
+	for _, want := range []string{"spell_casts[0].caster", "spell_casts[1].spell", "spell_casts[1].source_refs"} {
+		if !strings.Contains(result.Message, want) {
+			t.Fatalf("Message = %q, want %q", result.Message, want)
+		}
+	}
+	for _, want := range []string{"Fire Bolt", "source unit 4", "Aria", "complete replacement"} {
+		if !strings.Contains(result.CorrectionGuidance, want) {
+			t.Fatalf("CorrectionGuidance = %q, want %q", result.CorrectionGuidance, want)
+		}
+	}
+	if strings.Contains(result.CorrectionGuidance, "spell_casts[") {
+		t.Fatalf("CorrectionGuidance exposed operator path: %q", result.CorrectionGuidance)
+	}
+}
+
 func TestValidatorSpecCheckpointAndRegistration(t *testing.T) {
 	if got := New(Options{}).CheckpointFingerprints(); len(got) != 1 || got[0].Name != "policy" || got[0].Value != policy {
 		t.Fatalf("CheckpointFingerprints() = %#v, want local policy", got)
@@ -77,3 +102,7 @@ func requestWithValue(value dnd.SpellList) contracts.TypedValidationRequest[dnd.
 func validSpellList() dnd.SpellList {
 	return dnd.SpellList{SpellCasts: []dnd.SpellCast{{Caster: "Aria", Spell: "Cure Wounds", SourceRefs: []source.SourceRef{{SourceID: "session", StartUnitID: 1, EndUnitID: 1}}}}}
 }
+
+func refsAt(unitID int) []source.SourceRef {
+	return []source.SourceRef{{SourceID: "session", StartUnitID: unitID, EndUnitID: unitID}}
+}

internal/modules/dnd/validate/spells/source_refs/validator.go

@@ -8,6 +8,7 @@ import (
 	"gitea.maximumdirect.net/eric/notarius/internal/framework/contracts"
 	"gitea.maximumdirect.net/eric/notarius/internal/framework/pipeline"
 	"gitea.maximumdirect.net/eric/notarius/internal/modules/dnd"
+	"gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/shared"
 	"gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/shared/diagnostics"
 	spellshape "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/validate/spells/shape"
 )
@@ -15,7 +16,7 @@ import (
 const (
 	Key        = "extract/dnd/spells/source_refs"
 	ReasonCode = "invalid_source_refs"
-	policy     = "dnd.spells.validator.source_refs.v2"
+	policy     = "dnd.spells.validator.source_refs.v3"
 )
 
 type Options struct{}
@@ -40,57 +41,35 @@ func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationReq
 		return contracts.ValidationResult{Approved: true}, nil
 	}
 	index := source.NewDocumentIndex(req.Source)
-	var coverage *chunkCoverage
+	var coverage shared.ChunkCoverage
 	if req.Stage == string(pipeline.StageExtract) {
-		coverage = newChunkCoverage(req.Chunk)
+		coverage = shared.NewChunkCoverage(req.Chunk)
 	}
 	issues := make([]string, 0)
+	var corrections diagnostics.Corrections
 	for spellIndex, spell := range req.Value.SpellCasts {
 		for refIndex, ref := range spell.SourceRefs {
+			record := fmt.Sprintf("Affected spell %s by caster %s, citing %s.", diagnostics.Quote(spell.Spell), diagnostics.Quote(spell.Caster), diagnostics.SourceRefRange(ref))
 			if err := index.ValidateRef(ref); err != nil {
 				issues = append(issues, fmt.Sprintf("spell_casts[%d].source_refs[%d]: %s", spellIndex, refIndex, diagnostics.Truncate(err.Error())))
+				corrections.Add("valid-range", "Use positive source range endpoints that occur in the supplied transcript, with the earlier unit first.", record)
 				continue
 			}
-			if coverage != nil && !coverage.contains(req.Source, ref) {
+			if req.Stage == string(pipeline.StageExtract) && !coverage.Contains(index, req.Source, ref) {
 				issues = append(issues, fmt.Sprintf("spell_casts[%d].source_refs[%d]: source reference is outside the current extraction chunk", spellIndex, refIndex))
+				corrections.Add("chunk-range", "Use only source ranges wholly contained in the supplied extraction chunk.", record)
 			}
 		}
 	}
 	if len(issues) > 0 {
-		return rejection(diagnostics.Aggregate("invalid spell source references", issues)), nil
+		return rejection(
+			diagnostics.Aggregate("invalid spell source references", issues),
+			corrections.Guidance("Correct every rejected spell citation and return the complete replacement spell-cast list"),
+		), nil
 	}
 	return contracts.ValidationResult{Approved: true}, nil
 }
 
-type chunkCoverage struct {
-	sourceID string
-	unitIDs  map[int]struct{}
-}
-
-func newChunkCoverage(chunk *source.Chunk) *chunkCoverage {
-	coverage := &chunkCoverage{sourceID: chunk.SourceID, unitIDs: make(map[int]struct{}, len(chunk.Units))}
-	for _, unit := range chunk.Units {
-		coverage.unitIDs[unit.ID] = struct{}{}
-	}
-	return coverage
-}
-
-func (coverage *chunkCoverage) contains(doc *source.SourceDocument, ref source.SourceRef) bool {
-	if coverage == nil || doc == nil || ref.SourceID != coverage.sourceID {
-		return false
-	}
-	start, startOK := source.UnitIndex(doc, ref.StartUnitID)
-	end, endOK := source.UnitIndex(doc, ref.EndUnitID)
-	if !startOK || !endOK || start > end {
-		return false
-	}
-	for position := start; position <= end; position++ {
-		if _, found := coverage.unitIDs[doc.Units[position].ID]; !found {
-			return false
-		}
-	}
-	return true
-}
 func Spec() pipeline.ValidatorSpec {
 	return pipeline.ValidatorSpec{Key: Key, ExecutionClass: contracts.ExecutionClassDeterministic}
 }
@@ -110,6 +89,6 @@ func DecodeOptions(options map[string]any) (Options, error) {
 	return Options{}, nil
 }
 func validateOptions(options map[string]any) error { _, err := DecodeOptions(options); return err }
-func rejection(message string) contracts.ValidationResult {
-	return contracts.ValidationResult{Approved: false, ReasonCode: ReasonCode, Message: message, CorrectionGuidance: "Return spell casts whose source references identify valid transcript ranges within the supplied extraction chunk and directly support the named spell and caster."}
+func rejection(message, guidance string) contracts.ValidationResult {
+	return contracts.ValidationResult{Approved: false, ReasonCode: ReasonCode, Message: message, CorrectionGuidance: guidance}
 }

internal/modules/dnd/validate/spells/source_refs/validator_test.go

@@ -33,6 +33,14 @@ func TestValidatorRejectsInvalidSourceRefs(t *testing.T) {
 	if result.ReasonCode != ReasonCode {
 		t.Fatalf("ReasonCode = %q, want %q", result.ReasonCode, ReasonCode)
 	}
+	for _, want := range []string{"Cure Wounds", "Aria", "source unit 99", "complete replacement"} {
+		if !strings.Contains(result.CorrectionGuidance, want) {
+			t.Fatalf("CorrectionGuidance = %q, want %q", result.CorrectionGuidance, want)
+		}
+	}
+	if strings.Contains(result.CorrectionGuidance, "spell_casts[") || strings.Contains(result.CorrectionGuidance, ReasonCode) {
+		t.Fatalf("CorrectionGuidance exposed internal diagnostics: %q", result.CorrectionGuidance)
+	}
 }
 
 func TestValidatorRejectsMissingSourceDocument(t *testing.T) {