Add D&D refactoring plan and isolate merger results

docs/roadmap/implementation.md

@@ -0,0 +1,327 @@
+# D&D Module Refactoring Implementation Plan
+
+## Objective
+
+Address the five remaining D&D module refactoring findings without changing
+artifact schemas, prompt content, module keys, validator policy, warning or
+rejection semantics, durable media types, or pipeline configuration.
+
+The completed implementation must:
+
+- make every D&D append-order merger return independently owned nested data and
+  preserve absent versus present-empty list representation consistently;
+- use one source-document index per operation wherever D&D code repeatedly
+  validates or resolves source references;
+- centralize the strict candidate JSON encoding and decoding mechanics shared
+  by all five D&D artifact codecs while retaining artifact ownership in each
+  codec package;
+- remove unused source-reference canonicalization results from the NPC and
+  combat-turn normalizers; and
+- remove the unused internal NPC identity and registry aliases.
+
+This plan implements internal refactoring only. It does not add extractors,
+fields, classifications, prompts, validators, compatibility shims, or user
+configuration.
+
+## Governing Constraints
+
+Follow:
+
+- [Architecture](../policy/architecture.md), especially explicit stage
+  ownership, domain-first dependencies, and the rule against abstractions that
+  serve only hypothetical reuse;
+- [Testing Policy](../policy/testing.md), especially behavior-level tests,
+  focused regression coverage for data integrity, and avoidance of tests that
+  merely detect private implementation changes;
+- [Documentation Policy](../policy/documentation.md), keeping current
+  implementation detail in [Module Internals](../internal/modules.md); and
+- the implemented [D&D extractor contract](../internal/modules.md#dd-extractor-contract).
+
+Preserve these decisions throughout the work:
+
+- Keep the five artifact-specific merger functions. Do not replace them with a
+  callback-driven generic merger.
+- Use `source.DocumentIndex` as the canonical index for repeated source
+  reference validation and position lookup. Do not introduce another
+  independent unit-ID map or validation implementation.
+- Keep artifact schemas, approved-value validation, kinds, media types, and
+  contextual errors in their artifact codec packages.
+- Keep artifact-specific response mapping, chronology sorting, normalization,
+  and validators local. The shared work in this plan is limited to demonstrated
+  mechanics with identical semantics.
+- Preserve existing error classifications, diagnostic scopes, ordering,
+  truncation, and approval behavior. Exact non-contractual wording may remain
+  unchanged where practical, but tests should assert stable semantics rather
+  than duplicate entire error strings.
+- Do not add benchmarks or performance-sensitive timing assertions. Verify
+  index reuse through design and ordinary behavioral tests, not through
+  brittle implementation or latency detectors.
+
+Each stage below is one implementation prompt. Complete and verify a stage
+before beginning the next.
+
+## Stage 1: Harmonize D&D Merger Ownership And Presence
+
+Update the D&D append-order mergers in
+`internal/modules/dnd/register/merge.go`.
+
+Implementation:
+
+1. Add artifact-specific clone functions for spell casts and NPCs, including a
+   fresh copy of each non-nil `SourceRefs` slice. Match the existing
+   combat-turn and NPC-interaction ownership behavior.
+2. Change `appendSpellLists` and `appendNPCLists` to append cloned records
+   rather than shallow-copying records from their inputs.
+3. Change `appendSpellLists` to use the same list-presence rule as the other
+   four mergers:
+   - no inputs, or only inputs with a nil `SpellCasts` field, produce a nil
+     `SpellCasts` field;
+   - any non-nil input field makes the result field non-nil, including when all
+     present fields are empty; and
+   - records retain deterministic input and record order.
+4. Do not alter scene-description merge behavior: its record contains a scalar
+   source reference and requires no nested clone.
+
+Tests:
+
+- Extend registrar merger tests at the package-level merge boundary.
+- For spells and NPCs, mutate a merged record's first source reference and
+  prove that the corresponding input remains unchanged.
+- Cover no inputs, nil-only inputs, present-empty input, and ordered populated
+  inputs for spells. Retain the existing equivalent NPC, combat-turn,
+  interaction, and scene coverage without duplicating it elsewhere.
+
+Verification:
+
+```sh
+go test ./internal/modules/dnd/register
+```
+
+## Stage 2: Add Indexed Citation Resolution And Migrate Relatedness Validators
+
+Replace repeated validation and endpoint scans in D&D citation text assembly
+with one operation-scoped indexed resolver.
+
+Implementation:
+
+1. In `internal/modules/dnd/shared`, introduce a small citation resolver that:
+   - is constructed from one non-nil `*source.SourceDocument`;
+   - retains that immutable operation input and one
+     `source.NewDocumentIndex(doc)`;
+   - validates each requested range through `DocumentIndex.ValidateRef`;
+   - resolves endpoints through `DocumentIndex.Position`;
+   - includes overlapping source units only once;
+   - returns cited unit text in document order, joined exactly as today; and
+   - preserves the current contextual failure for an invalid source range.
+2. Replace the top-level repeated-scan `CitedText` API with the resolver API.
+   Do not retain two production implementations of citation traversal.
+3. Construct one resolver per relatedness-validation invocation and reuse it
+   for every record in that request.
+4. Migrate the spell, NPC, combat-turn, NPC-interaction, and scene-description
+   relatedness validators. Preserve all current deferral, warning, token
+   matching, warning-cap, and diagnostic behavior.
+
+Tests:
+
+- Adapt shared citation tests to the resolver's package-level behavior.
+- Preserve coverage for nil documents, invalid ranges, overlapping ranges,
+  disjoint ranges, document ordering, and newline joining where those risks are
+  currently covered.
+- Run existing relatedness tests unchanged except for construction/API updates
+  required by the refactor. Do not add tests that inspect the resolver's map,
+  count index construction, or assert performance.
+
+Verification:
+
+```sh
+go test ./internal/modules/dnd/shared \
+  ./internal/modules/dnd/validate/spells/source_relatedness \
+  ./internal/modules/dnd/validate/npcs/source_relatedness \
+  ./internal/modules/dnd/validate/combatturns/source_relatedness \
+  ./internal/modules/dnd/validate/npcinteractions/source_relatedness \
+  ./internal/modules/dnd/validate/scenedescriptions/source_relatedness
+```
+
+## Stage 3: Reuse Document Indexes Across D&D Validators
+
+Migrate repeated source-reference validation and position lookup in rejecting
+and invariant validators to one `source.DocumentIndex` per validation
+invocation.
+
+Implementation:
+
+1. Construct the index only after any existing shape-deferral or request
+   precondition logic that should run first.
+2. Use `DocumentIndex.ValidateRef` inside source-reference loops in the spell,
+   NPC, combat-turn, NPC-interaction, and scene-description source-reference
+   validators.
+3. Pass an operation-scoped index into invariant helper functions that
+   repeatedly validate references or compare source positions. Apply this to
+   combat turns, NPC interactions, and scene descriptions where relevant.
+4. Replace scene invariant `source.UnitIndex` calls inside sort comparisons
+   with `DocumentIndex.Position`.
+5. Preserve extract-stage chunk-containment checks, validator deferral,
+   aggregate diagnostic order, reason codes, and truncation. The index changes
+   lookup mechanics only.
+6. Do not extract the artifact-specific validator loops into a generic callback
+   helper.
+
+Tests:
+
+- Existing validator suites should continue to express the behavioral
+  contract. Add or change tests only if a source-document boundary is not
+  already protected.
+- Do not add source-code inspection tests or assertions that a particular
+  helper was called.
+
+Verification:
+
+```sh
+go test ./internal/modules/dnd/validate/...
+```
+
+## Stage 4: Reuse Document Indexes In Domain And Normalization Logic
+
+Finish the operation-scoped index migration outside validators and simplify
+the NPC and combat-turn canonicalization call sites.
+
+Implementation:
+
+1. In spell and combat-turn normalization, construct one
+   `source.DocumentIndex` for the normalization operation and pass it through
+   duplicate-identity helpers instead of calling `source.ValidateRef` for each
+   record and reference.
+2. In scene-description normalization, replace its local unit-position map and
+   repeated `source.ValidateRef` calls with one `source.DocumentIndex`. Use the
+   same index for validation and chronological ordering.
+3. Update `internal/modules/dnd/npcinteractions` canonical helpers so callers
+   performing repeated validity checks can supply and reuse an operation-scoped
+   index. Propagate the index through interaction normalization and invariant
+   validation as needed. Do not keep a second loop that delegates to
+   `source.ValidateRef`.
+4. Review `internal/modules/dnd/shared/unit_refs.go`. Where one extraction
+   mapping operation resolves multiple unit IDs, construct one document index
+   at the operation boundary and resolve all unit IDs through it. Preserve the
+   existing field-scoped errors and raw candidate mapping behavior.
+5. Remove the NPC and combat-turn `canonicalizeSourceRefs` wrappers that return
+   change and duplicate-count values their callers discard. Call
+   `SourceRefOrder.Canonicalize` directly and perform the single behavioral
+   comparison each normalizer actually needs.
+6. Keep the spell normalizer's richer `canonicalizeSourceRefs` helper because
+   it intentionally distinguishes ordering changes from duplicate removal for
+   diagnostics.
+7. Do not alter normalization identities, collapse eligibility, ordering,
+   warning scopes, or checkpoint fingerprints.
+
+Tests:
+
+- Run the existing extractor, domain-helper, and normalizer tests as behavioral
+  regression coverage.
+- Add coverage only for an observable boundary found to be missing; do not
+  test tuple arity, helper presence, map construction, or private call paths.
+
+Verification:
+
+```sh
+go test ./internal/modules/dnd/extract/... \
+  ./internal/modules/dnd/npcinteractions \
+  ./internal/modules/dnd/normalize/...
+```
+
+## Stage 5: Centralize Strict D&D Candidate JSON Mechanics
+
+Extract the identical candidate JSON encoding and strict decoding behavior used
+by the five D&D artifact codecs.
+
+Implementation:
+
+1. Add a narrowly named shared package beneath
+   `internal/modules/dnd/codec/` for generic typed candidate JSON mechanics.
+   It may depend only on the Go standard library and must not import an
+   artifact-specific codec or D&D artifact type.
+2. Provide generic helpers that:
+   - encode a typed candidate with `json.Marshal`;
+   - decode exactly one typed JSON value;
+   - reject unknown fields with `DisallowUnknownFields`;
+   - reject trailing JSON values; and
+   - wrap failures with a caller-supplied artifact label so existing codec
+     errors retain useful context.
+3. Migrate the spell, NPC, combat-turn, NPC-interaction, and
+   scene-description codecs to delegate only `EncodeCandidate` and
+   `DecodeCandidate` mechanics to the helper.
+4. Keep `Codec`, `New`, schema loading, kind, version, media type,
+   `Encode`/`Decode` approved-value validation, and artifact-specific
+   validation in each owning package.
+5. Preserve the exact accepted and rejected JSON language. In particular, do
+   not make candidate decoding permissive, add normalization, or merge durable
+   schemas.
+
+Tests:
+
+- Give the shared package focused table-driven tests for successful typed
+  round-trip, unknown-field rejection, malformed JSON, and trailing-value
+  rejection.
+- Retain each artifact codec's package-level boundary tests and maintained
+  durable fixtures. Remove only tests that become exact redundant copies of
+  helper mechanism tests and provide no artifact-boundary confidence.
+- Do not consolidate artifact fixtures or schemas.
+
+Verification:
+
+```sh
+go test ./internal/modules/dnd/codec/...
+```
+
+## Stage 6: Remove Unused NPC Aliases And Close Documentation
+
+Remove the unnecessary internal API surface and update the canonical
+implemented-behavior documentation after all preceding refactors are present.
+
+Implementation:
+
+1. Delete `npcs/registry.New`; `Resolve` remains the single registry
+   construction entry point.
+2. Delete `npcs/identity.Validate`; `ValidateList` remains the explicit
+   whole-list validation entry point.
+3. Confirm there are no production or test callers before deletion. Do not add
+   compatibility aliases or deprecation shims: these are internal,
+   pre-release APIs.
+4. Update [Module Internals](../internal/modules.md) concisely to reflect:
+   - independently owned nested merger output;
+   - operation-scoped indexed source-reference and citation traversal; and
+   - the shared strict candidate JSON mechanism with artifact-owned durable
+     codec policy.
+5. Do not add these implementation details to user configuration, integration
+   contracts, or ADRs. No user-visible or durable contract changes are
+   intended.
+6. Run formatting over changed Go files and inspect the final diff for
+   accidental schema, prompt, configuration, or fingerprint changes.
+
+Tests and verification:
+
+```sh
+go test ./internal/modules/dnd/...
+go test ./...
+go vet ./...
+go build ./cmd/notarius
+```
+
+The final implementation is complete when:
+
+- all commands above pass;
+- the five artifact mergers preserve order, presence, and independent nested
+  ownership;
+- repeated D&D source-reference operations use one document index at their
+  operation boundary;
+- all five codecs share strict candidate JSON mechanics without surrendering
+  artifact-specific policy;
+- the NPC and combat normalizers compute only the canonicalization results they
+  use;
+- the two unused aliases are absent; and
+- `git diff` shows no prompt, schema, durable artifact, module-key,
+  configuration, or checkpoint-policy changes.
+
+## Open Questions
+
+None. The implementation choices required for this refactoring are resolved
+above.

internal/modules/dnd/register/merge.go

@@ -7,12 +7,21 @@ import (
 
 func appendSpellLists(values []dnd.SpellList) (dnd.SpellList, error) {
 	count := 0
+	present := false
 	for _, value := range values {
+		if value.SpellCasts != nil {
+			present = true
+		}
 		count += len(value.SpellCasts)
 	}
+	if !present {
+		return dnd.SpellList{}, nil
+	}
 	combined := dnd.SpellList{SpellCasts: make([]dnd.SpellCast, 0, count)}
 	for _, value := range values {
-		combined.SpellCasts = append(combined.SpellCasts, value.SpellCasts...)
+		for _, spellCast := range value.SpellCasts {
+			combined.SpellCasts = append(combined.SpellCasts, cloneSpellCast(spellCast))
+		}
 	}
 	return combined, nil
 }
@@ -31,7 +40,9 @@ func appendNPCLists(values []dnd.NPCList) (dnd.NPCList, error) {
 	}
 	combined := dnd.NPCList{NPCs: make([]dnd.NPC, 0, count)}
 	for _, value := range values {
-		combined.NPCs = append(combined.NPCs, value.NPCs...)
+		for _, npc := range value.NPCs {
+			combined.NPCs = append(combined.NPCs, cloneNPC(npc))
+		}
 	}
 	return combined, nil
 }
@@ -105,6 +116,22 @@ func cloneCombatTurn(value dnd.CombatTurn) dnd.CombatTurn {
 	return clone
 }
 
+func cloneSpellCast(value dnd.SpellCast) dnd.SpellCast {
+	clone := value
+	if value.SourceRefs != nil {
+		clone.SourceRefs = append([]source.SourceRef(nil), value.SourceRefs...)
+	}
+	return clone
+}
+
+func cloneNPC(value dnd.NPC) dnd.NPC {
+	clone := value
+	if value.SourceRefs != nil {
+		clone.SourceRefs = append([]source.SourceRef(nil), value.SourceRefs...)
+	}
+	return clone
+}
+
 func cloneNPCInteraction(value dnd.NPCInteraction) dnd.NPCInteraction {
 	clone := value
 	if value.SourceRefs != nil {

internal/modules/dnd/register/register_test.go

@@ -285,6 +285,47 @@ func TestAppendNPCListsPreservesOrderAndArrayPresence(t *testing.T) {
 			}
 		})
 	}
+
+	input := []dnd.NPCList{{NPCs: []dnd.NPC{{Name: "first", SourceRefs: []source.SourceRef{{SourceID: "session", StartUnitID: 10, EndUnitID: 10}}}}}}
+	merged, err := appendNPCLists(input)
+	if err != nil {
+		t.Fatalf("appendNPCLists() error = %v", err)
+	}
+	merged.NPCs[0].SourceRefs[0].StartUnitID = 999
+	if input[0].NPCs[0].SourceRefs[0].StartUnitID == 999 {
+		t.Fatal("merged NPCs share source reference storage")
+	}
+}
+
+func TestAppendSpellListsPreservesOrderPresenceAndOwnership(t *testing.T) {
+	tests := []struct {
+		name string
+		in   []dnd.SpellList
+		want dnd.SpellList
+	}{
+		{name: "no values", in: nil, want: dnd.SpellList{}},
+		{name: "nil values", in: []dnd.SpellList{{}, {}}, want: dnd.SpellList{}},
+		{name: "present empty", in: []dnd.SpellList{{SpellCasts: []dnd.SpellCast{}}}, want: dnd.SpellList{SpellCasts: []dnd.SpellCast{}}},
+		{name: "ordered values", in: []dnd.SpellList{{SpellCasts: []dnd.SpellCast{{Spell: "first"}}}, {SpellCasts: []dnd.SpellCast{{Spell: "second"}}}}, want: dnd.SpellList{SpellCasts: []dnd.SpellCast{{Spell: "first"}, {Spell: "second"}}}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := appendSpellLists(tt.in)
+			if err != nil || !reflect.DeepEqual(got, tt.want) {
+				t.Fatalf("appendSpellLists() = %#v, error = %v, want %#v", got, err, tt.want)
+			}
+		})
+	}
+
+	input := []dnd.SpellList{{SpellCasts: []dnd.SpellCast{{Spell: "Shield", SourceRefs: []source.SourceRef{{SourceID: "session", StartUnitID: 10, EndUnitID: 10}}}}}}
+	merged, err := appendSpellLists(input)
+	if err != nil {
+		t.Fatalf("appendSpellLists() error = %v", err)
+	}
+	merged.SpellCasts[0].SourceRefs[0].StartUnitID = 999
+	if input[0].SpellCasts[0].SourceRefs[0].StartUnitID == 999 {
+		t.Fatal("merged spell casts share source reference storage")
+	}
 }
 
 func TestAppendNPCInteractionListsPreservesOrderPresenceAndOwnership(t *testing.T) {