diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md deleted file mode 100644 index bb60eb6..0000000 --- a/docs/roadmap/audit.md +++ /dev/null @@ -1,826 +0,0 @@ -# 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, package surface conventions, and -the documentation of the common extractor contract for future lanes. - -The shared-code question is **yes, but only for two narrow D&D-wide -responsibilities**: common chunk-extraction preflight and document-aware -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. - -7. **The common D&D extractor contract is observable but only partially - documented as a requirement for future lanes.** - - **Category:** documentation and convention drift. - - **Affected modules:** all five extractors and future D&D extraction - modules. - - **Evidence:** all five reject unknown options, perform common request - preflight, return independently owned results, use package-local private - response types, and expose prompt/schema provenance plus checkpoint - fingerprints. The extension checklist in - [module internals](../internal/modules.md#adding-an-extension) covers - registration, package-owned assets, prompt ordering, and general option - and validation coverage, while - [LLM internals](../internal/llm.md#prompt-and-schema-assets) documents the - private-schema boundary. Neither location consolidates the remaining - behaviors into a normative D&D extractor contract. The missing spell/NPC - mapping fingerprints and uneven preflight and prompt-contract coverage - demonstrate that conventions discoverable from current packages can - still drift. - - **Impact:** an additional extractor can appear locally consistent while - silently accepting misspelled options, omitting a stable semantic input - from checkpoint identity, returning aliased mutable data, implementing - incomplete preflight, or missing focused contract coverage. - - **Target state and owner:** add a compact **D&D extractor contract** - subsection to `docs/internal/modules.md`, incorporated into or placed - immediately after **Adding An Extension**. Specify behavioral - responsibilities rather than filenames or boilerplate: - - reject unknown options unless the option namespace is intentionally - extensible; - - use the shared request preflight contract while retaining receiver, - dependency, and lane-specific checks locally; - - return results and exposed metadata that are independently owned and - safe for caller mutation; - - keep the private response DTO, structural schema, schema identity, - provider-response mapping, durable artifact conversion, and - lane-specific diagnostics package-owned; - - include every stable semantic input that can change durable results in - checkpoint identity, including prompt, schema, mapping, - canonicalization, prepared reference projection, identity, - normalization, and trimming policies when applicable; and - - consider focused behavioral coverage for construction and - registration, option rejection, preflight, provider failures, - structured-output decoding, mapping and ownership, prompt - role/input/cache order, and checkpoint invalidation. - - **Why documentation rather than another abstraction:** these are shared - obligations, not one shared implementation. A checklist makes omissions - visible without introducing a configurable metadata builder, generic - mapper, shared test fixture, mandatory file layout, or exact-output - change-detector tests. Detailed prompt, schema, pipeline, and testing - policies should remain linked rather than duplicated. - - **Migration/validation:** update the extension checklist and relevant - cross-links when the shared preflight helper is documented. Review the - text against all five lanes and the completed audit matrix. Do not require - exact prompt text, hashes, prefix lengths, test counts, filenames, or - fixture layouts. - -#### Optional improvement - -The current module documentation lists only four production consumers of -`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. | -| Document the D&D extractor contract | `docs/internal/modules.md`, linked to existing LLM and testing policies | The five lanes share behavioral obligations that should guide future extensions, but they do not justify a generic implementation or rigid package template. | - -### Rejected Or Deferred Candidates - -| 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 cleanup.** Remove unused `ArtifactType` constants - and add the spell metadata nil guard/test. -7. **Document the common extractor contract.** Update module documentation for - the shared helper consumers and boundaries, then add the D&D extractor - contract checklist described in Finding 7. Link to existing prompt, schema, - pipeline, and testing policies rather than copying them. - -Each scope can be reviewed and reverted independently. The sequence is not a -decision-complete implementation plan; implementation should still pin exact -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 deleted file mode 100644 index 1bf8547..0000000 --- a/docs/roadmap/implementation.md +++ /dev/null @@ -1,836 +0,0 @@ -# D&D Extraction Module Audit Implementation Plan - -Status: Ready for implementation - -This document turns the completed findings in -[D&D Extraction Module Refactoring Audit](audit.md#audit-results) into an -ordered implementation plan. Each stage is one bounded prompt for an LLM coding -agent. Execute the stages in order and complete each stage's gate before -starting the next. - -The target is a corrected and better-harmonized five-extractor family, not a -generic extractor framework. Preserve the intentional differences and rejected -abstractions recorded in the audit. - -## Global Instructions - -Every stage must: - -1. Read this document, the completed audit, `AGENTS.md`, - `docs/development.md`, and the task-specific documents identified there. - Follow all policies under `docs/policy/`. -2. Inspect the current working tree before editing. Preserve unrelated user - changes and do not assume a clean checkout. -3. Prefer the codebase knowledge graph for code discovery and call tracing. - Use direct text and file searches for documentation, configuration, prompt - assets, schemas, fixtures, and exact string comparison. -4. Implement only the current stage. Do not opportunistically begin a later - stage. -5. Keep artifact semantics, private response DTOs and schemas, lane-specific - prompts, mapping, diagnostics, and typed test support in their owning - packages unless this plan explicitly moves one responsibility. -6. Keep the new shared behavior under `internal/modules/dnd/shared`; do not - move D&D concepts into the generic framework. -7. Apply the testing policy. Protect observable behavior and semantic - checkpoint identity, not private helper usage, exact prompt text, hashes, - message lengths, source-file layout, or test counts. -8. Use focused tests while iterating. Run `gofmt` on changed Go files and - `git diff --check` before completing every stage. -9. Update current-behavior documentation only in the documentation stage, - after the corresponding behavior exists. -10. Do not alter durable artifact schemas, prompt prose, configuration - contracts, reference-slot policy, model-facing response DTO fields, or - extraction policy except where a stage explicitly requires it. - -The following audit decisions remain out of scope: - -- removal or continued support of the deprecated `roster` reference alias; -- renaming the spell private response-schema key or ID; -- shared prompt prose or response schemas; -- generic manifest, fingerprint, mapper, structured-call, error-prefix, or - registration builders; -- shared schema-test fixtures or provider fakes; and -- framework-level preflight or source-reference APIs. - -## Stage 1: Protect Spell And NPC Mapping Identity - -### Objective - -Close the stale-checkpoint gap before changing any mapping or ordering -behavior. - -### Scope - -Change only the spell and NPC extractor packages and the narrow integration or -checkpoint tests needed to prove their prepared fingerprints affect reuse. - -### Implementation - -1. Add an unexported mapping-policy constant to each extractor: - - ```go - // internal/modules/dnd/extract/spells - mappingPolicy = "dnd.spells.extract_mapping.v1" - - // internal/modules/dnd/extract/npcs - mappingPolicy = "dnd.npcs.extract_mapping.v1" - ``` - -2. In both packages: - - - add `"mapping_policy": mappingPolicy` to `ManifestMetadata`; - - add `{Name: "mapping_policy", Value: mappingPolicy}` to - `CheckpointFingerprints`; - - preserve all existing prompt, response-schema, catalog, registry, - identity, and digest metadata and fingerprints; and - - preserve nil-receiver behavior as it exists at the start of this stage. - -3. Do not change mapping, source-reference ordering, prompt or schema content, - or artifact output in this stage. - -### Tests - -- Extend each package's metadata/fingerprint test to prove that the mapping - policy is present with the declared semantic value and that no existing - fingerprint disappears. -- In the existing prepared-pipeline integration coverage for spells and NPCs, - assert that the scoped prepared fingerprint list contains: - - - `extract::dnd/spells:mapping_policy`; and - - `extract::dnd/npcs:mapping_policy`; - - using the actual lane IDs from the fixtures. -- Add or extend one checkpoint-resume test at the prepared pipeline boundary - to prove that a differing or missing scoped mapping-policy fingerprint - prevents reuse with the normal identity-mismatch reason. Do not duplicate - the checkpoint loader's complete mismatch matrix in both lane suites. - -### Validation - -Run: - -```sh -go test ./internal/modules/dnd/extract/spells -go test ./internal/modules/dnd/extract/npcs -go test ./internal/modules/integration -go test ./internal/framework/checkpoint ./internal/framework/pipeline -git diff --check -``` - -### Completion Gate - -Spell and NPC mapping policies participate in manifest provenance, prepared -checkpoint identity, and restore decisions, while extraction output remains -unchanged. - -## Stage 2: Introduce Document-Aware Source-Reference Ordering - -### Objective - -Create the one shared D&D source-reference primitive accepted by the audit and -establish it with the already document-aware NPC-interaction consumers. - -### Scope - -Change `internal/modules/dnd/shared`, -`internal/modules/dnd/npcinteractions`, and the NPC-interaction normalizer and -invariant validator that currently call the model helper. Do not migrate the -other extractors or normalizers yet. - -### Shared API - -Add a document-position index with this public surface: - -```go -type SourceRefOrder struct { - // private immutable snapshot -} - -func NewSourceRefOrder(doc *source.SourceDocument) SourceRefOrder -func (o SourceRefOrder) Less(left, right source.SourceRef) bool -func (o SourceRefOrder) EarliestValid(refs []source.SourceRef) (position int, ok bool) -func (o SourceRefOrder) Canonicalize(refs []source.SourceRef) []source.SourceRef -``` - -The constructor must snapshot only the source ID and unit-ID-to-position map; -the returned value must not retain or mutate the document. - -### Required Semantics - -`Less` must preserve the established NPC-interaction ordering: - -1. compare differing source IDs lexically; -2. after equal source IDs, order resolvable start endpoints by indexed - document position and before unresolvable start endpoints; -3. use the literal start unit ID as the deterministic tie or invalid fallback; -4. apply the same document-position, resolvability, and literal fallback rules - to end endpoints; and -5. return false for exactly equal references. - -`EarliestValid` must: - -- consider only references accepted by `source.ValidateRef` for the indexed - document; -- return the smallest document position of a valid start endpoint; -- ignore invalid candidates without mutating or deleting them; and -- return `(0, false)` for a nil document, an empty set, or no valid reference. - -`Canonicalize` must: - -- return `nil` for nil input and an independently owned non-nil empty slice for - non-nil empty input; -- clone the input; -- stable-sort it with `Less`; -- remove only exactly equal `source.SourceRef` values after sorting; and -- never rewrite, repair, or discard a distinct invalid candidate. - -A zero-value `SourceRefOrder` must be safe and deterministic, using literal -fallback ordering and treating every reference as invalid for -`EarliestValid`. - -### Migration - -1. Replace the implementation of the focused - `internal/modules/dnd/npcinteractions` canonicalization/ordering helpers - with the shared primitive. -2. Update its normalizer and invariant validator to use - `shared.NewSourceRefOrder(doc)` directly where practical. -3. Remove the old exported `SourceRefLess` and `CanonicalizeSourceRefs` - functions if no production or test caller remains. Retain - NPC-interaction-specific exact artifact identity and list comparison in the - focused model package. -4. This migration must preserve NPC-interaction output exactly, so do not bump - its normalization, validation, or extraction policy in this stage. - -### Tests - -Shared table tests must cover: - -- non-monotonic document unit IDs; -- valid and invalid endpoints; -- references to another source ID; -- stable ties; -- exact duplicates versus merely similar ranges; -- nil and non-nil empty inputs; -- input/output alias safety; -- mutation of the source document after constructing the index; and -- `EarliestValid` ignoring invalid candidates. - -Retain or adapt focused NPC-interaction tests to prove its artifact ordering -and invariant behavior. Do not add tests that merely assert that a package -calls the shared helper. - -### Validation - -Run: - -```sh -go test ./internal/modules/dnd/shared -go test ./internal/modules/dnd/npcinteractions -go test ./internal/modules/dnd/normalize/npcinteractions -go test ./internal/modules/dnd/validate/npcinteractions/... -git diff --check -``` - -### Completion Gate - -The shared API has exhaustive behavioral protection, NPC interactions retain -their prior output, and no duplicate document-aware comparator remains in the -NPC-interaction model. - -## Stage 3: Migrate Normalization And Invariant Consumers - -### Objective - -Make downstream spell, NPC, and combat-turn canonicalization use document -order before changing extractor output. - -### Scope - -Change only: - -- `internal/modules/dnd/normalize/spells`; -- `internal/modules/dnd/normalize/npcs`; -- `internal/modules/dnd/normalize/combatturns`; and -- `internal/modules/dnd/validate/combatturns/invariants`. - -### Implementation - -1. Construct one `shared.SourceRefOrder` from the normalization or validation - request's source document and pass it through the relevant local operation. - Do not rebuild the index inside artifact or reference loops. -2. Replace numeric source-reference sorting, exact deduplication, and repeated - `source.UnitIndex` scans with `Less`, `Canonicalize`, and `EarliestValid` as - applicable. -3. Keep these responsibilities local: - - - spell catalog canonicalization and duplicate spell identity; - - NPC identity and grouping; - - combat actor identity, turn comparison, warnings, and duplicate policy; - - repair/warning counts and message construction; and - - artifact-specific tie-breakers. - -4. Derive local repair facts without extending the shared API: - - - compare the original and canonical slices to determine whether order - changed; - - compute exact duplicates removed from input and output lengths; and - - preserve the current warning reason codes and useful diagnostic context. - -5. Update checkpoint identity for every changed semantic owner: - - - add spell normalizer metadata and fingerprint - `normalization_policy = "dnd.spells.normalize.v1"` because it currently - has no local normalization-policy fingerprint; - - change NPC normalization policy to - `dnd.npcs.normalize.v2`; - - change combat-turn normalization policy to - `dnd.combat_turns.normalize.v2`; and - - change the combat-turn normalized-invariant validator policy to - `dnd.combat_turns.validator.normalized.v2`. - - Preserve the spell catalog fingerprint as a separate semantic input. - -6. Delete superseded local numeric reference comparators and earliest-position - scans after all callers in these packages move. - -### Tests - -For each affected artifact family, add or adapt focused tests using a valid -document whose unit IDs are deliberately non-monotonic. Prove: - -- source references follow document order; -- artifact order uses the earliest valid document position where applicable; -- invalid candidates remain deterministic and available to validators; -- exact duplicate and warning/repair behavior is unchanged except for the - corrected ordering; and -- returned slices do not alias inputs. - -Update metadata/fingerprint tests for the exact semantic policy values above. -Do not duplicate all shared `SourceRefOrder` edge cases in each package. - -### Validation - -Run: - -```sh -go test ./internal/modules/dnd/normalize/spells -go test ./internal/modules/dnd/normalize/npcs -go test ./internal/modules/dnd/normalize/combatturns -go test ./internal/modules/dnd/validate/combatturns/invariants -git diff --check -``` - -### Completion Gate - -All downstream consumers in scope use one document-position index per -operation, non-monotonic IDs are handled correctly, local artifact semantics -remain local, and every changed semantic policy invalidates prior checkpoints. - -## Stage 4: Migrate Spell And NPC Extraction - -### Objective - -Correct source-reference and artifact ordering in the spell and NPC -extractors. - -### Scope - -Change only the spell and NPC extractor packages plus their focused tests. - -### Implementation - -1. Build one `shared.SourceRefOrder` from `req.Source` after common request - prerequisites have passed. -2. Use `Canonicalize` for mapped durable source references. Preserve: - - - model-produced invalid candidates; - - attachment of the current transcript source ID; - - exact-only deduplication; - - package-owned DTO conversion and identity logic; and - - independent ownership of returned artifacts and references. - -3. For spell artifact ordering, replace numeric `earliestSourceUnit` behavior - with `EarliestValid` document positions: - - - artifacts with valid evidence sort by their earliest valid transcript - position; - - valid-evidence artifacts sort before artifacts with no valid evidence; - - existing artifact-specific deterministic tie-breakers remain in their - current order; and - - invalid evidence is retained even though it does not select the earliest - position. - -4. Use document-aware reference comparison in NPC ordering while preserving - NPC identity and artifact tie-breakers. -5. Remove the superseded local numeric comparators and scans. -6. Bump the Stage 1 mapping policies: - - - `dnd.spells.extract_mapping.v2`; and - - `dnd.npcs.extract_mapping.v2`. - - Update both manifest metadata and checkpoint tests through the constants. - -### Tests - -Add focused regression fixtures with non-monotonic unit IDs for: - -- spell reference ordering and spell artifact ordering; -- NPC reference and artifact ordering; -- mixed valid and invalid evidence; -- exact duplicate references; -- stable artifact ties; and -- output mutation safety. - -Retain existing malformed model-output, provider-error, catalog, identity, and -reference-grounding coverage. - -### Validation - -Run: - -```sh -go test ./internal/modules/dnd/extract/spells -go test ./internal/modules/dnd/extract/npcs -git diff --check -``` - -### Completion Gate - -Spell and NPC extraction follow transcript order for valid evidence, preserve -invalid candidates deterministically, and advertise the new mapping semantics -through v2 policy fingerprints. - -## Stage 5: Migrate Combat-Turn And NPC-Interaction Extraction - -### Objective - -Complete source-reference harmonization across the four citation extractors -and remove superseded reference APIs. - -### Scope - -Change the combat-turn and NPC-interaction extractor packages and the D&D -shared package only. - -### Implementation - -1. In both extractors, construct one `shared.SourceRefOrder` from the request - source and use `Canonicalize` for mapped durable references. -2. Preserve actor/NPC identity, enum candidates, artifact ordering, - transcript-source attachment, warnings, provider behavior, and local DTO - mapping. -3. Bump mapping policies: - - - `dnd.combat_turns.extract_mapping.v2`; and - - `dnd.npc_interactions.extract_mapping.v2`. - -4. Delete all superseded local numeric source-reference comparators. -5. Delete `shared.SourceRefCandidate` and its isolated test. DTO-to-durable - mapping must continue to attach the trusted current document source ID - locally and must not trust a model-supplied source identity. -6. Search the complete D&D module tree for remaining numeric comparisons of - `SourceRef` endpoints. Retain a local comparator only if it implements a - documented artifact-specific policy; otherwise migrate it to - `SourceRefOrder`. - -### Tests - -Add focused non-monotonic-ID regression tests for both extractors, including -invalid candidates, exact duplicates, stable ties, and mutation safety. Update -metadata/fingerprint expectations for the v2 mapping policies. - -Do not add a test asserting that `SourceRefCandidate` or a local comparator is -absent; compilation and behavioral tests are sufficient. - -### Validation - -Run: - -```sh -go test ./internal/modules/dnd/extract/combatturns -go test ./internal/modules/dnd/extract/npcinteractions -go test ./internal/modules/dnd/shared -go test ./internal/modules/dnd/... -git diff --check -``` - -### Completion Gate - -All citation extractors use the shared document-aware mechanics, no unsafe -model-source candidate helper remains, and changed mapping behavior has explicit -checkpoint identity. - -## Stage 6: Consolidate Common Extraction Preflight - -### Objective - -Give the five extractors one owner for their common request prerequisites while -keeping lane-specific checks and error context local. - -### Scope - -Change `internal/modules/dnd/shared` and the five extractor packages. - -### Shared API - -Replace `ChunkPromptMaterial` with: - -```go -func PrepareChunkExtraction( - ctx context.Context, - req contracts.TypedExtractionRequest, -) (contracts.LLMInputMaterial, error) -``` - -### Required Semantics - -The helper must validate in this order: - -1. context is non-nil; -2. the context has no existing error; -3. source is non-nil; -4. chunk is non-nil; -5. the chunk contains at least one materialized unit; and -6. source input is cloned/defaulted and its content exactly matches the chunk - content. - -Preserve the current material defaults for name, media type, and size. The -returned `LLMInputMaterial` and its content must not alias the request. - -The helper returns domain-neutral error details without an extractor name. Each -extractor must wrap helper failures through its existing local error function -so diagnostics retain lane context. - -### Migration - -For all five extractors: - -- keep nil receiver and nil LLM-client checks local and before the shared - helper; -- call the helper once before specialized reference projection, provider - invocation, or mapping; -- retain catalog/NPC registry and lane-specific checks locally; -- preserve the existing provider-error prefixes and result types; and -- remove duplicated context/source/chunk/unit/material checks. - -Delete the exported `ChunkPromptMaterial` function after the fifth caller -migrates. Do not retain an alias solely for internal compatibility. - -### Tests - -- Move the full common preflight matrix to table-driven shared tests: - nil context, canceled context, nil source, nil chunk, empty units, - mismatched content, defaulted material, preserved explicit material, and - mutation safety. -- Each extractor package retains only tests that add value locally: - nil receiver/client, one representative wrapped preflight error, - specialized dependency/reference behavior, provider failure, and mapping. -- Remove redundant per-package common-branch tests once the shared contract - owns them; do not preserve five copies for coverage symmetry. - -### Validation - -Run: - -```sh -go test ./internal/modules/dnd/shared -go test ./internal/modules/dnd/extract/... -git diff --check -``` - -### Completion Gate - -All five extractors use `PrepareChunkExtraction`, validation order and error -context are preserved, common branches have one test owner, and no exported -compatibility alias remains. - -## Stage 7: Make Deterministic Validation Own Scene Semantics - -### Objective - -Remove duplicated semantic policy from the scene-description private transport -schema. - -### Scope - -Change only the scene-description extractor's private schema and focused schema -and extraction tests, plus shape-validator tests if a missing semantic case is -discovered. - -### Implementation - -1. In `dnd_scene_descriptions_llm.v1.json`, retain: - - - the schema dialect and ID; - - the top-level object type; - - `additionalProperties: false`; - - required `kind`, `title`, and `summary` fields; and - - string types for all three fields. - -2. Remove: - - - the `kind` enum; and - - `minLength` from `title` and `summary`. - -3. Do not change `SchemaVersion`, `ResponseSchemaKey`, `ResponseSchemaID`, - `ResponseSchemaName`, the private DTO, prompt text, or durable artifact - schema. The schema content digest will change and must naturally invalidate - prior extractor checkpoint identity. -4. Keep the scene-description shape validator as the sole owner of supported - kinds and non-blank title/summary semantics. - -### Tests - -- Update private-schema tests so unsupported kinds and empty strings are valid - transport values, while missing fields, wrong JSON types, unknown fields, - collections, and malformed JSON remain rejected. -- Ensure shape-validator tests cover unsupported kind, empty and whitespace-only - title, and empty and whitespace-only summary. -- Add one extractor-level test proving a structurally valid but semantically - invalid provider response is decoded and returned for deterministic - validation rather than rejected by the private schema boundary. -- Assert the schema digest is valid and mutation-safe, but do not freeze its - exact hash. - -### Validation - -Run: - -```sh -go test ./internal/modules/dnd/extract/scenedescriptions -go test ./internal/modules/dnd/validate/scenedescriptions/shape -git diff --check -``` - -### Completion Gate - -The private schema owns only transport structure, the deterministic validator -owns scene semantics, and the changed schema digest invalidates stale -checkpoints without a version rename. - -## Stage 8: Protect Prompt Order And Cache Boundaries - -### Objective - -Give all four citation extractors the same level of behavioral protection as -the scene-description prompt without freezing prompt content. - -### Scope - -Change only the Scriptorium asset tests for spells, NPCs, combat turns, and NPC -interactions. Do not edit manifests or prompt assets unless a test exposes an -actual mismatch with the already documented current contract. - -### Expected Prepared Sequences - -Using each package's real embedded registry and representative inputs, assert -these complete ordered message identities: - -| Lane | Ordered messages | -| --- | --- | -| NPC | system, extraction evidence, identity, campaign references, task, instructions, transcript | -| Spell | system, extraction evidence, identity, campaign references, NPC registry, spell catalog, task, instructions, transcript | -| Combat turn | system, extraction evidence, identity, campaign references, NPC registry, task, instructions, transcript | -| NPC interaction | system, extraction evidence, identity, campaign references, names-only NPC registry, task, instructions, transcript | - -For NPCs, assert ephemeral cache boundaries on identity, campaign references, -and instructions. For spells, combat turns, and NPC interactions, assert -ephemeral boundaries on identity, campaign references, the NPC registry, and -instructions. Assert that every other message, including the final transcript, -has no cache control. - -Use the prepared message role plus rendered input/source identity already -available from Scriptorium to distinguish messages. If an asset-only message -does not expose a stable identity, assert its position and role without -asserting exact prose. - -### Tests - -Consolidate overlapping assertions within each package where that improves -clarity. Retain existing asset registration, content-safety, and prompt digest -tests when they protect distinct risks. - -Do not assert: - -- exact prompt text; -- prefix length or total byte count; -- an exact prompt hash; -- private manifest file layout; or -- that a particular shared helper or asset path was used. - -### Validation - -Run: - -```sh -go test ./internal/modules/dnd/extract/spells -go test ./internal/modules/dnd/extract/npcs -go test ./internal/modules/dnd/extract/combatturns -go test ./internal/modules/dnd/extract/npcinteractions -git diff --check -``` - -### Completion Gate - -All four suites protect the complete documented role/input order and cache -flags through real prompt preparation, without change-detector assertions. - -## Stage 9: Package Hygiene And Extractor Documentation - -### Objective - -Remove the remaining misleading package surface and document the common -contract future D&D extractors must follow. - -### Scope - -Change the spell and combat-turn extractor packages and -`docs/internal/modules.md`. Include no new production abstraction. - -### Code Cleanup - -1. Delete the unused singular `ArtifactType` constants from the spell and - combat-turn extractor packages. `ArtifactKind` and the durable typed model - remain authoritative. -2. Add the same nil guard used by the other extractors to spell - `ManifestMetadata`: - - ```go - if e == nil { - return nil - } - ``` - -3. Add one focused zero-value spell metadata test. Do not add tests for the - absence of deleted constants. - -### Documentation - -Update `docs/internal/modules.md` as the canonical current-behavior owner: - -1. Replace the stale `ChunkPromptMaterial` consumer description with - `PrepareChunkExtraction` and name all five current extractor consumers. -2. Add a compact `### D&D Extractor Contract` subsection within or immediately - after `## Adding An Extension`. -3. State requirements as behaviors and ownership boundaries, not mandatory - filenames: - - - reject unknown options unless an option namespace is intentionally - extensible; - - use shared common preflight while retaining receiver, dependency, and - lane-specific checks locally; - - return independently owned results and exposed metadata safe for caller - mutation; - - keep the private response DTO, structural schema and identity, - provider-response mapping, durable conversion, and lane diagnostics - package-owned; - - include every stable semantic input capable of changing durable output in - checkpoint identity, considering prompts, schemas, mapping, - canonicalization, prepared reference projections, identity, - normalization, and trimming as applicable; and - - consider focused behavioral coverage for construction/registration, - option rejection, preflight, provider failure, structured decoding, - mapping/ownership, prompt role/input/cache order, and checkpoint - invalidation. - -4. Link rather than duplicate: - - - prompt ordering, shared-asset, cache, and private-schema rules in - `docs/internal/llm.md`; - - checkpoint and reference behavior in `docs/internal/pipeline.md`; - - architecture ownership rules; and - - the testing policy. - -5. Explicitly avoid prescribing exact prompt content or length, hashes, test - counts, filenames, fixture layouts, or generic implementation builders. - -### Validation - -Run: - -```sh -go test ./internal/modules/dnd/extract/spells -go test ./internal/modules/dnd/extract/combatturns -go test ./internal/modules/dnd/shared -git diff --check -``` - -Validate every new documentation link and confirm the text describes behavior -implemented by Stages 1 through 8. - -### Completion Gate - -The misleading exports are gone, spell metadata is nil-safe, the shared -preflight documentation is current, and future extractors have one concise -normative checklist linked to the canonical detailed policies. - -## Stage 10: Integrated Regression And Audit Closure - -### Objective - -Verify the complete refactor as one system and remove any residual duplication -or stale references introduced or exposed by the migration. - -### Scope - -This is a verification and narrowly scoped correction stage. Do not introduce -new abstractions or expand product behavior. - -### Required Review - -1. Re-read every audit finding and confirm it is addressed: - - - document-aware reference and spell artifact ordering; - - spell/NPC mapping fingerprints; - - shared common extraction preflight; - - deterministic ownership of scene semantics; - - complete prompt-order/cache tests; - - package hygiene; and - - the documented D&D extractor contract. - -2. Search for and resolve only genuine leftovers: - - - numeric ordering of D&D `SourceRef` endpoints where document order is the - intended policy; - - production calls to removed helpers; - - stale references to `ChunkPromptMaterial`, `SourceRefCandidate`, the - deleted NPC-interaction comparators, or deleted `ArtifactType` constants; - - missing policy bumps for behavior changed by this plan; - - raw schema or prompt bytes in diagnostics; and - - documentation that still describes pre-refactor behavior. - -3. Confirm the intentional differences and rejected abstractions in the audit - remain intact. -4. Confirm each changed fingerprint value is bounded, deterministic, - non-secret, and included at the correct component scope. -5. Confirm no test added by this work is a prefix-length, exact-hash, - helper-usage, or file-layout change detector. - -### Validation - -Run the full repository checks: - -```sh -go test -count=1 ./... -go vet ./... -go build ./cmd/notarius -gofmt -l . -git diff --check -``` - -`gofmt -l .` must print no files. If a command fails, diagnose and correct only -failures caused by this implementation. Report unrelated pre-existing failures -without modifying their owners. - -### Completion Gate - -All audit findings are implemented, every repository-wide check passes except -any clearly reported pre-existing failure, current-behavior documentation is -accurate, and no superseded helper or policy copy remains. - -## Open Questions - -None. The audit findings, API ownership, semantic policy values, migration -order, test boundaries, documentation owner, and intentionally deferred -product decisions are specified above. diff --git a/internal/core/source/source_test.go b/internal/core/source/source_test.go index a58167c..2bf9307 100644 --- a/internal/core/source/source_test.go +++ b/internal/core/source/source_test.go @@ -249,6 +249,9 @@ func TestValidateRefValid(t *testing.T) { if err := ValidateRef(doc, ref); err != nil { t.Fatalf("ValidateRef() error = %v, want nil", err) } + if err := NewDocumentIndex(doc).ValidateRef(ref); err != nil { + t.Fatalf("DocumentIndex.ValidateRef() error = %v, want nil", err) + } } func TestValidateRefRejectsMalformedReferences(t *testing.T) { @@ -301,13 +304,56 @@ func TestValidateRefRejectsMalformedReferences(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := ValidateRef(validDocument(), tt.ref) - - requireErrorFragments(t, err, tt.fragments...) + doc := validDocument() + validators := []struct { + name string + validate func(SourceRef) error + }{ + {name: "document", validate: func(ref SourceRef) error { return ValidateRef(doc, ref) }}, + {name: "index", validate: NewDocumentIndex(doc).ValidateRef}, + } + for _, validator := range validators { + t.Run(validator.name, func(t *testing.T) { + requireErrorFragments(t, validator.validate(tt.ref), tt.fragments...) + }) + } }) } } +func TestDocumentIndexSnapshotsIdentityAndUnitPositions(t *testing.T) { + doc := &SourceDocument{ + ID: "source-1", + Units: []SourceUnit{ + {ID: 30}, + {ID: 10}, + {ID: 30}, + }, + } + index := NewDocumentIndex(doc) + doc.ID = "changed" + doc.Units[0].ID = 99 + + if position, ok := index.Position(30); !ok || position != 0 { + t.Fatalf("Position(30) = %d, %t, want 0, true", position, ok) + } + if position, ok := index.Position(10); !ok || position != 1 { + t.Fatalf("Position(10) = %d, %t, want 1, true", position, ok) + } + ref := SourceRef{SourceID: "source-1", StartUnitID: 30, EndUnitID: 10} + if err := index.ValidateRef(ref); err != nil { + t.Fatalf("ValidateRef() error = %v, want nil", err) + } +} + +func TestZeroDocumentIndexIsSafe(t *testing.T) { + var index DocumentIndex + if position, ok := index.Position(1); ok || position != 0 { + t.Fatalf("Position(1) = %d, %t, want 0, false", position, ok) + } + requireErrorFragments(t, index.ValidateRef(SourceRef{}), "source document must not be nil") +} + func TestUnitIndex(t *testing.T) { doc := validDocument() diff --git a/internal/core/source/validation.go b/internal/core/source/validation.go index 9bea5ec..df1b1c6 100644 --- a/internal/core/source/validation.go +++ b/internal/core/source/validation.go @@ -5,6 +5,46 @@ import ( "strings" ) +// DocumentIndex is an immutable snapshot of a source document's identity and +// unit positions for repeated source-reference operations. +type DocumentIndex struct { + documentID string + positions map[int]int + hasDocument bool +} + +// NewDocumentIndex snapshots doc without retaining or mutating it. +func NewDocumentIndex(doc *SourceDocument) DocumentIndex { + if doc == nil { + return DocumentIndex{} + } + positions := make(map[int]int, len(doc.Units)) + for position, unit := range doc.Units { + if _, exists := positions[unit.ID]; !exists { + positions[unit.ID] = position + } + } + return DocumentIndex{ + documentID: doc.ID, + positions: positions, + hasDocument: true, + } +} + +// Position returns the indexed document position for unitID. +func (i DocumentIndex) Position(unitID int) (int, bool) { + position, ok := i.positions[unitID] + return position, ok +} + +// ValidateRef validates ref against the indexed document snapshot. +func (i DocumentIndex) ValidateRef(ref SourceRef) error { + if !i.hasDocument { + return fmt.Errorf("source document must not be nil") + } + return validateRef(i.documentID, i.Position, ref) +} + func ValidateDocument(doc *SourceDocument) error { if doc == nil { return fmt.Errorf("source document must not be nil") @@ -60,6 +100,12 @@ func ValidateRef(doc *SourceDocument, ref SourceRef) error { if doc == nil { return fmt.Errorf("source document must not be nil") } + return validateRef(doc.ID, func(unitID int) (int, bool) { + return UnitIndex(doc, unitID) + }, ref) +} + +func validateRef(documentID string, position func(int) (int, bool), ref SourceRef) error { if isBlank(ref.SourceID) { return fmt.Errorf("source ref source_id must not be empty") } @@ -72,15 +118,15 @@ func ValidateRef(doc *SourceDocument, ref SourceRef) error { if ref.EndUnitID <= 0 { return fmt.Errorf("source ref end_unit_id must be positive") } - if ref.SourceID != doc.ID { - return fmt.Errorf("source ref source_id %q does not match document id %q", ref.SourceID, doc.ID) + if ref.SourceID != documentID { + return fmt.Errorf("source ref source_id %q does not match document id %q", ref.SourceID, documentID) } - startIndex, ok := UnitIndex(doc, ref.StartUnitID) + startIndex, ok := position(ref.StartUnitID) if !ok { return fmt.Errorf("source ref start_unit_id %d was not found", ref.StartUnitID) } - endIndex, ok := UnitIndex(doc, ref.EndUnitID) + endIndex, ok := position(ref.EndUnitID) if !ok { return fmt.Errorf("source ref end_unit_id %d was not found", ref.EndUnitID) } diff --git a/internal/modules/dnd/extract/combatturns/canonicalize.go b/internal/modules/dnd/extract/combatturns/canonicalize.go index 754dc7a..ea8aaa5 100644 --- a/internal/modules/dnd/extract/combatturns/canonicalize.go +++ b/internal/modules/dnd/extract/combatturns/canonicalize.go @@ -8,31 +8,46 @@ import ( "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/shared" ) +type orderedCombatTurnResponse struct { + value combatTurnResponse + earliest int + hasEvidence bool +} + func canonicalizeResponse(response *extractionResponse, order shared.SourceRefOrder, sourceID string) { if response == nil { return } + ordered := make([]orderedCombatTurnResponse, len(response.CombatTurns)) for index := range response.CombatTurns { - canonicalizeCombatTurn(&response.CombatTurns[index], order, sourceID) - } - sort.SliceStable(response.CombatTurns, func(i, j int) bool { - left, leftOK := order.EarliestValid(canonicalSourceRefs(response.CombatTurns[i].SourceRefs, sourceID)) - right, rightOK := order.EarliestValid(canonicalSourceRefs(response.CombatTurns[j].SourceRefs, sourceID)) - if leftOK != rightOK { - return leftOK + earliest, hasEvidence := canonicalizeCombatTurn(&response.CombatTurns[index], order, sourceID) + ordered[index] = orderedCombatTurnResponse{ + value: response.CombatTurns[index], + earliest: earliest, + hasEvidence: hasEvidence, } - if !leftOK { + } + sort.SliceStable(ordered, func(i, j int) bool { + if ordered[i].hasEvidence != ordered[j].hasEvidence { + return ordered[i].hasEvidence + } + if !ordered[i].hasEvidence { return false } - return left < right + return ordered[i].earliest < ordered[j].earliest }) + for index := range ordered { + response.CombatTurns[index] = ordered[index].value + } } -func canonicalizeCombatTurn(turn *combatTurnResponse, order shared.SourceRefOrder, sourceID string) { +func canonicalizeCombatTurn(turn *combatTurnResponse, order shared.SourceRefOrder, sourceID string) (int, bool) { if turn == nil { - return + return 0, false } - turn.SourceRefs = combatResponseRefs(order.Canonicalize(canonicalSourceRefs(turn.SourceRefs, sourceID))) + refs := order.Canonicalize(canonicalSourceRefs(turn.SourceRefs, sourceID)) + turn.SourceRefs = combatResponseRefs(refs) + return order.EarliestValid(refs) } func canonicalCombatTurnList(response extractionResponse, sourceID string) dnd.CombatTurnList { diff --git a/internal/modules/dnd/extract/combatturns/scriptorium_assets_test.go b/internal/modules/dnd/extract/combatturns/scriptorium_assets_test.go index 97e383c..291f1d8 100644 --- a/internal/modules/dnd/extract/combatturns/scriptorium_assets_test.go +++ b/internal/modules/dnd/extract/combatturns/scriptorium_assets_test.go @@ -65,14 +65,15 @@ func TestScriptoriumPromptPreparesRequiredInputs(t *testing.T) { for index, want := range []struct { role string cached bool + marker string }{ - {role: "system"}, - {role: "user"}, + {role: "system", marker: "Dungeons & Dragons gameplay transcripts"}, + {role: "user", marker: "Transcript units are the only evidence"}, + {role: "user", cached: true, marker: "most specific supported in-world"}, {role: "user", cached: true}, {role: "user", cached: true}, - {role: "user", cached: true}, - {role: "user"}, - {role: "user", cached: true}, + {role: "user", marker: "combat-turn artifacts"}, + {role: "user", cached: true, marker: "turn_kind"}, {role: "user"}, } { if index >= len(prepared.Messages) { @@ -89,6 +90,9 @@ func TestScriptoriumPromptPreparesRequiredInputs(t *testing.T) { } else if message.CacheControl != nil { t.Errorf("message %d cache control = %#v, want nil", index, message.CacheControl) } + if want.marker != "" && !strings.Contains(message.Content, want.marker) { + t.Errorf("message %d content does not contain purpose marker %q", index, want.marker) + } } if len(prepared.Messages) != 8 { t.Fatalf("prepared prompt has %d messages, want 8", len(prepared.Messages)) diff --git a/internal/modules/dnd/extract/npcinteractions/canonicalize.go b/internal/modules/dnd/extract/npcinteractions/canonicalize.go index d785f1b..8a12301 100644 --- a/internal/modules/dnd/extract/npcinteractions/canonicalize.go +++ b/internal/modules/dnd/extract/npcinteractions/canonicalize.go @@ -8,31 +8,46 @@ import ( "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/shared" ) +type orderedInteractionResponse struct { + value interactionResponse + earliest int + hasEvidence bool +} + func canonicalizeResponse(response *extractionResponse, order shared.SourceRefOrder, sourceID string) { if response == nil { return } + ordered := make([]orderedInteractionResponse, len(response.Interactions)) for index := range response.Interactions { - canonicalizeInteraction(&response.Interactions[index], order, sourceID) - } - sort.SliceStable(response.Interactions, func(i, j int) bool { - left, leftOK := order.EarliestValid(canonicalSourceRefs(response.Interactions[i].SourceRefs, sourceID)) - right, rightOK := order.EarliestValid(canonicalSourceRefs(response.Interactions[j].SourceRefs, sourceID)) - if leftOK != rightOK { - return leftOK + earliest, hasEvidence := canonicalizeInteraction(&response.Interactions[index], order, sourceID) + ordered[index] = orderedInteractionResponse{ + value: response.Interactions[index], + earliest: earliest, + hasEvidence: hasEvidence, } - if !leftOK { + } + sort.SliceStable(ordered, func(i, j int) bool { + if ordered[i].hasEvidence != ordered[j].hasEvidence { + return ordered[i].hasEvidence + } + if !ordered[i].hasEvidence { return false } - return left < right + return ordered[i].earliest < ordered[j].earliest }) + for index := range ordered { + response.Interactions[index] = ordered[index].value + } } -func canonicalizeInteraction(interaction *interactionResponse, order shared.SourceRefOrder, sourceID string) { +func canonicalizeInteraction(interaction *interactionResponse, order shared.SourceRefOrder, sourceID string) (int, bool) { if interaction == nil { - return + return 0, false } - interaction.SourceRefs = interactionResponseRefs(order.Canonicalize(canonicalSourceRefs(interaction.SourceRefs, sourceID))) + refs := order.Canonicalize(canonicalSourceRefs(interaction.SourceRefs, sourceID)) + interaction.SourceRefs = interactionResponseRefs(refs) + return order.EarliestValid(refs) } func canonicalInteractionList(response extractionResponse, sourceID string) dnd.NPCInteractionList { diff --git a/internal/modules/dnd/extract/npcinteractions/scriptorium_assets_test.go b/internal/modules/dnd/extract/npcinteractions/scriptorium_assets_test.go index 7a6f5a7..1d5af4e 100644 --- a/internal/modules/dnd/extract/npcinteractions/scriptorium_assets_test.go +++ b/internal/modules/dnd/extract/npcinteractions/scriptorium_assets_test.go @@ -54,14 +54,15 @@ func TestRegisterPromptAssetsAndPrepareInteractionPrompt(t *testing.T) { for index, want := range []struct { role string cached bool + marker string }{ - {role: "system"}, - {role: "user"}, + {role: "system", marker: "Dungeons & Dragons gameplay transcripts"}, + {role: "user", marker: "Transcript units are the only evidence"}, + {role: "user", cached: true, marker: "most specific supported in-world"}, {role: "user", cached: true}, {role: "user", cached: true}, - {role: "user", cached: true}, - {role: "user"}, - {role: "user", cached: true}, + {role: "user", marker: "interaction occurrences"}, + {role: "user", cached: true, marker: "Use exactly one kind per occurrence"}, {role: "user"}, } { if index >= len(prepared.Messages) { @@ -78,6 +79,9 @@ func TestRegisterPromptAssetsAndPrepareInteractionPrompt(t *testing.T) { } else if message.CacheControl != nil { t.Errorf("message %d cache control = %#v, want nil", index, message.CacheControl) } + if want.marker != "" && !strings.Contains(message.Content, want.marker) { + t.Errorf("message %d content does not contain purpose marker %q", index, want.marker) + } } if len(prepared.Messages) != 8 { t.Fatalf("prepared prompt has %d messages, want 8", len(prepared.Messages)) diff --git a/internal/modules/dnd/extract/npcs/canonicalize.go b/internal/modules/dnd/extract/npcs/canonicalize.go index a61c17a..fc8e706 100644 --- a/internal/modules/dnd/extract/npcs/canonicalize.go +++ b/internal/modules/dnd/extract/npcs/canonicalize.go @@ -9,31 +9,46 @@ import ( "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/shared" ) +type orderedNPCResponse struct { + value npcResponse + earliest int + hasEvidence bool +} + func canonicalizeResponse(response *extractionResponse, order shared.SourceRefOrder, sourceID string) { if response == nil { return } + ordered := make([]orderedNPCResponse, len(response.NPCs)) for index := range response.NPCs { - canonicalizeNPC(&response.NPCs[index], order, sourceID) - } - sort.SliceStable(response.NPCs, func(i, j int) bool { - left, leftOK := order.EarliestValid(canonicalSourceRefs(response.NPCs[i].SourceRefs, sourceID)) - right, rightOK := order.EarliestValid(canonicalSourceRefs(response.NPCs[j].SourceRefs, sourceID)) - if leftOK != rightOK { - return leftOK + earliest, hasEvidence := canonicalizeNPC(&response.NPCs[index], order, sourceID) + ordered[index] = orderedNPCResponse{ + value: response.NPCs[index], + earliest: earliest, + hasEvidence: hasEvidence, } - if !leftOK { + } + sort.SliceStable(ordered, func(i, j int) bool { + if ordered[i].hasEvidence != ordered[j].hasEvidence { + return ordered[i].hasEvidence + } + if !ordered[i].hasEvidence { return false } - return left < right + return ordered[i].earliest < ordered[j].earliest }) + for index := range ordered { + response.NPCs[index] = ordered[index].value + } } -func canonicalizeNPC(npc *npcResponse, order shared.SourceRefOrder, sourceID string) { +func canonicalizeNPC(npc *npcResponse, order shared.SourceRefOrder, sourceID string) (int, bool) { if npc == nil { - return + return 0, false } - npc.SourceRefs = npcResponseRefs(order.Canonicalize(canonicalSourceRefs(npc.SourceRefs, sourceID))) + refs := order.Canonicalize(canonicalSourceRefs(npc.SourceRefs, sourceID)) + npc.SourceRefs = npcResponseRefs(refs) + return order.EarliestValid(refs) } func canonicalNPCList(response extractionResponse, sourceID string) dnd.NPCList { diff --git a/internal/modules/dnd/extract/npcs/scriptorium_assets_test.go b/internal/modules/dnd/extract/npcs/scriptorium_assets_test.go index e22047c..8dc28e6 100644 --- a/internal/modules/dnd/extract/npcs/scriptorium_assets_test.go +++ b/internal/modules/dnd/extract/npcs/scriptorium_assets_test.go @@ -45,13 +45,14 @@ func TestRegisterPromptAssetsAndPrepareNPCPrompt(t *testing.T) { for index, want := range []struct { role string cached bool + marker string }{ - {role: "system"}, - {role: "user"}, - {role: "user", cached: true}, - {role: "user", cached: true}, - {role: "user"}, + {role: "system", marker: "Dungeons & Dragons gameplay transcripts"}, + {role: "user", marker: "Transcript units are the only evidence"}, + {role: "user", cached: true, marker: "most specific supported in-world"}, {role: "user", cached: true}, + {role: "user", marker: "individually identifiable"}, + {role: "user", cached: true, marker: "observed display name"}, {role: "user"}, } { if index >= len(prepared.Messages) { @@ -68,6 +69,9 @@ func TestRegisterPromptAssetsAndPrepareNPCPrompt(t *testing.T) { } else if message.CacheControl != nil { t.Errorf("message %d cache control = %#v, want nil", index, message.CacheControl) } + if want.marker != "" && !strings.Contains(message.Content, want.marker) { + t.Errorf("message %d content does not contain purpose marker %q", index, want.marker) + } } if len(prepared.Messages) != 7 { t.Fatalf("prepared prompt has %d messages, want 7", len(prepared.Messages)) diff --git a/internal/modules/dnd/extract/spells/canonicalize.go b/internal/modules/dnd/extract/spells/canonicalize.go index 2a28f36..76387bb 100644 --- a/internal/modules/dnd/extract/spells/canonicalize.go +++ b/internal/modules/dnd/extract/spells/canonicalize.go @@ -8,31 +8,46 @@ import ( "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/shared" ) +type orderedSpellResponse struct { + value spellCastResponse + earliest int + hasEvidence bool +} + func canonicalizeResponse(response *extractionResponse, order shared.SourceRefOrder, sourceID string) { if response == nil { return } + ordered := make([]orderedSpellResponse, len(response.SpellCasts)) for index := range response.SpellCasts { - canonicalizeSpellCast(&response.SpellCasts[index], order, sourceID) - } - sort.SliceStable(response.SpellCasts, func(i, j int) bool { - left, leftOK := order.EarliestValid(spellSourceRefs(response.SpellCasts[i].SourceRefs, sourceID)) - right, rightOK := order.EarliestValid(spellSourceRefs(response.SpellCasts[j].SourceRefs, sourceID)) - if leftOK != rightOK { - return leftOK + earliest, hasEvidence := canonicalizeSpellCast(&response.SpellCasts[index], order, sourceID) + ordered[index] = orderedSpellResponse{ + value: response.SpellCasts[index], + earliest: earliest, + hasEvidence: hasEvidence, } - if !leftOK { + } + sort.SliceStable(ordered, func(i, j int) bool { + if ordered[i].hasEvidence != ordered[j].hasEvidence { + return ordered[i].hasEvidence + } + if !ordered[i].hasEvidence { return false } - return left < right + return ordered[i].earliest < ordered[j].earliest }) + for index := range ordered { + response.SpellCasts[index] = ordered[index].value + } } -func canonicalizeSpellCast(spell *spellCastResponse, order shared.SourceRefOrder, sourceID string) { +func canonicalizeSpellCast(spell *spellCastResponse, order shared.SourceRefOrder, sourceID string) (int, bool) { if spell == nil { - return + return 0, false } - spell.SourceRefs = spellResponseRefs(order.Canonicalize(spellSourceRefs(spell.SourceRefs, sourceID))) + refs := order.Canonicalize(spellSourceRefs(spell.SourceRefs, sourceID)) + spell.SourceRefs = spellResponseRefs(refs) + return order.EarliestValid(refs) } func spellSourceRefs(refs []spellSourceRefResponse, sourceID string) []source.SourceRef { diff --git a/internal/modules/dnd/extract/spells/scriptorium_assets_test.go b/internal/modules/dnd/extract/spells/scriptorium_assets_test.go index 2b77956..d313e64 100644 --- a/internal/modules/dnd/extract/spells/scriptorium_assets_test.go +++ b/internal/modules/dnd/extract/spells/scriptorium_assets_test.go @@ -24,15 +24,16 @@ func TestScriptoriumPromptPreparesTranscriptReferencesAndTaskMessages(t *testing for index, want := range []struct { role string cached bool + marker string }{ - {role: "system"}, - {role: "user"}, - {role: "user", cached: true}, + {role: "system", marker: "Dungeons & Dragons gameplay transcripts"}, + {role: "user", marker: "Transcript units are the only evidence"}, + {role: "user", cached: true, marker: "most specific supported in-world"}, {role: "user", cached: true}, {role: "user", cached: true}, {role: "user"}, - {role: "user"}, - {role: "user", cached: true}, + {role: "user", marker: "spell-cast artifacts"}, + {role: "user", cached: true, marker: "source references must collectively support"}, {role: "user"}, } { if index >= len(prepared.Messages) { @@ -49,6 +50,9 @@ func TestScriptoriumPromptPreparesTranscriptReferencesAndTaskMessages(t *testing } else if message.CacheControl != nil { t.Errorf("message %d cache control = %#v, want nil", index, message.CacheControl) } + if want.marker != "" && !strings.Contains(message.Content, want.marker) { + t.Errorf("message %d content does not contain purpose marker %q", index, want.marker) + } } if len(prepared.Messages) != 9 { t.Fatalf("prepared prompt has %d messages, want 9", len(prepared.Messages)) diff --git a/internal/modules/dnd/normalize/spells/normalizer.go b/internal/modules/dnd/normalize/spells/normalizer.go index 0e3dd4a..bbb700a 100644 --- a/internal/modules/dnd/normalize/spells/normalizer.go +++ b/internal/modules/dnd/normalize/spells/normalizer.go @@ -17,7 +17,7 @@ import ( const ( Key = "dnd/spells" - normalizationPolicy = "dnd.spells.normalize.v1" + normalizationPolicy = "dnd.spells.normalize.v2" NormalizationPolicy = normalizationPolicy ) @@ -157,20 +157,15 @@ func cloneSpellCast(input dnd.SpellCast) dnd.SpellCast { } func canonicalizeSourceRefs(order shared.SourceRefOrder, input []source.SourceRef) ([]source.SourceRef, bool, int) { - canonical := order.Canonicalize(input) - return canonical, !sourceRefsEqual(input, canonical), len(input) - len(canonical) -} - -func sourceRefsEqual(left, right []source.SourceRef) bool { - if (left == nil) != (right == nil) || len(left) != len(right) { - return false - } - for index := range left { - if left[index] != right[index] { - return false + orderChanged := false + for index := 1; index < len(input); index++ { + if order.Less(input[index], input[index-1]) { + orderChanged = true + break } } - return true + canonical := order.Canonicalize(input) + return canonical, orderChanged, len(input) - len(canonical) } type duplicateGroup struct { diff --git a/internal/modules/dnd/normalize/spells/normalizer_test.go b/internal/modules/dnd/normalize/spells/normalizer_test.go index 5837741..709e210 100644 --- a/internal/modules/dnd/normalize/spells/normalizer_test.go +++ b/internal/modules/dnd/normalize/spells/normalizer_test.go @@ -104,6 +104,9 @@ func TestIdentityAndMetadataAreDefensive(t *testing.T) { if err != nil { t.Fatal(err) } + if NormalizationPolicy != "dnd.spells.normalize.v2" { + t.Fatalf("NormalizationPolicy = %q, want v2 policy", NormalizationPolicy) + } fingerprints := normalizer.CheckpointFingerprints() if len(fingerprints) != 2 || fingerprints[0].Name != "effective_catalog" || fingerprints[0].Value != normalizer.effectiveCatalog.Digest() || fingerprints[1] != (pipeline.CheckpointFingerprint{Name: "normalization_policy", Value: normalizationPolicy}) { @@ -116,7 +119,7 @@ func TestIdentityAndMetadataAreDefensive(t *testing.T) { } metadata := normalizer.ManifestMetadata() - if metadata["catalog_base_id"] != spellcatalog.SRD5E2014ID || metadata["catalog_digest"] != normalizer.effectiveCatalog.Digest() { + if metadata["catalog_base_id"] != spellcatalog.SRD5E2014ID || metadata["catalog_digest"] != normalizer.effectiveCatalog.Digest() || metadata["normalization_policy"] != normalizationPolicy { t.Fatalf("metadata = %#v, want catalog identity", metadata) } metadata["catalog_overlay_ids"].([]string)[0] = "changed" @@ -205,11 +208,32 @@ func TestNormalizeSortsAndDeduplicatesExactSourceReferences(t *testing.T) { if !reflect.DeepEqual(result.Value.SpellCasts[0].SourceRefs, wantRefs) { t.Fatalf("source refs = %#v, want %#v", result.Value.SpellCasts[0].SourceRefs, wantRefs) } - if len(result.Warnings) != 1 || result.Warnings[0].ReasonCode != ReasonCodeSourceReferencesNormalized || !strings.Contains(result.Warnings[0].Message, "original count 6") || !strings.Contains(result.Warnings[0].Message, "final count 5") || !strings.Contains(result.Warnings[0].Message, "duplicates removed 1") { + if len(result.Warnings) != 1 || result.Warnings[0].ReasonCode != ReasonCodeSourceReferencesNormalized || !strings.Contains(result.Warnings[0].Message, "original count 6") || !strings.Contains(result.Warnings[0].Message, "final count 5") || !strings.Contains(result.Warnings[0].Message, "order changed true") || !strings.Contains(result.Warnings[0].Message, "duplicates removed 1") { t.Fatalf("warnings = %#v, want source normalization warning", result.Warnings) } } +func TestNormalizeReportsDuplicateRemovalWithoutOrderChange(t *testing.T) { + ref1 := source.SourceRef{SourceID: "source", StartUnitID: 1, EndUnitID: 1} + ref2 := source.SourceRef{SourceID: "source", StartUnitID: 2, EndUnitID: 2} + input := dnd.SpellList{SpellCasts: []dnd.SpellCast{{ + Spell: "Cure Wounds", + SourceRefs: []source.SourceRef{ref1, ref1, ref2}, + }}} + + result, err := newNormalizer(t).Normalize(context.Background(), normalizeRequest(input)) + if err != nil { + t.Fatalf("Normalize() error = %v, want nil", err) + } + if len(result.Warnings) != 1 || result.Warnings[0].ReasonCode != ReasonCodeSourceReferencesNormalized { + t.Fatalf("warnings = %#v, want one source normalization warning", result.Warnings) + } + message := result.Warnings[0].Message + if !strings.Contains(message, "order changed false") || !strings.Contains(message, "duplicates removed 1") { + t.Fatalf("warning = %q, want duplicate-only repair without order change", message) + } +} + func TestNormalizeOrdersReferencesBySourceDocumentPosition(t *testing.T) { doc := &source.SourceDocument{ID: "source", Units: []source.SourceUnit{{ID: 30}, {ID: 10}}} input := dnd.SpellList{SpellCasts: []dnd.SpellCast{{ diff --git a/internal/modules/dnd/shared/source_ref_order.go b/internal/modules/dnd/shared/source_ref_order.go index c0395bb..06a9699 100644 --- a/internal/modules/dnd/shared/source_ref_order.go +++ b/internal/modules/dnd/shared/source_ref_order.go @@ -2,7 +2,6 @@ package shared import ( "sort" - "strings" "gitea.maximumdirect.net/eric/notarius/internal/core/source" ) @@ -10,8 +9,8 @@ import ( // SourceRefOrder provides a stable snapshot of a source document's unit // ordering for source-reference comparison and canonicalization. type SourceRefOrder struct { - sourceID string - positions map[int]int + sourceID string + index source.DocumentIndex } // NewSourceRefOrder captures the source identity and unit positions from doc. @@ -19,13 +18,7 @@ func NewSourceRefOrder(doc *source.SourceDocument) SourceRefOrder { if doc == nil { return SourceRefOrder{} } - positions := make(map[int]int, len(doc.Units)) - for position, unit := range doc.Units { - if _, exists := positions[unit.ID]; !exists { - positions[unit.ID] = position - } - } - return SourceRefOrder{sourceID: doc.ID, positions: positions} + return SourceRefOrder{sourceID: doc.ID, index: source.NewDocumentIndex(doc)} } // Less orders references by source identity, then document positions when @@ -34,14 +27,13 @@ func (o SourceRefOrder) Less(left, right source.SourceRef) bool { if left.SourceID != right.SourceID { return left.SourceID < right.SourceID } - positions := o.positionsFor(left.SourceID) - if lessEndpoint(positions, left.StartUnitID, right.StartUnitID) { + if o.lessEndpoint(left.SourceID, left.StartUnitID, right.StartUnitID) { return true } - if lessEndpoint(positions, right.StartUnitID, left.StartUnitID) { + if o.lessEndpoint(left.SourceID, right.StartUnitID, left.StartUnitID) { return false } - return lessEndpoint(positions, left.EndUnitID, right.EndUnitID) + return o.lessEndpoint(left.SourceID, left.EndUnitID, right.EndUnitID) } // EarliestValid returns the earliest document position among valid refs. @@ -52,14 +44,10 @@ func (o SourceRefOrder) EarliestValid(refs []source.SourceRef) (int, bool) { found := false earliest := 0 for _, ref := range refs { - if ref.SourceID != o.sourceID || strings.TrimSpace(ref.SourceID) != ref.SourceID || ref.StartUnitID <= 0 || ref.EndUnitID <= 0 { - continue - } - start, startOK := o.positions[ref.StartUnitID] - end, endOK := o.positions[ref.EndUnitID] - if !startOK || !endOK || start > end { + if o.index.ValidateRef(ref) != nil { continue } + start, _ := o.index.Position(ref.StartUnitID) if !found || start < earliest { earliest = start found = true @@ -87,16 +75,9 @@ func (o SourceRefOrder) Canonicalize(refs []source.SourceRef) []source.SourceRef return unique } -func (o SourceRefOrder) positionsFor(sourceID string) map[int]int { - if o.sourceID == "" || sourceID != o.sourceID { - return nil - } - return o.positions -} - -func lessEndpoint(positions map[int]int, left, right int) bool { - leftPosition, leftOK := positions[left] - rightPosition, rightOK := positions[right] +func (o SourceRefOrder) lessEndpoint(sourceID string, left, right int) bool { + leftPosition, leftOK := o.position(sourceID, left) + rightPosition, rightOK := o.position(sourceID, right) if leftOK != rightOK { return leftOK } @@ -105,3 +86,10 @@ func lessEndpoint(positions map[int]int, left, right int) bool { } return left < right } + +func (o SourceRefOrder) position(sourceID string, unitID int) (int, bool) { + if o.sourceID == "" || sourceID != o.sourceID { + return 0, false + } + return o.index.Position(unitID) +}