Add feature roadmap and implementation plan for D&D spell normalization module
This commit is contained in:
@@ -1,180 +0,0 @@
|
||||
# D&D Spell Extraction Quality
|
||||
|
||||
## Status
|
||||
|
||||
The accepted baseline implementation is complete. It provides effective
|
||||
SRD-plus-overlay catalog composition, canonical-name prompt grounding,
|
||||
deterministic catalog validation, the maintained `retries: 2` policy, and
|
||||
provenance and checkpoint identity coverage through the assembled offline
|
||||
workflow. Checkpoint identity includes independently scoped extractor and
|
||||
validator fingerprints of the effective catalog, closing the earlier gap in
|
||||
which raw overlay changes invalidated reuse but embedded catalog changes did
|
||||
not.
|
||||
|
||||
External quality evaluation is pending. This repository contains the
|
||||
maintained example and offline fake-LLM coverage, but no approved
|
||||
human-reviewed transcript corpus or authorized live-model evaluation was
|
||||
available for this implementation run. The maintained example can be run
|
||||
from the repository root with:
|
||||
|
||||
```sh
|
||||
go run ./cmd/notarius run dnd-session \
|
||||
--config examples/dnd-spells-production.config.yml \
|
||||
--input examples/seriatim-minimal-transcript.json \
|
||||
--chunk_cache bypass
|
||||
```
|
||||
|
||||
That command requires the configured Scriptorium profile credential. When an
|
||||
approved profile and reviewed corpus are available, record anonymized
|
||||
aggregate false positives, false negatives, unknown-name rejections,
|
||||
acceptance by attempt, and model-call cost here without committing transcripts
|
||||
or raw prompts. Feedback-aware repair and semantic LLM validation remain
|
||||
deferred until that baseline evaluation is available.
|
||||
|
||||
## Objective
|
||||
|
||||
Improve the precision and consistency of `dnd/spells` extraction by first
|
||||
grounding the LLM in an effective spell catalog composed from the embedded SRD
|
||||
5.1 catalog and configured overlays, rejecting invalid catalog names
|
||||
deterministically, and measuring the existing retry mechanism. Preserve a clear
|
||||
separation between response shape, domain policy, retry orchestration, and later
|
||||
semantic review.
|
||||
|
||||
## Accepted Baseline Scope
|
||||
|
||||
### Ground extraction with a catalog input
|
||||
|
||||
Provide the spell catalog to the extraction prompt as a separate input rather
|
||||
than embedding all spell names as an enum in the private LLM response schema.
|
||||
The response schema should continue to define transport shape, while the
|
||||
catalog remains the single source of truth for recognized spell names.
|
||||
|
||||
Build the prompt input from the effective catalog. Include canonical names
|
||||
only; aliases remain available to deterministic lookup but do not belong in the
|
||||
prompt. Omit levels and class memberships unless evaluation demonstrates that
|
||||
they improve extraction. Instruct the model to return canonical catalog names
|
||||
and to use reference material only for disambiguation, never as source
|
||||
evidence.
|
||||
|
||||
The effective catalog must support configured overlays from the first
|
||||
implementation so non-SRD, sourcebook, and homebrew spells can participate in
|
||||
the same grounding and validation path. Overlay composition must validate
|
||||
canonical names, aliases, duplicates, conflicts, and provenance
|
||||
deterministically. The prompt and validator must consume the same resolved
|
||||
catalog; configuration and catalog identity must participate in effective
|
||||
pipeline and checkpoint identity wherever they can change accepted output.
|
||||
|
||||
This design avoids duplicating the catalog in a schema asset and allows
|
||||
deterministic validation to produce a specific diagnostic for an unknown name.
|
||||
|
||||
### Add deterministic catalog validation
|
||||
|
||||
Add a typed deterministic validator for extracted spell names and include it in
|
||||
the production default chain after shape validation. It should use the catalog
|
||||
lookup rules so case, repeated whitespace, and supported apostrophe variants do
|
||||
not cause false rejections.
|
||||
|
||||
The validator should validate only; canonical display-name replacement belongs
|
||||
in the future D&D spell normalizer. It must not silently discard an individual
|
||||
unknown cast from an otherwise valid extraction result.
|
||||
|
||||
The initial target chain is:
|
||||
|
||||
```yaml
|
||||
validators:
|
||||
- generic/valid_json
|
||||
- generic/valid_json_schema
|
||||
- extract/dnd/spells/shape
|
||||
- extract/dnd/spells/catalog
|
||||
- extract/dnd/spells/source_refs
|
||||
- extract/dnd/spells/source_relatedness
|
||||
```
|
||||
|
||||
A name absent from the effective base-plus-overlay catalog should reject the
|
||||
extraction result. Overlay support must not weaken this policy implicitly.
|
||||
|
||||
### Measure ordinary extraction retries
|
||||
|
||||
Configure the spell extraction binding with `retries: 2`. This means one
|
||||
initial extraction attempt plus at most two additional attempts for a module
|
||||
error, validator error, or validator rejection. The retry wraps extraction and
|
||||
the complete extraction validator chain.
|
||||
|
||||
Keep the framework-wide retry default at zero. The two-retry policy is specific
|
||||
to the spell extraction workflow and should not silently apply to deterministic
|
||||
stages or future LLM modules with different cost and failure characteristics.
|
||||
|
||||
The baseline deliberately retains current retry behavior: each retry repeats
|
||||
the same extraction request without the rejected candidate or validator
|
||||
diagnostic. Measure this behavior before introducing corrective prompts so the
|
||||
effects of catalog grounding, deterministic validation, and later repair-aware
|
||||
retry can be distinguished.
|
||||
|
||||
### Baseline evaluation policy
|
||||
|
||||
Exercise the grounded extractor against a small human-reviewed transcript set.
|
||||
Record false positives, false negatives, unknown-name rejections, retry
|
||||
outcomes, acceptance by attempt, and model-call cost. Preserve representative
|
||||
cases that can compare ordinary retry with a future repair-aware strategy.
|
||||
|
||||
Do not add an LLM validator during this milestone. Finite catalog membership is
|
||||
deterministic, and semantic LLM validation needs evidence of a specific failure
|
||||
mode before its extra cost and nondeterminism are justified.
|
||||
|
||||
## Target-State Acceptance Criteria
|
||||
|
||||
- The extraction prompt receives canonical names from the effective catalog
|
||||
without maintaining a second handwritten spell list or schema enum; aliases
|
||||
are excluded from the prompt.
|
||||
- Configured overlays compose deterministically with the embedded catalog, and
|
||||
the extractor and validator consume the same effective catalog.
|
||||
- Every accepted spell name resolves through the effective catalog, including
|
||||
aliases recognized only by deterministic lookup.
|
||||
- Unknown spell names produce a scoped deterministic rejection and can consume
|
||||
the configured extraction retry budget.
|
||||
- Two retries result in no more than three extraction calls for a rejected
|
||||
chunk, excluding any later separately configured LLM validator calls.
|
||||
- Discarded-attempt warnings are not promoted, and an exhausted rejection
|
||||
remains a rejected pipeline outcome under existing runner semantics.
|
||||
- The default production validator chain, maintained examples, and applicable
|
||||
current-behavior documentation are updated alongside implementation.
|
||||
|
||||
## Deferred Retry And Validation Work
|
||||
|
||||
Reassess the following only after the baseline evaluation is available:
|
||||
|
||||
1. Add bounded, field-specific structured validation issues rather than relying
|
||||
only on a reason code and free-form message. Validators should report all
|
||||
related issues they can safely identify in one pass.
|
||||
2. Classify retryable outcomes. Provider failures and actionable validation
|
||||
rejections may retry; cancellation must stop; configuration, schema-loading,
|
||||
and internal invariant failures should fail without spending more LLM calls.
|
||||
3. Add an optional typed repair capability to the extractor contract. The
|
||||
framework should transport the rejected candidate and diagnostics without
|
||||
constructing a domain prompt; each module should own its repair prompt.
|
||||
4. Evaluate a hybrid two-retry policy: first repair the rejected candidate with
|
||||
structured feedback, then use a fresh extraction without the candidate if
|
||||
repair is still rejected. Compare it with repeated repair and the ordinary
|
||||
retry baseline.
|
||||
5. Consider a narrowly scoped LLM validator only if human review demonstrates
|
||||
semantic failures that deterministic checks cannot resolve, such as
|
||||
distinguishing a true cast from discussion, intent, table chatter, or an
|
||||
effect inferred from general D&D knowledge.
|
||||
6. Implement catalog-aware name canonicalization in the D&D normalizer after
|
||||
catalog-validator behavior is stable; this is separate from retry repair.
|
||||
7. Define the measurements and thresholds that would justify feedback-aware
|
||||
repair or semantic LLM validation. The immediate baseline records evidence
|
||||
but does not need to establish those gates.
|
||||
|
||||
Repair responses should remain complete extraction replacements rather than
|
||||
patches. Diagnostics must be treated as bounded data, particularly if a future
|
||||
LLM-backed validator can contribute their text.
|
||||
|
||||
## Resolved Scope Decisions
|
||||
|
||||
- Catalog overlays are part of the immediate implementation, not a later
|
||||
extension.
|
||||
- Prompt grounding includes canonical spell names only. Aliases participate
|
||||
only in deterministic lookup and validation.
|
||||
- Measurements and decision thresholds for feedback-aware repair and semantic
|
||||
LLM validation are deferred future work and do not block the baseline.
|
||||
136
docs/roadmap/dnd-spell-normalization.md
Normal file
136
docs/roadmap/dnd-spell-normalization.md
Normal file
@@ -0,0 +1,136 @@
|
||||
# D&D Spell Normalization
|
||||
|
||||
## Status
|
||||
|
||||
Proposed as the next D&D pipeline milestone. This feature completes the first
|
||||
domain-specific normalize stage for the spell pipeline before work begins on
|
||||
NPC and combat-turn artifacts.
|
||||
|
||||
## Objective
|
||||
|
||||
Replace the spell lane's no-op normalizer with a deterministic D&D normalizer
|
||||
that emits canonical catalog names, removes only clearly identical duplicate
|
||||
casts, preserves source provenance, and makes every mutation visible through
|
||||
scoped warnings.
|
||||
|
||||
The milestone should improve the consistency of durable spell output without
|
||||
adding another LLM call or introducing fuzzy repair policy.
|
||||
|
||||
## Target Behavior
|
||||
|
||||
### Effective catalog construction
|
||||
|
||||
The normalizer constructs the same immutable SRD-plus-overlay effective
|
||||
catalog used by spell extraction and catalog validation. It declares the
|
||||
optional `spell_catalog` normalize-stage reference slot and resolves it during
|
||||
pipeline preparation, before source parsing or module execution.
|
||||
|
||||
An overlay-capable pipeline binds the same catalog file independently at the
|
||||
extract and normalize stages. This repetition is intentional: references are
|
||||
stage-local, and the normalizer must not reach into a constructed extractor or
|
||||
depend on extractor-private state.
|
||||
|
||||
The normalizer contributes its effective catalog digest through
|
||||
`pipeline.CheckpointFingerprintProvider`. Changing the embedded catalog,
|
||||
composition policy, or normalize-stage overlay therefore invalidates reusable
|
||||
normalize checkpoints.
|
||||
|
||||
### Canonical spell names
|
||||
|
||||
For every spell cast, look up the extracted name using the effective catalog's
|
||||
existing case, whitespace, apostrophe, and alias rules. Replace a recognized
|
||||
value with its canonical display name. This is the only spell-name repair in
|
||||
the initial feature.
|
||||
|
||||
Do not use edit distance, phonetic matching, model judgment, or another fuzzy
|
||||
heuristic. If a value does not resolve, retain it unchanged and emit a scoped
|
||||
warning; the configured normalize validator chain remains responsible for
|
||||
acceptance or rejection.
|
||||
|
||||
Emit a warning for each changed spell name. Diagnostics should identify the
|
||||
artifact index and the original and canonical values without modifying other
|
||||
fields.
|
||||
|
||||
### Source-reference normalization
|
||||
|
||||
Sort each cast's source references by source identity, start unit, and end
|
||||
unit, then remove exact duplicate references. Do not merge adjacent or merely
|
||||
overlapping ranges, because doing so could broaden the evidence attributed to
|
||||
an event.
|
||||
|
||||
The normalizer must not synthesize source references, alter source-unit
|
||||
boundaries, or use auxiliary references as evidence.
|
||||
|
||||
### Conservative duplicate collapse
|
||||
|
||||
After name and source-reference canonicalization, treat two casts as the same
|
||||
event only when all of the following match:
|
||||
|
||||
- canonical spell name;
|
||||
- caster after case folding and whitespace normalization; and
|
||||
- the complete canonical source-reference set.
|
||||
|
||||
Collapse each such group into its first occurrence, preserving stable pipeline
|
||||
order. Retain the first cast's caster, effect, and narrative description. Do
|
||||
not combine prose fields or select a winner based on length, confidence, or
|
||||
model-like semantic judgment. The retained cast receives the group's already
|
||||
canonical source-reference set.
|
||||
|
||||
Emit one scoped warning per collapsed group, including the retained index and
|
||||
the removed indices. Casts with different evidence remain distinct even when
|
||||
their spell and caster match. In particular, adjacency at a chunk or scene
|
||||
boundary is not sufficient evidence of duplication.
|
||||
|
||||
### Production composition and validation
|
||||
|
||||
Register the typed spell normalizer in the D&D family and make it the default
|
||||
normalizer for the maintained production spell pipeline. Keep the artifact
|
||||
kind and durable spell-list schema unchanged.
|
||||
|
||||
Add a normalize-stage production validator chain using the existing generic
|
||||
JSON and JSON Schema validators followed by the existing spell shape, catalog,
|
||||
source-reference, and source-relatedness validators in their current order.
|
||||
Explicit validator overrides remain authoritative.
|
||||
|
||||
The maintained overlay-capable example should bind `spell_catalog` at both the
|
||||
extract and normalize stages. The base-only example should continue to work
|
||||
without a catalog reference.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- Base and overlay aliases are written using canonical catalog display names.
|
||||
- Unknown names are never guessed, deleted, or silently changed.
|
||||
- Source references are deterministically sorted and exactly deduplicated.
|
||||
- Only casts with matching canonical spell, normalized caster, and identical
|
||||
evidence sets collapse; near or adjacent events remain separate.
|
||||
- Normalization preserves the first cast's non-name fields and stable order.
|
||||
- Every name change and duplicate collapse emits a scoped warning.
|
||||
- Extract, normalize, and catalog-validation catalog identities agree for the
|
||||
same bound references, and catalog changes invalidate normalize checkpoints.
|
||||
- The maintained production pipeline and current-behavior documentation use
|
||||
the D&D normalizer without changing the durable artifact schema.
|
||||
|
||||
## Evaluation
|
||||
|
||||
Maintain a small human-reviewed set of representative spell outputs covering
|
||||
canonical names, aliases, repeated casts, duplicate model output, and scene
|
||||
boundaries. Use it to review normalization behavior and warnings, not as a
|
||||
claim that LLM extraction is deterministically correct.
|
||||
|
||||
Record cases where likely duplicates remain because their evidence differs.
|
||||
Those examples should inform later LLM-assisted deduplication work rather than
|
||||
causing this deterministic milestone to adopt broader heuristics.
|
||||
|
||||
## Deferred Work
|
||||
|
||||
- Fuzzy correction of unknown spell names.
|
||||
- Collapsing casts based only on overlapping, adjacent, or semantically similar
|
||||
evidence.
|
||||
- LLM-assisted duplicate proposals or prose reconciliation.
|
||||
- A generic deduplication contract based on stable artifact-element IDs.
|
||||
- Repair-aware extraction retries or LLM-backed semantic validation.
|
||||
- Changes to the spell artifact schema, including stable cast IDs.
|
||||
|
||||
An LLM-backed normalizer is explicitly outside this milestone. If later
|
||||
evaluation justifies one, shared spell-catalog prompt material should be
|
||||
designed at that time around the actual normalization or repair request.
|
||||
@@ -9,15 +9,12 @@ not as committed release dates.
|
||||
|
||||
### Solidify Spell Extraction
|
||||
|
||||
- Evaluate the implemented baseline in
|
||||
[D&D Spell Extraction Quality](dnd-spell-extraction.md), then reconsider its
|
||||
[deferred retry and validation work](dnd-spell-extraction.md#deferred-retry-and-validation-work).
|
||||
- Replace the no-op spell normalizer with a D&D-specific implementation that
|
||||
canonicalizes recognized spell names, performs only high-confidence repairs,
|
||||
retains uncertain values for review, and emits scoped warnings.
|
||||
- Conservatively collapse duplicate spell casts when canonical spell, caster,
|
||||
and source evidence establish that they represent the same event. Merge and
|
||||
canonicalize their source references rather than relying on spell name alone.
|
||||
- Implement the deterministic catalog-aware normalizer defined in
|
||||
[D&D Spell Normalization](dnd-spell-normalization.md), including conservative
|
||||
exact-evidence duplicate collapse.
|
||||
- Evaluate ordinary extraction retries and the completed normalization path
|
||||
against a human-reviewed transcript set before adding repair-aware retries or
|
||||
an LLM-backed semantic validator.
|
||||
- Maintain a small set of human-reviewed transcripts and outputs for prompt,
|
||||
validator, and normalizer development. Treat model-quality review as an
|
||||
iterative human evaluation aid, not a deterministic correctness gate.
|
||||
|
||||
@@ -1,458 +0,0 @@
|
||||
# D&D Spell Extraction Quality Implementation Plan
|
||||
|
||||
## Status
|
||||
|
||||
The accepted baseline implementation is complete through the assembled
|
||||
offline workflow. External quality evaluation remains pending as recorded in
|
||||
[D&D Spell Extraction Quality](dnd-spell-extraction.md). This plan covers that
|
||||
baseline, not its deferred repair-aware retry or semantic LLM-validation work.
|
||||
|
||||
This active plan replaces the completed test-suite implementation record that
|
||||
previously occupied this filename. That review remains documented in
|
||||
[Test Suite Policy Review](tests.md) and repository history; its stages are not
|
||||
instructions for this feature.
|
||||
|
||||
## Objective
|
||||
|
||||
Implement an overlay-capable effective spell catalog, ground every
|
||||
`dnd/spells` extraction request with its canonical names, reject extracted
|
||||
names that do not resolve through that catalog, and establish the existing
|
||||
same-request behavior with `retries: 2` as the measured baseline.
|
||||
|
||||
The completed feature must fail malformed catalog configuration before source
|
||||
parsing or any LLM call, keep aliases out of the prompt, preserve the current
|
||||
private LLM response schema, and use one effective catalog policy for both the
|
||||
extractor and validator.
|
||||
|
||||
## Governing Documents
|
||||
|
||||
Before changing code, read and follow:
|
||||
|
||||
- [D&D Spell Extraction Quality](dnd-spell-extraction.md) for accepted product
|
||||
intent, target state, and deferred work;
|
||||
- [Architecture](../policy/architecture.md) for dependency direction,
|
||||
preparation, domain ownership, validation, reference provenance, and LLM
|
||||
boundaries;
|
||||
- [Documentation Policy](../policy/documentation.md) for canonical document
|
||||
ownership and current-versus-future behavior;
|
||||
- [Testing Policy](../policy/testing.md) for behavioral test ownership and
|
||||
offline/default-suite requirements;
|
||||
- [Pipeline Internals](../internal/pipeline.md), [Module Internals](../internal/modules.md),
|
||||
and [LLM Runtime](../internal/llm.md) for the implementation boundaries being
|
||||
changed; and
|
||||
- [Configuration](../config.md) and the existing D&D integration contracts for
|
||||
current user-visible contracts.
|
||||
|
||||
Preserve unrelated worktree changes. Do not implement anything listed under
|
||||
the feature roadmap's deferred retry and validation section.
|
||||
|
||||
## Fixed Design Decisions
|
||||
|
||||
### Configuration and overlay transport
|
||||
|
||||
Add one optional extract-reference slot named `spell_catalog` to `dnd/spells`.
|
||||
It accepts one UTF-8 `application/json` file with a maximum size of 1 MiB. Use
|
||||
the normal reference binding and materialization path, so configuration-relative
|
||||
paths, CLI overrides, redaction, provenance, and checkpoint reference digests
|
||||
continue to work without a D&D-specific filesystem reader.
|
||||
|
||||
The slot binds an overlay bundle rather than one catalog. One file can therefore
|
||||
carry multiple sourcebook, campaign, or homebrew catalogs without adding list
|
||||
values to the general configuration reference schema. The canonical lane-local
|
||||
form is:
|
||||
|
||||
```yaml
|
||||
artifacts:
|
||||
spells:
|
||||
extract:
|
||||
module: dnd/spells
|
||||
retries: 2
|
||||
references:
|
||||
spell_catalog: ./campaign-spells.json
|
||||
```
|
||||
|
||||
The existing pipeline-level reference-default behavior may also bind the slot.
|
||||
Do not add module options, environment variables, or a new CLI flag for catalog
|
||||
overlays.
|
||||
|
||||
### Overlay bundle contract
|
||||
|
||||
Define and document this strict JSON shape as version 1:
|
||||
|
||||
```json
|
||||
{
|
||||
"schema_version": "notarius.dnd.spell-catalog-overlay.v1",
|
||||
"catalogs": [
|
||||
{
|
||||
"id": "campaign.example",
|
||||
"ruleset": "dnd-5e-2014",
|
||||
"source": {
|
||||
"title": "Example campaign spells",
|
||||
"version": "1",
|
||||
"url": "",
|
||||
"license": ""
|
||||
},
|
||||
"spells": [
|
||||
{
|
||||
"name": "Aegis of Emberfall",
|
||||
"aliases": ["Emberfall Aegis"]
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
```
|
||||
|
||||
Use `encoding/json` with unknown-field rejection and reject trailing JSON
|
||||
values. Require `schema_version` and `catalogs` at the bundle level;
|
||||
`id`, `ruleset`, `source`, and `spells` on each catalog; `title` on each source;
|
||||
and `name` on each spell. Require the exact schema version, at least one
|
||||
catalog, unique trimmed nonempty catalog IDs, the `dnd-5e-2014` ruleset, a
|
||||
trimmed nonempty source title, at least one spell per catalog, and trimmed
|
||||
nonempty canonical names. `source.version`, `source.url`, and `source.license`
|
||||
are optional strings and may be empty so private homebrew material is not
|
||||
forced to claim a publication or license. `aliases` may be omitted or supplied
|
||||
as an array, but it must not be JSON `null`, and every supplied alias must be
|
||||
trimmed and nonempty. The integration contract must state that catalog content
|
||||
is contextual domain data and never source evidence for a cast.
|
||||
|
||||
Overlay spells intentionally carry names and aliases only. Do not invent
|
||||
unknown spell levels or class memberships, and do not weaken the richer
|
||||
embedded SRD catalog contract to accommodate overlay-only recognition data.
|
||||
|
||||
### Effective catalog composition
|
||||
|
||||
Keep the existing embedded SRD catalog and introduce a separate immutable
|
||||
effective recognition-catalog type in `internal/modules/dnd/spells/catalog`.
|
||||
It must expose defensive-copy access to globally sorted canonical names,
|
||||
normalized lookup returning the canonical display name, the ruleset, overlay
|
||||
IDs, and a deterministic semantic digest.
|
||||
|
||||
Compose the effective catalog using these rules:
|
||||
|
||||
1. Start with every canonical SRD name and its aliases.
|
||||
2. Sort overlay catalogs by ID before composition; file order must not choose a
|
||||
winner.
|
||||
3. Normalize lookup keys using the package's existing case, whitespace, and
|
||||
apostrophe rules.
|
||||
4. A new canonical key adds a spell. An overlay canonical key equal to an
|
||||
existing canonical key augments that same spell; retain the established
|
||||
canonical display spelling and merge aliases.
|
||||
5. If equal normalized canonical keys have different display spellings, reject
|
||||
the bundle instead of selecting one by order.
|
||||
6. Repeated aliases mapping to the same canonical spell are idempotent.
|
||||
Canonical-name or alias collisions mapping to different spells are errors,
|
||||
including collisions with the embedded catalog.
|
||||
7. Return canonical names and aliases in deterministic sorted order regardless
|
||||
of input ordering. Do not expose mutable backing slices or maps.
|
||||
|
||||
Compute the effective-catalog digest from a canonical semantic representation
|
||||
that includes the base catalog identity, ruleset, sorted overlay identity and
|
||||
source metadata, and sorted effective canonical-name/alias mappings. Formatting
|
||||
or object-key order alone must not change this semantic digest. Raw reference
|
||||
digests continue to protect checkpoint identity when overlay file bytes change.
|
||||
|
||||
### Construction-time reference availability
|
||||
|
||||
Extend `pipeline.BuildRequest` with a defensively cloned
|
||||
`contracts.ReferenceSet`. `pipeline.Prepare` must provide the materialized
|
||||
references belonging to each constructed stage implementation and to every
|
||||
validator in that stage's chain:
|
||||
|
||||
- chunk references to the chunker and chunk validators;
|
||||
- lane extract references to the extractor and extract validators;
|
||||
- lane merge references to the merger and merge validators; and
|
||||
- lane normalize references to the normalizer and normalize validators.
|
||||
|
||||
Input and output receive an empty set because they cannot declare references.
|
||||
Keep runtime request references unchanged. Construction-time references exist
|
||||
so immutable modules can validate and retain derived data before source parsing;
|
||||
runtime references remain available for run operations and validator context.
|
||||
Do not put D&D catalog types into framework contracts or shared module
|
||||
dependencies.
|
||||
|
||||
Both the spell extractor and catalog validator must construct their immutable
|
||||
effective catalog from the same `spell_catalog` reference bytes. Centralize
|
||||
overlay selection and catalog composition in the D&D catalog package so the two
|
||||
implementations cannot drift. Reject more than one materialized item in the
|
||||
slot even though current configuration produces only one.
|
||||
|
||||
### Prompt grounding
|
||||
|
||||
Add one required structured-completion input named `spell_catalog` with media
|
||||
type `application/json`. Its generated content is exactly a small object whose
|
||||
`spell_names` value is the globally sorted array of effective canonical names.
|
||||
Do not include aliases, levels, classes, source metadata, raw overlay JSON, or
|
||||
spell descriptions.
|
||||
|
||||
Add a package-owned prompt fragment that presents this input as the allowed
|
||||
spell-name catalog. Update task instructions to require canonical catalog names
|
||||
and prohibit returning names absent from the catalog. Keep campaign references
|
||||
separate and retain the rule that references cannot establish that a cast
|
||||
occurred. Do not add an enum to `dnd_spells_llm.v1.json` and do not change the
|
||||
durable `dnd/spell-list` artifact schema.
|
||||
|
||||
Use the digest of the generated prompt-input bytes as its input digest. Extend
|
||||
extractor manifest metadata with the effective catalog semantic digest, base
|
||||
catalog ID, and sorted overlay IDs; do not expose catalog content or local file
|
||||
content in manifests or debug summaries. Existing reference provenance owns
|
||||
overlay origin URI, media type, byte size, and raw digest.
|
||||
|
||||
### Deterministic validation
|
||||
|
||||
Add typed validator key `extract/dnd/spells/catalog` under
|
||||
`internal/modules/dnd/validate/spells/catalog`. Construct it from the effective
|
||||
catalog at preparation time and give it no module options or LLM profile.
|
||||
|
||||
If spell shape is invalid, approve without catalog diagnostics so the shape
|
||||
validator remains the owner of shape rejection. Otherwise, check every
|
||||
nonempty spell name through normalized effective-catalog lookup. Approve
|
||||
canonical names and aliases without mutating the artifact. If any names are
|
||||
unknown, reject the complete extraction result with stable reason code
|
||||
`unknown_spell` and a bounded message identifying all affected spell indices
|
||||
and names. Sort diagnostics by artifact index and cap displayed issues at 20,
|
||||
reporting the number omitted. Do not silently delete casts, convert aliases to
|
||||
canonical names, or emit future structured validation issues in this feature.
|
||||
|
||||
Register the validator immediately after `extract/dnd/spells/shape` in the
|
||||
production default chain. Explicit validator overrides remain authoritative.
|
||||
|
||||
### Retry baseline
|
||||
|
||||
Make no runner, retry, rejection, or validation-result contract changes. The
|
||||
maintained production example continues to set `extract.retries: 2`, meaning
|
||||
one initial extraction plus at most two identical-request attempts around the
|
||||
complete validator chain. Rejections exhausted after three attempts remain
|
||||
nonfatal rejected outputs under current runner semantics.
|
||||
|
||||
Do not add repair prompts, previous candidates, rejection feedback, structured
|
||||
validation issues, error retryability classification, or an LLM validator.
|
||||
|
||||
## Stage 0 — Baseline and scope control
|
||||
|
||||
1. Read the governing documents and inspect the current worktree. Record and
|
||||
preserve changes not owned by this plan.
|
||||
2. Confirm that the embedded catalog loads 319 spells and 779 class
|
||||
memberships and that the maintained production example already contains
|
||||
`extract.retries: 2`.
|
||||
3. Run the baseline commands:
|
||||
|
||||
```sh
|
||||
go test ./...
|
||||
go vet ./...
|
||||
go build ./cmd/notarius
|
||||
```
|
||||
|
||||
4. If the baseline fails for an unrelated reason, stop and report it. Do not
|
||||
modify unrelated behavior or tests to obtain a clean baseline.
|
||||
|
||||
Stage 0 is complete when the baseline and preserved worktree scope are known.
|
||||
|
||||
## Stage 1 — Inject materialized references during preparation
|
||||
|
||||
1. Add `References contracts.ReferenceSet` to `pipeline.BuildRequest` and clone
|
||||
it in `cloneBuildRequest`.
|
||||
2. Refactor `pipeline.Prepare`, `prepareLane`, and `prepareValidatorChain` so
|
||||
each builder receives the correct materialized reference target described in
|
||||
the fixed decisions. Do not change operation-request delivery.
|
||||
3. Update builder/preparation tests to prove:
|
||||
- construction receives the correct target-specific reference content;
|
||||
- extractor and extract validators receive independent clones of the same
|
||||
logical set;
|
||||
- mutation by one builder cannot affect another builder, the resolved
|
||||
pipeline, or runtime requests; and
|
||||
- empty/unmaterialized reference targets remain valid for callers that use
|
||||
`Prepare` directly.
|
||||
4. Update [Pipeline Internals](../internal/pipeline.md) and
|
||||
[Module Internals](../internal/modules.md) to describe construction-time
|
||||
reference delivery as current behavior once the code lands.
|
||||
5. Run focused framework tests plus all repository-wide validation commands.
|
||||
|
||||
Do not change configuration syntax, reference multiplicity, reference
|
||||
materialization, manifests, or checkpoint formats in this stage.
|
||||
|
||||
Stage 1 is complete when all stage builders receive isolated target references
|
||||
at preparation and existing pipelines behave unchanged.
|
||||
|
||||
## Stage 2 — Implement overlay parsing and effective catalog composition
|
||||
|
||||
1. Add package-owned overlay DTOs, strict decoding, semantic validation, and
|
||||
immutable effective-catalog composition under
|
||||
`internal/modules/dnd/spells/catalog`. Keep overlay DTOs private; expose only
|
||||
the minimal effective-catalog API needed by extraction and validation.
|
||||
2. Preserve `LoadSRD5E2014`, existing SRD metadata, and the existing `Spell`
|
||||
API. Do not represent overlay-only entries as incomplete SRD `Spell` values.
|
||||
3. Add a helper that resolves the optional `spell_catalog` slot from a cloned
|
||||
`ReferenceSet`, enforces zero-or-one JSON item, and returns the base-only or
|
||||
base-plus-overlay effective catalog. Keep all filesystem access in framework
|
||||
reference materialization.
|
||||
4. Add focused behavioral tests for:
|
||||
- base-only canonical names, normalized lookup, and immutability;
|
||||
- a new overlay spell and an alias accepted only by lookup;
|
||||
- augmentation of an existing canonical spell;
|
||||
- deterministic results and semantic digest under reordered JSON catalogs,
|
||||
spells, aliases, and object keys;
|
||||
- duplicate IDs, wrong ruleset/schema version, empty required values,
|
||||
unknown fields, trailing JSON, and empty bundles;
|
||||
- same-key display conflicts and every cross-spell canonical/alias collision
|
||||
category; and
|
||||
- rejection of multiple items or a non-JSON item in the reference slot.
|
||||
5. Add the external overlay format contract under `docs/integrations/` and
|
||||
update the internal catalog inventory only after the behavior exists.
|
||||
6. Run the catalog package tests, import-boundary tests, and repository-wide
|
||||
validation commands.
|
||||
|
||||
Prefer table-driven coverage for meaningful invalid-input categories, but do
|
||||
not duplicate every parser case across higher layers.
|
||||
|
||||
Stage 2 is complete when one immutable effective catalog deterministically
|
||||
represents the embedded base plus a valid overlay bundle and rejects ambiguous
|
||||
composition before execution.
|
||||
|
||||
## Stage 3 — Ground the spell extractor
|
||||
|
||||
1. Declare the optional `spell_catalog` reference slot on both the extractor
|
||||
runtime and `ModuleSpec`, accepting only `application/json`, `Multiple:
|
||||
false`, and `MaxBytes: 1048576`. Keep the existing campaign-reference slots
|
||||
unchanged.
|
||||
2. Update the extractor builder and constructor to resolve and retain the
|
||||
effective catalog at preparation. A malformed overlay must make `Prepare`
|
||||
fail before raw input is read or the LLM is called.
|
||||
3. Generate the canonical-name-only JSON prompt material from the retained
|
||||
catalog and add it to every structured completion request. Extend the
|
||||
Scriptorium prompt declaration and package-owned prompt assets accordingly.
|
||||
4. Update prompt wording to distinguish the allowed spell-name catalog from
|
||||
campaign references and source evidence. Leave both response schemas
|
||||
unchanged.
|
||||
5. Add effective catalog identity to extractor manifest metadata without
|
||||
exposing names, aliases, or raw overlay content. Include the new prompt
|
||||
fragment in prompt hashing.
|
||||
6. Update focused tests to protect:
|
||||
- base-only requests contain all and only sorted canonical names;
|
||||
- aliases and non-name metadata do not appear in prompt input;
|
||||
- overlay canonical names do appear;
|
||||
- prompt/schema preparation succeeds offline with the required input;
|
||||
- prompt and manifest diagnostics omit source, reference, overlay, alias,
|
||||
and catalog-name content; and
|
||||
- malformed overlays fail construction without invoking the fake LLM.
|
||||
7. Update the configuration reference-slot catalog, internal module docs, LLM
|
||||
internals where needed, and the external overlay contract in the same stage.
|
||||
8. Add a safe maintained overlay example file and bind it from the production
|
||||
D&D example. Keep the minimal example base-only. Ensure example tests load
|
||||
and materialize the referenced file, rather than checking YAML syntax alone.
|
||||
9. Run focused extractor, prompt-asset, CLI example, and integration tests plus
|
||||
all repository-wide validation commands.
|
||||
|
||||
Stage 3 is complete when every spell extraction is grounded by the immutable
|
||||
effective catalog and all overlay failures occur before paid work.
|
||||
|
||||
## Stage 4 — Add catalog validation and production policy
|
||||
|
||||
1. Implement and register `extract/dnd/spells/catalog` as specified above,
|
||||
reusing the same catalog resolver as the extractor.
|
||||
2. Insert it immediately after the shape validator in
|
||||
`internal/modules/dnd/register`. Do not reorder or otherwise change the
|
||||
remaining default validators.
|
||||
3. Add package-level tests for canonical names, lookup normalization, aliases,
|
||||
overlay spells, multiple unknown casts, the 20-issue message bound, shape
|
||||
deferral, immutability, strict empty options, execution class, and
|
||||
registration.
|
||||
4. Update production composition tests to protect registration, exact default
|
||||
chain placement, and construction from the same materialized overlay.
|
||||
5. Add one assembled offline retry test using the production D&D registration
|
||||
and a fake structured LLM, with two subcases: one remains unknown through
|
||||
exhaustion, and one becomes catalog-valid on a retry. Configure a test-local
|
||||
retry count and assert calls are bounded by `retries + 1`, exhausted
|
||||
rejection is nonfatal, no rejected attempt advances to merge, and only the
|
||||
accepted attempt contributes warnings and output. Do not repeat generic
|
||||
retry cases already owned by framework tests.
|
||||
6. Update the implemented-validator catalog in Configuration and the D&D
|
||||
validator inventory in Module Internals.
|
||||
7. Run focused validator, registrar, integration, and CLI tests plus all
|
||||
repository-wide validation commands.
|
||||
|
||||
Stage 4 is complete when no artifact containing a name outside the effective
|
||||
catalog can pass the production spell extraction chain.
|
||||
|
||||
## Stage 5 — Verify provenance, identity, and assembled behavior
|
||||
|
||||
1. Add or extend the narrowest existing CLI/checkpoint contract tests to prove
|
||||
that changing overlay file bytes changes checkpoint identity through
|
||||
reference provenance, while semantically reordered overlay content retains
|
||||
the same extractor-reported effective catalog digest. Do not alter
|
||||
checkpoint schemas or path formats.
|
||||
2. Verify that changing the configured overlay binding changes resolved
|
||||
pipeline identity, and that manifest reference provenance records the
|
||||
overlay origin, media type, size, and raw digest without content.
|
||||
3. Exercise a representative production-composed run with an overlay-only
|
||||
canonical spell and confirm prompt grounding, validation acceptance, output,
|
||||
manifest catalog metadata, and reference provenance agree.
|
||||
4. Verify base-only configuration remains valid and produces no overlay
|
||||
provenance or overlay IDs.
|
||||
5. Run:
|
||||
|
||||
```sh
|
||||
go test ./...
|
||||
go vet ./...
|
||||
go build ./cmd/notarius
|
||||
go test -race ./internal/framework/pipeline ./internal/cli ./internal/modules/dnd/extract/spells ./internal/modules/dnd/validate/spells/catalog ./internal/modules/integration
|
||||
```
|
||||
|
||||
Stage 5 is complete when the assembled offline workflow protects catalog
|
||||
grounding, validation, provenance, retry count, and checkpoint invalidation at
|
||||
their appropriate test boundaries.
|
||||
|
||||
## Stage 6 — Documentation, evaluation handoff, and completion
|
||||
|
||||
1. Review every affected current-behavior owner and remove roadmap-style
|
||||
wording from implemented contracts:
|
||||
- `docs/config.md` owns the slot name, media type, byte limit, binding form,
|
||||
validator key, and default chain;
|
||||
- the new `docs/integrations/` document owns overlay JSON shape and
|
||||
compatibility rules;
|
||||
- `docs/internal/pipeline.md`, `docs/internal/modules.md`, and
|
||||
`docs/internal/overview.md` own construction and component behavior;
|
||||
- maintained copyable files remain under `examples/`; and
|
||||
- operations documentation changes only if runtime handling actually
|
||||
changes.
|
||||
2. Check all links and ensure examples contain no credentials, private paths,
|
||||
transcripts, or copyrighted spell descriptions.
|
||||
3. If an approved LLM profile and human-reviewed transcript corpus are
|
||||
available, run the base-only and overlay-capable baseline with
|
||||
`retries: 2`. Record anonymized aggregate false positives, false negatives,
|
||||
unknown-name rejections, acceptance by attempt, and model-call cost in the
|
||||
feature roadmap without committing sensitive transcripts or raw prompts.
|
||||
4. If those external evaluation inputs are unavailable, mark evaluation as
|
||||
pending external execution and provide the operator with the exact config
|
||||
and command used by the maintained example. Do not fabricate quality
|
||||
results, invoke a paid model without authorization, or make live evaluation
|
||||
part of the default Go test suite.
|
||||
5. Update [D&D Spell Extraction Quality](dnd-spell-extraction.md) status to
|
||||
distinguish completed implementation from pending or completed evaluation.
|
||||
Remove completed baseline items from [Future Work](future.md), retaining a
|
||||
concise link for deferred repair-aware retry and LLM-validator work.
|
||||
6. Run `git diff --check` and the full Stage 5 validation set once more after
|
||||
documentation and example changes.
|
||||
|
||||
Stage 6 is complete when current behavior is documented in its canonical
|
||||
owners, the feature roadmap accurately records implementation/evaluation
|
||||
status, deferred work remains unimplemented, and all required checks pass.
|
||||
|
||||
## Explicit Non-Goals
|
||||
|
||||
- Changing the framework-wide retry default or retry orchestration.
|
||||
- Supplying rejected candidates or validation feedback to retry attempts.
|
||||
- Structured validation-issue contracts or retryability classification.
|
||||
- An LLM-backed spell validator.
|
||||
- Spell-name mutation or catalog-aware normalization.
|
||||
- Multiple reference bindings per generic slot or list-valued reference
|
||||
configuration.
|
||||
- Spell descriptions, mechanics, unknown overlay levels, or class metadata.
|
||||
- A configuration-version bump, environment override, checkpoint migration,
|
||||
or checkpoint-format change.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None. The overlay transport, file contract, composition policy, preparation
|
||||
boundary, prompt content, validator behavior, retry baseline, provenance, and
|
||||
test ownership are fixed by this plan.
|
||||
@@ -1,461 +0,0 @@
|
||||
# Test Suite Policy Review
|
||||
|
||||
## Context
|
||||
|
||||
Notarius now has a canonical [Testing Policy](../policy/testing.md). The policy
|
||||
defines a risk-based approach that favors durable behavioral coverage while
|
||||
removing redundant, brittle, misleading, or obsolete tests whose lifetime cost
|
||||
exceeds their protective value.
|
||||
|
||||
Much of the existing test suite predates that policy. Recent CLI and
|
||||
configuration work has already applied several of its principles, but the suite
|
||||
has not been reviewed consistently as a whole. This roadmap calls for that
|
||||
review before additional domain capabilities materially expand the number of
|
||||
tests and fixtures.
|
||||
|
||||
The review is not based on a presumption that old tests are bad, that direct
|
||||
tests of package-private behavior must be removed, or that the suite should be
|
||||
made smaller at any cost. Existing tests should be retained when they provide
|
||||
durable and nonredundant protection for a meaningful risk. The goal is a leaner
|
||||
and clearer allocation of test ownership together with credible protection for
|
||||
important behavior.
|
||||
|
||||
## Objective
|
||||
|
||||
Review the complete Notarius test suite against the Testing Policy, identify
|
||||
both low-value coverage and meaningful protection gaps, and define the
|
||||
justified target revisions. The ordered execution plan is maintained separately
|
||||
in [Implementation](implementation.md).
|
||||
|
||||
The review must answer:
|
||||
|
||||
- Which important contracts, invariants, failure modes, and integration
|
||||
boundaries does the current suite protect?
|
||||
- Where do multiple tests protect the same behavior without providing distinct
|
||||
failure isolation or integration confidence?
|
||||
- Which tests are coupled to incidental implementation details, duplicated
|
||||
policy literals, closed-world inventories, unstable formatting, or mock
|
||||
choreography?
|
||||
- Which important risks can still fail silently despite the existing suite?
|
||||
- Which tests should be retained, consolidated, rewritten, or deleted, and
|
||||
which new tests are warranted?
|
||||
|
||||
## Review Scope
|
||||
|
||||
Review all committed Go tests and their supporting fixtures. Organize the work
|
||||
by behavioral layer rather than treating test count or package coverage as the
|
||||
unit of quality.
|
||||
|
||||
### CLI and configuration
|
||||
|
||||
Review command parsing, configuration decoding and precedence, validation,
|
||||
effective configuration, run controls, reference selection, output/cache/debug
|
||||
state, production composition, and maintained examples.
|
||||
|
||||
Pay particular attention to:
|
||||
|
||||
- duplicated assertions across parser, resolver, CLI, and assembled-run tests;
|
||||
- tests that restate complete defaults or registry contents rather than
|
||||
protecting operator-visible behavior;
|
||||
- exact error or output assertions broader than the documented CLI contract;
|
||||
- fixture mutation that can silently stop establishing a test precondition;
|
||||
and
|
||||
- whether representative CLI workflows provide sufficient assembled coverage.
|
||||
|
||||
### Framework and durable state
|
||||
|
||||
Review pipeline preparation and execution, artifact contracts, validation and
|
||||
retry behavior, LLM scheduling and transport seams, chunk-plan storage,
|
||||
checkpoint compatibility, debug bundles, and file persistence.
|
||||
|
||||
Presume durable protection is important for data integrity, serialization,
|
||||
compatibility, cancellation, concurrency, cache correctness, resume behavior,
|
||||
atomic or failure-safe persistence, and recovery. Look for opportunities to
|
||||
replace many narrow structural tests with a smaller invariant, round-trip, or
|
||||
behavior-level test only when protection is not weakened.
|
||||
|
||||
### Modules and domain behavior
|
||||
|
||||
Review generic, seriatim, and D&D module tests, including codecs, chunking,
|
||||
extraction, merging, normalization, validators, registration, prompt assets,
|
||||
and module integration.
|
||||
|
||||
Confirm that domain rules and artifact schemas have clear test ownership.
|
||||
Identify tests that merely reproduce schemas, prompt asset inventories, or
|
||||
implementation structure, while preserving tests that protect compatibility,
|
||||
source provenance, normalization, validation, or other consequential domain
|
||||
invariants.
|
||||
|
||||
### Cross-cutting suite quality
|
||||
|
||||
Across all packages, evaluate:
|
||||
|
||||
- deterministic, offline, credential-free execution;
|
||||
- isolation from mutable machine and process-global state;
|
||||
- appropriate use of real collaborators, fakes, stubs, and mocks;
|
||||
- stable behavioral assertions and useful failure diagnostics;
|
||||
- golden files and large snapshots;
|
||||
- helper and fixture complexity;
|
||||
- test runtime, race safety, repetition stability, and parallel-execution
|
||||
assumptions; and
|
||||
- semantic ownership and duplication across package, integration, and
|
||||
end-to-end layers.
|
||||
|
||||
Use coverage only as a diagnostic to locate unexpectedly untested critical
|
||||
branches. Do not recommend tests solely to increase a percentage or make
|
||||
coverage uniform across packages.
|
||||
|
||||
## Review Method
|
||||
|
||||
1. Read the Testing Policy and the canonical documentation for each subsystem
|
||||
before judging its tests.
|
||||
2. Establish a clean baseline with the repository validation commands, focused
|
||||
race tests where concurrency or shared state is relevant, and a coverage
|
||||
report used only for investigation.
|
||||
3. Inventory tests and map each meaningful test or closely related group to the
|
||||
contract, invariant, integration boundary, or regression it protects.
|
||||
4. Inspect production code only as needed to understand the protected behavior,
|
||||
identify the stable boundary, and detect untested risk. Do not infer a
|
||||
contract merely from current implementation detail.
|
||||
5. Evaluate marginal value across layers. Similar assertions are not redundant
|
||||
when one owns a package contract and another distinctly proves production
|
||||
wiring or end-to-end integration.
|
||||
6. Record evidence for every proposed change. Name the affected test or fixture,
|
||||
the realistic defect it currently catches or fails to catch, and why the
|
||||
recommendation improves confidence or reduces unnecessary friction.
|
||||
7. Check historical context when a test appears unusually specific. Preserve a
|
||||
regression test when the underlying defect remains plausible and
|
||||
consequential, even if its purpose is not obvious from the current code.
|
||||
|
||||
Do not modify production code or tests during the review. If the review reveals
|
||||
incorrect production behavior, report it separately from test-suite
|
||||
harmonization rather than treating a changed test expectation as the fix.
|
||||
|
||||
## Review Deliverable
|
||||
|
||||
Produce an evidence-backed report organized by priority and subsystem. Each
|
||||
finding must classify the proposed disposition as one of:
|
||||
|
||||
- **retain:** valuable protection at an appropriate boundary;
|
||||
- **consolidate:** overlapping protection that can be represented more simply;
|
||||
- **rewrite:** meaningful protection expressed through a brittle or misleading
|
||||
boundary;
|
||||
- **delete:** no sufficient plausible defect or distinct protection justifies
|
||||
the lifetime cost; or
|
||||
- **add:** a consequential risk lacks credible protection.
|
||||
|
||||
For consolidate, rewrite, delete, and add findings, describe the protected risk,
|
||||
current evidence, recommended boundary, and expected effect on confidence and
|
||||
maintenance. Do not produce a raw list of every test when a package or related
|
||||
group shares one clear disposition.
|
||||
|
||||
Distinguish required changes from optional cleanup. Absence of a finding is not
|
||||
evidence that a package needs more tests.
|
||||
|
||||
## Completion Criteria
|
||||
|
||||
The review is complete when:
|
||||
|
||||
- the full suite has been considered at an appropriate behavioral grouping;
|
||||
- important test ownership and integration boundaries are mapped;
|
||||
- every recommended change is supported by a concrete risk and evidence;
|
||||
- high-risk behavior without credible protection is identified;
|
||||
- redundant or brittle protection is distinguished from valuable intentional
|
||||
overlap;
|
||||
- production defects, if any, are reported separately; and
|
||||
- every target revision is specific enough to support a decision-complete
|
||||
implementation roadmap.
|
||||
|
||||
## Revisions Needed
|
||||
|
||||
### Review baseline
|
||||
|
||||
The review was completed on 2026-07-18 against 90 committed Go test files,
|
||||
536 named tests, approximately 21,900 lines of test code, and the committed
|
||||
fixtures under `testdata/` and `examples/`.
|
||||
|
||||
The baseline is clean:
|
||||
|
||||
- `go test ./...`, `go vet ./...`, and `go build ./cmd/notarius` pass;
|
||||
- `go test -race` passes for `internal/framework/pipeline`,
|
||||
`internal/framework/llm`, `internal/cli`, and
|
||||
`internal/modules/integration`;
|
||||
- five shuffled repetitions of the full suite pass; and
|
||||
- the default suite is offline, credential-free, and fast. The only HTTP
|
||||
behavior uses local test servers or deliberately unreachable loopback
|
||||
endpoints without making provider calls.
|
||||
|
||||
Coverage was used only to investigate risk. It confirms broad behavioral
|
||||
coverage in CLI, configuration, source, pipeline, and production module
|
||||
packages. It also exposes the material checkpoint gap described below: most
|
||||
filesystem loader paths and all source, merge, and normalize checkpoint
|
||||
round trips are unexercised. No production defect was identified during this
|
||||
review.
|
||||
|
||||
### Test ownership that should be retained
|
||||
|
||||
The following overlap is intentional and should remain:
|
||||
|
||||
- CLI command, run, cache, reference, state-hardening, production-composition,
|
||||
and maintained-example tests own exit classification, option mapping,
|
||||
physical state placement, default production wiring, and representative
|
||||
assembled workflows. Package tests continue to own the underlying parsing,
|
||||
resolution, and persistence rules.
|
||||
- Configuration contract tests own file decoding, precedence, validation,
|
||||
redaction, and effective resolution. Pipeline resolver tests own module,
|
||||
capability, typed-variant, validator-chain, and reference resolution after
|
||||
configuration has produced a profile.
|
||||
- Pipeline runner tests own bounded concurrency, deterministic ordering,
|
||||
cancellation, retries, rejection propagation, checkpoint decisions, debug
|
||||
recording, candidate/final encoding, and terminal failure behavior. The one
|
||||
cross-family concurrency test remains valuable because it proves that the
|
||||
framework worker bound and shared provider scheduler remain independent in
|
||||
an assembled production-style pipeline.
|
||||
- Source, chunk-plan store, file I/O, debug-bundle, output encoder, artifact
|
||||
codec, and manifest tests own data integrity, confinement, serialization,
|
||||
compatibility, atomic publication, redaction, and round trips. The exact
|
||||
checkpoint schema identifier and exact artifact-schema digest tests are
|
||||
compatibility tests, not incidental constant assertions, and must remain.
|
||||
- Seriatim adapter, D&D scene chunker and spell extractor, D&D validators,
|
||||
generic chunker and validators, prompt preparation, and durable spell codec
|
||||
tests own their domain rules and external or durable schemas. Leaf
|
||||
constructor/spec/registration tests remain justified because the extension
|
||||
contract explicitly requires each leaf to be independently buildable.
|
||||
- The centralized production import-boundary test remains a valuable
|
||||
executable architecture rule. Its synthetic rejected fixture is necessary
|
||||
to prove that the checker itself is active.
|
||||
|
||||
Absence from the revisions below means the current behavioral group has an
|
||||
appropriate owner and no justified change was found.
|
||||
|
||||
### Required findings
|
||||
|
||||
#### P0 - add filesystem checkpoint compatibility and recovery coverage
|
||||
|
||||
**Disposition: add.** `internal/framework/checkpoint/recorder_test.go` currently
|
||||
proves only that an empty successful extract checkpoint can be written and
|
||||
loaded. Coverage confirms that `FilesystemLoader.Source`, `Merge`, `Normalize`,
|
||||
manifest validation, payload digest validation, and most recorder status paths
|
||||
are otherwise silent. CLI resume coverage proves root selection and one happy
|
||||
reuse, while pipeline checkpoint tests use controlled collaborators; neither
|
||||
owns the filesystem format.
|
||||
|
||||
Add package-level tests at the real recorder/loader boundary that:
|
||||
|
||||
- prove `NewIdentity` is deterministic under reordered lanes, references, and
|
||||
fingerprints, changes when pipeline, source/input, selected lanes, runtime
|
||||
overrides, references, or provenance change, and produces a confined stable
|
||||
relative path;
|
||||
- round-trip representative source, extract, merge, and normalize payloads,
|
||||
including serialized artifact identity, warnings, extract rejections,
|
||||
metadata, and dependency fingerprints;
|
||||
- prove caller mutation cannot alter recorded or loaded values and retain the
|
||||
existing restrictive-permission expectation through real files; and
|
||||
- mutate one persisted artifact at a time to prove that missing or malformed
|
||||
JSON, old or unknown workspace schema versions, wrong identity/stage/lane/
|
||||
module/status/dependencies, incomplete codec identity, invalid base64, and
|
||||
content/output-digest mismatches yield a non-reused decision with useful
|
||||
category context rather than a panic or silent reuse.
|
||||
|
||||
Use relationships and category fragments rather than snapshotting complete
|
||||
manifests or error sentences. This addition protects cache correctness,
|
||||
compatibility, recovery, and sensitive durable state; it is not intended to
|
||||
raise a coverage percentage.
|
||||
|
||||
#### P1 - remove tests that exercise only their own fakes or obsolete APIs
|
||||
|
||||
**Disposition: delete.** Remove the fake-behavior tests in
|
||||
`internal/framework/contracts/contracts_test.go`:
|
||||
`TestFakeExtractorReturnsTypedOutput`, `TestFakeChunkerReturnsSourcePlan`,
|
||||
`TestFakeChunkerReceivesPerRunContext`,
|
||||
`TestFakeExtractorReceivesChunkAndAmbientContext`,
|
||||
`TestFakeMergeNormalizeAndOutputContracts`, and `TestReferenceSetDataTypes`.
|
||||
They detect changes in test helpers or Go struct assignment, not defects in
|
||||
production contracts. Retain the compile-time interface assertions and the
|
||||
clone, JSON omission, and content-ownership tests, which protect type
|
||||
compatibility and non-leakage invariants.
|
||||
|
||||
**Disposition: delete.** Remove
|
||||
`internal/framework/llm/scriptorium_api_test.go`. It was introduced as
|
||||
pre-adapter API grounding and still mirrors unused Scriptorium request/result
|
||||
fields, option constructors, and sentinel errors. Production compilation now
|
||||
grounds the API actually used, while `scriptorium_client_test.go` and module
|
||||
prompt-preparation tests protect adapter mapping, local integration,
|
||||
cancellation, validation, profile selection, and credential redaction.
|
||||
|
||||
**Disposition: delete.** Remove
|
||||
`TestHelpersReturnValidationResults` from
|
||||
`internal/framework/validate/validate_test.go`; it is a compile-only assignment
|
||||
that adds no protection beyond the function signatures and the two behavioral
|
||||
helper tests.
|
||||
|
||||
**Disposition: delete.** Remove the direct
|
||||
`TestTypedNormalizerPreservesValue` test for the generic no-op normalizer. Its
|
||||
single assignment is already exercised through resolver, runner, production
|
||||
composition, and maintained-example output tests. Retain the direct append-order
|
||||
merger test because input ordering is a meaningful transformation invariant.
|
||||
|
||||
#### P1 - remove misleading D&D integration claims
|
||||
|
||||
**Disposition: delete.** In
|
||||
`internal/modules/integration/dnd_spells_runner_test.go`, delete:
|
||||
|
||||
- `TestRunnerDoesNotExtractSpellMentionedOnlyInPartyReference`, whose fake LLM
|
||||
is programmed to return no spells and therefore cannot prove the claimed
|
||||
extraction policy;
|
||||
- `TestRunnerCarriesDNDSpellCastWithInvalidSourceRefToSerializedOutput`, which
|
||||
omits the production validator chain and duplicates the extractor's explicit
|
||||
invalid-evidence handoff test; and
|
||||
- `TestRunnerRejectsMalformedDNDSpellsArtifactAtSerializationBoundary`, whose
|
||||
final-codec invariant is already owned by the codec and runner candidate/
|
||||
final-encoding tests.
|
||||
|
||||
Retain the representative Seriatim-to-spell assembled workflow and the test
|
||||
that proves party/glossary references and provenance cross the production
|
||||
module boundary. The policy that reference material is not source evidence is
|
||||
durably protected by extractor mapping, prompt-input assertions, source-ref
|
||||
validators, and the production validator-chain composition test; it cannot be
|
||||
proved by controlling an LLM stub's semantic answer.
|
||||
|
||||
#### P1 - consolidate superseded configuration and CLI state tests
|
||||
|
||||
**Disposition: consolidate.** Delete `internal/core/config/v3_test.go` after
|
||||
moving its two distinct protections into the current contract owners:
|
||||
|
||||
- add the version-2 migration rejection case to the strict file-decoding cases
|
||||
in `file_config_contract_test.go`; and
|
||||
- ensure the positive per-user chunk-plan/checkpoint root separation remains
|
||||
in `env_contract_test.go`.
|
||||
|
||||
The remaining default, precedence, invalid-source, redaction, and removed-field
|
||||
assertions are already more completely owned by `file_config_contract_test.go`,
|
||||
`env_contract_test.go`, `validation_contract_test.go`, and
|
||||
`redaction_test.go`.
|
||||
|
||||
**Disposition: consolidate.** Delete `internal/cli/state_surfaces_test.go`.
|
||||
Move the shared `emptyLookup` helper to
|
||||
`contract_test_helpers_test.go`. Its debug flag syntax, allocation timing,
|
||||
no-debug absence, version-3 validation, and removed-field cases are already
|
||||
owned by `run_contract_test.go`, `state_hardening_test.go`,
|
||||
`command_contract_test.go`, and the configuration contract tests. Preserve the
|
||||
state matrix, failure retention, no-debug terminal-writer, and pre-resolution
|
||||
debug allocation protections in those current owners.
|
||||
|
||||
**Disposition: rewrite.** Keep
|
||||
`TestDefaultCLICompositionResolvesMaintainedConfigurations` as a narrow default
|
||||
composition smoke test, but run one representative config-validation command
|
||||
through empty `Options` rather than revalidating every maintained example.
|
||||
`example_contract_test.go` owns both maintained examples. Remove the repeated
|
||||
maintained-example loops from `TestProductionCatalogCoversMaintainedConfigurations`
|
||||
and `TestProductionConfigValidationCoversModuleAndVariantFailures`; retain the
|
||||
production registry/codec/default-chain checks and the distinct production
|
||||
failure cases.
|
||||
|
||||
#### P1 - consolidate resolver and module-composition duplication
|
||||
|
||||
**Disposition: delete.** Remove
|
||||
`internal/framework/pipeline/default_modules_test.go`. Its large local catalog
|
||||
only re-proves default binding selection, already owned by
|
||||
`profile_test.go` and `effective_config_contract_test.go`; production default
|
||||
keys and wiring are separately exercised by leaf registration, the production
|
||||
catalog, default CLI composition, and the maintained end-to-end example.
|
||||
|
||||
**Disposition: delete.** Remove
|
||||
`internal/modules/seriatim/input/transcript/config_test.go`. It builds an
|
||||
entire fake catalog to repeat generic resolver success, deterministic digest,
|
||||
missing-capability, and unknown-lane behavior. Retain Seriatim parsing and leaf
|
||||
registration tests, the Seriatim runner integration, D&D cross-family
|
||||
capability tests, resolver contract tests, and production CLI examples.
|
||||
|
||||
**Disposition: consolidate.** In
|
||||
`internal/modules/integration/dnd_spells_config_test.go`, retain only the two
|
||||
cross-family capability contracts: Seriatim must provide `source.transcript`
|
||||
to the spell extractor, and the spell extractor must provide
|
||||
`dnd.spell_casts` to append-order. Express them as one compact table over a
|
||||
programmatic profile/catalog. Remove the generic successful-resolution,
|
||||
stable-digest, and unknown-lane cases, which are already exercised by the
|
||||
retained runner integration and resolver/configuration owners.
|
||||
|
||||
#### P1 - replace brittle assertions with behavioral ones
|
||||
|
||||
**Disposition: rewrite.** In `internal/core/source/source_test.go`, make
|
||||
`ValidateRef` the owner of malformed reference categories. `ValidateDocument`
|
||||
should retain one propagation/context case plus the document-only self-reference
|
||||
invariant instead of repeating missing, foreign, and reversed-reference cases.
|
||||
Replace complete internal error-sentence equality with the smallest stable
|
||||
category and field fragments. Retain exact acceptance/rejection boundaries,
|
||||
duplicate detection, deterministic digests, and reference ordering.
|
||||
|
||||
**Disposition: rewrite.** Replace the decoded-schema-structure walk in
|
||||
`internal/modules/dnd/chunk/scenes/schema_test.go` with representative JSON
|
||||
Schema validation: accept a valid source-unit-boundary response and reject old
|
||||
segment fields, non-positive bounds, invalid enums, empty caveats, and unknown
|
||||
properties. Likewise, change the private spell response-schema assertion in
|
||||
`internal/modules/dnd/extract/spells/schema_test.go` to accept the LLM transport
|
||||
shape without `source_id` and reject a response that supplies canonical
|
||||
`source_id`. Retain schema identity/hash, mutation safety, diagnostics
|
||||
non-leakage, DTO decoding, and the codec's durable fixture tests. These rewrites
|
||||
preserve schema regression protection without coupling tests to nested map
|
||||
layout or unsafe type assertions.
|
||||
|
||||
**Disposition: rewrite.** In `internal/framework/llm/schema_registry_test.go`,
|
||||
assert that returned keys are sorted and required framework test schemas are
|
||||
present without asserting that the registry contains exactly two entries.
|
||||
Retain the explicit negative D&D lookup because it protects the framework/
|
||||
domain ownership boundary.
|
||||
|
||||
#### P2 - make composition and architecture tests open to legitimate extension
|
||||
|
||||
**Disposition: rewrite.** The family registrar tests in
|
||||
`internal/modules/generic/register`, `internal/modules/seriatim/register`, and
|
||||
`internal/modules/dnd/register` currently assert closed-world key and asset
|
||||
inventories that duplicate the CLI production catalog. Change them to require
|
||||
the family-owned registrations they need, permit unrelated future additions,
|
||||
and prove representative entries can be built or prepared. Keep exact order
|
||||
for the D&D default spell validator chain because that order is documented
|
||||
production policy. Keep missing-dependency-before-mutation and contextual
|
||||
duplicate-registration cases.
|
||||
|
||||
**Disposition: consolidate.** Move the source/framework independence rules
|
||||
from `internal/framework/chunkplan/import_boundaries_test.go` into the
|
||||
centralized `internal/modules/import_boundaries_test.go` checker and delete the
|
||||
second repository walker. Preserve both unique rules: `internal/core/source`
|
||||
may import neither framework nor modules, and `internal/framework/chunkplan`
|
||||
may not import modules. Add rule-level cases so a broken checker still fails.
|
||||
|
||||
#### Documentation alignment
|
||||
|
||||
**Disposition: rewrite.** When the affected tests move or are deleted, update
|
||||
the `Tests To Inspect` sections in `docs/internal/state.md`,
|
||||
`docs/internal/pipeline.md`, `docs/internal/modules.md`, and
|
||||
`docs/internal/llm.md`. They currently name files such as
|
||||
`internal/cli/state_surfaces_test.go` and `internal/cli/run_test.go` that will
|
||||
be deleted or do not exist. Point each document at the retained contract,
|
||||
state-hardening, production-composition, example, checkpoint, and integration
|
||||
owners without recreating an exhaustive test inventory.
|
||||
|
||||
### Optional cleanup
|
||||
|
||||
No additional cleanup is recommended now. In particular, do not mechanically
|
||||
convert the large resolver and runner suites to table-driven form, merge all
|
||||
leaf registration tests into family registrars, add tests for trivial accessor
|
||||
coverage, or introduce a golden-update framework. Those changes do not provide
|
||||
enough additional confidence to justify their immediate cost.
|
||||
|
||||
### Target state
|
||||
|
||||
The revision is complete when:
|
||||
|
||||
- every checkpoint stage has a real filesystem round trip and incompatible or
|
||||
corrupted state is demonstrably recomputed rather than silently reused;
|
||||
- all protection named as retained above remains present at its stated owner;
|
||||
- the fake-only, obsolete, misleading, duplicate, and closed-world assertions
|
||||
named in the required findings are removed or rewritten exactly as specified;
|
||||
- maintained examples are each owned by one example contract plus one narrow
|
||||
default-composition smoke path, rather than repeated across production tests;
|
||||
- default tests remain deterministic, offline, credential-free, order
|
||||
independent, and race-clean;
|
||||
- failures identify the violated behavioral category without snapshotting
|
||||
complete incidental diagnostics;
|
||||
- internal documentation points to existing retained test owners; and
|
||||
- the suite reaches this state without changing production behavior.
|
||||
Reference in New Issue
Block a user