diff --git a/internal/cli/spell_catalog_identity_contract_test.go b/internal/cli/spell_catalog_identity_contract_test.go index 3e1558f..2746a68 100644 --- a/internal/cli/spell_catalog_identity_contract_test.go +++ b/internal/cli/spell_catalog_identity_contract_test.go @@ -263,7 +263,7 @@ func TestChangedSemanticSpellCatalogFingerprintCannotResumeRecordedCheckpoint(t if _, decision := changedLoader.Normalize("spells", spellnormalize.Key, normalizeDependencies); decision.Reused { t.Fatalf("changed normalize fingerprint decision = %#v, want normalize checkpoint cold miss", decision) } - changedMapping := replaceCheckpointFingerprintValue(t, fingerprints, extractSpellMappingFingerprintName(), "dnd.spells.extract_mapping.v2") + changedMapping := replaceCheckpointFingerprintValue(t, fingerprints, extractSpellMappingFingerprintName(), "dnd.spells.extract_mapping.v3") assertOnlyCheckpointFingerprintChanged(t, fingerprints, changedMapping, extractSpellMappingFingerprintName()) _, mappingLoader, err := checkpointHandlersForRun(settings, Options{}, materialized, changedMapping, []byte("same input"), nil, nil, "", "", true) if err != nil { diff --git a/internal/modules/dnd/extract/npcs/canonicalize.go b/internal/modules/dnd/extract/npcs/canonicalize.go index 2e41c31..a61c17a 100644 --- a/internal/modules/dnd/extract/npcs/canonicalize.go +++ b/internal/modules/dnd/extract/npcs/canonicalize.go @@ -6,18 +6,19 @@ import ( "gitea.maximumdirect.net/eric/notarius/internal/core/source" "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd" "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/npcs/identity" + "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/shared" ) -func canonicalizeResponse(response *extractionResponse, doc *source.SourceDocument) { +func canonicalizeResponse(response *extractionResponse, order shared.SourceRefOrder, sourceID string) { if response == nil { return } for index := range response.NPCs { - canonicalizeNPC(&response.NPCs[index]) + canonicalizeNPC(&response.NPCs[index], order, sourceID) } sort.SliceStable(response.NPCs, func(i, j int) bool { - left, leftOK := earliestSourceIndex(doc, response.NPCs[i]) - right, rightOK := earliestSourceIndex(doc, response.NPCs[j]) + left, leftOK := order.EarliestValid(canonicalSourceRefs(response.NPCs[i].SourceRefs, sourceID)) + right, rightOK := order.EarliestValid(canonicalSourceRefs(response.NPCs[j].SourceRefs, sourceID)) if leftOK != rightOK { return leftOK } @@ -28,67 +29,11 @@ func canonicalizeResponse(response *extractionResponse, doc *source.SourceDocume }) } -func canonicalizeNPC(npc *npcResponse) { +func canonicalizeNPC(npc *npcResponse, order shared.SourceRefOrder, sourceID string) { if npc == nil { return } - sort.SliceStable(npc.SourceRefs, func(i, j int) bool { - left := npc.SourceRefs[i] - right := npc.SourceRefs[j] - if unitSortValue(left.StartUnitID) != unitSortValue(right.StartUnitID) { - return unitSortValue(left.StartUnitID) < unitSortValue(right.StartUnitID) - } - return unitSortValue(left.EndUnitID) < unitSortValue(right.EndUnitID) - }) - npc.SourceRefs = dedupeSourceRefs(npc.SourceRefs) -} - -func dedupeSourceRefs(refs []npcSourceRefResponse) []npcSourceRefResponse { - if len(refs) < 2 { - return refs - } - out := refs[:0] - var previous npcSourceRefResponse - for index, ref := range refs { - if index > 0 && sameSourceRef(previous, ref) { - continue - } - out = append(out, ref) - previous = ref - } - return out -} - -func sameSourceRef(left npcSourceRefResponse, right npcSourceRefResponse) bool { - return left.StartUnitID == right.StartUnitID && left.EndUnitID == right.EndUnitID -} - -func earliestSourceIndex(doc *source.SourceDocument, npc npcResponse) (int, bool) { - earliest := 0 - found := false - for _, ref := range npc.SourceRefs { - start := ref.StartUnitID - end := ref.EndUnitID - if start > 0 && end > 0 { - startIndex, startOK := source.UnitIndex(doc, start) - endIndex, endOK := source.UnitIndex(doc, end) - if !startOK || !endOK || startIndex > endIndex { - continue - } - if !found || startIndex < earliest { - earliest = startIndex - found = true - } - } - } - return earliest, found -} - -func unitSortValue(value int) int { - if value <= 0 { - return int(^uint(0) >> 1) - } - return value + npc.SourceRefs = npcResponseRefs(order.Canonicalize(canonicalSourceRefs(npc.SourceRefs, sourceID))) } func canonicalNPCList(response extractionResponse, sourceID string) dnd.NPCList { @@ -120,3 +65,14 @@ func canonicalSourceRefs(values []npcSourceRefResponse, sourceID string) []sourc } return out } + +func npcResponseRefs(values []source.SourceRef) []npcSourceRefResponse { + if values == nil { + return nil + } + out := make([]npcSourceRefResponse, len(values)) + for index, value := range values { + out[index] = npcSourceRefResponse{StartUnitID: value.StartUnitID, EndUnitID: value.EndUnitID} + } + return out +} diff --git a/internal/modules/dnd/extract/npcs/extractor.go b/internal/modules/dnd/extract/npcs/extractor.go index 60946a6..b5865f0 100644 --- a/internal/modules/dnd/extract/npcs/extractor.go +++ b/internal/modules/dnd/extract/npcs/extractor.go @@ -13,7 +13,7 @@ import ( const Key = "dnd/npcs" -const mappingPolicy = "dnd.npcs.extract_mapping.v1" +const mappingPolicy = "dnd.npcs.extract_mapping.v2" var requiredCapabilities = []string{ "chunks", @@ -129,6 +129,7 @@ func (e *Extractor) Extract(ctx context.Context, req contracts.TypedExtractionRe if err != nil { return contracts.TypedExtractionResult[dnd.NPCList]{}, extractorErrorf("%w", err) } + order := shared.NewSourceRefOrder(req.Source) var response extractionResponse if _, err := e.llm.CompleteStructured(ctx, contracts.StructuredCompletionRequest{ @@ -141,7 +142,7 @@ func (e *Extractor) Extract(ctx context.Context, req contracts.TypedExtractionRe }, &response); err != nil { return contracts.TypedExtractionResult[dnd.NPCList]{}, extractorErrorf("complete structured output: %w", err) } - canonicalizeResponse(&response, req.Source) + canonicalizeResponse(&response, order, req.Source.ID) return contracts.TypedExtractionResult[dnd.NPCList]{Value: canonicalNPCList(response, req.Source.ID)}, nil } diff --git a/internal/modules/dnd/extract/npcs/extractor_test.go b/internal/modules/dnd/extract/npcs/extractor_test.go index dfa85b9..7efd9ce 100644 --- a/internal/modules/dnd/extract/npcs/extractor_test.go +++ b/internal/modules/dnd/extract/npcs/extractor_test.go @@ -84,6 +84,43 @@ func TestExtractOrdersNPCsBySourcePositionRatherThanUnitID(t *testing.T) { } } +func TestExtractUsesDocumentOrderForNPCReferencesAndStableTies(t *testing.T) { + client := &fakeNPCsLLMClient{response: extractionResponse{NPCs: []npcResponse{ + {Name: "Later", SourceRefs: responseSourceRefs(10, 10)}, + {Name: "First", SourceRefs: []npcSourceRefResponse{ + {StartUnitID: 10, EndUnitID: 10}, + {StartUnitID: 30, EndUnitID: 30}, + {StartUnitID: 30, EndUnitID: 30}, + {StartUnitID: 999, EndUnitID: 0}, + }}, + {Name: "Second", SourceRefs: responseSourceRefs(30, 30)}, + }}} + req := extractionRequest() + req.Source.Units = []source.SourceUnit{{ID: 30}, {ID: 10}} + req.Chunk.Units = append([]source.SourceUnit(nil), req.Source.Units...) + req.Chunk.Ref = source.SourceRef{SourceID: req.Source.ID, StartUnitID: 30, EndUnitID: 10} + + result, err := newExtractor(t, client).Extract(context.Background(), req) + if err != nil { + t.Fatalf("Extract() error = %v", err) + } + if got := []string{result.Value.NPCs[0].Name, result.Value.NPCs[1].Name, result.Value.NPCs[2].Name}; !reflect.DeepEqual(got, []string{"First", "Second", "Later"}) { + t.Fatalf("NPC order = %#v, want document chronology with stable equal-evidence ties", got) + } + refs := result.Value.NPCs[0].SourceRefs + if got := []int{refs[0].StartUnitID, refs[1].StartUnitID, refs[2].StartUnitID}; !reflect.DeepEqual(got, []int{30, 10, 999}) { + t.Fatalf("source refs = %#v, want document order with exact duplicate removed", refs) + } + refs[0].StartUnitID = 777 + for _, npc := range client.response.NPCs { + for _, ref := range npc.SourceRefs { + if ref.StartUnitID == 777 { + t.Fatal("result source references alias the model response") + } + } + } +} + func TestExtractPassesCampaignReferencesAsPromptInputs(t *testing.T) { client := &fakeNPCsLLMClient{response: extractionResponse{NPCs: []npcResponse{}}} req := extractionRequest() diff --git a/internal/modules/dnd/extract/spells/canonicalize.go b/internal/modules/dnd/extract/spells/canonicalize.go index c5a508d..2a28f36 100644 --- a/internal/modules/dnd/extract/spells/canonicalize.go +++ b/internal/modules/dnd/extract/spells/canonicalize.go @@ -5,18 +5,19 @@ import ( "gitea.maximumdirect.net/eric/notarius/internal/core/source" "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd" + "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/shared" ) -func canonicalizeResponse(response *extractionResponse) { +func canonicalizeResponse(response *extractionResponse, order shared.SourceRefOrder, sourceID string) { if response == nil { return } for index := range response.SpellCasts { - canonicalizeSpellCast(&response.SpellCasts[index]) + canonicalizeSpellCast(&response.SpellCasts[index], order, sourceID) } sort.SliceStable(response.SpellCasts, func(i, j int) bool { - left, leftOK := earliestSourceUnit(response.SpellCasts[i]) - right, rightOK := earliestSourceUnit(response.SpellCasts[j]) + left, leftOK := order.EarliestValid(spellSourceRefs(response.SpellCasts[i].SourceRefs, sourceID)) + right, rightOK := order.EarliestValid(spellSourceRefs(response.SpellCasts[j].SourceRefs, sourceID)) if leftOK != rightOK { return leftOK } @@ -27,53 +28,33 @@ func canonicalizeResponse(response *extractionResponse) { }) } -func canonicalizeSpellCast(spell *spellCastResponse) { - sort.SliceStable(spell.SourceRefs, func(i, j int) bool { - left := spell.SourceRefs[i] - right := spell.SourceRefs[j] - if left.StartUnitID != right.StartUnitID { - return unitSortValue(left.StartUnitID) < unitSortValue(right.StartUnitID) - } - return unitSortValue(left.EndUnitID) < unitSortValue(right.EndUnitID) - }) - spell.SourceRefs = dedupeSourceRefs(spell.SourceRefs) +func canonicalizeSpellCast(spell *spellCastResponse, order shared.SourceRefOrder, sourceID string) { + if spell == nil { + return + } + spell.SourceRefs = spellResponseRefs(order.Canonicalize(spellSourceRefs(spell.SourceRefs, sourceID))) } -func dedupeSourceRefs(refs []spellSourceRefResponse) []spellSourceRefResponse { - if len(refs) < 2 { - return refs +func spellSourceRefs(refs []spellSourceRefResponse, sourceID string) []source.SourceRef { + if refs == nil { + return nil } - out := refs[:0] - var previous spellSourceRefResponse + values := make([]source.SourceRef, len(refs)) for index, ref := range refs { - if index > 0 && sameSourceRef(previous, ref) { - continue - } - out = append(out, ref) - previous = ref + values[index] = source.SourceRef{SourceID: sourceID, StartUnitID: ref.StartUnitID, EndUnitID: ref.EndUnitID} } - return out + return values } -func sameSourceRef(left spellSourceRefResponse, right spellSourceRefResponse) bool { - return left.StartUnitID == right.StartUnitID && left.EndUnitID == right.EndUnitID -} - -func earliestSourceUnit(spell spellCastResponse) (int, bool) { - for _, ref := range spell.SourceRefs { - start := ref.StartUnitID - if start > 0 { - return start, true - } +func spellResponseRefs(refs []source.SourceRef) []spellSourceRefResponse { + if refs == nil { + return nil } - return 0, false -} - -func unitSortValue(value int) int { - if value <= 0 { - return int(^uint(0) >> 1) + values := make([]spellSourceRefResponse, len(refs)) + for index, ref := range refs { + values[index] = spellSourceRefResponse{StartUnitID: ref.StartUnitID, EndUnitID: ref.EndUnitID} } - return value + return values } func canonicalSpellList(response extractionResponse, sourceID string) dnd.SpellList { diff --git a/internal/modules/dnd/extract/spells/extractor.go b/internal/modules/dnd/extract/spells/extractor.go index 64801a2..978f03c 100644 --- a/internal/modules/dnd/extract/spells/extractor.go +++ b/internal/modules/dnd/extract/spells/extractor.go @@ -17,7 +17,7 @@ const Key = "dnd/spells" const ArtifactType = "dnd.spell_cast" const SchemaVersion = "v1" -const mappingPolicy = "dnd.spells.extract_mapping.v1" +const mappingPolicy = "dnd.spells.extract_mapping.v2" const ( NPCRegistryReferenceSlot = npcregistry.ReferenceSlot @@ -185,6 +185,7 @@ func (e *Extractor) Extract(ctx context.Context, req contracts.TypedExtractionRe if err != nil { return contracts.TypedExtractionResult[dnd.SpellList]{}, extractorErrorf("%w", err) } + order := shared.NewSourceRefOrder(req.Source) npcRegistry, err := e.npcResolver.Resolve(req.References) if err != nil { return contracts.TypedExtractionResult[dnd.SpellList]{}, extractorErrorf("resolve NPC registry: %w", err) @@ -204,7 +205,7 @@ func (e *Extractor) Extract(ctx context.Context, req contracts.TypedExtractionRe }, &response); err != nil { return contracts.TypedExtractionResult[dnd.SpellList]{}, extractorErrorf("complete structured output: %w", err) } - canonicalizeResponse(&response) + canonicalizeResponse(&response, order, req.Source.ID) return contracts.TypedExtractionResult[dnd.SpellList]{Value: canonicalSpellList(response, req.Source.ID)}, nil } diff --git a/internal/modules/dnd/extract/spells/extractor_test.go b/internal/modules/dnd/extract/spells/extractor_test.go index 7e9b65f..0514572 100644 --- a/internal/modules/dnd/extract/spells/extractor_test.go +++ b/internal/modules/dnd/extract/spells/extractor_test.go @@ -284,6 +284,57 @@ func TestExtractOrdersAndDeduplicatesEvidence(t *testing.T) { } } +func TestExtractUsesDocumentOrderForReferencesAndSpellCasts(t *testing.T) { + client := &fakeSpellsLLMClient{response: extractionResponse{SpellCasts: []spellCastResponse{ + {Caster: "Later", Spell: "Fire Bolt", SourceRefs: responseSourceRefs(10, 10)}, + {Caster: "Earlier", Spell: "Cure Wounds", SourceRefs: []spellSourceRefResponse{ + {StartUnitID: 10, EndUnitID: 10}, + {StartUnitID: 30, EndUnitID: 30}, + {StartUnitID: 30, EndUnitID: 30}, + {StartUnitID: 999, EndUnitID: 0}, + }}, + {Caster: "Unavailable", Spell: "Healing Word", SourceRefs: []spellSourceRefResponse{{StartUnitID: 999, EndUnitID: 0}}}, + }}} + req := extractionRequest() + req.Source.Units = []source.SourceUnit{{ID: 30}, {ID: 10}} + req.Chunk.Units = append([]source.SourceUnit(nil), req.Source.Units...) + req.Chunk.Ref = source.SourceRef{SourceID: req.Source.ID, StartUnitID: 30, EndUnitID: 10} + + result, err := newExtractor(t, client).Extract(context.Background(), req) + if err != nil { + t.Fatalf("Extract() error = %v", err) + } + if got := []string{result.Value.SpellCasts[0].Spell, result.Value.SpellCasts[1].Spell, result.Value.SpellCasts[2].Spell}; !reflect.DeepEqual(got, []string{"Cure Wounds", "Fire Bolt", "Healing Word"}) { + t.Fatalf("spell order = %#v, want document chronology followed by invalid evidence", got) + } + refs := result.Value.SpellCasts[0].SourceRefs + if got := []int{refs[0].StartUnitID, refs[1].StartUnitID, refs[2].StartUnitID}; !reflect.DeepEqual(got, []int{30, 10, 999}) { + t.Fatalf("source refs = %#v, want document order with exact duplicate removed", refs) + } + refs[0].StartUnitID = 777 + for _, spell := range client.response.SpellCasts { + for _, ref := range spell.SourceRefs { + if ref.StartUnitID == 777 { + t.Fatal("result source references alias the model response") + } + } + } +} + +func TestExtractPreservesStableSpellOrderForEqualEvidence(t *testing.T) { + client := &fakeSpellsLLMClient{response: extractionResponse{SpellCasts: []spellCastResponse{ + {Caster: "First", Spell: "Cure Wounds", SourceRefs: responseSourceRefs(2, 2)}, + {Caster: "Second", Spell: "Fire Bolt", SourceRefs: responseSourceRefs(2, 2)}, + }}} + result, err := newExtractor(t, client).Extract(context.Background(), extractionRequest()) + if err != nil { + t.Fatalf("Extract() error = %v", err) + } + if got := []string{result.Value.SpellCasts[0].Caster, result.Value.SpellCasts[1].Caster}; !reflect.DeepEqual(got, []string{"First", "Second"}) { + t.Fatalf("equal-evidence order = %#v, want stable response order", got) + } +} + func TestExtractPreservesInvalidEvidenceForValidators(t *testing.T) { client := &fakeSpellsLLMClient{response: extractionResponse{SpellCasts: []spellCastResponse{{ Caster: "Aria", Spell: "Cure Wounds", diff --git a/internal/modules/integration/dnd_npc_grounded_test.go b/internal/modules/integration/dnd_npc_grounded_test.go index e2abb6f..77d229a 100644 --- a/internal/modules/integration/dnd_npc_grounded_test.go +++ b/internal/modules/integration/dnd_npc_grounded_test.go @@ -41,8 +41,8 @@ func TestNPCOutputGroundsSpellAndCombatConsumersThroughOneOperation(t *testing.T t.Fatalf("Prepare() error = %v", err) } for name, value := range map[string]string{ - "extract:npcs:dnd/npcs:mapping_policy": "dnd.npcs.extract_mapping.v1", - "extract:spells:dnd/spells:mapping_policy": "dnd.spells.extract_mapping.v1", + "extract:npcs:dnd/npcs:mapping_policy": "dnd.npcs.extract_mapping.v2", + "extract:spells:dnd/spells:mapping_policy": "dnd.spells.extract_mapping.v2", } { assertFingerprintValue(t, prepared.CheckpointFingerprints(), name, value) }