From 249e49c928f833efb115ece24c2700a90f46bdfd Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 7 Jul 2026 15:50:00 -0500 Subject: [PATCH] Update planning roadmap and add a staged imnplementation plan for the validator registry --- docs/roadmap/implementation.md | 486 +++++++++++++++++++++++++++++++++ docs/roadmap/validation.md | 121 +++++--- 2 files changed, 568 insertions(+), 39 deletions(-) create mode 100644 docs/roadmap/implementation.md diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md new file mode 100644 index 0000000..94ace03 --- /dev/null +++ b/docs/roadmap/implementation.md @@ -0,0 +1,486 @@ +# Validation Refactor Implementation Plan + +This plan implements the target state described in +[validation.md](validation.md). Follow the stages in order. Do not skip the +focused tests for a stage before moving to the next one. + +## Stage 1: Replace Legacy Validator Contracts With One Raw Output Contract + +Goal: make the raw module-output validation boundary the only framework +validator contract. + +Tasks: + +- In `internal/framework/contracts`, replace the legacy candidate-oriented + validator contract with a module-output contract. +- Remove these legacy types after updating callers: + - `ValidationDecision`; + - candidate-oriented `ValidationRequest`; + - candidate-oriented `ValidationResult`; + - legacy `Validator` behavior that works over `artifacts.ArtifactCandidate`. +- Remove `RawValidator`, `RawValidationRequest`, and `RawValidationResult`. + The new `Validator` contract covers the current raw validation use case + directly. +- Define execution class metadata: + + ```go + type ExecutionClass string + + const ( + ExecutionClassDeterministic ExecutionClass = "deterministic" + ExecutionClassLLMBacked ExecutionClass = "llm_backed" + ) + ``` + +- Define the new request shape around immutable module output. It should carry: + - `Stage` as a string-compatible stage identifier; + - `LaneID`; + - `ModuleKey`; + - `Source` and `SourceID`; + - `SourceInput`; + - `SessionID`; + - `References`; + - `LLMClient`; + - `LLMProfile`; + - `Options`; + - `Metadata`; + - `Schema`; + - `Payload`; + - chunk provenance for extract validation; + - `Chunks` for chunk validation; + - ordered `ExtractOutputs` for merge validation; + - `MergeOutput` for normalize validation. +- Define the result shape as one decision over the whole module output: + + ```go + type ValidationResult struct { + Approved bool + ReasonCode string + Message string + DiagnosticArtifactPath string + Warnings []Warning + } + ``` + +- Define `Validator` as: + + ```go + type Validator interface { + Name() string + ExecutionClass() ExecutionClass + Validate(ctx context.Context, req ValidationRequest) (ValidationResult, error) + } + ``` + +- Update tests under `internal/framework/contracts` so fake validators exercise + the new contract and no longer reference artifact candidates. +- Keep `artifacts.ArtifactCandidate` only if other non-validation code still + needs it. If it becomes unused after later stages, remove it in Stage 6. + +Design requirements: + +- Validators must not mutate request payloads, source documents, chunks, or + upstream outputs. +- Validator execution errors are framework errors. Validator semantic rejection + is represented by `ValidationResult{Approved:false}`. +- Empty validation chains still approve output by default. + +Focused tests: + +```sh +go test ./internal/framework/contracts +go test ./internal/framework/validate +``` + +## Stage 2: Add Validator Specs, Chain Mappings, And Manifest Provenance + +Goal: make validators centrally registered and make default chains explicit, +reviewable, and auditable. + +Tasks: + +- Replace the current `pipeline.ValidatorRegistry` internals so it registers + validator specs, not generic module specs. +- Add a `pipeline.ValidatorSpec` with at least: + - `Key`; + - `ExecutionClass`. +- Keep validator construction lazy through `ValidatorRegistry.Build(key)`. +- Add `ValidatorRegistry.Spec(key)` and sorted registered-spec accessors for + catalog and manifest use. +- Remove `pipeline.RawValidationRegistry` after replacing its callers with the + new chain registry. The new chain registry should be keyed by: + - stage; + - module key. +- Add a production/default mapping type such as: + + ```go + type ValidatorChainMapping struct { + Stage ModuleStage + Module string + Validators []ModuleBinding + } + ``` + +- Add a chain registry with these behaviors: + - duplicate stage/module mappings are rejected; + - unknown stages are rejected; + - empty default chains may be represented by absence of a mapping; + - lookup returns a defensive copy. +- Extend `pipeline.ModuleCatalog` and `pipeline.Registries` to carry the chain + mapping registry alongside module and validator registries. +- Add manifest types under `internal/core/artifacts` for resolved validator + chain provenance. Use a top-level manifest field so chunk, extract, merge, and + normalize chains can all be represented: + + ```go + type ValidatorChainManifest struct { + Stage string `json:"stage"` + LaneID string `json:"lane_id,omitempty"` + ModuleKey string `json:"module_key"` + Validators []ValidatorManifest `json:"validators"` + } + + type ValidatorManifest struct { + Key string `json:"key"` + ExecutionClass string `json:"execution_class"` + } + ``` + +- Remove `ArtifactLaneManifest.Validators` and update affected tests and JSON + output documentation to use top-level validator-chain provenance instead. + +Design requirements: + +- The production chain mapping is central catalog policy, not module-owned + behavior. +- A resolved run manifest must record the exact chain used for each validation + point, including explicit empty chains. + +Focused tests: + +```sh +go test ./internal/framework/pipeline +go test ./internal/core/artifacts +``` + +## Stage 3: Implement Stage-Scoped Config Overrides + +Goal: allow pipeline config to override default validator mappings for each +validatable stage while preserving unset versus explicit empty semantics. + +Tasks: + +- Add stage-local validator override syntax to module bindings for only: + - `chunk`; + - artifact lane `extract`; + - artifact lane `merge`; + - artifact lane `normalize`. +- Do not revive artifact-lane-level `validators` as an active chain. Keep that + field rejected with an error that directs users to + `extract.validators`, `merge.validators`, or `normalize.validators`. +- Update file-config parsing so the implementation can distinguish: + - validators omitted; + - `validators: []`; + - `validators: [ ... ]`. +- Represent this in pipeline profiles with this explicit override type: + + ```go + type ValidatorOverride struct { + Set bool + Validators []ModuleBinding + } + ``` + +- Validate configured validators during config validation: + - validator module key must be non-empty; + - `references` are not supported on validator bindings; + - nested validator overrides inside validator bindings are not supported; + - retries are not supported on validator bindings in this pass; + - explicit `llm_profile` is allowed only for LLM-backed validators and is + rejected for deterministic validators during resolution. +- During pipeline resolution: + - unset override uses the central default chain; + - explicit empty override resolves to an empty chain and approves by default; + - explicit non-empty override resolves exactly the configured validators in + configured order; + - every referenced validator key must exist in the validator registry; + - configured order must not be silently reordered. +- Update `--llm-profile` behavior: + - continue overriding chunk, extract, merge, and normalize module bindings; + - do not override validator bindings unless validator-specific profile + override behavior is explicitly added in a later roadmap. +- Update Scriptorium explicit profile validation so it includes LLM-backed + validators with explicit `llm_profile` values, and ignores deterministic + validators. + +Design requirements: + +- Validator compatibility with a module or stage is not enforced during config + resolution. +- Empty resolved chains are valid and should be recorded in manifest + provenance. +- Config validation must fail before runtime if a non-empty override references + an unknown validator. + +Focused tests: + +```sh +go test ./internal/core/config +go test ./internal/framework/pipeline +go test ./internal/cli +``` + +## Stage 4: Wire Validator Execution Into The Runner + +Goal: run resolved validator chains at chunk, extract, merge, and normalize +validation points. + +Tasks: + +- Replace runner calls to the old raw validation registry with calls to the + resolved validator chain for the current validation point. +- Build validators from `ValidatorRegistry` at runtime in resolved order. +- For each validator call, populate the new `contracts.ValidationRequest` with: + - immutable module output payload; + - stage/module/lane/source/chunk provenance; + - schema metadata and in-memory schema content when available; + - source input material; + - session ID; + - resolved references for the target stage; + - LLM client/profile/options/metadata for the validator; + - upstream outputs needed by merge and normalize validators. +- Preserve current retry behavior: + - semantic rejection returns a `RejectedOutput` and may be retried according + to the module binding's retry setting; + - validator execution errors are framework errors and are retried under the + same module binding retry setting; + - final rejection is recorded and does not pass downstream. +- Preserve current warning behavior: + - warnings from accepted attempts are promoted; + - warnings from discarded retry attempts are not promoted; + - warning-only validators return `Approved:true` with warnings. +- Continue enforcing framework-level chunk invariants in the runner before + chunk validators run. +- Populate manifest validator-chain provenance after resolution and before + output encoding. +- Keep rejected output manifest entries compatible with current `rejected.json` + and `manifest.json` shapes where practical. + +Design requirements: + +- Validators are read-only. Do not let a validator return rewritten payloads or + replacement stage outputs. +- If a validator chain is empty, approve without building validators. +- If a validator is mapped to an unsuitable output shape, the validator should + return a clear execution error or an explicit documented approval. + +Focused tests: + +```sh +go test ./internal/framework/pipeline +go test ./internal/cli +``` + +## Stage 5: Add Generic Validators + +Goal: provide reusable validators needed for incremental module development and +production structural checks. + +Tasks: + +- Add `internal/validators/generic/always_accept`. + - Key: `generic/always_accept`. + - Execution class: deterministic. + - Always returns approved. +- Add `internal/validators/generic/always_reject`. + - Key: `generic/always_reject`. + - Execution class: deterministic. + - Always returns rejected with stable reason code `always_reject`. +- Add `internal/validators/generic/valid_json`. + - Key: `generic/valid_json`. + - Execution class: deterministic. + - Accepts syntactically valid JSON. + - Rejects invalid JSON with stable reason code `invalid_json`. +- Add `internal/validators/generic/valid_json_schema`. + - Key: `generic/valid_json_schema`. + - Execution class: deterministic. + - Uses the module output's in-memory JSON schema content. + - Rejects invalid JSON with `invalid_json`. + - Rejects schema non-conformance with `json_schema_invalid`. + - Returns a validator execution error when no schema content is available. +- Make `github.com/santhosh-tekuri/jsonschema/v6` a direct dependency for + schema validation. +- Extend `contracts.ResponseSchema` with an in-memory-only field: + + ```go + JSONSchema []byte `json:"-"` + ``` + + Module outputs should carry schema bytes to validators without serializing raw + schema content into manifests, diagnostics, or default output files. +- Update D&D scene and D&D spell modules to populate in-memory schema content on + their output schema values. +- Do not add a shared validator test helper package in this pass. Keep tests + local unless a later cleanup finds substantial duplication. + +Design requirements: + +- Generic validators must not depend on concrete production modules. +- Tests must not assert exact embedded production prompt text. +- Raw schema content must not be emitted in diagnostics or manifests. + +Focused tests: + +```sh +go test ./internal/validators/generic/... +go test ./internal/modules/chunk/dnd/scenes +go test ./internal/modules/extract/dnd/spells +go test ./internal/framework/pipeline +``` + +## Stage 6: Migrate D&D Spell Validators + +Goal: move D&D spell validation behavior out of the extractor package and into +raw-output validators. + +Tasks: + +- Add `internal/validators/extract/dnd/spells/shape`. + - Key: `extract/dnd/spells/shape`. + - Execution class: deterministic. + - Parses raw spell JSON and rejects malformed spell-cast payloads or missing + required fields. +- Add `internal/validators/extract/dnd/spells/source_refs`. + - Key: `extract/dnd/spells/source_refs`. + - Execution class: deterministic. + - Parses raw spell JSON and rejects missing or invalid source references. + - Use `internal/core/source.ValidateRef` for source reference validation. +- Add `internal/validators/extract/dnd/spells/source_relatedness`. + - Key: `extract/dnd/spells/source_relatedness`. + - Execution class: deterministic. + - Warning-only validator. + - Approves output and emits `spell_not_near_source` warnings when a spell name + is not found in the cited source text. +- Keep D&D-specific validator parsing local to validator packages or a validator + helper package. Do not import concrete extractor packages from validators. +- Remove `internal/modules/extract/dnd/spells/validator.go` and its + candidate-oriented tests after equivalent validator-package tests exist. +- Remove now-unused legacy candidate validation helpers and artifact candidate + types if they have no remaining callers. + +Design requirements: + +- The spell extractor should remain responsible for prompt/schema/provenance + and raw output production, not accept/reject policy. +- `source_relatedness` must not reject output. It should only warn. +- The default D&D spell chain must preserve the intended order: + `generic/valid_json`, `generic/valid_json_schema`, + `extract/dnd/spells/shape`, `extract/dnd/spells/source_refs`, + `extract/dnd/spells/source_relatedness`. + +Focused tests: + +```sh +go test ./internal/validators/extract/dnd/spells/... +go test ./internal/modules/extract/dnd/spells +go test ./internal/framework/pipeline +``` + +## Stage 7: Register Production Validators And Defaults + +Goal: make production validation policy explicit and active. + +Tasks: + +- Register all generic validators in `internal/cli/catalog.go`. +- Register all D&D spell validators in `internal/cli/catalog.go`. +- Register default production chain mappings centrally near production module + registration. +- Add the default chain for `extract` module `dnd/spells`: + - `generic/valid_json`; + - `generic/valid_json_schema`; + - `extract/dnd/spells/shape`; + - `extract/dnd/spells/source_refs`; + - `extract/dnd/spells/source_relatedness`. +- Do not add default chains for other modules unless the stage has a meaningful + validator for that output shape. +- Update catalog construction tests so available modules, available validators, + and default mappings can be reviewed together. +- Add CLI tests proving: + - default D&D spell validators run; + - explicit empty override disables the default chain; + - explicit non-empty override replaces the default chain; + - configured validator order is preserved; + - unknown configured validator keys fail during validation/resolution; + - deterministic validators reject explicit `llm_profile`; + - LLM-backed validator profile validation checks explicit profile IDs. + +Design requirements: + +- Production defaults must be reviewable without constructing concrete modules. +- Empty default chains approve by default. +- The manifest records resolved chains for default chains, empty overrides, and + explicit override chains. + +Focused tests: + +```sh +go test ./internal/cli +go test ./internal/core/config +go test ./internal/framework/pipeline +``` + +## Stage 8: Documentation, Examples, And Cleanup + +Goal: make implemented validation behavior canonical outside roadmap docs and +remove stale roadmap instructions. + +Tasks: + +- Update `docs/policy/architecture.md` so validation policy reflects central + mappings and validator packages rather than module-owned validator chains. +- Update `docs/config.md` to document stage-local validator overrides: + - unset uses defaults; + - explicit empty disables validators; + - explicit non-empty replaces defaults in configured order. +- Update `docs/cli.md` to list production validators and describe validation + profile behavior. +- Update `docs/internal/pipeline.md` with the implemented validator contract, + chain mapping, retry behavior, and manifest provenance. +- Update `docs/internal/modules.md` to remove stale claims that modules own + validator defaults. +- Update `docs/integrations/json-output.md` for any manifest shape changes. +- Update `docs/troubleshooting.md` with validator-chain debugging guidance. +- Leave maintained example configs unchanged unless a validator override example + is added intentionally with corresponding CLI/config tests. Keep examples + secret-free and loadable. +- Replace `docs/roadmap/implementation.md` with a concise completed note after + all stages are implemented. +- Update `docs/roadmap/validation.md` so it no longer describes completed work + as future work. Keep only deferred validation work, if any remains. + +Focused documentation checks: + +```sh +rg -n "reserved for future configurable validator chains|configured validators are not supported|candidate-oriented|RawValidationRegistry" docs internal +``` + +Expected result after implementation: no stale current-behavior documentation +describes configured validators as rejected, and no production code path depends +on the legacy candidate-oriented validator contract. + +## Full Validation + +Run after every stage that changes shared contracts, and at the end: + +```sh +go test ./... +go vet ./... +go build ./cmd/notarius +``` + +Also run focused packages added by this plan: + +```sh +go test ./internal/validators/... +``` diff --git a/docs/roadmap/validation.md b/docs/roadmap/validation.md index a27b653..f5970bc 100644 --- a/docs/roadmap/validation.md +++ b/docs/roadmap/validation.md @@ -1,17 +1,29 @@ # Validation System Refactor This roadmap defines the target state for making validation a first-class, -composable pipeline concern. Current validation behavior is partly module-owned: -the `dnd/spells` extractor defines built-in validators inside the module package, -and the runner falls back to extractor-provided validators when a lane does not -configure validators. The desired end state is that validator implementations, -validator registration, and default module-to-validator mappings are explicit, -reviewable, and independent of concrete module packages. +composable pipeline concern. + +Current pipeline behavior is raw-output based. The runner can execute +`contracts.RawValidator` chains from `pipeline.RawValidationRegistry` for +`chunk`, `extract`, `merge`, and `normalize` outputs. Empty chains approve by +default, validator rejection records a rejected raw output, and rejected output +does not pass to the next stage. Production currently registers no raw +validators, and non-empty pipeline-configured validator lists are rejected so +they cannot appear in manifests without executing. + +Legacy candidate validator contracts and D&D spell validators still exist under +`internal/modules/extract/dnd/spells`, but they are not part of the current +runner path. The desired end state is that validator implementations, validator +registration, and default module-to-validator mappings are explicit, reviewable, +and independent of concrete module packages. ## Goals - Move artifact and module-output validation behavior out of `internal/modules` and into `internal/validators`. +- Retire or replace the legacy candidate-oriented `contracts.Validator`, + `ValidationRequest`, and `ValidationResult` path after equivalent raw-output + validators exist. - Keep each validator in its own package. - Mirror the stage and domain shape of `internal/modules` where a validator is module-specific. @@ -22,6 +34,8 @@ reviewable, and independent of concrete module packages. - Make default production module-to-validator mappings centralized and human-readable. - Allow pipeline configuration to override default mappings for advanced use. +- Preserve the distinction between an unset validator override and an explicit + empty validator override. - Treat an empty validator set as valid and equivalent to approval. - Preserve the rule that module output passes forward unless a validator rejects it. @@ -32,6 +46,8 @@ reviewable, and independent of concrete module packages. ## Non-Goals - Do not create a general workflow engine or arbitrary validation DAG. +- Do not revive the legacy artifact-candidate validation model as the primary + runner path. - Do not enforce validator compatibility with a module or stage in this pass. - Do not move ordinary runtime invariant checks into validator packages. - Do not require every module to have validators. @@ -43,9 +59,9 @@ reviewable, and independent of concrete module packages. ## Validation Boundary Validation packages should own approve/reject/warning evaluation of successfully -returned module outputs. This means logic that decides whether a chunk result, -raw extract output, raw merge output, raw normalize output, or raw LLM response -should continue through the pipeline belongs in `internal/validators`. +returned module outputs. This means logic that decides whether a chunk result or +raw extract, merge, or normalize payload should continue through the pipeline +belongs in `internal/validators`. The boundary is: @@ -63,6 +79,9 @@ Other validation-like checks should remain with their owning packages: - input parsing and source-format validation stay in input modules; - source document and source reference invariants stay in `internal/core/source`; +- generic framework chunk invariants that make extraction possible stay in the + runner, such as non-empty chunk content, valid unit ranges, and canonical + source-unit ordering; - config validation stays in `internal/core/config`; - registry, profile, and pipeline consistency checks stay in framework and CLI code; @@ -77,6 +96,7 @@ Validators should answer module-output questions such as: - is returned content syntactically valid JSON; - does returned JSON conform to the module's declared schema; +- does the returned media type match the module or pipeline policy; - are required domain fields present and non-empty; - are source references valid and appropriately grounded; - does domain-specific output satisfy the configured policy. @@ -120,6 +140,10 @@ internal/validators/extract/dnd/spells/source_refs internal/validators/extract/dnd/spells/source_relatedness ``` +D&D spell validation policy belongs under +`internal/validators/extract/dnd/spells`, split by concern rather than bundled +inside the extractor module. + Generic validators may live under stage-specific generic paths when they operate on a particular stage output shape: @@ -237,46 +261,53 @@ The validator framework should support validation of outputs from `chunk`, enough for stage-specific validators to inspect the output they care about while ignoring irrelevant fields. -The request should carry: +The current `contracts.RawValidationRequest` is the right starting point. It +already carries stage, lane, module, source, source and chunk provenance, +response schema metadata, raw payload, and run metadata. The final contract +should evolve from that raw-output shape rather than from the legacy +artifact-candidate `ValidationRequest`. -- stage name; -- module key; -- raw module output content when available; -- response schema metadata when the module declares one; -- source document; -- source input material; +Additional fields needed for the full validator system include: + +- source input material when a validator needs to compare module output to the + original source payload; - session ID; -- references; +- resolved references for the validated target; - LLM client and profile for LLM-backed validators; -- options and metadata; -- chunk output when validating a chunk module; -- stage-specific typed envelopes when the stage owns them, such as chunk - envelopes for chunk validation. +- validator options; +- chunk output collections when validating a chunk module; +- ordered upstream output envelopes when validating merge or normalize behavior. -A shared `ModuleOutput` envelope should represent the validation boundary. -Validators may inspect raw returned content and any already-existing typed -stage output, but they must not modify it. +A shared module-output envelope should represent the validation boundary. +Validators may inspect raw returned content and any already-existing typed stage +envelope, such as `SourceChunk` values for chunk validation, but they must not +modify it. Conceptually: ```go type ModuleOutput struct { - Stage pipeline.Stage + Stage pipeline.ModuleStage ModuleKey string + LaneID string RawContent []byte MediaType string - ResponseSchema *llm.ResponseSchemaMetadata + ResponseSchema contracts.ResponseSchema - Chunks []contracts.SourceChunk - Warnings []contracts.Warning + SourceID string + ChunkID string + ChunkIndex int + Chunks []contracts.SourceChunk + Warnings []contracts.Warning } ``` The final implementation does not need to use this exact shape, but it should -preserve the boundary: returned raw module output can enter validation before -any separate materialization step converts it into a stage-specific typed -representation. +preserve the boundary: returned raw module output enters validation as immutable +module output. Validators may parse raw bytes internally to decide approve, +reject, or warn, but parsing inside a validator must not create or replace the +payload passed to later stages. The result should continue to express validator identity, warnings, and explicit decisions. For output collections, the implementation should define an explicit @@ -292,10 +323,10 @@ implicit pre-validation rejection. ## Module Development Workflow -The validation system should make iterative module development easier. A module -author should be able to start with an explicit empty validator mapping and -inspect returned raw LLM output without first satisfying JSON syntax, schema, -media-type, or domain validators. +The validation system should make iterative module development easier. Once +pipeline overrides are implemented, a module author should be able to start with +an explicit empty validator mapping and inspect returned raw LLM output without +first satisfying JSON syntax, schema, media-type, or domain validators. A typical development path should be: @@ -349,10 +380,20 @@ The validator registry should expose registered validator specs without building validators, including key and execution class. Building a validator should still be available for runtime execution. +The mapping surface should preserve the current useful behavior of stage/module +lookup and empty-chain approval while adding validator specs, execution-class +metadata, production registration, config override integration, and manifest +reporting of the resolved chain. + ## Pipeline Overrides Pipeline configuration should be able to override the central default mapping -for a module binding. Override semantics should distinguish three states: +for a module binding. Current configuration validation rejects non-empty +validator lists, and the current config/profile structs do not preserve whether +an empty list was explicitly configured or simply omitted. The target config +model must preserve that distinction. + +Override semantics should distinguish three states: - unset validators: use the central production/default mapping; - explicit empty validators: run no validators and pass output forward; @@ -405,7 +446,8 @@ behavior. ## Documentation Impact -When implemented, current-behavior docs and policy should be updated together: +Current-behavior docs and policy should describe the implemented validation +system once the refactor is complete: - `docs/policy/architecture.md` should describe centralized validator mappings rather than module-owned validator chains. @@ -413,8 +455,9 @@ When implemented, current-behavior docs and policy should be updated together: validator defaults. - Internal validation docs should describe validator package ownership, mapping precedence, empty-chain approval behavior, and LLM-backed validator support. -- User/config docs should describe how pipeline validator overrides work once the - syntax is implemented. +- User/config docs should replace the current "configured validators are + reserved and rejected" language with the implemented pipeline override + contract. Roadmap docs should not remain the canonical description of implemented validation behavior after the refactor is complete.