767 lines
45 KiB
Markdown
767 lines
45 KiB
Markdown
# 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.
|