diff --git a/docs/validator-refactor.md b/docs/validator-refactor.md new file mode 100644 index 0000000..cd4319b --- /dev/null +++ b/docs/validator-refactor.md @@ -0,0 +1,688 @@ +# Validator Refactor Plan + +## Purpose + +This document describes a concrete package-ownership refactor for Audita's built-in validators. + +Target outcome: +- each built-in validator has its own package under `internal/validators//`; +- the rest of the application depends on the existing common validator runtime contract; +- validator-specific construction and configuration are owned by validator-specific packages; +- shared validator runtime machinery remains centralized where reuse is high; +- runner and module code do not need to know concrete validator implementation types. + +This is not a runtime redesign. Audita already has the important validator abstraction: the runner receives validators through a common interface, calls `Validate`, and consumes validator results without needing to understand validator internals. This plan preserves that model and improves package ownership around it. + +## Current state + +The current codebase already has the core runtime pieces: + +- `contracts.Validator` in `internal/framework/contracts`; +- `Validate(ctx, req)` semantics; +- validator request/result/decision models; +- runner orchestration that treats validators as decision producers; +- stable built-in validator keys; +- a built-in validator registry and built-in module chain definitions; +- deterministic and LLM-backed validator implementations; +- embedded prompt assets and prompt metadata for LLM-backed validators. + +Current validator ownership is split across two layers: + +- `internal/validators/` + - built-in key constants; + - built-in registry; + - built-in module chain definitions. + +- `internal/framework/validators/` + - request/result/decision models; + - deterministic validator implementation details; + - generic LLM-backed validator runtime; + - batching; + - prompt/response and diagnostics helpers; + - cardinality enforcement and shared validation helpers. + +This refactor should make `internal/validators//` the visible home of each built-in validator, while keeping shared runtime machinery in `internal/framework/validators`. + +## Goals + +Required goals: + +- Mirror the module package pattern by giving each built-in validator its own package under `internal/validators`. +- Preserve the existing `contracts.Validator` runtime contract. +- Keep built-in validator keys stable. +- Keep built-in module validator chains stable. +- Preserve current runtime behavior and report shape unless explicitly noted. +- Remove concrete validator type knowledge from the runner. +- Make production registry construction route through validator-owned packages. +- Localize the `protected_terms` glossary-stage special case inside the `protected_terms` validator package. +- Keep LLM batching, diagnostics, response parsing, cardinality checks, and shared helper behavior centralized. + +Secondary goals: + +- Make validator construction read similarly to module construction. +- Keep validator-specific tests close to validator-specific packages. +- Reduce the impression that `internal/framework/validators` owns the built-in validator catalog. +- Make future built-in validators easier to add without expanding a central framework file. + +## Non-goals + +Do not implement any of the following as part of this refactor: + +- User-defined validator plugins. +- User-configurable validator chains. +- New validator types. +- Changes to validator decision cardinality rules. +- Changes to prompt assets or prompt text. +- Changes to structured response schemas. +- Changes to scheduler behavior. +- Changes to output schemas, reports, diagnostics shape, or correction ledger shape except where a stable validator key already appears. +- A new competing validator interface. + +## Architectural principle + +This should be a package-ownership cleanup over an already sound runtime abstraction. + +The correct bias is: + +- keep runtime semantics stable; +- move validator identity and construction into validator-owned packages; +- keep shared machinery centralized; +- remove concrete implementation leakage from the runner; +- prefer wrappers first and deeper cleanup second. + +## Target package layout + +Target end state: + +```text +internal/framework/validators/ + models.go + runtime.go + llm_runtime.go + llm_batching.go + diagnostics.go + cardinality.go + prompt_helpers.go + protected_vocabulary.go + +internal/validators/ + registry.go + chains.go + metadata.go + interfaces.go + +internal/validators/confidence_threshold/ + validator.go + validator_test.go + +internal/validators/original_text_presence/ + validator.go + validator_test.go + +internal/validators/non_empty_corrected_text/ + validator.go + validator_test.go + +internal/validators/no_effect/ + validator.go + validator_test.go + +internal/validators/protected_terms/ + validator.go + validator_test.go + +internal/validators/spoken_form_plausibility/ + validator.go + validator_test.go + +internal/validators/meaning_reversal_review/ + validator.go + validator_test.go + +internal/validators/editorial_review/ + validator.go + validator_test.go + +internal/validators/grammar_review/ + validator.go + validator_test.go + +internal/validators/spoken_word_review/ + validator.go + validator_test.go +``` + +Notes: + +- `internal/framework/validators` remains the shared runtime layer. +- `internal/validators/` owns construction and validator-specific configuration. +- `internal/validators/registry.go` remains the built-in production registry entrypoint. +- `internal/validators/chains.go` remains the built-in module chain resolver. +- It is acceptable for `internal/framework/validators` to retain a generic shared LLM validator runtime type if only validator-owned packages construct it. + +## Runtime contract + +Keep `contracts.Validator` as the framework-facing contract. + +Do not introduce a second competing validator interface. If helper interfaces are needed, they should supplement the existing contract rather than replace it. + +The runner should continue to operate on validators as opaque components: + +- receive `[]contracts.Validator`; +- call `Validate(ctx, req)`; +- consume returned validator results; +- remove rejected proposals from the eligible set; +- preserve deterministic-before-LLM execution ordering; +- preserve existing decision cardinality rules. + +## Validator metadata + +Add a small metadata surface to avoid concrete implementation checks in the runner. + +Recommended location: + +- `internal/validators/interfaces.go` or `internal/validators/metadata.go` + +This package must stay low-level: + +- it may import `internal/framework/contracts`; +- it must not import validator-specific packages; +- it must not import the registry. + +Suggested API: + +```go +type ExecutionClass string + +const ( + ExecutionClassDeterministic ExecutionClass = "deterministic" + ExecutionClassLLMBacked ExecutionClass = "llm_backed" +) + +type ClassifiedValidator interface { + contracts.Validator + ExecutionClass() ExecutionClass +} +``` + +Runner behavior: + +- if a validator implements `ClassifiedValidator`, use `ExecutionClass()`; +- otherwise treat it as deterministic by default; +- never type-assert against concrete framework validator types such as `*frameworkvalidators.LLMBackedValidator`. + +This removes concrete implementation knowledge from the runner while preserving ordering semantics. + +## Validator package constructors + +Each validator package should expose a constructor that returns `contracts.Validator`. + +Use the simplest constructor that honestly reflects the validator's dependencies. + +Acceptable patterns: + +```go +func New() (contracts.Validator, error) +``` + +```go +func New(opts Options) (contracts.Validator, error) +``` + +```go +func NewGlossaryStage(opts Options) (contracts.Validator, error) +``` + +Guidance: + +- Use `New()` only when construction truly requires no runtime dependencies. +- Use `New(opts Options)` when the existing registry already supplies dependencies such as config, glossary, LLM client, scheduler, diagnostics context, or prompt metadata. +- Keep `Options` package-local unless several validators genuinely share the same option structure. +- Do not force zero-argument constructors if doing so would hide dependencies in globals or cause construction-time behavior to become implicit. +- Constructor names should make module-sensitive behavior explicit, especially for `protected_terms`. + +## Built-in validator registry + +Keep `internal/validators/registry.go` as the production wiring layer. + +After this refactor, registry entries should call validator-package constructors rather than framework concrete implementations. + +Example target shape: + +```go +{Key: KeyConfidenceThreshold, Build: confidencethreshold.New} +{Key: KeyGrammarReview, Build: grammarreview.New} +``` + +The registry should continue to provide: + +- stable validator keys; +- built-in validator metadata; +- clear lookup/build failures for unknown keys; +- stable production construction behavior. + +The registry should not become a user plugin system. + +## Built-in validator keys + +Preserve these keys exactly: + +- `confidence_threshold` +- `original_text_presence` +- `non_empty_corrected_text` +- `no_effect` +- `protected_terms` +- `spoken_form_plausibility` +- `meaning_reversal_review` +- `editorial_review` +- `grammar_review` +- `spoken_word_review` + +These keys must continue to appear consistently in: + +- registry entries; +- built-in chain definitions; +- reports; +- diagnostics metadata or paths where validator identity appears; +- correction ledger entries; +- utilization diagnostics; +- tests; +- documentation. + +## Built-in module chains + +Preserve the current effective built-in module chains exactly unless an existing test proves the documented chain differs from the runtime and the runtime behavior is clearly the intended source of truth. + +Do not use this refactor to revisit validator policy. + +The chain resolver may continue to live in `internal/validators/chains.go`, but it should construct validators through package-owned constructors. + +## Treatment of deterministic validators + +Move ownership into per-validator packages: + +- `internal/validators/confidence_threshold` +- `internal/validators/original_text_presence` +- `internal/validators/non_empty_corrected_text` +- `internal/validators/no_effect` +- `internal/validators/protected_terms` + +Implementation guidance: + +- If the implementation is short and self-contained, it may live directly in the validator package. +- If logic is shared, keep shared helpers in `internal/framework/validators`. +- Do not duplicate cardinality helpers or vocabulary helpers. +- Prefer behavior-preserving wrappers first, then move implementation details only where it clearly improves ownership. + +The goal is not to empty `internal/framework/validators`; the goal is to stop making it look like the built-in validator catalog. + +## Protected terms special case + +`protected_terms` has module-sensitive behavior and must be handled carefully. + +Target ownership: + +- `internal/validators/protected_terms` owns both general and glossary-stage construction. + +Suggested constructors: + +```go +func New(opts Options) (contracts.Validator, error) +func NewGlossaryStage(opts Options) (contracts.Validator, error) +``` + +or, if no options are needed: + +```go +func New() (contracts.Validator, error) +func NewGlossaryStage() (contracts.Validator, error) +``` + +Behavior requirements: + +- non-glossary modules keep existing protected-term behavior; +- glossary-stage behavior remains stricter if that is the current runtime behavior; +- both variants continue to report the stable key `protected_terms`; +- chain resolution should not directly reference framework concrete protected-term validator types. + +Add or preserve explicit tests for: + +- glossary-stage protected-term behavior; +- non-glossary protected-term behavior; +- stable `protected_terms` identity in reports and correction ledger entries. + +## Treatment of LLM-backed validators + +Move ownership into per-validator packages: + +- `internal/validators/spoken_form_plausibility` +- `internal/validators/meaning_reversal_review` +- `internal/validators/editorial_review` +- `internal/validators/grammar_review` +- `internal/validators/spoken_word_review` + +Recommended implementation: + +- each package provides a thin wrapper over shared LLM runtime machinery; +- each package owns the stable validator key and constructor; +- each package selects or configures the appropriate prompt ID/type through existing prompt registry surfaces; +- each package exposes `ExecutionClass() == llm_backed` either directly or through a wrapper; +- shared framework code continues to own batching, diagnostics, structured response parsing, model resolution, and decision mapping. + +Avoid this anti-pattern: + +- five copied versions of the generic LLM-backed validator runtime. + +Acceptable shared runtime: + +- a generic shared `LLMBackedValidator`; +- a shared `NewLLMBackedValidator(...)` factory; +- a shared LLM validator executor configured by validator packages. + +The important boundary is that validator-specific ownership is visible under `internal/validators/`, even if shared execution remains centralized. + +## Prompt ownership + +Do not change prompt assets in this refactor. + +Prompt assets already live under `internal/prompts`. Validator packages may own prompt selection/configuration by referring to existing prompt IDs, but they should not duplicate prompt text or move Markdown assets. + +For example, a package may configure the shared LLM runtime with: + +- validator key: `grammar_review`; +- prompt ID: `validators.grammar_review`; +- structured response schema: `validator_decision_set`; +- execution class: `llm_backed`. + +But this refactor should not alter prompt text, prompt metadata, prompt IDs, prompt hashes, or prompt rendering behavior except where imports must be adjusted. + +## Import-cycle rules + +Avoid import cycles by keeping dependencies acyclic. + +Preferred dependency direction: + +```text +internal/framework/contracts +internal/framework/validators +internal/prompts + ↑ +internal/validators/ + ↑ +internal/validators/registry and chains + ↑ +module construction / runner wiring +``` + +Rules: + +- validator-specific packages must not import `internal/validators/registry`; +- the registry may import validator-specific packages; +- runner may import low-level validator metadata but should not import validator-specific packages; +- metadata interfaces should live in a low-level package that does not import registry or validator-specific packages; +- shared framework runtime must not import the built-in registry. + +## Recommended implementation passes + +Use three implementation passes unless blocked. + +### Pass 1: Metadata, wrappers, and registry switch + +Purpose: +- introduce execution metadata; +- remove runner dependency on concrete framework validator types; +- create per-validator packages as thin wrappers; +- switch the registry to construct validators through those packages. + +Tasks: +- add validator execution classification metadata; +- update runner ordering logic to use metadata; +- create one package per built-in validator under `internal/validators`; +- keep wrappers behavior-preserving; +- preserve stable keys; +- preserve built-in chains; +- switch registry build closures to per-package constructors; +- add tests for registry completeness, metadata classification, and ordering behavior. + +Acceptance criteria: +- runner no longer type-asserts against framework concrete validator types; +- every built-in validator has a package; +- registry builds all validators through package constructors; +- deterministic-before-LLM behavior is unchanged; +- all existing tests pass. + +### Pass 2: Protected terms localization, framework cleanup, and test locality + +Purpose: +- localize the `protected_terms` special case; +- reduce framework built-in ownership; +- move or add tests near validator packages. + +Tasks: +- add explicit protected-terms constructors for general and glossary-stage behavior; +- update chain resolution to call those constructors; +- remove direct chain/registry references to protected-term framework concrete types; +- reduce exported framework validator types where package wrappers fully own construction; +- keep shared runtime helpers in `internal/framework/validators`; +- move or add validator-specific tests under the validator packages where practical; +- retain shared runtime tests in the framework package. + +Acceptance criteria: +- protected-term behavior is unchanged; +- glossary-stage behavior is explicitly tested; +- framework package no longer appears to own the built-in validator catalog; +- validator-specific behavior is tested near validator packages where practical; +- all existing tests pass. + +### Pass 3: Documentation and review + +Purpose: +- align docs with the final structure; +- verify no accidental behavior drift or scope creep. + +Tasks: +- update `docs/architecture.md`; +- update `docs/validators.md`; +- update any public-contract or diagnostics docs only if validator package ownership needs mention there; +- document the distinction between validator-owned packages and shared validator runtime; +- document the 1.0 boundary that validator chains remain built-in and not user-configurable; +- run `go test ./...`; +- review imports and package boundaries for accidental cycles or leakage. + +Acceptance criteria: +- docs describe the new package layout accurately; +- docs do not imply plugin support or user-configurable chains; +- tests pass; +- git diff shows package-ownership refactor only. + +## File-by-file change guide + +Expected direct edits: + +- `internal/framework/runner/runner.go` + - replace concrete LLM validator type check with execution metadata interface check. + +- `internal/validators/interfaces.go` or `internal/validators/metadata.go` + - define execution classification types and optional interface. + +- `internal/validators/registry.go` + - import per-validator packages; + - route construction through package constructors; + - preserve stable keys. + +- `internal/validators/chains.go` + - preserve chain key lists and order; + - use protected-terms glossary-stage constructor where appropriate; + - avoid concrete framework validator types. + +- `internal/framework/validators/deterministic.go` + - reduce or remove exported built-in validator concrete types if wrappers fully own construction; + - keep shared helpers where useful. + +- `internal/framework/validators/llm_validators.go` + - keep shared LLM runtime; + - reduce exported surface only if safe; + - do not duplicate LLM runtime across validator packages. + +Expected new files: + +- `internal/validators/confidence_threshold/validator.go` +- `internal/validators/original_text_presence/validator.go` +- `internal/validators/non_empty_corrected_text/validator.go` +- `internal/validators/no_effect/validator.go` +- `internal/validators/protected_terms/validator.go` +- `internal/validators/spoken_form_plausibility/validator.go` +- `internal/validators/meaning_reversal_review/validator.go` +- `internal/validators/editorial_review/validator.go` +- `internal/validators/grammar_review/validator.go` +- `internal/validators/spoken_word_review/validator.go` +- corresponding package-local tests where practical. + +Expected documentation edits: + +- `docs/architecture.md` +- `docs/validators.md` + +Possibly update: + +- `docs/public-contract.md` +- `docs/diagnostics.md` + +Only update README if the high-level project description would otherwise be inaccurate. + +## Test strategy + +Recommended test distribution: + +- `internal/validators//validator_test.go` + - package-specific constructor and behavior tests. + +- `internal/validators/registry_test.go` + - registry wiring and key coverage. + +- `internal/validators/module_chains_test.go` + - built-in module chain coverage. + +- `internal/framework/runner/runner_test.go` + - orchestration semantics only, including deterministic-before-LLM ordering. + +- `internal/framework/validators/*_test.go` + - shared runtime behavior only. + +Required test coverage: + +- every built-in validator package exists and constructs successfully; +- every built-in key resolves through the registry; +- every built-in module chain resolves; +- unknown validator keys fail deterministically; +- runner ordering uses execution classification, not concrete type checks; +- unclassified validators default to deterministic; +- protected-terms glossary-stage behavior is preserved; +- stable validator keys remain present in reports, diagnostics, utilization diagnostics, and correction ledger entries where currently applicable; +- no live LLM credentials are needed for `go test ./...`. + +## Ordering and safety rules + +Implementation rules: + +- preserve validator keys exactly; +- preserve built-in module chain order exactly; +- preserve `protected_terms` glossary-stage behavior exactly; +- preserve deterministic-before-LLM ordering; +- preserve decision cardinality enforcement; +- preserve report field names and validator identity strings; +- preserve prompt assets and prompt metadata; +- preserve structured response schemas; +- preserve scheduler semantics; +- preserve output schemas; +- avoid duplicating shared LLM runtime code; +- keep tests passing after each implementation pass. + +## Risks and mitigations + +### Risk: accidental behavior drift in protected terms + +Why it matters: +- `protected_terms` has module-sensitive behavior. + +Mitigation: +- add or preserve explicit tests for glossary-stage and non-glossary-stage behavior; +- use named constructors rather than hidden request-based branching where the stage variant is selected at chain build time. + +### Risk: runner ordering regressions + +Why it matters: +- deterministic validators must still run before LLM-backed validators. + +Mitigation: +- add or retain tests proving ordering with a mixed validator list; +- treat unknown or unclassified validators as deterministic. + +### Risk: over-duplicating LLM validator code + +Why it matters: +- LLM-backed validators share batching, diagnostics, parsing, and structured-output handling. + +Mitigation: +- keep one shared LLM runtime implementation; +- use thin validator-package wrappers. + +### Risk: import cycles + +Why it matters: +- registry imports validator packages; +- validator packages import contracts and shared framework runtime; +- runner imports contracts and metadata. + +Mitigation: +- keep metadata low-level; +- keep registry out of validator-specific packages; +- keep shared framework runtime independent of the built-in registry. + +### Risk: excessive cleanup during refactor + +Why it matters: +- broader cleanup can obscure behavior changes and increase review risk. + +Mitigation: +- wrappers first; +- deeper cleanup only after registry construction is package-owned; +- do not combine with prompt, scheduler, report, config, or output-schema changes. + +## Acceptance criteria + +The refactor is complete when all of the following are true: + +- every built-in validator has its own package under `internal/validators`; +- production validator registry constructs validators via those packages; +- module packages still work with `[]contracts.Validator` and require no validator implementation knowledge; +- runner has no concrete-type dependency on framework validator implementations; +- `protected_terms` glossary-stage handling is owned by the validator package; +- built-in validator keys are unchanged; +- built-in module chain order and behavior are unchanged; +- LLM-backed validators still use shared runtime machinery; +- prompt assets and structured response schemas are unchanged; +- tests pass with `go test ./...`; +- docs describe the new package ownership model. + +## Suggested follow-up cleanup after this refactor + +Not required for this refactor, but worth considering later: + +- introduce a validator factory in `internal/framework/validators` only if future dynamic construction becomes necessary; +- consider whether `contracts.ValidationRequest = validators.Request` should remain an alias or become a contract-owned type; +- consider whether chain definitions should eventually move closer to module registry or module packages if module-specific validator policy becomes more configurable; +- consider whether `editorial_review` remains useful if no production module uses it; +- consider a later user-configurable chain system only if there is a clear product need. + +## Summary + +This refactor should make validators mirror modules at the package ownership level without changing Audita's runtime model. + +The desired end state is: + +- validator packages own built-in validator construction; +- the registry owns production wiring; +- the runner owns orchestration only; +- shared framework code owns reusable runtime mechanics; +- tests and docs reflect those boundaries.