328 lines
14 KiB
Markdown
328 lines
14 KiB
Markdown
# 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.
|