Files
notarius/docs/roadmap/tests.md

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

  1. Read the Testing Policy and the canonical documentation for each subsystem before judging its tests.
  2. Establish a clean baseline with the repository validation commands, focused race tests where concurrency or shared state is relevant, and a coverage report used only for investigation.
  3. Inventory tests and map each meaningful test or closely related group to the contract, invariant, integration boundary, or regression it protects.
  4. Inspect production code only as needed to understand the protected behavior, identify the stable boundary, and detect untested risk. Do not infer a contract merely from current implementation detail.
  5. Evaluate marginal value across layers. Similar assertions are not redundant when one owns a package contract and another distinctly proves production wiring or end-to-end integration.
  6. Record evidence for every proposed change. Name the affected test or fixture, the realistic defect it currently catches or fails to catch, and why the recommendation improves confidence or reduces unnecessary friction.
  7. Check historical context when a test appears unusually specific. Preserve a regression test when the underlying defect remains plausible and consequential, even if its purpose is not obvious from the current code.

Do not modify production code or tests during the review. If the review reveals incorrect production behavior, report it separately from test-suite harmonization rather than treating a changed test expectation as the fix.

Review Deliverable

Produce an evidence-backed report organized by priority and subsystem. Each finding must classify the proposed disposition as one of:

  • retain: valuable protection at an appropriate boundary;
  • consolidate: overlapping protection that can be represented more simply;
  • rewrite: meaningful protection expressed through a brittle or misleading boundary;
  • delete: no sufficient plausible defect or distinct protection justifies the lifetime cost; or
  • add: a consequential risk lacks credible protection.

For consolidate, rewrite, delete, and add findings, describe the protected risk, current evidence, recommended boundary, and expected effect on confidence and maintenance. Do not produce a raw list of every test when a package or related group shares one clear disposition.

Distinguish required changes from optional cleanup. Absence of a finding is not evidence that a package needs more tests.

Completion Criteria

The review is complete when:

  • the full suite has been considered at an appropriate behavioral grouping;
  • important test ownership and integration boundaries are mapped;
  • every recommended change is supported by a concrete risk and evidence;
  • high-risk behavior without credible protection is identified;
  • redundant or brittle protection is distinguished from valuable intentional overlap;
  • production defects, if any, are reported separately; and
  • every target revision is specific enough to support a decision-complete implementation roadmap.

Revisions Needed

Review baseline

The review was completed on 2026-07-18 against 90 committed Go test files, 536 named tests, approximately 21,900 lines of test code, and the committed fixtures under testdata/ and examples/.

The baseline is clean:

  • go test ./..., go vet ./..., and go build ./cmd/notarius pass;
  • go test -race passes for internal/framework/pipeline, internal/framework/llm, internal/cli, and internal/modules/integration;
  • five shuffled repetitions of the full suite pass; and
  • the default suite is offline, credential-free, and fast. The only HTTP behavior uses local test servers or deliberately unreachable loopback endpoints without making provider calls.

Coverage was used only to investigate risk. It confirms broad behavioral coverage in CLI, configuration, source, pipeline, and production module packages. It also exposes the material checkpoint gap described below: most filesystem loader paths and all source, merge, and normalize checkpoint round trips are unexercised. No production defect was identified during this review.

Test ownership that should be retained

The following overlap is intentional and should remain:

  • CLI command, run, cache, reference, state-hardening, production-composition, and maintained-example tests own exit classification, option mapping, physical state placement, default production wiring, and representative assembled workflows. Package tests continue to own the underlying parsing, resolution, and persistence rules.
  • Configuration contract tests own file decoding, precedence, validation, redaction, and effective resolution. Pipeline resolver tests own module, capability, typed-variant, validator-chain, and reference resolution after configuration has produced a profile.
  • Pipeline runner tests own bounded concurrency, deterministic ordering, cancellation, retries, rejection propagation, checkpoint decisions, debug recording, candidate/final encoding, and terminal failure behavior. The one cross-family concurrency test remains valuable because it proves that the framework worker bound and shared provider scheduler remain independent in an assembled production-style pipeline.
  • Source, chunk-plan store, file I/O, debug-bundle, output encoder, artifact codec, and manifest tests own data integrity, confinement, serialization, compatibility, atomic publication, redaction, and round trips. The exact checkpoint schema identifier and exact artifact-schema digest tests are compatibility tests, not incidental constant assertions, and must remain.
  • Seriatim adapter, D&D scene chunker and spell extractor, D&D validators, generic chunker and validators, prompt preparation, and durable spell codec tests own their domain rules and external or durable schemas. Leaf constructor/spec/registration tests remain justified because the extension contract explicitly requires each leaf to be independently buildable.
  • The centralized production import-boundary test remains a valuable executable architecture rule. Its synthetic rejected fixture is necessary to prove that the checker itself is active.

Absence from the revisions below means the current behavioral group has an appropriate owner and no justified change was found.

Required findings

P0 - add filesystem checkpoint compatibility and recovery coverage

Disposition: add. internal/framework/checkpoint/recorder_test.go currently proves only that an empty successful extract checkpoint can be written and loaded. Coverage confirms that FilesystemLoader.Source, Merge, Normalize, manifest validation, payload digest validation, and most recorder status paths are otherwise silent. CLI resume coverage proves root selection and one happy reuse, while pipeline checkpoint tests use controlled collaborators; neither owns the filesystem format.

Add package-level tests at the real recorder/loader boundary that:

  • prove NewIdentity is deterministic under reordered lanes, references, and fingerprints, changes when pipeline, source/input, selected lanes, runtime overrides, references, or provenance change, and produces a confined stable relative path;
  • round-trip representative source, extract, merge, and normalize payloads, including serialized artifact identity, warnings, extract rejections, metadata, and dependency fingerprints;
  • prove caller mutation cannot alter recorded or loaded values and retain the existing restrictive-permission expectation through real files; and
  • mutate one persisted artifact at a time to prove that missing or malformed JSON, old or unknown workspace schema versions, wrong identity/stage/lane/ module/status/dependencies, incomplete codec identity, invalid base64, and content/output-digest mismatches yield a non-reused decision with useful category context rather than a panic or silent reuse.

Use relationships and category fragments rather than snapshotting complete manifests or error sentences. This addition protects cache correctness, compatibility, recovery, and sensitive durable state; it is not intended to raise a coverage percentage.

P1 - remove tests that exercise only their own fakes or obsolete APIs

Disposition: delete. Remove the fake-behavior tests in internal/framework/contracts/contracts_test.go: TestFakeExtractorReturnsTypedOutput, TestFakeChunkerReturnsSourcePlan, TestFakeChunkerReceivesPerRunContext, TestFakeExtractorReceivesChunkAndAmbientContext, TestFakeMergeNormalizeAndOutputContracts, and TestReferenceSetDataTypes. They detect changes in test helpers or Go struct assignment, not defects in production contracts. Retain the compile-time interface assertions and the clone, JSON omission, and content-ownership tests, which protect type compatibility and non-leakage invariants.

Disposition: delete. Remove internal/framework/llm/scriptorium_api_test.go. It was introduced as pre-adapter API grounding and still mirrors unused Scriptorium request/result fields, option constructors, and sentinel errors. Production compilation now grounds the API actually used, while scriptorium_client_test.go and module prompt-preparation tests protect adapter mapping, local integration, cancellation, validation, profile selection, and credential redaction.

Disposition: delete. Remove TestHelpersReturnValidationResults from internal/framework/validate/validate_test.go; it is a compile-only assignment that adds no protection beyond the function signatures and the two behavioral helper tests.

Disposition: delete. Remove the direct TestTypedNormalizerPreservesValue test for the generic no-op normalizer. Its single assignment is already exercised through resolver, runner, production composition, and maintained-example output tests. Retain the direct append-order merger test because input ordering is a meaningful transformation invariant.

P1 - remove misleading D&D integration claims

Disposition: delete. In internal/modules/integration/dnd_spells_runner_test.go, delete:

  • TestRunnerDoesNotExtractSpellMentionedOnlyInPartyReference, whose fake LLM is programmed to return no spells and therefore cannot prove the claimed extraction policy;
  • TestRunnerCarriesDNDSpellCastWithInvalidSourceRefToSerializedOutput, which omits the production validator chain and duplicates the extractor's explicit invalid-evidence handoff test; and
  • TestRunnerRejectsMalformedDNDSpellsArtifactAtSerializationBoundary, whose final-codec invariant is already owned by the codec and runner candidate/ final-encoding tests.

Retain the representative Seriatim-to-spell assembled workflow and the test that proves party/glossary references and provenance cross the production module boundary. The policy that reference material is not source evidence is durably protected by extractor mapping, prompt-input assertions, source-ref validators, and the production validator-chain composition test; it cannot be proved by controlling an LLM stub's semantic answer.

P1 - consolidate superseded configuration and CLI state tests

Disposition: consolidate. Delete internal/core/config/v3_test.go after moving its two distinct protections into the current contract owners:

  • add the version-2 migration rejection case to the strict file-decoding cases in file_config_contract_test.go; and
  • ensure the positive per-user chunk-plan/checkpoint root separation remains in env_contract_test.go.

The remaining default, precedence, invalid-source, redaction, and removed-field assertions are already more completely owned by file_config_contract_test.go, env_contract_test.go, validation_contract_test.go, and redaction_test.go.

Disposition: consolidate. Delete internal/cli/state_surfaces_test.go. Move the shared emptyLookup helper to contract_test_helpers_test.go. Its debug flag syntax, allocation timing, no-debug absence, version-3 validation, and removed-field cases are already owned by run_contract_test.go, state_hardening_test.go, command_contract_test.go, and the configuration contract tests. Preserve the state matrix, failure retention, no-debug terminal-writer, and pre-resolution debug allocation protections in those current owners.

Disposition: rewrite. Keep TestDefaultCLICompositionResolvesMaintainedConfigurations as a narrow default composition smoke test, but run one representative config-validation command through empty Options rather than revalidating every maintained example. example_contract_test.go owns both maintained examples. Remove the repeated maintained-example loops from TestProductionCatalogCoversMaintainedConfigurations and TestProductionConfigValidationCoversModuleAndVariantFailures; retain the production registry/codec/default-chain checks and the distinct production failure cases.

P1 - consolidate resolver and module-composition duplication

Disposition: delete. Remove internal/framework/pipeline/default_modules_test.go. Its large local catalog only re-proves default binding selection, already owned by profile_test.go and effective_config_contract_test.go; production default keys and wiring are separately exercised by leaf registration, the production catalog, default CLI composition, and the maintained end-to-end example.

Disposition: delete. Remove internal/modules/seriatim/input/transcript/config_test.go. It builds an entire fake catalog to repeat generic resolver success, deterministic digest, missing-capability, and unknown-lane behavior. Retain Seriatim parsing and leaf registration tests, the Seriatim runner integration, D&D cross-family capability tests, resolver contract tests, and production CLI examples.

Disposition: consolidate. In internal/modules/integration/dnd_spells_config_test.go, retain only the two cross-family capability contracts: Seriatim must provide source.transcript to the spell extractor, and the spell extractor must provide dnd.spell_casts to append-order. Express them as one compact table over a programmatic profile/catalog. Remove the generic successful-resolution, stable-digest, and unknown-lane cases, which are already exercised by the retained runner integration and resolver/configuration owners.

P1 - replace brittle assertions with behavioral ones

Disposition: rewrite. In internal/core/source/source_test.go, make ValidateRef the owner of malformed reference categories. ValidateDocument should retain one propagation/context case plus the document-only self-reference invariant instead of repeating missing, foreign, and reversed-reference cases. Replace complete internal error-sentence equality with the smallest stable category and field fragments. Retain exact acceptance/rejection boundaries, duplicate detection, deterministic digests, and reference ordering.

Disposition: rewrite. Replace the decoded-schema-structure walk in internal/modules/dnd/chunk/scenes/schema_test.go with representative JSON Schema validation: accept a valid source-unit-boundary response and reject old segment fields, non-positive bounds, invalid enums, empty caveats, and unknown properties. Likewise, change the private spell response-schema assertion in internal/modules/dnd/extract/spells/schema_test.go to accept the LLM transport shape without source_id and reject a response that supplies canonical source_id. Retain schema identity/hash, mutation safety, diagnostics non-leakage, DTO decoding, and the codec's durable fixture tests. These rewrites preserve schema regression protection without coupling tests to nested map layout or unsafe type assertions.

Disposition: rewrite. In internal/framework/llm/schema_registry_test.go, assert that returned keys are sorted and required framework test schemas are present without asserting that the registry contains exactly two entries. Retain the explicit negative D&D lookup because it protects the framework/ domain ownership boundary.

P2 - make composition and architecture tests open to legitimate extension

Disposition: rewrite. The family registrar tests in internal/modules/generic/register, internal/modules/seriatim/register, and internal/modules/dnd/register currently assert closed-world key and asset inventories that duplicate the CLI production catalog. Change them to require the family-owned registrations they need, permit unrelated future additions, and prove representative entries can be built or prepared. Keep exact order for the D&D default spell validator chain because that order is documented production policy. Keep missing-dependency-before-mutation and contextual duplicate-registration cases.

Disposition: consolidate. Move the source/framework independence rules from internal/framework/chunkplan/import_boundaries_test.go into the centralized internal/modules/import_boundaries_test.go checker and delete the second repository walker. Preserve both unique rules: internal/core/source may import neither framework nor modules, and internal/framework/chunkplan may not import modules. Add rule-level cases so a broken checker still fails.

Documentation alignment

Disposition: rewrite. When the affected tests move or are deleted, update the Tests To Inspect sections in docs/internal/state.md, docs/internal/pipeline.md, docs/internal/modules.md, and docs/internal/llm.md. They currently name files such as internal/cli/state_surfaces_test.go and internal/cli/run_test.go that will be deleted or do not exist. Point each document at the retained contract, state-hardening, production-composition, example, checkpoint, and integration owners without recreating an exhaustive test inventory.

Optional cleanup

No additional cleanup is recommended now. In particular, do not mechanically convert the large resolver and runner suites to table-driven form, merge all leaf registration tests into family registrars, add tests for trivial accessor coverage, or introduce a golden-update framework. Those changes do not provide enough additional confidence to justify their immediate cost.

Target state

The revision is complete when:

  • every checkpoint stage has a real filesystem round trip and incompatible or corrupted state is demonstrably recomputed rather than silently reused;
  • all protection named as retained above remains present at its stated owner;
  • the fake-only, obsolete, misleading, duplicate, and closed-world assertions named in the required findings are removed or rewritten exactly as specified;
  • maintained examples are each owned by one example contract plus one narrow default-composition smoke path, rather than repeated across production tests;
  • default tests remain deterministic, offline, credential-free, order independent, and race-clean;
  • failures identify the violated behavioral category without snapshotting complete incidental diagnostics;
  • internal documentation points to existing retained test owners; and
  • the suite reaches this state without changing production behavior.