From 6e21c83fd86a32678cf3a260f797115fc70ec6d6 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 24 Jul 2026 14:08:36 +0000 Subject: [PATCH] Add D&D module code audit implementation plan --- docs/roadmap/audit.md | 68 +- docs/roadmap/implementation.md | 1187 +++++++++++++++++++++----------- 2 files changed, 863 insertions(+), 392 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 44dfde0..bb60eb6 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -427,7 +427,8 @@ reference ordering assume numeric unit IDs instead of source-document order, and spell/NPC checkpoint identities omit stable mapping policies. Both can change durable results or reuse results produced under different semantics. Smaller drift exists in common request preflight, scene semantic-validation -ownership, prompt cache-boundary coverage, and package surface conventions. +ownership, prompt cache-boundary coverage, package surface conventions, and +the documentation of the common extractor contract for future lanes. The shared-code question is **yes, but only for two narrow D&D-wide responsibilities**: common chunk-extraction preflight and document-aware @@ -630,6 +631,61 @@ hidden result aliasing, or unregistered audited extractor was found. - **Migration/validation:** graph search found no production consumers; compile all internal packages and add one spell zero-value metadata test. +7. **The common D&D extractor contract is observable but only partially + documented as a requirement for future lanes.** + - **Category:** documentation and convention drift. + - **Affected modules:** all five extractors and future D&D extraction + modules. + - **Evidence:** all five reject unknown options, perform common request + preflight, return independently owned results, use package-local private + response types, and expose prompt/schema provenance plus checkpoint + fingerprints. The extension checklist in + [module internals](../internal/modules.md#adding-an-extension) covers + registration, package-owned assets, prompt ordering, and general option + and validation coverage, while + [LLM internals](../internal/llm.md#prompt-and-schema-assets) documents the + private-schema boundary. Neither location consolidates the remaining + behaviors into a normative D&D extractor contract. The missing spell/NPC + mapping fingerprints and uneven preflight and prompt-contract coverage + demonstrate that conventions discoverable from current packages can + still drift. + - **Impact:** an additional extractor can appear locally consistent while + silently accepting misspelled options, omitting a stable semantic input + from checkpoint identity, returning aliased mutable data, implementing + incomplete preflight, or missing focused contract coverage. + - **Target state and owner:** add a compact **D&D extractor contract** + subsection to `docs/internal/modules.md`, incorporated into or placed + immediately after **Adding An Extension**. Specify behavioral + responsibilities rather than filenames or boilerplate: + - reject unknown options unless the option namespace is intentionally + extensible; + - use the shared request preflight contract while retaining receiver, + dependency, and lane-specific checks locally; + - return results and exposed metadata that are independently owned and + safe for caller mutation; + - keep the private response DTO, structural schema, schema identity, + provider-response mapping, durable artifact conversion, and + lane-specific diagnostics package-owned; + - include every stable semantic input that can change durable results in + checkpoint identity, including prompt, schema, mapping, + canonicalization, prepared reference projection, identity, + normalization, and trimming policies when applicable; and + - consider focused behavioral coverage for construction and + registration, option rejection, preflight, provider failures, + structured-output decoding, mapping and ownership, prompt + role/input/cache order, and checkpoint invalidation. + - **Why documentation rather than another abstraction:** these are shared + obligations, not one shared implementation. A checklist makes omissions + visible without introducing a configurable metadata builder, generic + mapper, shared test fixture, mandatory file layout, or exact-output + change-detector tests. Detailed prompt, schema, pipeline, and testing + policies should remain linked rather than duplicated. + - **Migration/validation:** update the extension checklist and relevant + cross-links when the shared preflight helper is documented. Review the + text against all five lanes and the completed audit matrix. Do not require + exact prompt text, hashes, prefix lengths, test counts, filenames, or + fixture layouts. + #### Optional improvement The current module documentation lists only four production consumers of @@ -679,6 +735,7 @@ error wrapping, or secret handling. | Harmonize prompt contract tests | Four citation extractor test suites | Each manifest owns its role/input/cache sequence; consistent assertions should stay beside distinct assets. | | Harmonize preflight coverage | All five package suites after shared preflight | Shared tests own common branches; local tests retain package-visible context and specializations. | | Harmonize package hygiene | Spell/combat extractors and D&D shared | Delete unused exports/artificial API and align spell metadata nil behavior without introducing a new abstraction. | +| Document the D&D extractor contract | `docs/internal/modules.md`, linked to existing LLM and testing policies | The five lanes share behavioral obligations that should guide future extensions, but they do not justify a generic implementation or rigid package template. | ### Rejected Or Deferred Candidates @@ -717,9 +774,12 @@ error wrapping, or secret handling. 5. **Close prompt contract coverage.** Add full role/input/cache-boundary assertions to each citation prompt suite without shared fixtures or exact text/hash assertions. -6. **Apply low-risk package/documentation cleanup.** Remove unused - `ArtifactType` constants, add the spell metadata nil guard/test, and update - module documentation for the shared helper consumers and boundaries. +6. **Apply low-risk package cleanup.** Remove unused `ArtifactType` constants + and add the spell metadata nil guard/test. +7. **Document the common extractor contract.** Update module documentation for + the shared helper consumers and boundaries, then add the D&D extractor + contract checklist described in Finding 7. Link to existing prompt, schema, + pipeline, and testing policies rather than copying them. Each scope can be reviewed and reverted independently. The sequence is not a decision-complete implementation plan; implementation should still pin exact diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 41975f4..1bf8547 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,133 +1,815 @@ -# D&D Extraction Module Audit Execution Plan +# D&D Extraction Module Audit Implementation Plan -Status: Ready for audit execution +Status: Ready for implementation -This document divides the audit defined in -[D&D Extraction Module Refactoring Audit Strategy](audit.md) into five prompts. -Execute the stages in order. Each stage is a read-only code audit: it may update -`docs/roadmap/audit.md`, but it must not change production code, tests, prompts, -schemas, examples, current-behavior documentation, or configuration. +This document turns the completed findings in +[D&D Extraction Module Refactoring Audit](audit.md#audit-results) into an +ordered implementation plan. Each stage is one bounded prompt for an LLM coding +agent. Execute the stages in order and complete each stage's gate before +starting the next. -The word "implementation" in this filename refers to implementing the audit -strategy, not implementing refactors. +The target is a corrected and better-harmonized five-extractor family, not a +generic extractor framework. Preserve the intentional differences and rejected +abstractions recorded in the audit. ## Global Instructions Every stage must: -1. Read `docs/roadmap/audit.md` in full before beginning. -2. Follow `AGENTS.md`, `docs/development.md`, and all policies under - `docs/policy/`. +1. Read this document, the completed audit, `AGENTS.md`, + `docs/development.md`, and the task-specific documents identified there. + Follow all policies under `docs/policy/`. +2. Inspect the current working tree before editing. Preserve unrelated user + changes and do not assume a clean checkout. 3. Prefer the codebase knowledge graph for code discovery and call tracing. - Use direct file and text searches for prompt assets, JSON Schemas, - configuration, documentation, and exact string comparison. -4. Inspect all five extractors where the stage calls for comparison: - `spells`, `npcs`, `combatturns`, `npcinteractions`, and - `scenedescriptions`. -5. Cite exact files and symbols for every observation written to `audit.md`. -6. Distinguish confirmed facts, provisional interpretations, intentional - specialization, and actionable findings. -7. Avoid treating visual similarity, file count, or line count as sufficient - evidence for sharing. -8. Preserve findings from earlier stages unless new evidence disproves them. - When revising an earlier conclusion, edit it in place and record the reason; - do not append contradictory conclusions. -9. Keep implementation out of scope. Do not modify code to test whether a - proposed refactor is convenient. -10. Leave the repository otherwise unchanged and report any pre-existing dirty - worktree state before proceeding. + Use direct text and file searches for documentation, configuration, prompt + assets, schemas, fixtures, and exact string comparison. +4. Implement only the current stage. Do not opportunistically begin a later + stage. +5. Keep artifact semantics, private response DTOs and schemas, lane-specific + prompts, mapping, diagnostics, and typed test support in their owning + packages unless this plan explicitly moves one responsibility. +6. Keep the new shared behavior under `internal/modules/dnd/shared`; do not + move D&D concepts into the generic framework. +7. Apply the testing policy. Protect observable behavior and semantic + checkpoint identity, not private helper usage, exact prompt text, hashes, + message lengths, source-file layout, or test counts. +8. Use focused tests while iterating. Run `gofmt` on changed Go files and + `git diff --check` before completing every stage. +9. Update current-behavior documentation only in the documentation stage, + after the corresponding behavior exists. +10. Do not alter durable artifact schemas, prompt prose, configuration + contracts, reference-slot policy, model-facing response DTO fields, or + extraction policy except where a stage explicitly requires it. -## Working Results Structure +The following audit decisions remain out of scope: -Stages 1 through 4 maintain one working area at the end of `audit.md`: +- removal or continued support of the deprecated `roster` reference alias; +- renaming the spell private response-schema key or ID; +- shared prompt prose or response schemas; +- generic manifest, fingerprint, mapper, structured-call, error-prefix, or + registration builders; +- shared schema-test fixtures or provider fakes; and +- framework-level preflight or source-reference APIs. -```markdown -## Audit Results - -Status: In progress - -### Baseline And Module Matrix -### Divergence Register -### Prompt, Schema, And LLM Review -### Extraction And Canonicalization Review -### Duplication Register -### Contextual Architecture And Ownership Review -### Candidate Decisions -``` - -Create headings when their owning stage begins. Later stages may refine earlier -sections but must not duplicate them. Use compact tables where they improve -five-way comparison. Keep detailed evidence in the relevant review section and -keep the divergence and duplication registers concise. - -Each divergence-register entry must have: - -- a stable identifier such as `D-01`; -- affected modules; -- observed difference; -- provisional classification from the audit strategy; -- evidence links; -- impact or reason it may be harmless; and -- status: open, confirmed, intentional, rejected, or superseded. - -Each duplication-register entry must have: - -- a stable identifier such as `R-01`; -- participating modules; -- repeated responsibility; -- exact, structural, or policy-duplication classification; -- meaningful differences; -- candidate owner, if any; -- evidence links; and -- provisional outcome: extract, harmonize without sharing, retain separately, - or defer. - -Do not assign final severity until Stage 5 has traced ownership and evaluated -impact. - -## Stage 1: Inventory And Convention Matrix +## Stage 1: Protect Spell And NPC Mapping Identity ### Objective -Establish the complete, factual five-module baseline and identify convention -differences without yet recommending shared abstractions. +Close the stale-checkpoint gap before changing any mapping or ordering +behavior. -### Required review +### Scope -For every extractor, inspect: +Change only the spell and NPC extractor packages and the narrow integration or +checkpoint tests needed to prove their prepared fingerprints affect reuse. -- production and test file inventory; -- module key, artifact kind, capabilities, and execution class; -- constructor inputs and retained prepared state; -- option decoding and unknown-option behavior; -- `ModuleSpec`, registration builder, and reference slots; -- manifest metadata and checkpoint fingerprint providers; -- exported versus package-private surface; -- embedded asset registration; -- principal extractor entry point and result type; and -- package-local test organization. +### Implementation -Trace registration into production composition far enough to confirm that the -declared contract is the one actually selected. Inspect neighboring packages -only as required to verify an identity or ownership fact. +1. Add an unexported mapping-policy constant to each extractor: -### Write to `audit.md` + ```go + // internal/modules/dnd/extract/spells + mappingPolicy = "dnd.spells.extract_mapping.v1" -Create `## Audit Results`, mark it `Status: In progress`, and add: + // internal/modules/dnd/extract/npcs + mappingPolicy = "dnd.npcs.extract_mapping.v1" + ``` -1. `### Baseline And Module Matrix` - - one row per module; - - columns for every comparison dimension in the audit strategy; - - concise facts with links rather than judgments; and - - an explicit note where a dimension is not applicable. -2. `### Divergence Register` - - record every observed organizational, naming, construction, registration, - provenance, or test-layout difference; - - classify only as required specialization, permitted variation, convention - drift, architectural divergence, or undetermined; and - - do not propose helper extraction in this stage. +2. In both packages: -Also add a short baseline-validation note recording: + - add `"mapping_policy": mappingPolicy` to `ManifestMetadata`; + - add `{Name: "mapping_policy", Value: mappingPolicy}` to + `CheckpointFingerprints`; + - preserve all existing prompt, response-schema, catalog, registry, + identity, and digest metadata and fingerprints; and + - preserve nil-receiver behavior as it exists at the start of this stage. + +3. Do not change mapping, source-reference ordering, prompt or schema content, + or artifact output in this stage. + +### Tests + +- Extend each package's metadata/fingerprint test to prove that the mapping + policy is present with the declared semantic value and that no existing + fingerprint disappears. +- In the existing prepared-pipeline integration coverage for spells and NPCs, + assert that the scoped prepared fingerprint list contains: + + - `extract::dnd/spells:mapping_policy`; and + - `extract::dnd/npcs:mapping_policy`; + + using the actual lane IDs from the fixtures. +- Add or extend one checkpoint-resume test at the prepared pipeline boundary + to prove that a differing or missing scoped mapping-policy fingerprint + prevents reuse with the normal identity-mismatch reason. Do not duplicate + the checkpoint loader's complete mismatch matrix in both lane suites. + +### Validation + +Run: + +```sh +go test ./internal/modules/dnd/extract/spells +go test ./internal/modules/dnd/extract/npcs +go test ./internal/modules/integration +go test ./internal/framework/checkpoint ./internal/framework/pipeline +git diff --check +``` + +### Completion Gate + +Spell and NPC mapping policies participate in manifest provenance, prepared +checkpoint identity, and restore decisions, while extraction output remains +unchanged. + +## Stage 2: Introduce Document-Aware Source-Reference Ordering + +### Objective + +Create the one shared D&D source-reference primitive accepted by the audit and +establish it with the already document-aware NPC-interaction consumers. + +### Scope + +Change `internal/modules/dnd/shared`, +`internal/modules/dnd/npcinteractions`, and the NPC-interaction normalizer and +invariant validator that currently call the model helper. Do not migrate the +other extractors or normalizers yet. + +### Shared API + +Add a document-position index with this public surface: + +```go +type SourceRefOrder struct { + // private immutable snapshot +} + +func NewSourceRefOrder(doc *source.SourceDocument) SourceRefOrder +func (o SourceRefOrder) Less(left, right source.SourceRef) bool +func (o SourceRefOrder) EarliestValid(refs []source.SourceRef) (position int, ok bool) +func (o SourceRefOrder) Canonicalize(refs []source.SourceRef) []source.SourceRef +``` + +The constructor must snapshot only the source ID and unit-ID-to-position map; +the returned value must not retain or mutate the document. + +### Required Semantics + +`Less` must preserve the established NPC-interaction ordering: + +1. compare differing source IDs lexically; +2. after equal source IDs, order resolvable start endpoints by indexed + document position and before unresolvable start endpoints; +3. use the literal start unit ID as the deterministic tie or invalid fallback; +4. apply the same document-position, resolvability, and literal fallback rules + to end endpoints; and +5. return false for exactly equal references. + +`EarliestValid` must: + +- consider only references accepted by `source.ValidateRef` for the indexed + document; +- return the smallest document position of a valid start endpoint; +- ignore invalid candidates without mutating or deleting them; and +- return `(0, false)` for a nil document, an empty set, or no valid reference. + +`Canonicalize` must: + +- return `nil` for nil input and an independently owned non-nil empty slice for + non-nil empty input; +- clone the input; +- stable-sort it with `Less`; +- remove only exactly equal `source.SourceRef` values after sorting; and +- never rewrite, repair, or discard a distinct invalid candidate. + +A zero-value `SourceRefOrder` must be safe and deterministic, using literal +fallback ordering and treating every reference as invalid for +`EarliestValid`. + +### Migration + +1. Replace the implementation of the focused + `internal/modules/dnd/npcinteractions` canonicalization/ordering helpers + with the shared primitive. +2. Update its normalizer and invariant validator to use + `shared.NewSourceRefOrder(doc)` directly where practical. +3. Remove the old exported `SourceRefLess` and `CanonicalizeSourceRefs` + functions if no production or test caller remains. Retain + NPC-interaction-specific exact artifact identity and list comparison in the + focused model package. +4. This migration must preserve NPC-interaction output exactly, so do not bump + its normalization, validation, or extraction policy in this stage. + +### Tests + +Shared table tests must cover: + +- non-monotonic document unit IDs; +- valid and invalid endpoints; +- references to another source ID; +- stable ties; +- exact duplicates versus merely similar ranges; +- nil and non-nil empty inputs; +- input/output alias safety; +- mutation of the source document after constructing the index; and +- `EarliestValid` ignoring invalid candidates. + +Retain or adapt focused NPC-interaction tests to prove its artifact ordering +and invariant behavior. Do not add tests that merely assert that a package +calls the shared helper. + +### Validation + +Run: + +```sh +go test ./internal/modules/dnd/shared +go test ./internal/modules/dnd/npcinteractions +go test ./internal/modules/dnd/normalize/npcinteractions +go test ./internal/modules/dnd/validate/npcinteractions/... +git diff --check +``` + +### Completion Gate + +The shared API has exhaustive behavioral protection, NPC interactions retain +their prior output, and no duplicate document-aware comparator remains in the +NPC-interaction model. + +## Stage 3: Migrate Normalization And Invariant Consumers + +### Objective + +Make downstream spell, NPC, and combat-turn canonicalization use document +order before changing extractor output. + +### Scope + +Change only: + +- `internal/modules/dnd/normalize/spells`; +- `internal/modules/dnd/normalize/npcs`; +- `internal/modules/dnd/normalize/combatturns`; and +- `internal/modules/dnd/validate/combatturns/invariants`. + +### Implementation + +1. Construct one `shared.SourceRefOrder` from the normalization or validation + request's source document and pass it through the relevant local operation. + Do not rebuild the index inside artifact or reference loops. +2. Replace numeric source-reference sorting, exact deduplication, and repeated + `source.UnitIndex` scans with `Less`, `Canonicalize`, and `EarliestValid` as + applicable. +3. Keep these responsibilities local: + + - spell catalog canonicalization and duplicate spell identity; + - NPC identity and grouping; + - combat actor identity, turn comparison, warnings, and duplicate policy; + - repair/warning counts and message construction; and + - artifact-specific tie-breakers. + +4. Derive local repair facts without extending the shared API: + + - compare the original and canonical slices to determine whether order + changed; + - compute exact duplicates removed from input and output lengths; and + - preserve the current warning reason codes and useful diagnostic context. + +5. Update checkpoint identity for every changed semantic owner: + + - add spell normalizer metadata and fingerprint + `normalization_policy = "dnd.spells.normalize.v1"` because it currently + has no local normalization-policy fingerprint; + - change NPC normalization policy to + `dnd.npcs.normalize.v2`; + - change combat-turn normalization policy to + `dnd.combat_turns.normalize.v2`; and + - change the combat-turn normalized-invariant validator policy to + `dnd.combat_turns.validator.normalized.v2`. + + Preserve the spell catalog fingerprint as a separate semantic input. + +6. Delete superseded local numeric reference comparators and earliest-position + scans after all callers in these packages move. + +### Tests + +For each affected artifact family, add or adapt focused tests using a valid +document whose unit IDs are deliberately non-monotonic. Prove: + +- source references follow document order; +- artifact order uses the earliest valid document position where applicable; +- invalid candidates remain deterministic and available to validators; +- exact duplicate and warning/repair behavior is unchanged except for the + corrected ordering; and +- returned slices do not alias inputs. + +Update metadata/fingerprint tests for the exact semantic policy values above. +Do not duplicate all shared `SourceRefOrder` edge cases in each package. + +### Validation + +Run: + +```sh +go test ./internal/modules/dnd/normalize/spells +go test ./internal/modules/dnd/normalize/npcs +go test ./internal/modules/dnd/normalize/combatturns +go test ./internal/modules/dnd/validate/combatturns/invariants +git diff --check +``` + +### Completion Gate + +All downstream consumers in scope use one document-position index per +operation, non-monotonic IDs are handled correctly, local artifact semantics +remain local, and every changed semantic policy invalidates prior checkpoints. + +## Stage 4: Migrate Spell And NPC Extraction + +### Objective + +Correct source-reference and artifact ordering in the spell and NPC +extractors. + +### Scope + +Change only the spell and NPC extractor packages plus their focused tests. + +### Implementation + +1. Build one `shared.SourceRefOrder` from `req.Source` after common request + prerequisites have passed. +2. Use `Canonicalize` for mapped durable source references. Preserve: + + - model-produced invalid candidates; + - attachment of the current transcript source ID; + - exact-only deduplication; + - package-owned DTO conversion and identity logic; and + - independent ownership of returned artifacts and references. + +3. For spell artifact ordering, replace numeric `earliestSourceUnit` behavior + with `EarliestValid` document positions: + + - artifacts with valid evidence sort by their earliest valid transcript + position; + - valid-evidence artifacts sort before artifacts with no valid evidence; + - existing artifact-specific deterministic tie-breakers remain in their + current order; and + - invalid evidence is retained even though it does not select the earliest + position. + +4. Use document-aware reference comparison in NPC ordering while preserving + NPC identity and artifact tie-breakers. +5. Remove the superseded local numeric comparators and scans. +6. Bump the Stage 1 mapping policies: + + - `dnd.spells.extract_mapping.v2`; and + - `dnd.npcs.extract_mapping.v2`. + + Update both manifest metadata and checkpoint tests through the constants. + +### Tests + +Add focused regression fixtures with non-monotonic unit IDs for: + +- spell reference ordering and spell artifact ordering; +- NPC reference and artifact ordering; +- mixed valid and invalid evidence; +- exact duplicate references; +- stable artifact ties; and +- output mutation safety. + +Retain existing malformed model-output, provider-error, catalog, identity, and +reference-grounding coverage. + +### Validation + +Run: + +```sh +go test ./internal/modules/dnd/extract/spells +go test ./internal/modules/dnd/extract/npcs +git diff --check +``` + +### Completion Gate + +Spell and NPC extraction follow transcript order for valid evidence, preserve +invalid candidates deterministically, and advertise the new mapping semantics +through v2 policy fingerprints. + +## Stage 5: Migrate Combat-Turn And NPC-Interaction Extraction + +### Objective + +Complete source-reference harmonization across the four citation extractors +and remove superseded reference APIs. + +### Scope + +Change the combat-turn and NPC-interaction extractor packages and the D&D +shared package only. + +### Implementation + +1. In both extractors, construct one `shared.SourceRefOrder` from the request + source and use `Canonicalize` for mapped durable references. +2. Preserve actor/NPC identity, enum candidates, artifact ordering, + transcript-source attachment, warnings, provider behavior, and local DTO + mapping. +3. Bump mapping policies: + + - `dnd.combat_turns.extract_mapping.v2`; and + - `dnd.npc_interactions.extract_mapping.v2`. + +4. Delete all superseded local numeric source-reference comparators. +5. Delete `shared.SourceRefCandidate` and its isolated test. DTO-to-durable + mapping must continue to attach the trusted current document source ID + locally and must not trust a model-supplied source identity. +6. Search the complete D&D module tree for remaining numeric comparisons of + `SourceRef` endpoints. Retain a local comparator only if it implements a + documented artifact-specific policy; otherwise migrate it to + `SourceRefOrder`. + +### Tests + +Add focused non-monotonic-ID regression tests for both extractors, including +invalid candidates, exact duplicates, stable ties, and mutation safety. Update +metadata/fingerprint expectations for the v2 mapping policies. + +Do not add a test asserting that `SourceRefCandidate` or a local comparator is +absent; compilation and behavioral tests are sufficient. + +### Validation + +Run: + +```sh +go test ./internal/modules/dnd/extract/combatturns +go test ./internal/modules/dnd/extract/npcinteractions +go test ./internal/modules/dnd/shared +go test ./internal/modules/dnd/... +git diff --check +``` + +### Completion Gate + +All citation extractors use the shared document-aware mechanics, no unsafe +model-source candidate helper remains, and changed mapping behavior has explicit +checkpoint identity. + +## Stage 6: Consolidate Common Extraction Preflight + +### Objective + +Give the five extractors one owner for their common request prerequisites while +keeping lane-specific checks and error context local. + +### Scope + +Change `internal/modules/dnd/shared` and the five extractor packages. + +### Shared API + +Replace `ChunkPromptMaterial` with: + +```go +func PrepareChunkExtraction( + ctx context.Context, + req contracts.TypedExtractionRequest, +) (contracts.LLMInputMaterial, error) +``` + +### Required Semantics + +The helper must validate in this order: + +1. context is non-nil; +2. the context has no existing error; +3. source is non-nil; +4. chunk is non-nil; +5. the chunk contains at least one materialized unit; and +6. source input is cloned/defaulted and its content exactly matches the chunk + content. + +Preserve the current material defaults for name, media type, and size. The +returned `LLMInputMaterial` and its content must not alias the request. + +The helper returns domain-neutral error details without an extractor name. Each +extractor must wrap helper failures through its existing local error function +so diagnostics retain lane context. + +### Migration + +For all five extractors: + +- keep nil receiver and nil LLM-client checks local and before the shared + helper; +- call the helper once before specialized reference projection, provider + invocation, or mapping; +- retain catalog/NPC registry and lane-specific checks locally; +- preserve the existing provider-error prefixes and result types; and +- remove duplicated context/source/chunk/unit/material checks. + +Delete the exported `ChunkPromptMaterial` function after the fifth caller +migrates. Do not retain an alias solely for internal compatibility. + +### Tests + +- Move the full common preflight matrix to table-driven shared tests: + nil context, canceled context, nil source, nil chunk, empty units, + mismatched content, defaulted material, preserved explicit material, and + mutation safety. +- Each extractor package retains only tests that add value locally: + nil receiver/client, one representative wrapped preflight error, + specialized dependency/reference behavior, provider failure, and mapping. +- Remove redundant per-package common-branch tests once the shared contract + owns them; do not preserve five copies for coverage symmetry. + +### Validation + +Run: + +```sh +go test ./internal/modules/dnd/shared +go test ./internal/modules/dnd/extract/... +git diff --check +``` + +### Completion Gate + +All five extractors use `PrepareChunkExtraction`, validation order and error +context are preserved, common branches have one test owner, and no exported +compatibility alias remains. + +## Stage 7: Make Deterministic Validation Own Scene Semantics + +### Objective + +Remove duplicated semantic policy from the scene-description private transport +schema. + +### Scope + +Change only the scene-description extractor's private schema and focused schema +and extraction tests, plus shape-validator tests if a missing semantic case is +discovered. + +### Implementation + +1. In `dnd_scene_descriptions_llm.v1.json`, retain: + + - the schema dialect and ID; + - the top-level object type; + - `additionalProperties: false`; + - required `kind`, `title`, and `summary` fields; and + - string types for all three fields. + +2. Remove: + + - the `kind` enum; and + - `minLength` from `title` and `summary`. + +3. Do not change `SchemaVersion`, `ResponseSchemaKey`, `ResponseSchemaID`, + `ResponseSchemaName`, the private DTO, prompt text, or durable artifact + schema. The schema content digest will change and must naturally invalidate + prior extractor checkpoint identity. +4. Keep the scene-description shape validator as the sole owner of supported + kinds and non-blank title/summary semantics. + +### Tests + +- Update private-schema tests so unsupported kinds and empty strings are valid + transport values, while missing fields, wrong JSON types, unknown fields, + collections, and malformed JSON remain rejected. +- Ensure shape-validator tests cover unsupported kind, empty and whitespace-only + title, and empty and whitespace-only summary. +- Add one extractor-level test proving a structurally valid but semantically + invalid provider response is decoded and returned for deterministic + validation rather than rejected by the private schema boundary. +- Assert the schema digest is valid and mutation-safe, but do not freeze its + exact hash. + +### Validation + +Run: + +```sh +go test ./internal/modules/dnd/extract/scenedescriptions +go test ./internal/modules/dnd/validate/scenedescriptions/shape +git diff --check +``` + +### Completion Gate + +The private schema owns only transport structure, the deterministic validator +owns scene semantics, and the changed schema digest invalidates stale +checkpoints without a version rename. + +## Stage 8: Protect Prompt Order And Cache Boundaries + +### Objective + +Give all four citation extractors the same level of behavioral protection as +the scene-description prompt without freezing prompt content. + +### Scope + +Change only the Scriptorium asset tests for spells, NPCs, combat turns, and NPC +interactions. Do not edit manifests or prompt assets unless a test exposes an +actual mismatch with the already documented current contract. + +### Expected Prepared Sequences + +Using each package's real embedded registry and representative inputs, assert +these complete ordered message identities: + +| Lane | Ordered messages | +| --- | --- | +| NPC | system, extraction evidence, identity, campaign references, task, instructions, transcript | +| Spell | system, extraction evidence, identity, campaign references, NPC registry, spell catalog, task, instructions, transcript | +| Combat turn | system, extraction evidence, identity, campaign references, NPC registry, task, instructions, transcript | +| NPC interaction | system, extraction evidence, identity, campaign references, names-only NPC registry, task, instructions, transcript | + +For NPCs, assert ephemeral cache boundaries on identity, campaign references, +and instructions. For spells, combat turns, and NPC interactions, assert +ephemeral boundaries on identity, campaign references, the NPC registry, and +instructions. Assert that every other message, including the final transcript, +has no cache control. + +Use the prepared message role plus rendered input/source identity already +available from Scriptorium to distinguish messages. If an asset-only message +does not expose a stable identity, assert its position and role without +asserting exact prose. + +### Tests + +Consolidate overlapping assertions within each package where that improves +clarity. Retain existing asset registration, content-safety, and prompt digest +tests when they protect distinct risks. + +Do not assert: + +- exact prompt text; +- prefix length or total byte count; +- an exact prompt hash; +- private manifest file layout; or +- that a particular shared helper or asset path was used. + +### Validation + +Run: + +```sh +go test ./internal/modules/dnd/extract/spells +go test ./internal/modules/dnd/extract/npcs +go test ./internal/modules/dnd/extract/combatturns +go test ./internal/modules/dnd/extract/npcinteractions +git diff --check +``` + +### Completion Gate + +All four suites protect the complete documented role/input order and cache +flags through real prompt preparation, without change-detector assertions. + +## Stage 9: Package Hygiene And Extractor Documentation + +### Objective + +Remove the remaining misleading package surface and document the common +contract future D&D extractors must follow. + +### Scope + +Change the spell and combat-turn extractor packages and +`docs/internal/modules.md`. Include no new production abstraction. + +### Code Cleanup + +1. Delete the unused singular `ArtifactType` constants from the spell and + combat-turn extractor packages. `ArtifactKind` and the durable typed model + remain authoritative. +2. Add the same nil guard used by the other extractors to spell + `ManifestMetadata`: + + ```go + if e == nil { + return nil + } + ``` + +3. Add one focused zero-value spell metadata test. Do not add tests for the + absence of deleted constants. + +### Documentation + +Update `docs/internal/modules.md` as the canonical current-behavior owner: + +1. Replace the stale `ChunkPromptMaterial` consumer description with + `PrepareChunkExtraction` and name all five current extractor consumers. +2. Add a compact `### D&D Extractor Contract` subsection within or immediately + after `## Adding An Extension`. +3. State requirements as behaviors and ownership boundaries, not mandatory + filenames: + + - reject unknown options unless an option namespace is intentionally + extensible; + - use shared common preflight while retaining receiver, dependency, and + lane-specific checks locally; + - return independently owned results and exposed metadata safe for caller + mutation; + - keep the private response DTO, structural schema and identity, + provider-response mapping, durable conversion, and lane diagnostics + package-owned; + - include every stable semantic input capable of changing durable output in + checkpoint identity, considering prompts, schemas, mapping, + canonicalization, prepared reference projections, identity, + normalization, and trimming as applicable; and + - consider focused behavioral coverage for construction/registration, + option rejection, preflight, provider failure, structured decoding, + mapping/ownership, prompt role/input/cache order, and checkpoint + invalidation. + +4. Link rather than duplicate: + + - prompt ordering, shared-asset, cache, and private-schema rules in + `docs/internal/llm.md`; + - checkpoint and reference behavior in `docs/internal/pipeline.md`; + - architecture ownership rules; and + - the testing policy. + +5. Explicitly avoid prescribing exact prompt content or length, hashes, test + counts, filenames, fixture layouts, or generic implementation builders. + +### Validation + +Run: + +```sh +go test ./internal/modules/dnd/extract/spells +go test ./internal/modules/dnd/extract/combatturns +go test ./internal/modules/dnd/shared +git diff --check +``` + +Validate every new documentation link and confirm the text describes behavior +implemented by Stages 1 through 8. + +### Completion Gate + +The misleading exports are gone, spell metadata is nil-safe, the shared +preflight documentation is current, and future extractors have one concise +normative checklist linked to the canonical detailed policies. + +## Stage 10: Integrated Regression And Audit Closure + +### Objective + +Verify the complete refactor as one system and remove any residual duplication +or stale references introduced or exposed by the migration. + +### Scope + +This is a verification and narrowly scoped correction stage. Do not introduce +new abstractions or expand product behavior. + +### Required Review + +1. Re-read every audit finding and confirm it is addressed: + + - document-aware reference and spell artifact ordering; + - spell/NPC mapping fingerprints; + - shared common extraction preflight; + - deterministic ownership of scene semantics; + - complete prompt-order/cache tests; + - package hygiene; and + - the documented D&D extractor contract. + +2. Search for and resolve only genuine leftovers: + + - numeric ordering of D&D `SourceRef` endpoints where document order is the + intended policy; + - production calls to removed helpers; + - stale references to `ChunkPromptMaterial`, `SourceRefCandidate`, the + deleted NPC-interaction comparators, or deleted `ArtifactType` constants; + - missing policy bumps for behavior changed by this plan; + - raw schema or prompt bytes in diagnostics; and + - documentation that still describes pre-refactor behavior. + +3. Confirm the intentional differences and rejected abstractions in the audit + remain intact. +4. Confirm each changed fingerprint value is bounded, deterministic, + non-secret, and included at the correct component scope. +5. Confirm no test added by this work is a prefix-length, exact-hash, + helper-usage, or file-layout change detector. + +### Validation + +Run the full repository checks: ```sh go test -count=1 ./... @@ -137,289 +819,18 @@ gofmt -l . git diff --check ``` -If a command fails, record the exact command, affected package, and concise -failure classification. Do not fix it during the audit. +`gofmt -l .` must print no files. If a command fails, diagnose and correct only +failures caused by this implementation. Report unrelated pre-existing failures +without modifying their owners. -### Completion gate +### Completion Gate -Stage 1 is complete when every matrix cell is populated or marked not -applicable, every observed baseline divergence has a stable register entry, and -the repository's starting validation state is recorded. - -## Stage 2: Prompts, Schemas, And LLM Boundaries - -### Objective - -Determine whether the five modules consistently present stable context, -references, lane instructions, transcripts, and structured-output contracts to -the LLM, and identify exact or near-duplicate assets without changing them. - -### Required review - -For every extractor: - -- read the complete prompt manifest and every referenced local and shared - asset; -- record the exact ordered message sequence, role, input, and cache-control - boundary; -- compare shared message files by identity and bytes, not by paraphrased - meaning; -- verify stable-to-variable ordering against `docs/internal/llm.md`; -- map declared prompt inputs to module reference slots and generated inputs; -- trace optional, required, empty, and generated reference projections; -- inspect prompt and schema registration, hashing, and diagnostic redaction; -- inspect the complete private response schema for identity, required fields, - nullability, strict objects, and semantic constraints; -- map schema fields to private DTOs and response mapping; and -- compare prompt and schema tests at their behavioral boundaries. - -Treat the scene-description whole-chunk evidence model as a specialization to -explain, not a presumption of drift. Likewise, treat catalog and NPC registry -inputs as subset-specific responsibilities unless evidence shows inconsistent -handling of the same contract. - -### Write to `audit.md` - -Add `### Prompt, Schema, And LLM Review` containing: - -- a five-way prompt-order and cache-boundary table; -- a prompt-input and reference-projection table; -- a private-schema and DTO ownership table; -- exact shared-asset usage; -- local assets with identical or near-identical content; -- content-safety and diagnostic observations; and -- evidence-backed deviations from documented LLM conventions. - -Update the divergence register for confirmed or newly discovered prompt, -schema, reference, provenance, and testing differences. - -Create `### Duplication Register` and add prompt-, schema-, metadata-, and -asset-related candidates. For each prompt candidate, state whether the text is -byte-identical, merely similar, or semantically different. Do not recommend a -shared prompt asset unless all intended consumers should receive future edits -atomically. - -### Completion gate - -Stage 2 is complete when every prompt message and input is accounted for, every -private schema field has an owner, cache-prefix claims are based on exact -message identity, and every asset-sharing candidate has a provisional keep or -share outcome. - -## Stage 3: Extraction, Evidence, And Canonicalization - -### Objective - -Compare runtime extraction flow and identify repeated algorithms or policy -without erasing artifact-specific semantics. - -### Required review - -For every extractor, inspect and trace: - -- request, context, source, chunk, and dependency validation; -- construction-time state versus operation-time overrides; -- structured request assembly and provider-error wrapping; -- response-to-artifact mapping; -- source identity attachment and unit-ID resolution; -- evidence range validation assumptions; -- canonical source-reference ordering and exact deduplication; -- artifact ordering and deterministic tie-breakers; -- enum or canonical-name handling; -- warnings and diagnostics; -- cloning and aliasing boundaries; and -- focused extractor, model, canonicalization, and malformed-output tests. - -Compare complete algorithms rather than function names alone. For similar -canonicalization helpers, identify which parts are: - -- identical source-reference mechanics; -- artifact-specific mapping; -- artifact-specific ordering policy; or -- validation that belongs to a later validator rather than extraction. - -Use call traces and complexity data to inspect repeated scans, allocations, -serialization, or high-cognitive-complexity paths. Report performance only when -a plausible workload and complexity impact exist. - -### Write to `audit.md` - -Add `### Extraction And Canonicalization Review` containing: - -- a five-way extraction-flow table; -- an evidence and ordering-policy table; -- clone, mutation, error, and warning observations; -- test-ownership comparisons; and -- code-quality or performance candidates with concrete impact. - -Update the divergence register, revising earlier provisional classifications -where runtime evidence explains or contradicts them. - -Expand the duplication register with request-validation, reference, -canonicalization, ordering, deduplication, mapping, error, and test-support -candidates. Give each candidate a provisional owner and explicitly identify the -artifact-specific code that must remain local. - -### Completion gate - -Stage 3 is complete when the full extraction path of all five modules is -accounted for, every similar canonicalization path has been decomposed into -shared mechanics versus domain policy, and every runtime duplication candidate -has a provisional ownership decision. - -## Stage 4: Contextual Architecture And Ownership Review - -### Objective - -Validate candidate findings against the surrounding D&D and framework -architecture, and decide which apparent similarities should actually be shared. - -### Required review - -For every open divergence and duplication candidate: - -- trace callers and consumers; -- inspect existing facilities in `internal/modules/dnd/shared`; -- inspect focused D&D registry, identity, catalog, codec, normalize, and - validate packages as relevant; -- verify the typed artifact and reference contracts; -- inspect production registration, default validator composition, and - checkpoint fingerprint assembly; -- verify documentation ownership and current durable contracts; and -- check whether a proposed generic helper has a genuine domain-neutral owner. - -Apply the shared-code hierarchy from `audit.md`: - -1. module-owned artifact semantics; -2. D&D-wide shared mechanics; -3. focused subset-specific D&D packages; and -4. framework-owned domain-neutral behavior. - -Reject or defer candidates whose API would require artifact-specific callbacks, -type erasure, module-key branching, a broad configuration object, or speculative -future consumers. - -### Write to `audit.md` - -Add `### Contextual Architecture And Ownership Review` containing: - -- traced ownership evidence for every open candidate; -- dependency-direction and layer-boundary conclusions; -- checkpoint, provenance, and reference compatibility conclusions; -- documentation or test ownership implications; and -- any product-contract questions that cannot be decided as refactors. - -Add `### Candidate Decisions`, with one row per divergence and duplication -identifier. Choose exactly one outcome: - -- extract now; -- harmonize without sharing; -- retain intentionally separate; -- reject as harmful abstraction; or -- defer pending a named missing requirement or product decision. - -For extract or harmonize outcomes, specify: - -- target owner; -- minimal responsibility and proposed API shape; -- participating modules; -- behavior that remains package-owned; -- migration order; -- relevant tests; and -- principal risks. - -Update every register entry to confirmed, intentional, rejected, superseded, or -explicitly deferred. No entry may remain merely open at the end of this stage. - -### Completion gate - -Stage 4 is complete when all candidates have traced ownership and a final -keep/share/harmonize/defer decision, no proposed helper violates dependency -direction, and all required product decisions are separated from executable -refactoring recommendations. - -## Stage 5: Synthesis And Final Audit - -### Objective - -Turn the working evidence into one concise, internally consistent audit that -can support roadmap decisions and a later implementation plan. - -### Required work - -1. Re-read the complete strategy and all working audit results. -2. Recheck every cited file and symbol against the current working tree. -3. Reconcile duplicate, overlapping, or contradictory observations. -4. Assign severity only to confirmed actionable findings: - - **high:** correctness, security, data integrity, or architectural failure - with substantial impact; - - **medium:** meaningful drift, duplication, or design weakness likely to - cause defects or costly divergence; - - **low:** localized maintainability, clarity, test-quality, or - documentation issue with limited immediate impact. -5. Keep optional improvements separate from findings. -6. Confirm intentional specializations and rejected sharing candidates are - documented so future work does not repeatedly reopen them without evidence. -7. Run the repository-wide validation commands from Stage 1 again and record - the final result. Do not modify code in response to failures. - -### Rewrite `audit.md` - -Preserve the strategy sections above `## Audit Results`, but replace the -provisional working area with this final structure: - -```markdown -## Audit Results - -Status: Complete - -### Executive Conclusion -### Final Module Comparison Matrix -### Prioritized Findings -### Intentional Differences To Preserve -### Shared-Code Decisions -### Rejected Or Deferred Candidates -### Recommended Refactoring Sequence -### Validation And Residual Risks -``` - -Requirements for the final sections: - -- `Executive Conclusion` directly answers the user's three audit questions. -- `Final Module Comparison Matrix` remains factual and compact. -- `Prioritized Findings` follows the finding standard in `audit.md`, is ordered - by severity and impact, and cites exact evidence. -- `Intentional Differences To Preserve` explains why harmonization would be - incorrect. -- `Shared-Code Decisions` records extract and harmonize recommendations, - proposed ownership, and why sharing is better than continued separation. -- `Rejected Or Deferred Candidates` records superficially attractive - abstractions and why they should not be pursued now. -- `Recommended Refactoring Sequence` groups accepted recommendations into - independently safe, dependency-ordered scopes. It is a sequence, not a - decision-complete implementation plan. -- `Validation And Residual Risks` records commands, results, limits of the - static audit, and any human or model-quality evaluation still needed. - -Remove the working divergence and duplication registers after their evidence -has been incorporated into the final sections. Do not leave raw stage notes, -provisional severities, or superseded conclusions in the completed audit. - -### Completion gate - -Stage 5 is complete when: - -- the final audit satisfies every deliverable and completion criterion in - `audit.md`; -- the three user questions receive explicit answers; -- every recommendation has evidence, ownership, scope, and rationale; -- no finding relies only on similarity or stylistic preference; -- validation results are current; -- no code or current-behavior files changed; and -- `git status --short` shows only the intended `audit.md` audit-result changes - plus any pre-existing user changes. +All audit findings are implemented, every repository-wide check passes except +any clearly reported pre-existing failure, current-behavior documentation is +accurate, and no superseded helper or policy copy remains. ## Open Questions -None. The audit stages, working-document structure, decision rules, and final -deliverable are fully specified. +None. The audit findings, API ownership, semantic policy values, migration +order, test boundaries, documentation owner, and intentionally deferred +product decisions are specified above.