diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md new file mode 100644 index 0000000..44dfde0 --- /dev/null +++ b/docs/roadmap/audit.md @@ -0,0 +1,766 @@ +# D&D Extraction Module Refactoring Audit Strategy + +Status: Strategy complete; final audit results recorded below + +## Purpose + +Define a disciplined audit of the five production D&D extraction modules: + +- `dnd/spells`; +- `dnd/npcs`; +- `dnd/combat-turns`; +- `dnd/npc-interactions`; and +- `dnd/scene-descriptions`. + +The audit will determine whether these modules follow a coherent set of +conventions, whether repeated implementation can be replaced by appropriately +scoped shared code or assets, and whether the accumulated design suggests other +maintainability improvements. + +This document defines how to perform the audit. It does not contain audit +findings and does not authorize production changes. + +## Audit Principles + +The audit must distinguish consistency from uniformity. The five modules should +use the same conventions where they perform the same responsibility, but a +module should remain different when its artifact semantics, evidence model, +reference requirements, or normalization policy require it. + +Recommendations must follow these principles: + +- Prefer evidence from current code, tests, prompts, schemas, configuration, + and documentation over naming or visual similarity. +- Treat a divergence as a finding only when it is unexplained, increases + maintenance or correctness risk, or violates an intended convention. +- Do not recommend an abstraction solely to reduce line count. A shared helper + must own one coherent invariant and make future correct changes easier. +- Preserve package ownership of artifact semantics, private response DTOs, + private structured-output schemas, and lane-specific prompts. +- Keep D&D-specific behavior in D&D packages. Move behavior into a generic + framework package only when the contract is demonstrably domain-neutral and + has a non-D&D consumer or a clear framework-owned responsibility. +- Evaluate prompt sharing by byte identity and semantic ownership. Prompt + caching benefits only when repeated message content and ordering are exactly + identical. +- Apply the testing policy to proposed refactors. Prefer behavioral protection + at stable boundaries and do not add change-detector tests for helper usage, + prompt length, exact hashes, or private file layout. + +## Scope + +### Primary scope + +Inspect the complete package-owned implementation beneath: + +- `internal/modules/dnd/extract/spells`; +- `internal/modules/dnd/extract/npcs`; +- `internal/modules/dnd/extract/combatturns`; +- `internal/modules/dnd/extract/npcinteractions`; and +- `internal/modules/dnd/extract/scenedescriptions`. + +For each package, include: + +- module identity, capabilities, construction, options, registration, and + execution class; +- reference-slot declarations and construction-time or operation-time + reference handling; +- checkpoint fingerprints and manifest metadata; +- request validation and structured LLM request preparation; +- private response DTOs, response-schema loading, and response mapping; +- source-reference resolution, evidence attachment, canonicalization, + ordering, and exact deduplication; +- embedded prompt manifests, shared and local assets, message order, cache + boundaries, and schema assets; +- errors, warnings, diagnostics, cloning, and mutation safety; and +- package-local tests and test support. + +### Contextual scope + +Inspect a neighboring component only when needed to determine ownership, +duplication, or compatibility: + +- `internal/modules/dnd/shared` and focused D&D subpackages used by more than + one extractor; +- the five artifact model and codec contracts; +- corresponding merge, normalize, and validate variants; +- production registration and default validator composition; +- pipeline reference, fingerprint, and LLM contracts; +- canonical current-behavior documentation and integration contracts; and +- representative production and integration tests. + +Contextual inspection is not a request to redesign every lane stage. Findings +outside extraction should be reported only when they directly explain an +extractor inconsistency or reveal a misplaced responsibility. + +### Exclusions + +Do not use this audit to: + +- change durable artifact schemas or extraction policy; +- redesign the fixed pipeline shape or ordered-step model; +- combine distinct artifacts into a larger D&D result; +- evaluate live-model output quality; +- introduce schema generation, a dependency-injection framework, or a general + module superclass; +- move domain rules into the generic framework; +- rewrite tests merely to make their file layout look alike; or +- implement any recommended refactor. + +If the audit exposes a product-contract concern, record it separately from +refactoring recommendations and identify the additional decision required. + +## Comparison Method + +### 1. Establish a module inventory + +Create one row per module in a working comparison matrix. Record exact current +facts rather than inferred conventions: + +| Dimension | Facts to record | +| --- | --- | +| Identity | Module key, artifact kind, capabilities, execution class | +| Files | Production files, embedded assets, focused test files | +| Construction | Dependencies, options, reference decoding, immutable prepared state | +| Registration | `ModuleSpec`, builder, option validation, declared slots | +| Provenance | Manifest metadata and checkpoint fingerprint keys and values | +| Prompt | Prompt ID/version, manifest messages, inputs, shared assets, cache boundaries | +| Schema | Private schema identity, strictness, loader, diagnostics behavior | +| Execution | Request validation, LLM call, response mapping, errors and warnings | +| Evidence | Source identity, range resolution, canonicalization, ordering, deduplication | +| Tests | Contract owner, malformed cases, integration coverage, test-only helpers | + +Use the matrix to identify exact agreement, intentional variation, and +unexplained variation. Do not infer a preferred convention from whichever +module was implemented first. Determine the preferred shape from architecture, +current documentation, shared contracts, and the clearest implementation. + +### 2. Classify every divergence + +Assign each observed difference one classification: + +- **Required specialization:** the artifact or reference contract requires the + difference. No harmonization is recommended. +- **Permitted variation:** implementations differ without meaningful + maintenance or correctness cost. +- **Convention drift:** equivalent responsibilities use different names, + layouts, error behavior, metadata, validation, or tests without a reason. +- **Architectural divergence:** responsibility is placed in the wrong layer or + bypasses a shared contract. +- **Undetermined:** more evidence or a policy decision is needed. + +For required specialization, document the reason briefly so a future audit +does not repeatedly flag it. For drift or architectural divergence, identify +the preferred convention and why it is preferable. + +### 3. Build a duplication inventory + +Search for three kinds of repetition: + +1. **Exact duplication:** identical Go logic, prompt text, schema fragments, or + test support. +2. **Structural duplication:** the same algorithm or lifecycle expressed with + renamed domain types. +3. **Policy duplication:** the same invariant is independently encoded in + several production or test layers. + +Trace callers and consumers before recommending extraction. Record: + +- the repeated responsibility; +- participating modules; +- meaningful semantic differences; +- change history or likely change cadence when discoverable; +- defect risk if copies drift; +- proposed owner and API shape; and +- code or assets that would remain module-owned. + +Similar code is not sufficient evidence. Prefer a shared abstraction when at +least one of the following is true: + +- three or more modules independently implement the same nontrivial invariant; +- two modules share correctness-sensitive behavior that must evolve together; +- existing duplicated code has already drifted or caused a defect; or +- an existing shared contract is being reimplemented locally. + +Avoid extraction when the shared API would need artifact-specific callbacks, +large configuration objects, type erasure, or branching on module identity. +Those are signs that visual similarity is masking separate responsibilities. + +## Convention Review + +Evaluate the following conventions across all five modules. + +### Package organization + +- Comparable responsibilities use predictable filenames and package-local + ownership. +- Optional specialized files, such as catalog or registry wiring, are present + only where the module has that responsibility. +- Exported identifiers are limited to framework and registration contracts. +- Test helpers remain local unless sharing them improves test clarity without + coupling independent suites. + +### Construction and registration + +- Required dependencies fail during construction. +- Options are decoded and unknown options rejected consistently. +- `ModuleSpec`, reference slots, artifact kind, execution class, and builder + behavior agree with runtime behavior. +- Construction resolves stable reference-derived state where possible, while + operation requests own genuinely run- or chunk-specific inputs. +- Returned specs, metadata, fingerprints, and byte slices have consistent + defensive-copy behavior. + +### Prompt and schema boundary + +- Shared prompt messages come from canonical shared assets; package assets + contain only lane-specific wording. +- Shared content is exactly identical across manifests and appears in the + documented cache-friendly order. +- Stable messages and cache boundaries precede the variable transcript. +- Declared prompt inputs match reference slots and generated projections. +- Private response schemas are strict structural envelopes and reject unknown + fields. +- Semantic validation remains in deterministic code at the intended boundary. +- Schema identity, version, digest, and diagnostics are exposed consistently + without leaking schema or prompt content. + +### Extraction and evidence + +- Request validation and contextual error wrapping follow one recognizable + pattern. +- Reference material aids disambiguation but never becomes transcript evidence. +- Source unit IDs are resolved against document order rather than numeric + assumptions. +- Mapping performs only the deterministic transformations owned by extraction. +- Canonical source references, stable ordering, and exact deduplication use + consistent policies where artifact semantics agree. +- Whole-chunk evidence in scene descriptions is treated as an intentional + specialization rather than forced through citation-oriented helpers. +- Results do not share mutable state with model responses, references, or + requests. + +### Provenance and diagnostics + +- Prompt and response-schema identities and hashes appear consistently in + manifest metadata. +- Checkpoint fingerprints cover every stable semantic input that could change + accepted output, without including credentials, paths, timestamps, or source + content. +- Registry and catalog projections use canonical bytes and retain bounded + provenance. +- Errors and diagnostics are contextual, bounded, and free of raw reference or + secret content. + +### Tests + +- Each important contract has one clear test owner. +- Equivalent risks receive comparable coverage without requiring identical test + file layouts. +- Schema tests cover required structure and unknown-field rejection without + duplicating every semantic validator case. +- Extractor tests cover request validation, response mapping, evidence, + ordering, references, and provider failures at the narrowest stable boundary. +- Prompt tests prepare real embedded assets and protect input placement, + ordering, cache boundaries, and content-safety properties without freezing + exact prompt text. +- Registration or production tests prove assembly once and do not repeat + package-local behavior unnecessarily. +- Obsolete, redundant, representation-specific, or impossible fixtures are + identified for deletion or simplification. + +## Shared-Code Decision Framework + +Recommend the narrowest owner that matches the repeated responsibility: + +1. Keep artifact semantics in the module that owns the artifact. +2. Use `internal/modules/dnd/shared` for D&D-wide mechanics with identical + semantics, such as prompt inputs or source-unit reference handling. +3. Use a focused D&D subpackage for behavior shared by a subset of lanes, such + as NPC registry projection, when it has a coherent domain contract. +4. Use a framework package only for transport-neutral or domain-neutral + behavior owned by the framework. + +Evaluate these candidate categories without assuming they should be extracted: + +- extractor request precondition validation; +- private response-schema loading and metadata assembly; +- prompt asset registration and hashing; +- source-reference conversion, canonical ordering, and exact deduplication; +- manifest metadata and checkpoint fingerprint assembly; +- NPC registry resolution and names-only projection; +- strict option decoding and module registration; +- diagnostic redaction and bounded error context; and +- repeated test fixtures or schema-validation utilities. + +For prompts and schemas: + +- Factor prompt wording into a shared asset only when every consumer needs the + exact same text and should receive future changes atomically. +- Prefer an existing shared asset over a new near-duplicate. +- Do not create shared prompt fragments solely because prose is similar. +- Keep private response schemas package-owned unless a genuine shared wire + contract exists. +- Do not introduce shared JSON Schema fragments or generation unless the audit + demonstrates a maintenance problem that outweighs tooling and indirection. + +## Additional Quality Review + +Beyond consistency and duplication, inspect: + +- functions with high cognitive complexity or responsibilities that can be + separated without obscuring the extraction flow; +- repeated linear scans inside loops, avoidable serialization, unnecessary + allocations, or per-chunk reconstruction of stable state; +- hidden mutation, aliasing, or inconsistent clone boundaries; +- fingerprint omissions that could permit stale checkpoint reuse; +- prompt inputs or references repeated unnecessarily across messages; +- unreachable defensive checks or validation performed redundantly at several + layers; +- errors that lose module, field, source, or stage context; +- stale documentation, fixtures, names, compatibility aliases, or comments; +- public or package abstractions that have only one artificial consumer; and +- opportunities to delete code after a shared helper replaces it. + +Performance recommendations must identify a plausible workload and complexity +impact. Do not recommend micro-optimization without evidence. + +## Evidence Collection + +Perform the audit in this order: + +1. Read the architecture, testing, documentation, module, LLM, pipeline, and + relevant integration contracts. +2. Build the five-module comparison matrix from definitions and assets. +3. Use graph similarity only to identify candidates; read the complete + functions and trace their callers before classifying them. +4. Compare prompt manifests and shared assets byte-for-byte, then compare local + prompt semantics. +5. Compare private schemas structurally and map each field to its DTO, mapper, + durable artifact, and validator owner. +6. Trace reference and fingerprint data from construction through the LLM + request and checkpoint identity. +7. Review focused tests alongside the behavior they own. +8. Run the existing focused and repository-wide validation commands to + distinguish current failures from maintainability observations. + +Do not modify production code, tests, prompts, schemas, examples, or +current-behavior documentation during the audit. + +## Finding Standard + +Every reported finding must contain: + +- severity: high, medium, or low; +- category: convention drift, duplication, architecture, correctness, + performance, testing, or documentation; +- affected modules and exact file or symbol references; +- observed behavior and the convention or invariant it is compared against; +- concrete maintenance, correctness, cost, or security impact; +- recommended target state and ownership; +- why the recommendation is preferable to leaving the code separate; and +- validation or migration considerations. + +Order findings by severity and impact, not by module. Separate confirmed +findings from optional improvements. State explicitly when no issue is found in +a comparison area. + +For each apparent duplication, the audit must choose one outcome: + +- extract now; +- harmonize without sharing; +- retain intentionally separate; or +- defer pending a named missing requirement. + +Do not report speculative abstractions as findings. Record them, if useful, as +rejected or deferred candidates with the reason. + +## Audit Deliverable + +The completed audit should provide: + +1. an executive conclusion addressing convention consistency, shared-code + opportunities, and overall code quality; +2. the completed five-module comparison matrix; +3. prioritized findings with evidence and recommendations; +4. intentional differences that should be preserved; +5. rejected or deferred sharing candidates and rationale; +6. a proposed refactoring sequence grouped into independently safe changes; + and +7. validation commands and any residual risks. + +The audit should be actionable enough to support a later decision-complete +implementation plan, but it must not implement or silently commit any +recommendation. + +## Completion Criteria + +The strategy has been followed when: + +- all five primary packages and their prompts, schemas, registration, + provenance, reference handling, mapping, and tests have been compared; +- every divergence has a classification; +- every repeated candidate has an ownership and keep/share decision; +- architecture and testing-policy constraints are applied explicitly; +- findings cite exact evidence and explain impact; +- intentional specialization is documented alongside drift; +- repository validation results are recorded; and +- no production changes were made as part of the audit. + +## Audit Results + +Status: Complete + +### Executive Conclusion + +The five D&D extraction modules follow a coherent overall convention: all are +typed, production-registered extractors with strict private response schemas, +package-owned artifact mapping, bounded provenance, shared canonical prompt +assets, defensive result ownership, and deterministic downstream validation. +Their differences in catalog/registry state, reference slots, local prompt +assets, whole-chunk versus cited evidence, and scene prose cleanup are explained +by artifact semantics and should remain. + +The consistency question is therefore **mostly yes, with two material +exceptions**. Spell artifact ordering and all four citation extractors' +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. + +The shared-code question is **yes, but only for two narrow D&D-wide +responsibilities**: common chunk-extraction preflight and document-aware +source-reference ordering/canonicalization. They belong in +`internal/modules/dnd/shared`, not in the framework. Private response DTOs, +artifact mapping, structured LLM calls, prompt prose, response schemas, +metadata assembly, error prefixes, and typed test fakes should remain +package-owned; sharing them would require callbacks, type erasure, module +branching, or broad configuration. + +Overall code quality is **good**. Construction, registration, cloning, +diagnostics, schema loading, prompt composition, and validator composition are +clear and consistently tested. The repository passes all current validation. +The recommended work is targeted correction and consolidation, not a redesign +of the extractor family. + +### Final Module Comparison Matrix + +| Module | Common contract | Required specialization | Prompt/schema boundary | Evidence and ordering | Provenance and tests | +| --- | --- | --- | --- | --- | --- | +| Spells | `dnd/spells` -> `dnd/spell-list`; typed LLM extractor; strict private DTO/schema; append merge and typed normalize/validate ([spec](../../internal/modules/dnd/extract/spells/extractor.go#L207)) | Prepared spell catalog and optional NPC registry projection; catalog prompt input ([constructor](../../internal/modules/dnd/extract/spells/extractor.go#L73)) | Shared evidence/input messages plus local task, instructions, catalog, and transcript; semantic catalog validation remains deterministic ([manifest](../../internal/modules/dnd/extract/spells/assets/prompts/dnd.spells.yaml#L23)) | Attaches current transcript ID and preserves invalid candidates, but sorts artifacts/references by numeric unit ID rather than document position ([canonicalization](../../internal/modules/dnd/extract/spells/canonicalize.go#L10)) | Prompt/schema/catalog/NPC projection fingerprints, but no mapping-policy fingerprint; broad extractor tests, incomplete full prompt cache-order assertion ([fingerprints](../../internal/modules/dnd/extract/spells/extractor.go#L144)) | +| NPCs | `dnd/npcs` -> `dnd/npc-list`; same typed lifecycle ([spec](../../internal/modules/dnd/extract/npcs/extractor.go#L159)) | Deterministic NPC identity derivation; campaign references remain request context ([mapping](../../internal/modules/dnd/extract/npcs/canonicalize.go#L94)) | Shared evidence/input messages plus NPC-local task/instructions/transcript; private schema owns transport only ([manifest](../../internal/modules/dnd/extract/npcs/assets/prompts/dnd.npcs.yaml#L17)) | Artifact order resolves document positions; reference list still sorts numeric IDs before exact deduplication ([canonicalization](../../internal/modules/dnd/extract/npcs/canonicalize.go#L11)) | Prompt/schema/identity fingerprints, but no mapping-policy fingerprint; weakest focused preflight matrix and partial prompt ordering coverage ([fingerprints](../../internal/modules/dnd/extract/npcs/extractor.go#L91)) | +| Combat turns | `dnd/combat-turns` -> `dnd/combat-turn-list`; same typed lifecycle ([spec](../../internal/modules/dnd/extract/combatturns/extractor.go#L201)) | Optional prepared NPC registry; all combat kinds remain raw validator candidates ([constructor](../../internal/modules/dnd/extract/combatturns/extractor.go#L68)) | Shared evidence/NPC/input messages plus combat-local task/instructions/transcript ([manifest](../../internal/modules/dnd/extract/combatturns/assets/prompts/dnd.combat_turns.yaml#L20)) | Artifact order uses valid source positions; extractor, normalizer, and invariant validator duplicate differing reference-order mechanics ([extractor](../../internal/modules/dnd/extract/combatturns/canonicalize.go#L10), [normalizer](../../internal/modules/dnd/normalize/combatturns/normalizer.go#L228)) | Mapping policy is fingerprinted; complete request preconditions, mapping, registry, and provider tests, but incomplete full prompt cache-order assertion ([fingerprints](../../internal/modules/dnd/extract/combatturns/extractor.go#L126)) | +| NPC interactions | `dnd/npc-interactions` -> `dnd/npc-interaction-list`; same typed lifecycle ([spec](../../internal/modules/dnd/extract/npcinteractions/extractor.go#L203)) | Required NPC registry and names-only projection; exact identity remains in focused domain package ([constructor](../../internal/modules/dnd/extract/npcinteractions/extractor.go#L68)) | Shared evidence/NPC/input messages plus interaction-local task/instructions/transcript ([manifest](../../internal/modules/dnd/extract/npcinteractions/assets/prompts/dnd.npc_interactions.yaml#L20)) | Extractor references sort numeric IDs, while the interaction model already owns correct document-aware canonicalization at too-narrow a layer ([extractor](../../internal/modules/dnd/extract/npcinteractions/canonicalize.go#L10), [model helper](../../internal/modules/dnd/npcinteractions/canonical.go#L17)) | Mapping, identity, prompt, schema, and registry projection are fingerprinted; focused suite omits nil-receiver and full prompt cache-order cases ([fingerprints](../../internal/modules/dnd/extract/npcinteractions/extractor.go#L126)) | +| Scene descriptions | `dnd/scene-descriptions` -> `dnd/scene-description-list`; same typed lifecycle ([spec](../../internal/modules/dnd/extract/scenedescriptions/extractor.go#L174)) | One summary per chunk, whole-chunk evidence, and fingerprinted prose trimming; no catalog/NPC projection ([mapping](../../internal/modules/dnd/extract/scenedescriptions/extractor.go#L150)) | Local task/instructions plus shared input/transcript; schema currently duplicates semantic enum/non-empty policy owned by shape validation ([schema](../../internal/modules/dnd/extract/scenedescriptions/assets/schemas/dnd_scene_descriptions_llm.v1.json#L8), [validator](../../internal/modules/dnd/validate/scenedescriptions/shape/validator.go#L49)) | Whole materialized chunk becomes one source range; citation ordering is not applicable | Mapping policy is fingerprinted; strongest full prompt order/cache test, but preflight omits nil receiver/context cases ([fingerprints](../../internal/modules/dnd/extract/scenedescriptions/extractor.go#L98), [prompt test](../../internal/modules/dnd/extract/scenedescriptions/scriptorium_assets_test.go#L15)) | + +All five reject unknown options, register through the production D&D registrar, +return independently owned results, and use package-local structured response +types. No inconsistent secret handling, raw prompt/schema diagnostic exposure, +hidden result aliasing, or unregistered audited extractor was found. + +### Prioritized Findings + +#### High + +1. **Source references and spell artifacts can be durably ordered contrary to + transcript order.** + - **Category:** correctness and architecture. + - **Affected modules:** spell, NPC, combat-turn, and NPC-interaction + extractors; related spell/combat/NPC normalizers and combat invariants. + - **Evidence:** spell selects the smallest positive numeric unit ID + ([`earliestSourceUnit`](../../internal/modules/dnd/extract/spells/canonicalize.go#L62)); + all four citation extractors sort reference endpoints numerically + ([spell](../../internal/modules/dnd/extract/spells/canonicalize.go#L30), + [NPC](../../internal/modules/dnd/extract/npcs/canonicalize.go#L31), + [combat](../../internal/modules/dnd/extract/combatturns/canonicalize.go#L30), + [interaction](../../internal/modules/dnd/extract/npcinteractions/canonicalize.go#L30)). + Valid source documents require unique positive IDs, not monotonically + increasing IDs + ([`ValidateDocument`](../../internal/core/source/validation.go#L8)). + The interaction model demonstrates the correct document-aware comparison + ([`SourceRefLess`](../../internal/modules/dnd/npcinteractions/canonical.go#L51)). + - **Impact:** valid evidence can be reordered away from transcript order; + spell casts with invalid or later evidence can precede earlier valid + casts. This changes durable list order, evidence presentation, merge input, + and checkpointed results. + - **Target state and owner:** move a document-backed `SourceRefOrder` to + `internal/modules/dnd/shared`, with `EarliestValid` and `Canonicalize` + operations. Preserve invalid candidates, exact deduplication, stable ties, + nil/empty distinction, cloning, and deterministic invalid fallback. + Artifact comparison, DTO conversion, source-ID attachment, and repair + accounting remain local. + - **Why shared:** the rule is a D&D-wide evidence invariant already + implemented by extract, normalize, and validate consumers; continued + copies have already diverged. A shared position index also reduces repeated + `UnitIndex` scans from approximately `O(A log A * R * U)` ordering work to + `O(U + A*R + A log A)` for `A` artifacts, `R` references, and `U` units. + - **Migration/validation:** add non-monotonic, invalid, duplicate, nil/empty, + aliasing, stable-tie, and repair-count tests; migrate the interaction model + and combat normalize/invariant pair before extractors; add/bump mapping + policy fingerprints so old checkpoints miss intentionally. + +2. **Spell and NPC checkpoint identities omit stable mapping policies.** + - **Category:** correctness and data integrity. + - **Affected modules:** spell and NPC extractors. + - **Evidence:** both extractors perform deterministic ordering, + canonicalization, source attachment, and mapping + ([spell mapping](../../internal/modules/dnd/extract/spells/canonicalize.go#L10), + [NPC mapping](../../internal/modules/dnd/extract/npcs/canonicalize.go#L11)), + but their fingerprint providers name catalog/projection or identity + policies without a mapping policy + ([spell fingerprints](../../internal/modules/dnd/extract/spells/extractor.go#L144), + [NPC fingerprints](../../internal/modules/dnd/extract/npcs/extractor.go#L91)). + Prepared fingerprints are lane-scoped + ([collector](../../internal/framework/pipeline/prepared_fingerprints.go#L23)) + and restore requires exact normalized equality + ([comparison](../../internal/framework/checkpoint/loader.go#L301)). + - **Impact:** a mapping-policy code change can reuse a checkpoint produced + under older artifact/evidence semantics when prompt and schema bytes are + unchanged. + - **Target state and owner:** each extractor owns an explicit stable + mapping/canonicalization policy fingerprint. Metadata assembly stays + package-local. + - **Why preferable:** a local named semantic fingerprint directly closes + the reuse gap; a generic metadata builder would only hide module-specific + omissions behind configuration. + - **Migration/validation:** add provider and prepared-checkpoint restore + tests. Adding a fingerprint safely invalidates existing identities by full + list mismatch; no artifact payload migration is required. + +#### Medium + +3. **Common extraction preflight is copied across five production callers and + has already drifted in test protection.** + - **Category:** duplication and testing. + - **Affected modules:** all five extractors. + - **Evidence:** every `Extract` validates the same context/source/chunk/unit + prerequisites before calling + [`ChunkPromptMaterial`](../../internal/modules/dnd/shared/extraction_inputs.go#L12), + but focused coverage ranges from a complete spell/combat matrix to only + cancellation and source mismatch for NPCs + ([spell tests](../../internal/modules/dnd/extract/spells/extractor_test.go#L235), + [NPC tests](../../internal/modules/dnd/extract/npcs/extractor_test.go#L139)). + - **Impact:** validation order, error behavior, or a newly required common + precondition can diverge silently among modules. + - **Target state and owner:** add + `shared.PrepareChunkExtraction(ctx, req) (contracts.LLMInputMaterial, + error)` for cancellation, non-nil source/chunk, non-empty units, and + matching cloned material. Receiver/client checks, specialized references, + provider calls, typed results, and contextual wrapping remain local. + - **Why shared:** this extends the existing common material boundary with + one coherent invariant and serves five real callers without callbacks or + module configuration. + - **Migration/validation:** shared table tests own common inputs; package + tests retain nil receiver/dependency, specialized reference, wrapped + error, and provider cases. Preserve current validation order and useful + package context. + +4. **Scene semantic validity has two production owners.** + - **Category:** architecture. + - **Affected module:** scene descriptions. + - **Evidence:** the private response schema enforces the scene-kind enum and + non-empty title/summary + ([schema](../../internal/modules/dnd/extract/scenedescriptions/assets/schemas/dnd_scene_descriptions_llm.v1.json#L8)); + deterministic shape validation independently enforces the same policy + ([validator](../../internal/modules/dnd/validate/scenedescriptions/shape/validator.go#L49)). + The documented schema boundary assigns semantic enum/non-empty rules to + deterministic validators + ([LLM internals](../internal/llm.md#L169)). + - **Impact:** the two policies can drift and produce provider-dependent + rejection before typed validation, while non-LLM artifacts see only the + validator. + - **Target state and owner:** retain JSON type, required/nullability, and + unknown-field constraints in the package-private schema; make the scene + shape validator the sole semantic owner. + - **Why harmonize without sharing:** only scene has this duplicated policy; + moving it to the existing validator removes an owner without inventing an + abstraction. + - **Migration/validation:** update schema structural tests and validator + semantic tests, then expect the schema digest/checkpoint identity to + change. Verify provider-decoded invalid candidates reach deterministic + validation. + +#### Low + +5. **Four prompt suites do not fully protect the documented message-order and + cache-boundary contract.** + - **Category:** testing. + - **Affected modules:** spells, NPCs, combat turns, and NPC interactions. + - **Evidence:** scene descriptions asserts the complete prepared role order + and ephemeral/no-cache placement + ([test](../../internal/modules/dnd/extract/scenedescriptions/scriptorium_assets_test.go#L15)); + the four citation suites cover registration and selected inputs but not + the full documented sequence + ([spell tests](../../internal/modules/dnd/extract/spells/scriptorium_assets_test.go), + [NPC tests](../../internal/modules/dnd/extract/npcs/scriptorium_assets_test.go), + [combat tests](../../internal/modules/dnd/extract/combatturns/scriptorium_assets_test.go), + [interaction tests](../../internal/modules/dnd/extract/npcinteractions/scriptorium_assets_test.go)). + - **Impact:** manifest edits can move variable content into a cacheable + prefix or reorder stable grounding without a focused failure, increasing + request cost or reducing prompt quality. + - **Target state and owner:** each package test should assert its complete + documented prepared sequence and cache flags using the real registry. + - **Why local:** lane inputs differ and the behavior belongs to each prompt + manifest; shared test setup would obscure the boundary. + - **Migration/validation:** assert roles/input identities/cache flags, not + exact prompt text, byte counts, or hashes. + +6. **Small exported-surface and defensive-behavior drift remains.** + - **Category:** convention drift. + - **Affected modules:** spells, combat turns, and D&D shared. + - **Evidence:** spell and combat export unused singular `ArtifactType` + constants while production uses typed `ArtifactKind` + ([spell](../../internal/modules/dnd/extract/spells/extractor.go#L17), + [combat](../../internal/modules/dnd/extract/combatturns/extractor.go#L17)); + spell `ManifestMetadata` lacks the nil guard used by the other four + ([spell](../../internal/modules/dnd/extract/spells/extractor.go#L122), + [NPC example](../../internal/modules/dnd/extract/npcs/extractor.go#L74)); + [`SourceRefCandidate`](../../internal/modules/dnd/shared/unit_refs.go#L90) + has only a test caller, ignores its document parameter, and is unsafe for + extractor provenance because it trusts model-supplied source identity. + - **Impact:** the package surface presents competing artifact vocabulary, + zero-value behavior is inconsistent, and an artificial shared API invites + incorrect reuse. Immediate runtime impact is limited. + - **Target state and owner:** delete the unused constants; align spell nil + metadata behavior locally; delete `SourceRefCandidate` when the correct + shared reference API lands. + - **Why preferable:** deletion and local harmonization clarify existing + contracts without adding a helper. + - **Migration/validation:** graph search found no production consumers; + compile all internal packages and add one spell zero-value metadata test. + +#### Optional improvement + +The current module documentation lists only four production consumers of +`ChunkPromptMaterial`, omitting scene descriptions +([documentation](../internal/modules.md#L67)). Correct that list when the shared +preflight boundary is documented. This is localized documentation maintenance, +not a separate production design finding. + +No actionable issue was found in option strictness, production registration, +typed codec selection, result cloning, prompt/schema content redaction, local +prompt semantic ownership, catalog/NPC source-evidence separation, provider +error wrapping, or secret handling. + +### Intentional Differences To Preserve + +- **Prepared specialized state:** spells retains a catalog and optional NPC + resolver; combat retains an optional NPC resolver; interactions requires one; + NPC and scene extraction need only request material. Construction-time + identity belongs only where the artifact uses it. +- **Reference slots:** catalog and NPC registry slots follow grounding needs. + The interaction registry is required; combat/spell registry use is optional. + Campaign references remain prompt context and never become evidence. +- **Evidence model:** scene descriptions cites the whole materialized chunk; + forcing it through citation DTO/canonicalization machinery would weaken its + one-summary-per-chunk contract. +- **Prompt assets:** only byte-identical canonical messages are shared. Local + task, instruction, catalog, identity, and evidence wording changes with the + artifact and must not be coupled. +- **Private schemas and DTOs:** similar `source_refs` fragments do not form a + separately versioned wire contract. Package ownership keeps response changes + aligned with mapping and diagnostics. +- **Prose handling:** scene title/summary trimming is a deliberate fingerprinted + mapping policy; citation-lane names and enum candidates remain raw for their + normalizers and validators. +- **Test organization:** file layout, typed fakes, schema compilation helpers, + and real-registry prompt setup remain package-local. Equivalent risks need + comparable coverage, not identical fixtures or filenames. + +### Shared-Code Decisions + +| Decision | Owner and scope | Why this is the narrow correct boundary | +| --- | --- | --- | +| Extract common preflight | `internal/modules/dnd/shared`; validate common typed extraction request state and return cloned matching `LLMInputMaterial` | Five current callers repeat one prerequisite to the existing `ChunkPromptMaterial` boundary. Receiver/dependency/specialized checks remain readable and local. | +| Extract source-reference order | `internal/modules/dnd/shared`; document-backed `SourceRefOrder.EarliestValid` and `.Canonicalize` over durable `[]source.SourceRef` | Multiple extract, normalize, model, and validate consumers must evolve together; the rule is D&D-wide but has no demonstrated non-D&D/framework consumer. | +| Harmonize mapping fingerprints | Spell and NPC extractor packages | The missing values are module-semantic; local named fingerprints are safer than a configurable metadata builder. | +| Harmonize scene validation ownership | Private scene schema plus `validate/scenedescriptions/shape` | Remove semantic keywords from the transport schema and keep the already registered durable validator as sole owner. No sharing is needed. | +| 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. | + +### Rejected Or Deferred Candidates + +| Candidate | Decision and rationale | +| --- | --- | +| Deprecated `roster` slot asymmetry | **Deferred pending a product compatibility decision.** Configuration documents `roster` as a deprecated `party` alias ([config](../config.md#L526)), while scene removes it. Choose a removal release or uniform alias lifetime before changing slots and migration guidance. | +| Spell response-schema key/ID naming | **Deferred pending a provenance migration decision.** The spell names differ from the `_llm`/`.llm` convention, but schema identity is persisted metadata. Renaming needs an alias or an explicit manifest/checkpoint compatibility break. | +| Prompt registration/hash wrapper | **Rejected.** `shared.PromptAssetManifest` already owns coherent composition/hashing. Hiding filesystems, registries, `sync.Once`, and diagnostic nouns would require broad configuration. | +| Manifest/fingerprint builder | **Rejected.** Stable inputs are module-specific; a generic builder would accept the same keys/policies as arguments and could conceal omissions such as the current spell/NPC gap. | +| Generic artifact mapper | **Rejected.** Private DTO fields, identity, enums, and artifact types require callbacks, type erasure, or module branching. Only reference ordering is genuinely common. | +| Generic structured-LLM call wrapper | **Rejected.** Generic output and request configuration would hide an already clear framework client boundary without removing semantic work. | +| Shared error-prefix helper | **Rejected.** It would add a module-name parameter to replace three transparent lines and weaken local diagnostic ownership. | +| Shared response schemas or prompt prose | **Retain intentionally separate.** Similar structure/text is not an atomic shared contract; private schema and lane prompt changes should not propagate together. | +| Shared schema-test or provider-fake utilities | **Retain intentionally separate.** Typed DTOs and package assets are the behavior under test; central fixtures would couple suites and obscure failures. | +| Framework-level preflight/reference API | **Rejected.** No non-D&D consumer or framework-owned invariant was found. The D&D shared layer preserves dependency direction and domain ownership. | + +### Recommended Refactoring Sequence + +1. **Land checkpoint identity protection independently.** Add spell and NPC + mapping-policy fingerprints with provider and checkpoint-restore tests. + This is a small package-local change and protects all later semantic + migrations from stale reuse. +2. **Consolidate source-reference mechanics.** Add and exhaustively test the + D&D shared document-position index/API. Migrate the existing interaction + model helper first, then combat normalize plus invariants, then NPC/spell + normalizers, and finally the four citation extractors. Keep package mapping + and repair accounting local; remove numeric comparators and the unused + `SourceRefCandidate` only after all consumers move. +3. **Consolidate common extraction preflight.** Extend the existing shared + chunk-material boundary, migrate one extractor to establish error/validation + compatibility, then migrate the remaining four. Move common branch tables + to shared tests and retain package-context smoke tests. +4. **Resolve scene validation ownership independently.** Adjust schema tests, + remove semantic enum/non-empty constraints from the private schema, retain + validator cases, and verify the expected schema fingerprint/checkpoint miss. +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. + +Each scope can be reviewed and reverted independently. The sequence is not a +decision-complete implementation plan; implementation should still pin exact +fingerprint values, exported names, error compatibility, and per-package test +cases. + +### Validation And Residual Risks + +Final validation on 2026-07-24: + +```text +go test -count=1 ./... PASS +go vet ./... PASS +go build ./cmd/notarius PASS +gofmt -l . PASS (no files listed) +git diff --check PASS +git diff --no-index --check /dev/null docs/roadmap/audit.md + PASS (no whitespace errors; exit 1 denotes differences) +``` + +The audit also checked that cited relative paths exist in the current working +tree. The audit changed no production code, tests, prompts, schemas, examples, +or current-behavior documentation. + +Residual risks and limits: + +- This was a static and deterministic-test audit; it did not evaluate live + model extraction quality, prompt effectiveness, provider cache-hit rates, or + token cost. Prompt changes still need representative human/model evaluation. +- Existing tests use mostly monotonic unit IDs, so the ordering defect is not a + current failing test. The proposed non-monotonic fixtures are required before + changing behavior. +- The exact durable order expected for invalid references must remain + deterministic and diagnostics-friendly during API design; invalid references + must not be discarded merely because default validators usually reject them + later. +- Mapping and schema fingerprint additions intentionally invalidate prior + checkpoint identities. Operators should be told to expect recomputation; no + serialized artifact migration is otherwise indicated. +- The `roster` alias lifetime and spell schema-identity migration remain human + product/compatibility decisions. +- Passing repository checks establishes current deterministic correctness, not + absence of model-quality regressions or correctness under unrepresented + source-document shapes. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md new file mode 100644 index 0000000..41975f4 --- /dev/null +++ b/docs/roadmap/implementation.md @@ -0,0 +1,425 @@ +# D&D Extraction Module Audit Execution Plan + +Status: Ready for audit execution + +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. + +The word "implementation" in this filename refers to implementing the audit +strategy, not implementing refactors. + +## 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/`. +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. + +## Working Results Structure + +Stages 1 through 4 maintain one working area at the end of `audit.md`: + +```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 + +### Objective + +Establish the complete, factual five-module baseline and identify convention +differences without yet recommending shared abstractions. + +### Required review + +For every extractor, inspect: + +- 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. + +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. + +### Write to `audit.md` + +Create `## Audit Results`, mark it `Status: In progress`, and add: + +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. + +Also add a short baseline-validation note recording: + +```sh +go test -count=1 ./... +go vet ./... +go build ./cmd/notarius +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. + +### 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. + +## Open Questions + +None. The audit stages, working-document structure, decision rules, and final +deliverable are fully specified.