Add implementation roadmap to clean up D&D extract module validation issues

This commit is contained in:
2026-07-22 09:19:13 -05:00
parent 906d97b391
commit 23c55f8925
2 changed files with 129 additions and 693 deletions

View File

@@ -1,399 +1,174 @@
# Implementation Plan: Ordered Pipeline Follow-Up
# D&D Validation-Boundary Alignment Implementation Plan
## Status
Status: Proposed.
Completed on 2026-07-22. This document retains the implementation sequence for
historical context; current contracts are maintained in the canonical CLI,
configuration, operations, integration, and internal documentation linked from
the [development guide](../development.md).
Implement this plan in order. It repairs the immediate combat extraction
failure, makes D&D validation diagnostics consistently domain-owned, and then
aligns the spell and NPC private response boundaries with the same policy.
## Purpose
Do not change durable artifact schemas, artifact kinds, public Go types,
framework retry behavior, checkpoint formats, or the scene chunker. Existing
v1 private-schema identities may be corrected in place because Notarius has not
been run in production. Changed prompt and schema content will invalidate
development checkpoints through existing fingerprints.
Record the work that closed the correctness, observability, test, and
maintainability gaps in the implemented
[Ordered Pipeline Steps](ordered-pipeline-steps.md) feature. That feature
roadmap remains the authority for product intent, policy choices, acceptance
criteria, and exclusions. This document preserves the ordered,
decision-complete implementation sequence for the follow-up work.
## Cross-Stage Decisions
## Background
Private LLM schemas own the transport envelope: required fields, JSON types,
nullability, array/object shape, and unknown-field rejection. Deterministic
domain validators own semantic rules: supported enum values, nonblank values,
required non-empty collections, positive and resolvable source units,
catalog/identity policy, and normalized invariants.
The original implementation is complete in broad architecture. Pipelines now
resolve and prepare one canonical ordered-step model, execute hard barriers
between steps, hand accepted normalized artifacts to later operations as
canonical generated references, include generated identity in checkpoint and
output provenance, support `--recompute-step`, and use generated NPC output at
operation time in the D&D spell and combat consumers. Current documentation and
maintained examples describe that model.
For typed D&D artifacts, `generic/valid_json` remains first as a representation
sanity check. All deterministic domain validators that can reject a candidate
run next. `generic/valid_json_schema` runs after them as a durable-schema
backstop, followed by warning-only relatedness validators. This order gives
expected candidate failures bounded domain reason codes while retaining a final
check that typed encoding conforms to the durable contract.
The completed follow-up addressed these narrower gaps:
Do not expose raw JSON Schema errors or candidate values through the generic
validator. Do not add tests that require particular words or phrases to remain
in prompt prose.
- selective recomputation now hydrates an unselected predecessor from its
accepted normalized artifact without requiring its extract and merge state;
- the failed checkpoint decision is recorded before a required-predecessor
error returns;
- checkpoint reason codes are assigned explicitly rather than inferred from
human-readable prose;
- runner lane and checkpoint orchestration has explicit responsibility seams;
and
- CLI and resumed-producer acceptance coverage exercises the complete recovery
contract.
No new product feature is introduced by this plan. Preserve configuration file
version 3 and checkpoint workspace schema `notarius.workspace.v3`; the fixes do
not require a new persistent format.
## Instructions For Every Stage
Before changing code, read `docs/development.md`, all documents under
`docs/policy/`, and the task-specific internal documents named there. Use the
repository's code knowledge graph for code discovery before falling back to
text search.
Implement the stages in order. Each stage must leave the repository formatted,
building, and passing its focused tests. Preserve these invariants throughout:
- The pipeline retains one input, chunk plan, output encoder, run identity,
checkpoint identity, failure boundary, worker budget, and provider scheduler.
- Every selected module and validator is constructed before source parsing.
- Steps schedule fixed extract, merge, and normalize lanes; this work must not
introduce module-to-module calls or a general DAG scheduler.
- Generated artifacts remain cloned operation-time context, never source
evidence. Never emit their content, source material, credentials, or local
paths in decisions, manifests, logs, or summaries.
- Public ordering remains step order, lane ID, and source/chunk order as
applicable. Refactoring must not expose completion order.
- Tests must follow `docs/policy/testing.md`: protect observable recovery,
orchestration, and CLI contracts without asserting private helper calls,
exact prose, goroutine choreography, or full-document snapshots.
- Update current-behavior documentation in the same stage that changes the
corresponding behavior. Keep detailed implementation sequencing only here.
## Target Checkpoint Semantics
Use these definitions consistently in all stages:
- A **stage checkpoint** is internal resumable state for extract, merge, or
normalize. Ordinary resume may continue to reuse this state progressively.
- An **accepted lane artifact** is the one successful normalized artifact for a
`(step ID, lane ID, normalizer module)` under the current checkpoint identity.
It is the dependency exposed to a later step.
- An unselected required predecessor satisfies selective recomputation when its
accepted lane artifact can be validated and hydrated. Its extract and merge
stage checkpoints are not prerequisites for that handoff.
- A selected lane and its transitive dependents execute. They must never use
accepted-artifact hydration to bypass forced execution.
- If a required predecessor's accepted lane artifact is missing, rejected,
corrupt, non-canonical, or incompatible, fail the run before any dependent
lane starts. Do not implicitly rerun that predecessor.
- Ordinary resume without `--recompute-step` keeps its existing progressive
stage-reuse and cold-miss behavior.
## Stage 1: Decompose Runner And Checkpoint Orchestration
### Objective
Create explicit, testable ownership seams for ordered-step coordination,
per-lane execution, and per-stage checkpoint handling without changing
observable behavior.
## Stage 1: Repair Combat Extraction
### Changes
1. Keep `Runner.Run` as the pipeline-wide coordinator. Extract a small
step-coordination helper responsible only for iterating prepared steps,
building generated reference sets at each barrier, invoking the lane engine,
and merging deterministic outcomes.
2. Split `runLanes` in `internal/framework/pipeline/runner_concurrent.go` into
helpers with these responsibilities:
- initialize lane state and resolve extract checkpoint state in lane order;
- run the bounded extract worker/continuation engine;
- collect terminal lane results and choose failures deterministically; and
- merge lane-local output into step output.
Preserve the existing bounded channels, cancellation, drain behavior,
chunk-first dispatch, lane ordering, and step barrier.
3. Split `continueTypedLane` in
`internal/framework/pipeline/runner_typed.go` into stage-specific merge and
normalize helpers. Each helper should own dependency construction, checkpoint
loading and canonical validation, execution/retry/validation when needed,
checkpoint recording, debug envelopes, and its typed result. Use small
result structs rather than long parallel return lists.
4. Centralize repeated checkpoint-decision flow in one pipeline helper that can
apply forced-execution policy, validate canonical stored artifacts, record
the final observable decision, and return a contextual error. Do not yet
change categories, reason codes, or required-predecessor semantics; Stages 2
and 3 will change those deliberately.
5. Keep stage-specific code where the data shapes genuinely differ. Do not
introduce reflection, a generic stage state machine, or callbacks that hide
the fixed extract/merge/normalize lifecycle.
- Update the combat extraction instructions to enumerate the complete allowed
values:
- `turn_kind`: `turn`, `reaction`, `legendary_action`, `lair_action`,
`other`;
- action `category`: `attack`, `spell`, `movement`, `item`,
`ability_check`, `saving_throw`, `condition`, `other`.
- Keep the private combat schema structurally permissive and the durable schema
strict. Do not restore enum, minimum, `minLength`, or `minItems` constraints
to the private schema.
- Reorder the combat extraction default chain to:
`generic/valid_json`, combat shape, combat source references,
`generic/valid_json_schema`, combat source relatedness.
- Reorder the combat normalization default chain to:
`generic/valid_json`, combat shape, normalized invariants, combat source
references, `generic/valid_json_schema`, combat source relatedness.
- Preserve configured validator overrides as authoritative; only production
default composition changes.
### Tests
- Existing runner, checkpoint, barrier, ordering, cancellation, retry, debug,
and D&D integration tests must pass without expectation changes except moves
required by renamed private test fixtures.
- Add no tests for helper boundaries or collaborator call counts. Add a narrow
regression assertion only if the refactor exposes an observable behavior not
already protected.
- Add an assembled combat pipeline case whose raw LLM response contains an
unsupported turn kind or action category. Exhausted retries must produce a
non-fatal `invalid_combat_turn_shape` rejection owned by the combat shape
validator, not `json_schema_invalid` or a framework error.
- Retain coverage that a later valid retry succeeds and discarded-attempt
warnings/rejections do not become durable.
- Update registrar contract tests to assert the new extraction and
normalization order.
- Rely on behavioral enum/schema tests and prompt fingerprint coverage; do not
add prompt-word change-detector tests.
### Completion Gate
### Completion Check
Run `go test ./internal/framework/pipeline ./internal/framework/checkpoint
./internal/modules/integration` and the pipeline race tests. Review the diff to
confirm this stage changes structure only: serialized output, checkpoint state,
decision values, failure selection, and module invocation behavior must remain
unchanged.
Run `go test ./internal/modules/dnd/extract/combatturns
./internal/modules/dnd/validate/combatturns/... ./internal/modules/dnd/register
./internal/modules/integration` and `git diff --check`.
## Stage 2: Make Checkpoint Decisions Typed And Observable
### Objective
Assign stable decision categories and reason codes explicitly, and retain the
decision that causes a required-dependency failure.
## Stage 2: Make D&D Validator Ordering Consistent
### Changes
1. In `internal/framework/pipeline/checkpoint.go`, introduce string-backed
internal types and constants for checkpoint decision categories and reason
codes. Keep the current JSON strings and public artifact fields compatible.
Categories remain exactly `executed`, `reused`, `forced_recompute`, and
`dependency_invalidated`.
2. Replace prose inspection in `internal/framework/checkpoint/loader.go` with an
explicit decision constructor accepting category, reason code, and optional
detail. Remove every `strings.Contains` classification branch. Assign a code
at the validation site using this bounded vocabulary:
- `loading_disabled`, `checkpoint_missing`, `checkpoint_path_invalid`,
`checkpoint_read_failed`, and `checkpoint_decode_failed`;
- `workspace_schema_incompatible` and `identity_mismatch`;
- `stage_mismatch`, `step_mismatch`, `lane_mismatch`, `module_mismatch`, and
`status_not_reusable`;
- `dependency_mismatch` for the `dependency_invalidated` category;
- `artifact_payload_invalid`, `artifact_digest_mismatch`,
`artifact_codec_incompatible`, and `artifact_not_canonical`; and
- `checkpoint_reused` and `accepted_artifact_reused` for successful reuse,
and `recompute_step` for forced execution.
Retain an existing code not listed here only when a current external or
documented contract already depends on it.
3. Keep detail human-readable, UTF-8, bounded, and sanitized through one helper.
Detail may identify stage, step, lane, module, expected status, or schema
version, but must not include artifact/reference content, source content,
credentials, environment values, or local paths. Tests must assert codes and
safety properties, not exact detail prose.
4. Change the centralized runner decision flow from Stage 1 so the final loader
or canonical-validation decision is recorded before returning a
required-predecessor error. The contextual error must identify the step and
lane and include the stable reason code; it must not interpolate unsafe
loader detail.
5. Propagate typed values without lossy conversion through checkpoint events,
run manifests, debug summaries, and CLI diagnostics. Convert to strings only
at existing serialized boundaries unless changing an internal field type is
simpler and wire-compatible.
6. Update the canonical current-behavior owners in the same change:
`docs/internal/state.md` owns the internal decision flow, while
`docs/operations.md` owns operator diagnosis and any operator-visible reason
code table. Link rather than duplicate the table elsewhere.
- Move `generic/valid_json_schema` behind all rejecting domain validators in
every spell and NPC extraction and normalization default chain:
- spell extraction/normalization: shape, catalog, source references, schema,
then source relatedness;
- NPC extraction: shape, source references, schema, then source relatedness;
- NPC normalization: shape, identity, source references, schema, then source
relatedness.
- Keep `generic/valid_json` first and warning-only source relatedness last.
- Do not change validator implementations, reason codes, warning promotion,
retry counts, or user-provided chain order.
- Document the default-chain policy in the pipeline/module internals: domain
validators diagnose expected semantic failures and the generic schema
validator is the final rejecting representation backstop.
### Tests
- Add a table in `internal/framework/checkpoint` that exercises one
representative input per reason-code family and asserts category, code,
bounded valid UTF-8 detail, and absence of supplied secret/path sentinels.
- Add pipeline behavior tests proving a required-predecessor failure records the
underlying missing, corrupt, incompatible, or dependency-invalidated
decision before returning.
- Retain focused manifest/debug serialization assertions for category and code;
do not add exact-detail or full-manifest snapshots.
- Update production registrar tests for every affected chain.
- Add one representative spell and NPC assembled rejection proving that a
domain-invalid but encodable candidate is attributed to the owning domain
validator rather than the generic schema validator. Do not duplicate each
validator package's existing case matrix at integration level.
- Confirm explicitly configured validator chains retain their exact configured
order.
### Completion Gate
### Completion Check
Run `go test ./internal/framework/checkpoint ./internal/framework/pipeline
./internal/core/artifacts ./internal/core/debugbundle ./internal/cli`. Search the
production checkpoint package to confirm no decision category or reason code is
derived from diagnostic prose.
Run `go test ./internal/modules/dnd/register ./internal/modules/integration
./internal/framework/pipeline` and `git diff --check`.
## Stage 3: Hydrate Required Predecessors From Accepted Normalized Artifacts
### Objective
Make selective recomputation enforce the lane-level accepted-artifact contract:
a valid normalized producer artifact is sufficient even when its extract or
merge stage cache is unavailable.
## Stage 3: Align Spell and NPC Private Response Boundaries
### Changes
1. Extend the checkpoint loader contract with a dedicated accepted-normalized-
artifact lookup. Implement it in the filesystem loader and every no-op or
test implementation. The lookup receives step ID, lane ID, and normalizer
module key and returns the normalized artifact, its warnings, and an explicit
checkpoint decision. It must not require caller-supplied extract or merge
dependency fingerprints.
2. The filesystem lookup may reuse the existing normalize manifest and payload;
do not add a second persistent copy. It is reusable only when all of the
following hold:
- workspace schema is v3 and the non-empty stored checkpoint identity matches
the current invocation identity;
- stage, step, lane, normalizer module, and successful status match;
- the manifest output digest matches the payload; and
- the payload can subsequently be validated through the registered artifact
codec.
Skipping caller-supplied merge dependencies is safe only because the matched
non-empty checkpoint identity already binds the current input, resolved
topology and configuration, external references, runtime overrides, LLM
profiles, and component semantic fingerprints. Do not weaken or omit that
identity check.
A loader lacking a verifiable current identity must return an unavailable
decision rather than perform accepted-artifact reuse.
3. Add a pipeline hydration helper that decodes the returned serialized
artifact through the producer's registered codec, re-encodes it canonically,
and requires exact artifact kind, schema ID/name/version/digest, media type,
content bytes, and content digest. Return a runner-owned clone with producer
step, lane, module, and source identity. Any mismatch is an explicit bounded
decision and the bytes never reach a consumer.
4. Before normal execution of a lane marked in `RequireReusableLanes`, use the
accepted-artifact lookup:
- on success, mark the lane terminal without invoking extract, merge,
normalize, or their validators;
- append the accepted normalized output in the normal deterministic location
and restore only the warnings stored with that normalized checkpoint;
- record one `reused` normalize decision with a stable accepted-output reuse
reason code; do not synthesize extract/merge decisions, warnings, or
rejections that were not loaded; and
- allow the ordinary step barrier and generated handoff code to consume that
output exactly as it consumes a freshly executed output.
5. On missing, rejected, corrupt, non-canonical, or incompatible accepted state,
record the decision and fail the run before the dependent step begins. Do
not fall back to stage execution. Forced lanes must bypass this hydration
path and execute normally.
6. Leave ordinary resume unchanged when no lane is marked
`RequireReusableLanes`: it may reuse or recompute extract, merge, and
normalize progressively under the existing cold-miss rules.
7. Keep generated-reference fingerprints and provenance unchanged. Hydrating a
byte-identical producer must yield the same canonical handoff digest and
downstream dependency fingerprint as fresh execution.
8. Update `docs/internal/pipeline.md`, `docs/internal/state.md`, and
`docs/operations.md` in this stage to distinguish progressive stage reuse
from accepted normalized-artifact hydration and to document the fail-rather-
than-rerun rule for invalid required predecessors.
- Revise the existing v1 spell and NPC private schemas in place:
- retain required fields, JSON types, array/object structure,
`additionalProperties: false`, and omission of framework-assigned fields;
- remove `minLength`, `minItems`, and positive-number `minimum` constraints;
- leave the durable spell and NPC schemas unchanged.
- Replace `shared.UnitRef` in the private spell and NPC response DTOs with
integer candidates so zero and negative unit IDs survive decoding and mapping
into `source.SourceRef` for deterministic source validation.
- Update canonicalization and ordering helpers to operate on candidate integers
without repairing invalid values. Valid positive IDs retain current output,
ordering, and exact-deduplication behavior; invalid ranges remain available
to validators.
- Keep malformed JSON, missing/unknown fields, wrong JSON types, and
non-integer source IDs as LLM-boundary errors.
- Update LLM/module internals and the spell/NPC integration contracts to state
the structural-private/semantic-validator ownership boundary.
### Tests
- At the pipeline/checkpoint boundary, create a valid producer normalize
checkpoint while omitting or corrupting its extract and merge checkpoints.
Select a later step for recomputation and prove the producer hydrates, no
producer operation or validator runs, and the dependent receives the exact
canonical artifact.
- Cover missing, rejected-status, corrupt, non-canonical, wrong-codec-identity,
and wrong-content-digest normalize state. Assert failure and the recorded
stable decision before any consumer invocation.
- Prove forced producers execute instead of hydrating, while a reusable
predecessor and an unrelated lane remain reusable.
- Prove fresh and hydrated producer outputs create identical generated
provenance and downstream checkpoint fingerprints without exposing content
or paths.
- For each private schema, prove structurally valid candidates with blank
strings, empty required collections, and nonpositive unit IDs pass the
private schema, while missing fields, unknown fields, and wrong JSON types do
not.
- Through raw-JSON LLM fakes, prove semantic values survive decoding and mapping
without repair.
- Add representative assembled cases showing:
- blank or empty spell/NPC fields are rejected by the appropriate shape
validator;
- nonpositive or nonexistent unit IDs are rejected by the appropriate source
validator; and
- exhausted validation retries remain non-fatal rejected outputs.
- Preserve existing valid mapping, source-position ordering, deduplication,
catalog, identity, checkpoint-fingerprint, and durable codec tests.
### Completion Gate
### Completion Check
Run `go test ./internal/framework/checkpoint ./internal/framework/pipeline
./internal/modules/integration` and the corresponding race tests. Manually
inspect one failed test fixture to confirm the accepted artifact remains on
disk but its bytes do not appear in the manifest, decision detail, debug
summary, or error.
Run `go test ./internal/modules/dnd/extract/spells
./internal/modules/dnd/extract/npcs ./internal/modules/dnd/validate/spells/...
./internal/modules/dnd/validate/npcs/... ./internal/modules/integration` and
`git diff --check`.
## Stage 4: Complete Recompute CLI And Recovery Acceptance Coverage
## Final Verification
### Objective
Run:
Exercise the operator-visible recomputation contract through stable boundaries
and close the acceptance-test omissions from the original plan.
### Changes
1. Add one table-driven CLI contract test for `--recompute-step` covering:
- a valid explicit step and the implicit `default` step;
- repeated flags and an empty or unknown step ID;
- use without `--resume`;
- use when checkpoint recording is disabled; and
- combination with `--only`.
Assert exit classification and stable identifying fragments or reason codes,
not complete prose.
2. Add one filesystem-backed CLI execution test using deterministic fake
modules and a three-step generated dependency chain plus one unrelated lane:
- perform a fresh checkpointed run;
- remove or corrupt only the required producer's extract and merge state
inside `t.TempDir()`, leaving its normalize artifact valid;
- resume with the middle step selected;
- assert the selected lane and transitive dependents execute, the predecessor
hydrates without module calls, the unrelated lane reuses, and output and
decision ordering are deterministic; and
- invalidate the producer normalize state in a subcase and assert the command
fails before dependent execution with the bounded decision preserved.
3. Add or extend one generic two-step pipeline test proving handoff succeeds
both from fresh producer execution and from accepted normalized-artifact
hydration. Keep this at the pipeline boundary if the CLI execution test
already proves flag wiring; do not duplicate every CLI case end to end.
4. Review existing recomputation policy tests. Retain the pure closure test
because it protects transitive selection, but remove or consolidate any new
test that merely repeats the CLI or pipeline behavior above.
5. Make only production changes revealed as necessary by these contract tests;
do not add new flag semantics or broaden the feature roadmap.
### Completion Gate
Run `go test ./internal/cli ./internal/framework/pipeline
./internal/framework/checkpoint`, then run the CLI, pipeline, and checkpoint
packages under the race detector. Confirm no test asserts internal loader call
counts, exact decision detail, filesystem layout outside a temp workspace, or
the exact length of any prompt or prefix.
## Stage 5: Current Documentation And Release Verification
### Objective
Reconcile all canonical documentation after the staged changes and verify the
complete feature against policy and roadmap.
### Changes
1. Re-read `docs/internal/pipeline.md`, `docs/internal/state.md`,
`docs/operations.md`, and `docs/cli.md` against the final code. Correct any
stale statements left by Stages 1 through 4 without duplicating their
canonical contracts.
2. Ensure internal documentation describes the coordinator, lane engine, stage
checkpoint flow, and accepted-artifact validation at the responsibility
level without listing volatile private helper names.
3. Confirm the operator-visible reason-code table has one canonical owner and
other documents link to it rather than maintaining parallel copies.
4. Remove stale implementation claims exposed by this work and ensure the
feature roadmap continues to describe target policy rather than task
sequencing.
### Verification
Run the repository-prescribed commands from `docs/development.md`:
```sh
```text
git diff --check
go test ./...
go vet ./...
go build ./cmd/notarius
go test -race ./internal/modules/dnd/... ./internal/framework/pipeline ./internal/cli ./internal/modules/integration
```
Also run race tests for the pipeline, checkpoint, CLI state, D&D NPC registry,
and D&D integration packages. Validate maintained examples/configurations
through the production catalog. Review one fresh run, ordinary resumed run,
forced-recompute run, hydrated-predecessor run, and invalid-predecessor failure
for deterministic ordering, correct decisions, bounded provenance, and absence
of generated content, secrets, or local paths in manifests and diagnostics.
### Completion Gate
The follow-up is complete only when every finding summarized above is protected
by a stable behavioral test, the current documentation matches the corrected
implementation, the full verification suite passes, and the worktree contains
no temporary adapters or TODOs introduced by these stages.
Review current-behavior documentation for stale statements that private spell,
NPC, or combat schemas own semantic validation. Confirm the scene schema and
prompt remain unchanged: their enumerations are explicitly communicated and
scene-plan construction has a distinct structural mapping boundary.
## Open Questions
None. The feature roadmap and this plan fix the required product and
architecture choices. If implementation reveals that accepted normalized
artifacts cannot be validated safely without a persistent format change, stop
and amend this plan rather than weakening identity, codec, or content
validation.
None. The stages above define the validation ownership, default ordering,
compatibility policy, diagnostic behavior, and test boundaries required for
implementation.