diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 04ad210..8d0b467 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -20,13 +20,12 @@ change only roadmap audit documents do not change that production target. Pending final synthesis. The initial baseline is healthy. The architecture, configuration/CLI, pipeline composition, reference/handoff, runtime, state, -LLM, generic/Seriatim, shared D&D, registry-family, and occurrence-family -reviews have found three High findings, five Medium findings, and twenty-two -Low findings, with no production dependency inversion, unbounded framework -worker pool, completion-order-dependent result -assembly, debug-to-cache coupling, model-visible credential material in the -embedded LLM assets, or domain leakage across the Seriatim and generic module -boundaries. +LLM, generic/Seriatim, shared D&D, registry-family, occurrence-family, and +spell/scene-family reviews have found three High findings, six Medium findings, +and twenty-three Low findings, with no production dependency inversion, +unbounded framework worker pool, completion-order-dependent result assembly, +debug-to-cache coupling, model-visible credential material in the embedded LLM +assets, or domain leakage across the Seriatim and generic module boundaries. ## Finding Index @@ -58,12 +57,14 @@ Final cross-area ordering is pending synthesis. | MOD-003 | Low | Simplicity | Remove unreachable JSON metadata clone helpers | | DND-CORE-001 | Low | Simplicity | Remove the unused lossy unit-reference constructor | | DND-REG-001 | Low | Correctness | Reject reversed evidence ranges at durable D&D codec boundaries | -| DND-REG-002 | Low | Correctness | Keep NPC extraction evidence inside the current chunk | +| DND-REG-002 | Low | Correctness | Keep NPC and spell extraction evidence inside the current chunk | | DND-REG-003 | Low | Efficiency | Index item and location duplicate groups | | DND-REG-004 | Low | Efficiency | Select location identity anchors without sorting | | DND-OCC-001 | Medium | Correctness | Align item occurrence evidence fields with the shared prompt | | DND-OCC-002 | Low | Simplicity | Remove unused campaign references from NPC occurrence normalization | | DND-OCC-003 | Low | Efficiency | Reuse canonical item occurrence evidence | +| DND-SCENE-001 | Medium | Correctness | Expose catalog aliases to spell extraction | +| DND-SCENE-002 | Low | Correctness | Make scene and spell normalization diagnostics complete and bounded | ## Findings @@ -201,7 +202,14 @@ Final cross-area ordering is pending synthesis. `cloneBuildRequest` (`prepare.go:272`–`299` and 325–327). Each request clone deep-copies materialized reference content as well as options, so typed builders receive two reference copies and as many as three option copies; - untyped stage and validator builders use fewer copies. + untyped stage and validator builders use fewer copies. Spell extractor, + normalizer, and catalog-validator construction then call + `spells/catalog.ResolveEffectiveCatalog`; that helper clones every slot and + every item byte slice in the already owned `ReferenceSet` + (`internal/modules/dnd/spells/catalog/effective.go:55`–`62` and 411–426), even + though it reads only the `spell_catalog` slot and decodes its content into a + new overlay value. On extractor construction this needlessly recopies the + optional NPC registry and campaign-context references as well. - **Impact:** Every preparation repeats allocation and byte copying for bounded external references and nested options, with the highest cost and a different ownership path specifically for typed lanes and validators. The @@ -210,7 +218,9 @@ Final cross-area ordering is pending synthesis. - **Recommendation:** Designate one private construction invocation as the ownership boundary and clone the complete `BuildRequest` exactly there. Store raw builders or remove the caller-side clone consistently so all stage - and validator registry variants follow the same single-copy rule. + and validator registry variants follow the same single-copy rule. Let spell + catalog resolution read the owned catalog slot directly instead of cloning + unrelated slots. - **Preserve:** Builders must continue to receive independently owned options, reference maps, slot slices, metadata, and content bytes; preparation must retain its own immutable resolved/reference state; nil, key/name, execution @@ -218,8 +228,11 @@ Final cross-area ordering is pending synthesis. - **Validation:** Extend construction hooks to mutate nested options and reference bytes for typed and untyped modules/validators, assert no aliasing with resolved or sibling requests, and use allocation/byte-copy observations - or a focused benchmark to confirm a single defensive copy; run - `go test ./internal/framework/contracts ./internal/framework/pipeline`. + or a focused benchmark to confirm a single defensive copy. Add a spell + catalog construction case with large unrelated slots and verify that + resolution neither copies nor mutates them; run + `go test ./internal/framework/contracts ./internal/framework/pipeline + ./internal/modules/dnd/spells/catalog`. - **Grouping:** Independent. ### References And Ordered Handoffs @@ -863,21 +876,26 @@ Final cross-area ordering is pending synthesis. - **Severity:** Low - **Category:** Correctness -- **Evidence:** The three registry and three occurrence integration contracts - require that a source range's start not follow its end +- **Evidence:** The three registry, three occurrence, spell, and scene- + description integration contracts require that a source range's start not + follow its end (`docs/integrations/dnd-npc-registry-artifacts.md:32`–`34`, `dnd-item-registry-artifacts.md:33`–`35`, and `dnd-location-registry-artifacts.md:34`–`36`; and `dnd-npc-occurrence-artifacts.md:33`–`36`, - `dnd-item-occurrence-artifacts.md:41`–`43`, and - `dnd-location-occurrence-artifacts.md:33`–`35`). All six durable codec - validators check only that both endpoints are positive + `dnd-item-occurrence-artifacts.md:41`–`43`, + `dnd-location-occurrence-artifacts.md:33`–`35`, + `dnd-spell-artifacts.md:31`–`35`, and + `dnd-scene-description-artifacts.md:33`–`35`). All eight reviewed durable + codec validators check only that both endpoints are positive (`internal/modules/dnd/codec/npcregistry/codec.go:96`–`106`, `codec/itemregistry/codec.go:97`–`107`, and `codec/locationregistry/codec.go:97`–`107`; and `codec/npcoccurrences/codec.go:99`–`109`, - `codec/itemoccurrences/codec.go:117`–`127`, and - `codec/locationoccurrences/codec.go:100`–`110`). Direct registry references are + `codec/itemoccurrences/codec.go:117`–`127`, + `codec/locationoccurrences/codec.go:100`–`110`, + `codec/spells/codec.go:89`–`99`, and + `codec/scenedescriptions/codec.go:101`–`109`). Direct registry references are decoded and identity-checked by `npcs/registry.loadRegistry`, `items/registry.loadRegistry`, and `locations/registry.loadRegistry`, but do not pass through the generated-output source-reference validators. A durable @@ -885,17 +903,21 @@ Final cross-area ordering is pending synthesis. `{start_unit_id: 2, end_unit_id: 1}` and otherwise valid fields is therefore accepted; for locations, `validIdentityReference` at `internal/modules/dnd/locations/identity/identity.go:170`–`171` also treats - that reversed range as a valid identity anchor. -- **Impact:** An externally supplied registry or occurrence list can be - accepted as approved even though its evidence cannot denote the documented - forward source interval. - NPC and item prompt projections then hide the malformed provenance, while a - location can derive and retain a durable ID from it; only later consumers - that happen to construct contextual grounding against the same source may - reject it. Generated pipeline outputs remain protected by their source- - reference validator chains, which limits current exposure. + that reversed range as a valid identity anchor. The immutable scene- + eligibility registry also decodes approved scene artifacts directly before + projecting ID, range, and kind + (`internal/modules/dnd/scenedescriptions/registry/registry.go:174`–`223`). +- **Impact:** An externally supplied D&D artifact can be accepted as approved + even though its evidence cannot denote the documented forward source + interval. + NPC and item prompt projections then hide the malformed provenance, a + location can derive and retain a durable ID from it, and scene gating can + retain the impossible interval as eligibility metadata; only later consumers + with source context may reject it. Generated pipeline outputs remain + protected by their source-reference validator chains, which limits current + exposure. - **Recommendation:** Add the order-independent structural condition - `start_unit_id <= end_unit_id` to all six reviewed codecs' durable + `start_unit_id <= end_unit_id` to all eight reviewed codecs' durable validation, and make location identity reject reversed anchors as a defense in depth. Keep document membership and current-chunk coverage in the existing @@ -904,46 +926,49 @@ Final cross-area ordering is pending synthesis. field/trailing-value rejection, location identity's earliest canonical anchor policy, and the separation between source-independent durable shape validation and source-dependent evidence validation. -- **Validation:** Add codec encode/decode cases for reversed ranges in all six +- **Validation:** Add codec encode/decode cases for reversed ranges in all eight reviewed families, a location identity case that refuses a reversed-only - anchor, and direct registry and occurrence reference cases proving malformed - referenced JSON is rejected; run the codec, identity, registry, occurrence, - and assembled CLI suites. + anchor, and direct registry, occurrence, spell, and scene-eligibility + reference cases proving malformed referenced JSON is rejected; run the + codec, identity, registry, spell, scene, occurrence, and assembled CLI suites. - **Grouping:** Independent. -### DND-REG-002 — Keep NPC extraction evidence inside the current chunk +### DND-REG-002 — Keep NPC and spell extraction evidence inside the current chunk - **Severity:** Low - **Category:** Correctness -- **Evidence:** The NPC, item, and location extractors receive only the current - chunk and all three default extract chains run their family source-reference - validator. Item and location validators require a non-nil extraction chunk - and reject references whose endpoints are not in it +- **Evidence:** The NPC, item, location, and spell extractors receive only the + current chunk and all four default extract chains run their family source- + reference validator. Item and location validators require a non-nil + extraction chunk and reject references whose endpoints are not in it (`internal/modules/dnd/validate/itemregistry/source_refs/validator.go:37`–`55` and `validate/locationregistry/source_refs/validator.go:37`–`55`). The NPC validator at `validate/npcregistry/source_refs/validator.go:36`–`53` checks only that a reference is valid somewhere in the complete source document; it neither requires `req.Chunk` during extraction nor checks chunk membership. - Its focused tests include reversed, foreign, and out-of-document ranges but - no off-chunk range or missing-chunk case. -- **Impact:** If an NPC extraction response supplies a valid unit ID from - another chunk, the candidate can pass evidence validation despite the model - never receiving that passage. A name that also appears at the off-chunk range - can pass advisory relatedness without proving the current candidate, causing - duplicated or misattributed registry provenance across chunk results. The - model usually copies visible unit IDs, which limits the likelihood. + The spell validator has the same document-only behavior + (`validate/spells/source_refs/validator.go:35`–`51`). Its tests exercise + valid, out-of-document, missing-document, malformed-shape, and bounded- + aggregate cases, but supply no stage or chunk and do not cover an existing + off-chunk range (`validator_test.go:15`–`73`). +- **Impact:** If an NPC or spell extraction response supplies a valid unit ID + from another chunk, the candidate can pass evidence validation despite the + model never receiving that passage. Relatedness can also succeed when the NPC + name or spell appears only at the cited off-chunk range, causing duplicated + or misattributed provenance across chunk results. The model usually copies + visible unit IDs, which limits the likelihood. - **Recommendation:** Match the item/location extraction contract: require the - current chunk when `req.Stage` is extract and reject NPC references outside - that chunk. Keep whole-document validation for normalize and other non- - extraction validation calls. + current chunk when `req.Stage` is extract and reject NPC and spell references + outside that chunk. Keep whole-document validation for normalize and other + non-extraction validation calls. - **Preserve:** Retain full-document source-ID/range validation, shape deferral, bounded aggregate diagnostics, normalization without a chunk, direct factual third-party mentions, and source-relatedness as an advisory check rather than an identity gate. -- **Validation:** Add NPC source-reference cases for an existing off-chunk - range, a missing extraction chunk, an accepted in-chunk range, and normalize- - stage validation without a chunk; run the NPC registry validator, extractor, - and assembled pipeline tests. +- **Validation:** Add NPC and spell source-reference cases for an existing off- + chunk range, a missing extraction chunk, an accepted in-chunk range, and + normalize-stage validation without a chunk; run both source-reference + validators, their extractors, and assembled pipeline tests. - **Grouping:** Independent. ### DND-REG-003 — Index item and location duplicate groups @@ -1113,6 +1138,86 @@ Final cross-area ordering is pending synthesis. per normalized record; run item occurrence normalize/validator tests. - **Grouping:** Independent. +### Spells, Scene Chunking, And Scene Descriptions + +### DND-SCENE-001 — Expose catalog aliases to spell extraction + +- **Severity:** Medium +- **Category:** Correctness +- **Evidence:** The overlay contract says optional aliases contribute to spell + recognition (`docs/integrations/dnd-spell-catalog-overlays.md:3`–`6` and + 57–68), and the effective catalog correctly maps each accepted alias to its + canonical spell (`internal/modules/dnd/spells/catalog/effective.go:215`–`247`). + The extractor's model-visible projection serializes only + `EffectiveCatalog.CanonicalNames()` as `spell_names` + (`internal/modules/dnd/extract/spells/catalog_prompt_input.go:13`–`28`), while + its prompt explicitly says aliases are absent and requires exact canonical + spelling (`assets/dnd/spells/prompts/spell-catalog.md:1`–`6`). Focused and + assembled tests codify the omission by rejecting `Emberfall Aegis` from the + prompt even though the configured overlay maps it to `Aegis of Emberfall` + (`internal/modules/dnd/extract/spells/extractor_test.go:88`–`103` and + `internal/cli/spell_catalog_identity_contract_test.go:412`–`414`). The + downstream catalog validator and normalizer can resolve an alias only if the + model already returns it; they cannot teach the model that alias-only + transcript language denotes the configured spell. +- **Impact:** A real cast expressed only with a campaign alias can be omitted or + rejected because the extraction model receives neither the alias nor its + canonical mapping. This silently defeats a documented purpose of overlay + configuration and can lose durable spell-cast occurrences; canonical names + and base-catalog casts remain unaffected. +- **Recommendation:** Project a deterministic source-free list of canonical + spell names with their recognized aliases into `spell_catalog`, and instruct + the model to return the associated canonical spelling. Continue to omit + catalog source metadata, paths, licenses, rules text, and provenance. Treat + the projection shape and prompt change as semantic fingerprint changes. +- **Preserve:** Keep overlays recognition-only rather than evidence, retain + strict collision rejection and canonical display names, require current- + transcript cast evidence and an in-world caster, preserve NPC provenance + separation, and keep the existing prompt order and cache boundaries. +- **Validation:** Add base and overlay projection tests for stable canonical- + alias mappings, repeated aliases, and metadata exclusion; update the + assembled prompt contract to require `Emberfall Aegis` only as an alias of + `Aegis of Emberfall`; retain catalog rejection, normalization, checkpoint- + invalidation, and retry tests; run the spell extractor, catalog, normalizer, + validator, and CLI suites. +- **Grouping:** Independent. + +### DND-SCENE-002 — Make scene and spell normalization diagnostics complete and bounded + +- **Severity:** Low +- **Category:** Correctness +- **Evidence:** The maintained D&D convention says deterministic normalizers + issue bounded warnings for changes and collapsed duplicates + (`docs/internal/dnd.md:129`–`132`). Spell normalization emits a warning for + every canonicalized or unresolved name and changed reference list, then one + for every duplicate group, and returns the concatenated slice directly + (`internal/modules/dnd/normalize/spells/normalizer.go:89`–`105`, 108–149, + and 177–226); unlike the occurrence and registry normalizers, it never calls + `diagnostics.LimitWarnings`. Scene-description normalization has the inverse + gap: it trims title/summary, reorders records, and removes exact duplicates + (`normalize/scenedescriptions/normalizer.go:70`–`124`) but always returns the + value without any warnings (lines 53–68). Its primary test performs all three + mutations without asserting diagnostic provenance + (`normalizer_test.go:15`–`43`). +- **Impact:** A large spell list can publish an unbounded number of warnings, + inflating manifests, debug artifacts, output files, and checkpoint payloads. + Scene normalization silently changes accepted durable content, so operators + cannot distinguish an unchanged scene list from repaired whitespace/order or + collapsed duplicates. Artifact values remain deterministic and valid. +- **Recommendation:** Pass spell warnings through the shared deterministic + limiter with a stable omission reason. Add bounded scene warnings that + distinguish prose trimming, canonical reordering, and exact duplicate + collapse while retaining stable input scopes and order. Emit no warning for + an already canonical value. +- **Preserve:** Keep current normalized values, source-position ordering, + conflict rejection, duplicate identities, input immutability, per-record + diagnostic order below the limit, and idempotent warning-free second passes. +- **Validation:** Add spell cases beyond `diagnostics.MaxWarnings` that assert + the stable omission summary, and scene cases for each mutation, combined + overflow, canonical input, and idempotence; run both normalizer suites and + assembled output/checkpoint warning tests. +- **Grouping:** Independent. +