Add filesystem checkpoint compatibility tests
This commit is contained in:
@@ -23,8 +23,9 @@ important behavior.
|
||||
## Objective
|
||||
|
||||
Review the complete Notarius test suite against the Testing Policy, identify
|
||||
both low-value coverage and meaningful protection gaps, and then develop a
|
||||
decision-complete staged implementation plan for the justified changes.
|
||||
both low-value coverage and meaningful protection gaps, and define the
|
||||
justified target revisions. The ordered execution plan is maintained separately
|
||||
in [Implementation](implementation.md).
|
||||
|
||||
The review must answer:
|
||||
|
||||
@@ -151,27 +152,6 @@ group shares one clear disposition.
|
||||
Distinguish required changes from optional cleanup. Absence of a finding is not
|
||||
evidence that a package needs more tests.
|
||||
|
||||
## Implementation-Plan Deliverable
|
||||
|
||||
After completing the review, write a decision-complete staged implementation
|
||||
plan based on the supported findings. The plan must:
|
||||
|
||||
- order work in small, independently verifiable package or behavior groups;
|
||||
- state exactly which protections are retained when tests are consolidated,
|
||||
rewritten, or deleted;
|
||||
- add tests only for identified meaningful risks;
|
||||
- avoid production behavior changes unless a separately identified production
|
||||
defect is expressly brought into scope;
|
||||
- identify focused and repository-wide validation for each stage; and
|
||||
- include acceptance criteria demonstrating that the resulting suite remains
|
||||
deterministic, offline, diagnostically useful, and sufficient under the
|
||||
Testing Policy.
|
||||
|
||||
The plan must not establish a coverage-percentage target, require mechanical
|
||||
conversion to table-driven tests, restore old tests wholesale, or equate fewer
|
||||
tests with success. It should prefer correct, idiomatic, and maintainable tests
|
||||
even when achieving the durable boundary requires more immediate work.
|
||||
|
||||
## Completion Criteria
|
||||
|
||||
The review is complete when:
|
||||
@@ -183,5 +163,299 @@ The review is complete when:
|
||||
- redundant or brittle protection is distinguished from valuable intentional
|
||||
overlap;
|
||||
- production defects, if any, are reported separately; and
|
||||
- the resulting implementation plan can be executed without requiring the
|
||||
implementing agent to make additional testing-policy decisions.
|
||||
- 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.
|
||||
|
||||
Reference in New Issue
Block a user