From 9d1356a20ee625bc885f7403deb874a01c332ef9 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 25 Jul 2026 12:51:36 +0000 Subject: [PATCH] Add D&D refactoring plan and isolate merger results --- docs/roadmap/implementation.md | 327 ++++++++++++++++++ internal/modules/dnd/register/merge.go | 31 +- .../modules/dnd/register/register_test.go | 41 +++ 3 files changed, 397 insertions(+), 2 deletions(-) create mode 100644 docs/roadmap/implementation.md diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md new file mode 100644 index 0000000..e9c7f45 --- /dev/null +++ b/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. diff --git a/internal/modules/dnd/register/merge.go b/internal/modules/dnd/register/merge.go index 3b98777..aab41ba 100644 --- a/internal/modules/dnd/register/merge.go +++ b/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 { diff --git a/internal/modules/dnd/register/register_test.go b/internal/modules/dnd/register/register_test.go index 2665a3c..e2fb136 100644 --- a/internal/modules/dnd/register/register_test.go +++ b/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) {