23 KiB
Test Suite Policy Review
Context
Notarius now has a canonical Testing Policy. 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.
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
- Read the Testing Policy and the canonical documentation for each subsystem before judging its tests.
- 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.
- Inventory tests and map each meaningful test or closely related group to the contract, invariant, integration boundary, or regression it protects.
- 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.
- 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.
- 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.
- 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 ./..., andgo build ./cmd/notariuspass;go test -racepasses forinternal/framework/pipeline,internal/framework/llm,internal/cli, andinternal/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
NewIdentityis 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; andTestRunnerRejectsMalformedDNDSpellsArtifactAtSerializationBoundary, 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.