Fix D&D extraction issues and retire the completed audit
This commit is contained in:
@@ -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:<lane-id>:dnd/spells:mapping_policy`; and
|
||||
- `extract:<lane-id>: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.
|
||||
Reference in New Issue
Block a user