diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 8d0b467..7ec55cf 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -20,9 +20,10 @@ 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, 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, +LLM, generic/Seriatim, shared D&D, registry-family, occurrence-family, +spell/scene-family, and combat/enemy-family reviews have found three High +findings, seven 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. @@ -57,14 +58,15 @@ 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 and spell extraction evidence inside the current chunk | -| DND-REG-003 | Low | Efficiency | Index item and location duplicate groups | +| DND-REG-002 | Low | Correctness | Keep extraction evidence inside the current chunk | +| DND-REG-003 | Low | Efficiency | Index duplicate groups across D&D families | | 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 | +| DND-SCENE-002 | Low | Correctness | Make D&D normalization and advisory diagnostics complete and bounded | +| DND-COMBAT-001 | Medium | Correctness | Enforce enemy-event semantics at the durable codec boundary | ## Findings @@ -876,26 +878,31 @@ Final cross-area ordering is pending synthesis. - **Severity:** Low - **Category:** Correctness -- **Evidence:** The three registry, three occurrence, spell, and scene- - description integration contracts require that a source range's start not - follow its end +- **Evidence:** The three registry, three occurrence, spell, scene-description, + combat-turn, and enemy-event 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`, `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 + `dnd-spell-artifacts.md:31`–`35`, + `dnd-scene-description-artifacts.md:33`–`35`, + `dnd-combat-turn-artifacts.md:31`–`33`, and + `dnd-enemy-event-artifacts.md:33`–`35`). Nine of the ten durable codecs check + at most 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`, `codec/locationoccurrences/codec.go:100`–`110`, - `codec/spells/codec.go:89`–`99`, and - `codec/scenedescriptions/codec.go:101`–`109`). Direct registry references are + `codec/spells/codec.go:89`–`99`, + `codec/scenedescriptions/codec.go:101`–`109`, and + `codec/combatturns/codec.go:95`–`106`); the enemy-event codec checks only JSON + field presence and accepts even non-positive endpoints + (`codec/enemyevents/codec.go:86`–`135`). 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 @@ -912,12 +919,14 @@ Final cross-area ordering is pending synthesis. 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 + retain the impossible interval as eligibility metadata. A malformed combat- + turn artifact can likewise be accepted for enemy grounding, whose compact + projection then strips the malformed provenance; 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 eight reviewed codecs' durable + `start_unit_id <= end_unit_id` to all ten 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 @@ -926,52 +935,64 @@ 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 eight - reviewed families, a location identity case that refuses a reversed-only - anchor, and direct registry, occurrence, spell, and scene-eligibility - reference cases proving malformed referenced JSON is rejected; run the +- **Validation:** Add codec encode/decode cases for reversed ranges in all ten + families, a location identity case that refuses a reversed-only + anchor, and direct registry, occurrence, spell, scene-eligibility, combat- + turn, and enemy-event reference cases proving malformed referenced JSON is + rejected; run the codec, identity, registry, spell, scene, occurrence, and assembled CLI suites. -- **Grouping:** Independent. +- **Grouping:** Coordinate the enemy-event codec change with DND-COMBAT-001; + the remaining nine codec changes are otherwise independent. -### DND-REG-002 — Keep NPC and spell extraction evidence inside the current chunk +### DND-REG-002 — Keep extraction evidence inside the current chunk - **Severity:** Low - **Category:** Correctness -- **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 +- **Evidence:** The NPC, item, location, spell, combat-turn, and enemy-event + extractors receive only the current chunk, and all six 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. - The spell validator has the same document-only behavior - (`validate/spells/source_refs/validator.go:35`–`51`). Its tests exercise + The spell and combat-turn validators have the same document-only behavior + (`validate/spells/source_refs/validator.go:35`–`51` and + `validate/combatturns/source_refs/validator.go:36`–`60`), while the enemy- + event validator correctly requires an extraction chunk and checks every + unit in each range (`validate/enemyevents/source_refs/validator.go:37`–`99`). + Spell and combat-turn 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 + off-chunk range (`validate/spells/source_refs/validator_test.go:15`–`73` and + `validate/combatturns/source_refs/validator_test.go:15`–`73`). +- **Impact:** If an NPC, spell, or combat-turn 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 + actor, 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 and spell references + current chunk when `req.Stage` is extract and reject NPC, spell, and combat- + turn 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 and spell source-reference cases for an existing off- +- **Validation:** Add NPC, spell, and combat-turn 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. + normalize-stage validation without a chunk; run the affected source-reference + validators, their extractors, and assembled pipeline tests; retain the enemy- + event in-chunk, off-chunk, and missing-chunk cases as the reference behavior. - **Grouping:** Independent. -### DND-REG-003 — Index item and location duplicate groups +### DND-REG-003 — Index duplicate groups across D&D families - **Severity:** Low - **Category:** Efficiency @@ -982,25 +1003,38 @@ Final cross-area ordering is pending synthesis. comparison key. `exactDuplicateGroups` in `normalize/locationregistry/normalizer.go:229`–`246` repeats that nested scan and performs `reflect.DeepEqual` over canonical source-reference slices for - each candidate group. With distinct records, both paths are quadratic before - any LLM reconciliation request is built. -- **Impact:** Large merged registries spend avoidable deterministic CPU in + each candidate group. Enemy-event normalization likewise scans all retained + events for every record and calls exact equality, which recanonicalizes both + evidence lists (`normalize/enemyevents/normalizer.go:207`–`227` and + `enemyevents/enemyevents.go:42`–`64`); its invariant validator repeats a + previous-event scan at `validate/enemyevents/invariants/validator.go:138`–`145`. + The knowledge graph reports loop depth two and transitive loop depth four for + enemy `collapseDuplicates`. With distinct records, all four paths are + quadratic. +- **Impact:** Large merged registries or enemy observation lists spend + avoidable deterministic CPU in normalization, with the location cost also proportional to citation-list - comparisons. Normal registry sizes and the later LLM request dominate today, - so the issue is low severity. + comparisons. Normal artifact sizes bound the impact, and registry + reconciliation's later LLM request dominates its two paths today, so the + issue is low severity. - **Recommendation:** Preserve first-seen group order while maintaining a local index from the complete duplicate identity to its group position. Use the existing comparison key for items and an exact, collision-safe key over the comparison name plus canonical source-reference sequence for locations; - verify equality on any hash collision rather than using lossy concatenation. + apply the same collision-safe complete identity index to enemy events in both + normalization and invariant validation. Verify equality on any hash + collision rather than using lossy concatenation. - **Preserve:** Item duplicates remain name-identity duplicates regardless of evidence; locations collapse only equal comparison names with exactly equal canonical evidence; group/member order, input-index provenance, warning scopes, and later proposal-only semantic reconciliation remain unchanged. + Enemy events continue to collapse only equal normalized subject identity, + kind, and complete canonical evidence; different outcomes, evidence, and + repeated observations remain distinct. - **Validation:** Add many-distinct and repeated-key cases that compare output groups and warning order with current fixtures, plus a focused benchmark or comparison-count hook demonstrating linear expected grouping work; run the - item and location normalizer tests. + item and location normalizer tests plus enemy normalizer and invariant tests. - **Grouping:** Independent. ### DND-REG-004 — Select location identity anchors without sorting @@ -1182,7 +1216,7 @@ Final cross-area ordering is pending synthesis. validator, and CLI suites. - **Grouping:** Independent. -### DND-SCENE-002 — Make scene and spell normalization diagnostics complete and bounded +### DND-SCENE-002 — Make D&D normalization and advisory diagnostics complete and bounded - **Severity:** Low - **Category:** Correctness @@ -1198,26 +1232,79 @@ Final cross-area ordering is pending synthesis. (`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`). + (`normalizer_test.go:15`–`43`). Combat-turn normalization also emits per- + actor, per-reference, per-reorder, and per-duplicate-group warnings without + applying the shared limiter (`normalize/combatturns/normalizer.go:131`–`188` + and 235–273). Its advisory relatedness validator returns one warning per + unrelated turn without a limiter + (`validate/combatturns/source_relatedness/validator.go:45`–`67`), whereas + both enemy-event equivalents use `diagnostics.LimitWarnings`. - **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 + Combat lists can have the same warning amplification. 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 +- **Recommendation:** Pass spell and combat-turn normalization warnings and + combat-turn relatedness warnings through the shared deterministic limiter + with stable omission reasons. 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. +- **Validation:** Add spell, combat-normalizer, and combat-relatedness cases + beyond `diagnostics.MaxWarnings` that assert stable omission summaries, and + scene cases for each mutation, combined overflow, canonical input, and + idempotence; run all affected normalizer/validator suites and assembled + output/checkpoint warning tests. - **Grouping:** Independent. +### Combat Turns And Enemy Events + +### DND-COMBAT-001 — Enforce enemy-event semantics at the durable codec boundary + +- **Severity:** Medium +- **Category:** Correctness +- **Evidence:** The enemy-event contract and embedded durable schema require a + non-empty subject, one of five event kinds, at least one source reference, + and a non-empty source ID with positive endpoints + (`docs/integrations/dnd-enemy-event-artifacts.md:19`–`35` and + `internal/modules/dnd/codec/enemyevents/assets/schemas/dnd_enemy_events.v1.json:4`–`34`). + `Codec.Decode` checks strict JSON and literal field presence, but + `validateRequiredJSONFields` does not enforce any of those value constraints; + `Codec.Encode` checks only that the event and reference slices are non-nil + (`codec/enemyevents/codec.go:52`–`95` and 98–160). The focused codec test + explicitly proves durable `Decode` accepts a whitespace name, unsupported + kind, blank source ID, zero start, and negative end + (`codec/enemyevents/codec_test.go:76`–`92`). The combat-turn codec, by + contrast, enforces all equivalent value constraints except the separately + recorded reversed-range condition (`codec/combatturns/codec.go:80`–`108`). +- **Impact:** A direct enemy-event artifact decode can report contract-invalid + JSON as approved typed data. More importantly, the supported validator- + override surface can replace both family validation chains; a model response + with these invalid values can then reach durable output because the final + codec encode does not re-establish its published schema. Default chains + reject the values, so maintained production configuration is protected. +- **Recommendation:** Preserve candidate encode/decode as the permissive retry + boundary, but make durable `Encode` and `Decode` apply one shared value + validator covering the schema's name, kind, non-empty references, source ID, + and positive endpoint constraints. Also apply the forward-range check in + DND-REG-001. Keep the raw-object presence pass on decode where omitted versus + zero-valued JSON fields must remain distinguishable. +- **Preserve:** Retain strict unknown-field and trailing-value rejection, + defensive source-reference ownership, empty top-level event lists, candidate + preservation before validators, and document/chunk membership checks in the + source-aware validator rather than the codec. +- **Validation:** Replace the semantic-acceptance durable codec case with + paired candidate-preservation and durable-rejection cases for every field, + including empty and reversed reference ranges. Add an assembled lane case + showing permissive validator overrides still cannot publish schema-invalid + enemy events; run the codec, enemy-event, pipeline, and CLI suites. +- **Grouping:** Coordinate with DND-REG-001; that finding owns the common + forward-range gap, while this finding owns enemy-event schema enforcement. +