Tighten NPC interaction validation and consistency

internal/modules/dnd/validate/npcinteractions/invariants/validator.go

@@ -5,14 +5,12 @@ import (
 	"context"
 	"fmt"
 	"sort"
-	"strconv"
-	"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/npcs/identity"
+	interactionmodel "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/npcinteractions"
 	npcregistry "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/npcs/registry"
 	"gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/shared/diagnostics"
 	interactionshape "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/validate/npcinteractions/shape"
@@ -78,7 +76,7 @@ func (v *Validator) CheckpointFingerprints() []pipeline.CheckpointFingerprint {
 }
 
 func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationRequest[dnd.NPCInteractionList]) (contracts.ValidationResult, error) {
-	if interactionshape.Validate(req.Value) != nil || !sourceRefsValid(req.Source, req.Value) {
+	if interactionshape.Validate(req.Value) != nil || !allSourceRefsValid(req.Source, req.Value) {
 		return contracts.ValidationResult{Approved: true}, nil
 	}
 	if v == nil || v.npcResolver == nil {
@@ -102,12 +100,10 @@ func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationReq
 	}, nil
 }
 
-func sourceRefsValid(doc *source.SourceDocument, value dnd.NPCInteractionList) bool {
+func allSourceRefsValid(doc *source.SourceDocument, value dnd.NPCInteractionList) bool {
 	for _, interaction := range value.Interactions {
-		for _, ref := range interaction.SourceRefs {
-			if source.ValidateRef(doc, ref) != nil {
-				return false
-			}
+		if !interactionmodel.ValidSourceRefs(doc, interaction.SourceRefs) {
+			return false
 		}
 	}
 	return true
@@ -123,7 +119,7 @@ func issuesFor(doc *source.SourceDocument, value dnd.NPCInteractionList, npcRegi
 		for refIndex := 1; refIndex < len(interaction.SourceRefs); refIndex++ {
 			previous := interaction.SourceRefs[refIndex-1]
 			current := interaction.SourceRefs[refIndex]
-			if sourceRefLess(doc, current, previous) {
+			if interactionmodel.SourceRefLess(doc, current, previous) {
 				issues = append(issues, fmt.Sprintf("%s.source_refs are not in canonical order at index %d", prefix, refIndex))
 			} else if current == previous {
 				issues = append(issues, fmt.Sprintf("%s.source_refs[%d] duplicates the previous reference", prefix, refIndex))
@@ -132,13 +128,13 @@ func issuesFor(doc *source.SourceDocument, value dnd.NPCInteractionList, npcRegi
 	}
 
 	if !sort.SliceIsSorted(value.Interactions, func(left, right int) bool {
-		return interactionLess(doc, value.Interactions[left], value.Interactions[right])
+		return interactionmodel.Less(doc, value.Interactions[left], value.Interactions[right])
 	}) {
 		issues = append(issues, "interactions are not in canonical order")
 	}
 	seen := make(map[string]int)
 	for index, interaction := range value.Interactions {
-		key := duplicateKey(interaction)
+		key := interactionmodel.ExactIdentity(interaction)
 		if previous, ok := seen[key]; ok {
 			issues = append(issues, fmt.Sprintf("interactions[%d] duplicates interaction %d", index, previous))
 			continue
@@ -148,105 +144,6 @@ func issuesFor(doc *source.SourceDocument, value dnd.NPCInteractionList, npcRegi
 	return issues
 }
 
-func interactionLess(doc *source.SourceDocument, left, right dnd.NPCInteraction) bool {
-	leftPosition, leftHasEvidence := earliestSourcePosition(doc, left)
-	rightPosition, rightHasEvidence := earliestSourcePosition(doc, right)
-	if leftHasEvidence != rightHasEvidence {
-		return leftHasEvidence
-	}
-	if leftHasEvidence && leftPosition != rightPosition {
-		return leftPosition < rightPosition
-	}
-	leftKey := identity.ComparisonKey(left.Name)
-	rightKey := identity.ComparisonKey(right.Name)
-	if leftKey != rightKey {
-		return leftKey < rightKey
-	}
-	if left.Name != right.Name {
-		return left.Name < right.Name
-	}
-	if left.Kind != right.Kind {
-		return left.Kind < right.Kind
-	}
-	return sourceRefsLess(doc, left.SourceRefs, right.SourceRefs)
-}
-
-func sourceRefsLess(doc *source.SourceDocument, left, right []source.SourceRef) bool {
-	for index := 0; index < len(left) && index < len(right); index++ {
-		if left[index] == right[index] {
-			continue
-		}
-		return sourceRefLess(doc, left[index], right[index])
-	}
-	return len(left) < len(right)
-}
-
-func sourceRefLess(doc *source.SourceDocument, left, right source.SourceRef) bool {
-	if left.SourceID != right.SourceID {
-		return left.SourceID < right.SourceID
-	}
-	leftStart, leftStartOK := source.UnitIndex(doc, left.StartUnitID)
-	rightStart, rightStartOK := source.UnitIndex(doc, right.StartUnitID)
-	if leftStartOK != rightStartOK {
-		return leftStartOK
-	}
-	if leftStartOK && leftStart != rightStart {
-		return leftStart < rightStart
-	}
-	if left.StartUnitID != right.StartUnitID {
-		return left.StartUnitID < right.StartUnitID
-	}
-	leftEnd, leftEndOK := source.UnitIndex(doc, left.EndUnitID)
-	rightEnd, rightEndOK := source.UnitIndex(doc, right.EndUnitID)
-	if leftEndOK != rightEndOK {
-		return leftEndOK
-	}
-	if leftEndOK && leftEnd != rightEnd {
-		return leftEnd < rightEnd
-	}
-	return left.EndUnitID < right.EndUnitID
-}
-
-func earliestSourcePosition(doc *source.SourceDocument, interaction dnd.NPCInteraction) (int, bool) {
-	found := false
-	earliest := 0
-	for _, ref := range interaction.SourceRefs {
-		if source.ValidateRef(doc, ref) != nil {
-			continue
-		}
-		position, ok := source.UnitIndex(doc, ref.StartUnitID)
-		if !ok || (found && position >= earliest) {
-			continue
-		}
-		earliest = position
-		found = true
-	}
-	return earliest, found
-}
-
-func duplicateKey(interaction dnd.NPCInteraction) string {
-	var key strings.Builder
-	writeKeyString(&key, interaction.Name)
-	writeKeyString(&key, string(interaction.Kind))
-	for _, ref := range interaction.SourceRefs {
-		writeKeyString(&key, ref.SourceID)
-		writeKeyInt(&key, ref.StartUnitID)
-		writeKeyInt(&key, ref.EndUnitID)
-	}
-	return key.String()
-}
-
-func writeKeyString(builder *strings.Builder, value string) {
-	builder.WriteString(strconv.Itoa(len(value)))
-	builder.WriteByte(':')
-	builder.WriteString(value)
-}
-
-func writeKeyInt(builder *strings.Builder, value int) {
-	builder.WriteString(strconv.Itoa(value))
-	builder.WriteByte(';')
-}
-
 func Spec() pipeline.ValidatorSpec {
 	return pipeline.ValidatorSpec{Key: Key, ExecutionClass: contracts.ExecutionClassDeterministic}
 }

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

@@ -16,7 +16,7 @@ import (
 const (
 	Key        = "extract/dnd/npc-interactions/source_refs"
 	ReasonCode = "invalid_npc_interaction_source_refs"
-	policy     = "dnd.npc_interactions.validator.source_refs.v1"
+	policy     = "dnd.npc_interactions.validator.source_refs.v2"
 )
 
 type Options struct{}
@@ -35,6 +35,9 @@ func (v *Validator) CheckpointFingerprints() []pipeline.CheckpointFingerprint {
 }
 
 func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationRequest[dnd.NPCInteractionList]) (contracts.ValidationResult, error) {
+	if req.Stage == string(pipeline.StageExtract) && req.Chunk == nil {
+		return contracts.ValidationResult{}, fmt.Errorf("NPC interaction source-reference validator requires the current extraction chunk")
+	}
 	if interactionshape.Validate(req.Value) != nil {
 		return contracts.ValidationResult{Approved: true}, nil
 	}
@@ -43,6 +46,13 @@ func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationReq
 		for refIndex, ref := range interaction.SourceRefs {
 			if err := source.ValidateRef(req.Source, ref); err != nil {
 				issues = append(issues, fmt.Sprintf("interactions[%d].source_refs[%d]: %s", interactionIndex, refIndex, diagnostics.Truncate(err.Error())))
+				continue
+			}
+			if req.Stage == string(pipeline.StageExtract) && !chunkContainsRef(req.Chunk, ref) {
+				issues = append(issues, fmt.Sprintf(
+					"interactions[%d].source_refs[%d]: source reference is outside the current extraction chunk",
+					interactionIndex, refIndex,
+				))
 			}
 		}
 	}
@@ -56,6 +66,19 @@ func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationReq
 	}, nil
 }
 
+func chunkContainsRef(chunk *source.Chunk, ref source.SourceRef) bool {
+	if chunk == nil || ref.SourceID != chunk.SourceID {
+		return false
+	}
+	startFound := false
+	endFound := false
+	for _, unit := range chunk.Units {
+		startFound = startFound || unit.ID == ref.StartUnitID
+		endFound = endFound || unit.ID == ref.EndUnitID
+	}
+	return startFound && endFound
+}
+
 func Spec() pipeline.ValidatorSpec {
 	return pipeline.ValidatorSpec{Key: Key, ExecutionClass: contracts.ExecutionClassDeterministic}
 }

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

@@ -29,6 +29,49 @@ func TestValidatorOwnsCurrentSourceUnitAndRangeValidation(t *testing.T) {
 	}
 }
 
+func TestValidatorRejectsDocumentValidEvidenceOutsideCurrentExtractionChunk(t *testing.T) {
+	doc := document()
+	chunk := &source.Chunk{
+		ID:       "chunk-0",
+		SourceID: doc.ID,
+		Units:    append([]source.SourceUnit(nil), doc.Units[:2]...),
+	}
+	value := validList()
+	result, err := New(Options{}).Validate(context.Background(), contracts.TypedValidationRequest[dnd.NPCInteractionList]{
+		Stage:  string(pipeline.StageExtract),
+		Source: doc,
+		Chunk:  chunk,
+		Value:  value,
+	})
+	if err != nil || !result.Approved {
+		t.Fatalf("contained evidence = %#v, %v", result, err)
+	}
+
+	value.Interactions[0].SourceRefs = []source.SourceRef{{
+		SourceID: doc.ID, StartUnitID: 2, EndUnitID: 3,
+	}}
+	result, err = New(Options{}).Validate(context.Background(), contracts.TypedValidationRequest[dnd.NPCInteractionList]{
+		Stage:  string(pipeline.StageExtract),
+		Source: doc,
+		Chunk:  chunk,
+		Value:  value,
+	})
+	if err != nil || result.Approved || !strings.Contains(result.Message, "outside the current extraction chunk") {
+		t.Fatalf("out-of-chunk evidence = %#v, %v", result, err)
+	}
+}
+
+func TestValidatorRequiresChunkDuringExtractValidation(t *testing.T) {
+	_, err := New(Options{}).Validate(context.Background(), contracts.TypedValidationRequest[dnd.NPCInteractionList]{
+		Stage:  string(pipeline.StageExtract),
+		Source: document(),
+		Value:  validList(),
+	})
+	if err == nil || !strings.Contains(err.Error(), "requires the current extraction chunk") {
+		t.Fatalf("Validate() error = %v", err)
+	}
+}
+
 func TestValidatorDefersShapeAndDoesNotMutate(t *testing.T) {
 	malformed := dnd.NPCInteractionList{Interactions: []dnd.NPCInteraction{{Name: "Mira Thorn"}}}
 	result, err := New(Options{}).Validate(context.Background(), request(document(), malformed))
@@ -55,7 +98,7 @@ func request(doc *source.SourceDocument, value dnd.NPCInteractionList) contracts
 }
 
 func document() *source.SourceDocument {
-	return &source.SourceDocument{ID: "session", Units: []source.SourceUnit{{ID: 1}, {ID: 2}}}
+	return &source.SourceDocument{ID: "session", Units: []source.SourceUnit{{ID: 1}, {ID: 2}, {ID: 3}}}
 }
 
 func validList() dnd.NPCInteractionList {

internal/modules/dnd/validate/npcinteractions/source_relatedness/validator.go

@@ -16,7 +16,8 @@ import (
 const (
 	Key               = "extract/dnd/npc-interactions/source_relatedness"
 	WarningReasonCode = "npc_interaction_not_near_source"
-	policy            = "dnd.npc_interactions.validator.source_relatedness.v1"
+	OmittedReasonCode = "npc_interaction_relatedness_warnings_omitted"
+	policy            = "dnd.npc_interactions.validator.source_relatedness.v2"
 )
 
 type Options struct{}
@@ -57,7 +58,10 @@ func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationReq
 			Message:    fmt.Sprintf("NPC interaction name %s was not found in cited source text", diagnostics.Quote(interaction.Name)),
 		})
 	}
-	return contracts.ValidationResult{Approved: true, Warnings: warnings}, nil
+	return contracts.ValidationResult{
+		Approved: true,
+		Warnings: diagnostics.LimitWarnings(warnings, "npc_interactions", OmittedReasonCode),
+	}, nil
 }
 
 func Spec() pipeline.ValidatorSpec {

internal/modules/dnd/validate/npcinteractions/source_relatedness/validator_test.go

@@ -10,6 +10,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/diagnostics"
 )
 
 func TestValidatorUsesOnlyCurrentTranscriptAndWarnsOncePerInteraction(t *testing.T) {
@@ -49,3 +50,26 @@ func TestValidatorDefersMalformedShapeAndInvalidRanges(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestValidatorBoundsWarnings(t *testing.T) {
+	count := diagnostics.MaxWarnings + 5
+	interactions := make([]dnd.NPCInteraction, count)
+	for index := range interactions {
+		interactions[index] = dnd.NPCInteraction{
+			Name:       "Missing NPC",
+			Kind:       dnd.NPCInteractionKindMentioned,
+			SourceRefs: []source.SourceRef{{SourceID: "session", StartUnitID: 1, EndUnitID: 1}},
+		}
+	}
+	result, err := New(Options{}).Validate(context.Background(), contracts.TypedValidationRequest[dnd.NPCInteractionList]{
+		Source: &source.SourceDocument{ID: "session", Units: []source.SourceUnit{{ID: 1, Text: "The party waits."}}},
+		Value:  dnd.NPCInteractionList{Interactions: interactions},
+	})
+	if err != nil || !result.Approved {
+		t.Fatalf("Validate() = %#v, %v", result, err)
+	}
+	if len(result.Warnings) != diagnostics.MaxWarnings ||
+		result.Warnings[len(result.Warnings)-1].ReasonCode != OmittedReasonCode {
+		t.Fatalf("warnings = %#v", result.Warnings)
+	}
+}