diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md new file mode 100644 index 0000000..3d66c2c --- /dev/null +++ b/docs/roadmap/implementation.md @@ -0,0 +1,481 @@ +# Test Suite Policy Review Implementation + +## Purpose + +This document is the decision-complete execution plan for implementing the +target revisions in [Test Suite Policy Review](tests.md). Follow the stages in +order. The feature roadmap owns the review evidence, desired test ownership, +required dispositions, and target state; this document owns sequencing, +file-level work, validation, and stop conditions. + +This is test-suite harmonization, not a production feature change. Do not +modify production behavior. If a new durable test exposes incorrect production +behavior, preserve the failing evidence, stop the affected stage, and report +the defect separately for explicit scoping. + +## Governing Policies + +Before implementation, read and follow: + +- [Testing Policy](../policy/testing.md), especially behavioral ownership, + semantic duplication, test doubles, exact diagnostics, and deletion criteria; +- [Architecture](../policy/architecture.md), especially dependency direction, + typed artifact boundaries, validation ownership, checkpoint safety, and + source/reference separation; and +- [Documentation Policy](../policy/documentation.md), especially canonical + ownership, current-versus-future behavior, and maintenance of test-routing + links. + +The following constraints apply to every stage: + +- preserve unrelated user changes in a dirty worktree; +- make no production-code changes unless separately authorized after reporting + a confirmed production defect; +- add no coverage-percentage target and do not use line coverage as a success + metric; +- do not mechanically convert tests to tables, consolidate leaf registration + tests, or introduce new test frameworks; +- keep the default suite deterministic, offline, credential-free, and safe for + repeated and parallel execution; +- use real filesystem collaborators with `t.TempDir()` for checkpoint behavior; +- assert typed/structured outcomes or stable category fragments rather than + complete incidental error wording; and +- do not update fixtures automatically or add a golden-update path. + +## Stage 0 - Establish the implementation baseline + +1. Read [Test Suite Policy Review](tests.md) completely, including the retained + ownership map and every required finding. +2. Inspect the worktree and preserve unrelated changes. Limit planned edits to + tests and the internal documentation routing identified below. +3. Run: + + ```sh + go test ./... + go vet ./... + go build ./cmd/notarius + go test -race ./internal/framework/pipeline ./internal/framework/llm ./internal/cli ./internal/modules/integration + ``` + +4. If the baseline fails for a reason unrelated to the planned work, stop and + report it. Do not rewrite expectations merely to obtain a clean baseline. + +Stage 0 is complete when the starting state and any pre-existing worktree +changes are recorded and all baseline commands pass. + +## Stage 1 - Protect filesystem checkpoint compatibility and recovery + +Complete this stage before deleting or consolidating existing tests so the +highest-risk protection is strengthened first. + +### 1.1 Checkpoint identity + +Create `internal/framework/checkpoint/identity_test.go`. Test through +`NewIdentity` and `Identity.RelativePath`, not private normalization helpers. + +Build one representative `pipeline.ResolvedPipeline` with two artifact lanes, +an input module, stable pipeline ID/digest, selected lanes, runtime +fingerprints, reference provenance, and provenance fingerprints. Add tests that +prove: + +- reordering selected lanes, runtime fingerprints, references, provenance + fingerprints, or resolved lanes does not change the resulting identity; +- duplicate or blank selected-lane entries do not change the normalized lane + set, and blank fingerprint entries do not change the identity; +- changing each meaningful identity input independently changes the digest: + pipeline ID or digest, input key, raw/source digest, selected lane set, + runtime override value, reference digest or target identity, and provenance + fingerprint; +- an omitted explicit input key falls back to the resolved input module; +- missing pipeline ID, pipeline digest, input key, and both source/input digests + are rejected with stable category context; and +- `RelativePath` is deterministic, slash-separated, relative, confined, and + includes the normalized pipeline/input and digest-derived hierarchy without + asserting incidental private prefix lengths beyond the documented layout. + +Use independent inputs for each mutation so one case cannot pass because a +different field also changed. + +### 1.2 All-stage recorder/loader round trips + +Create `internal/framework/checkpoint/filesystem_test.go`; leave the existing +schema-identifier test in `recorder_test.go`. + +Using one real root and one identity, exercise the exported recorder and loader +for: + +- source success with a valid document, self-references, metadata, and digest; +- extract success with at least one serialized artifact, schema identity, + content metadata, chunk provenance, warnings, dependency fingerprints, and + one rejected output so `StatusSucceededWithRejections` is round-tripped; +- merge success with one serialized artifact, warnings, and dependencies; and +- normalize success with one serialized artifact, warnings, and dependencies. + +For each stage, assert the loader returns `Reused`, restores the meaningful +values, and preserves serialized bytes and codec identity. Mutate the original +inputs after recording and mutate one loaded result before reloading; neither +mutation may alter persisted or subsequently loaded state. + +Inspect representative created directories and files to retain the `0700`/ +`0600` permission contract on platforms where Unix permission bits are +meaningful. Do not snapshot the full directory tree or complete JSON documents. + +### 1.3 Invalid and incompatible checkpoint state + +Seed valid state through the recorder, then copy or edit one artifact per +subtest. Drive every case through the exported loader method for that stage. +Require a non-reused decision and a short category fragment for: + +- missing artifact and malformed JSON; +- `WorkspaceSchemaVersionV1` and an unknown workspace schema version; +- mismatched checkpoint identity digest; +- wrong stage, lane, module, terminal status, or dependency fingerprint; +- incomplete serialized artifact kind/schema/schema digest; +- malformed base64 and content-digest mismatch; +- source document validation or source/output digest mismatch; and +- extract, merge, and normalize output-digest mismatch. + +Include one successful `StatusSucceededWithRejections` extract case and prove +that non-reusable running, failed, pending, or invalidated statuses remain +non-reused. Assert categories, not complete sentences. Missing state should be +a normal non-reuse decision; corrupt or incompatible state must never panic or +silently reuse. + +### 1.4 Stage validation + +Run: + +```sh +go test ./internal/framework/checkpoint ./internal/framework/pipeline ./internal/cli +go test -race ./internal/framework/checkpoint ./internal/framework/pipeline ./internal/cli +``` + +Stage 1 is complete when identity selection, every persisted stage, mutation +ownership, compatibility rejection, and corrupt-state recovery are protected at +the real filesystem boundary without production changes. + +## Stage 2 - Remove fake-only, obsolete, and misleading tests + +### 2.1 Framework contracts and helpers + +In `internal/framework/contracts/contracts_test.go`, delete exactly: + +- `TestFakeExtractorReturnsTypedOutput`; +- `TestFakeChunkerReturnsSourcePlan`; +- `TestFakeChunkerReceivesPerRunContext`; +- `TestFakeExtractorReceivesChunkAndAmbientContext`; +- `TestFakeMergeNormalizeAndOutputContracts`; and +- `TestReferenceSetDataTypes`. + +After removing those tests, delete fake methods/types or imports only when they +have no remaining test use. Retain the compile-time interface assertions and +the reference/material/artifact clone and JSON non-leakage tests. + +Delete `TestHelpersReturnValidationResults` from +`internal/framework/validate/validate_test.go`. Retain `TestApproved` and +`TestRejectedTrimsReasonAndMessage`. + +Delete `internal/modules/generic/normalize/noop/typed_test.go`. Do not remove the +no-op normalizer's resolver, registration, runner, production-composition, or +maintained-example coverage. Retain the direct append-order merger ordering +test. + +### 2.2 Obsolete Scriptorium grounding + +Delete `internal/framework/llm/scriptorium_api_test.go` in full. Do not move its +unused API inventory elsewhere. Retain and run `scriptorium_client_test.go`, +`asset_registry_test.go`, module-local prompt preparation, cancellation, +validation, profile, and credential-redaction tests. + +### 2.3 Misleading D&D integration tests + +In `internal/modules/integration/dnd_spells_runner_test.go`, delete exactly: + +- `TestRunnerDoesNotExtractSpellMentionedOnlyInPartyReference`; +- `TestRunnerCarriesDNDSpellCastWithInvalidSourceRefToSerializedOutput`; and +- `TestRunnerRejectsMalformedDNDSpellsArtifactAtSerializationBoundary`. + +Remove helpers/imports only if unused afterward. Retain +`TestRunnerProcessesSeriatimInputWithDNDSpellsExtractor` and +`TestRunnerPassesPartyAndGlossaryReferencesToDNDSpellsPrompt` as the two +representative cross-family workflows. + +### 2.4 Stage validation + +Run: + +```sh +go test ./internal/framework/contracts ./internal/framework/llm ./internal/framework/validate +go test ./internal/modules/generic/normalize/noop ./internal/modules/integration ./internal/modules/dnd/... +``` + +Stage 2 is complete when all named low-value tests are gone, retained owners +still pass, and no production file changed. + +## Stage 3 - Consolidate configuration, CLI state, and resolution ownership + +### 3.1 Version 3 configuration tests + +Before deleting `internal/core/config/v3_test.go`, preserve its only distinct +protections: + +- add a version-2 input to the strict file-decoding cases in + `file_config_contract_test.go`; require rejection and the migration category, + not the complete diagnostic; and +- ensure `env_contract_test.go` positively proves that empty configured cache + roots resolve to distinct `notarius/chunk-plans` and + `notarius/checkpoints` descendants of the supplied per-user cache root. + +Then delete `v3_test.go`. Do not duplicate its defaults, precedence, +redaction, invalid-source, or removed-field cases elsewhere. + +### 3.2 CLI state and maintained examples + +Move `emptyLookup` from `internal/cli/state_surfaces_test.go` to +`internal/cli/contract_test_helpers_test.go`, then delete +`state_surfaces_test.go` in full. Confirm its remaining behavior is still owned +by command/run contracts and `state_hardening_test.go`; do not transplant its +tests. + +In `internal/cli/production_contract_test.go`: + +- keep `TestDefaultCLICompositionResolvesMaintainedConfigurations`, but rename + it to `TestDefaultCLICompositionValidatesRepresentativeConfiguration` and + reduce it to one representative `config validate` command using empty/default + `Options`; this test owns fallback production composition, not + maintained-example enumeration; +- remove maintained-example resolution from + `TestProductionCatalogCoversMaintainedConfigurations` while retaining + required production registry members, typed codec/variant wiring, catalog + conversion, and the exact documented spell validator chain; and +- remove the maintained-example loop from + `TestProductionConfigValidationCoversModuleAndVariantFailures`, retaining one + valid baseline and each distinct failure mutation. + +Do not weaken `example_contract_test.go`; it remains the sole owner for loading, +resolving, listing, and executing the maintained examples. + +### 3.3 Default and Seriatim resolver duplication + +Delete `internal/framework/pipeline/default_modules_test.go` in full. + +Delete `internal/modules/seriatim/input/transcript/config_test.go` in full. +Delete `internal/modules/seriatim/input/transcript/testdata/pipeline.yml` with +it; the fixture is owned only by that deleted test file. Retain adapter parsing, +leaf registration, the Seriatim runner integration, generic resolver tests, +and production examples. + +### 3.4 D&D capability integration + +Rewrite `internal/modules/integration/dnd_spells_config_test.go` to own only two +cross-family capability failures: + +1. removing `source.transcript` from the Seriatim input spec must make the D&D + spell extractor incompatible; and +2. removing `dnd.spell_casts` from the spell extractor spec must make the + append-order merger incompatible. + +Use one compact table over a programmatically constructed profile and the +smallest catalog capable of resolution. Delete the successful-resolution, +stable-digest, and unknown-lane cases. Simplify or remove fixture/catalog +helpers that become unnecessary, but preserve helpers used by the retained +runner integration in sibling test files. + +### 3.5 Stage validation + +Run: + +```sh +go test ./internal/core/config ./internal/cli ./internal/framework/pipeline +go test ./internal/modules/seriatim/... ./internal/modules/integration +``` + +Stage 3 is complete when each behavior has the owner specified above, the +maintained examples are not redundantly enumerated, and all distinct migration, +default-composition, and capability protections remain. + +## Stage 4 - Rewrite brittle assertions at durable behavioral boundaries + +### 4.1 Source validation ownership and diagnostics + +In `internal/core/source/source_test.go`: + +- consolidate malformed `SourceRef` categories under the `ValidateRef` tests; +- reduce `TestValidateDocumentUnitReferences` to one case proving nested + reference failures receive unit/document context and one case proving the + document-only self-reference invariant; +- retain valid documents, required document/unit fields, duplicate IDs, + non-empty units, valid/reversed/missing references, unit lookup, and digest + sensitivity/determinism; and +- replace exact complete error equality with the minimum stable field/category + fragments needed to distinguish each failure. + +Do not add typed production errors during this pass. If stable fragments cannot +distinguish meaningful categories without a production change, retain the +narrowest current assertion and report that limitation rather than changing +production code. + +### 4.2 Private LLM response schemas + +In `internal/modules/dnd/chunk/scenes/schema_test.go`, replace the nested +`map[string]any` schema-structure walk with actual JSON Schema validation. +Add a small test helper that parses the instance and schema with +`jsonschema.UnmarshalJSON`, registers the schema with +`jsonschema.NewCompiler().AddResource`, compiles it, and calls +`schema.Validate`, matching the existing production validator boundary. Use a +representative valid scene response and mutations that reject: + +- obsolete segment-based boundary fields; +- non-positive start/end unit IDs; +- invalid `primary_mode` and `boundary_confidence` values; +- empty boundary caveats; and +- unknown properties. + +Retain identity/hash validity, DTO integer decoding, and mutation safety. + +In `internal/modules/dnd/extract/spells/schema_test.go`, validate one legal +private LLM response whose references omit `source_id`, and prove that adding a +canonical `source_id` is rejected. Retain response identity/hash, mutation +safety, and diagnostics non-leakage. Do not conflate this private transport +schema with the durable codec schema or alter the maintained durable fixture. + +### 4.3 Framework schema enumeration + +In `internal/framework/llm/schema_registry_test.go`, remove only the assertion +that `RegisteredResponseSchemas` has exactly two entries. Continue to assert: + +- returned keys are sorted; +- both required framework test schemas are present and valid; +- returned bytes are mutation-safe; +- diagnostics omit raw schema content; and +- D&D schemas are not registered in the domain-neutral framework registry. + +### 4.4 Stage validation + +Run: + +```sh +go test ./internal/core/source ./internal/framework/llm +go test ./internal/modules/dnd/chunk/scenes ./internal/modules/dnd/extract/spells ./internal/modules/dnd/codec/spells +``` + +Stage 4 is complete when failures express behavioral categories, private +schemas are validated by accepted/rejected instances, and durable schema +compatibility coverage remains unchanged. + +## Stage 5 - Make composition and architecture checks extension-friendly + +### 5.1 Family registrars + +Rewrite the success assertions in: + +- `internal/modules/generic/register/register_test.go`; +- `internal/modules/seriatim/register/register_test.go`; and +- `internal/modules/dnd/register/register_test.go`. + +Use required-membership helpers rather than exact equality for registered keys +and asset names. Prove representative family-owned entries are retrievable or +buildable through their registry boundary. Preserve: + +- validation of all required registry/asset dependencies before any mutation; +- contextual failure on duplicate family registration; +- absence of cross-family composition where that is an explicit architectural + ownership rule; and +- exact D&D spell default-validator order, because it is documented production + policy. + +Do not delete leaf spec/constructor/registration tests and do not move the CLI +production catalog into family tests. + +### 5.2 Central import-boundary enforcement + +Move the two rules from +`internal/framework/chunkplan/import_boundaries_test.go` into +`internal/modules/import_boundaries_test.go`: + +- production files under `internal/core/source` may import neither + `internal/framework` nor `internal/modules`; and +- production files under `internal/framework/chunkplan` may not import + `internal/modules`. + +Extend the centralized checker so its repository walk enforces those rules. +Add rule-level synthetic cases for both allowed and forbidden imports, using +the existing table/checker style. Retain the existing rejected fixture that +proves generic-to-concrete enforcement. Once both unique rules and checker +activation are protected centrally, delete +`internal/framework/chunkplan/import_boundaries_test.go`. + +### 5.3 Internal documentation routing + +Update only the `Tests To Inspect` routing needed to match the final suite in: + +- `docs/internal/state.md`; +- `docs/internal/pipeline.md`; +- `docs/internal/modules.md`; and +- `docs/internal/llm.md`. + +Replace deleted or nonexistent names such as +`internal/cli/state_surfaces_test.go` and `internal/cli/run_test.go` with concise +links or paths to the retained command/run contracts, state-hardening, +production-composition, maintained-example, checkpoint filesystem, and +cross-family integration owners. Do not create an exhaustive test inventory or +repeat subsystem contracts owned elsewhere. + +### 5.4 Stage validation + +Run: + +```sh +go test ./internal/modules/... ./internal/framework/chunkplan +go test ./internal/cli ./internal/framework/pipeline ./internal/framework/llm +``` + +Stage 5 is complete when legitimate family additions no longer require closed +inventory edits, all architectural rules remain executable in one checker, and +internal documentation names only existing retained owners. + +## Stage 6 - Repository acceptance and handoff + +1. Review the final diff against every required finding and retained-owner + statement in [Test Suite Policy Review](tests.md). Confirm that no production + `.go` file changed. +2. Run formatting on changed Go test files, then run: + + ```sh + go test ./... + go vet ./... + go build ./cmd/notarius + go test -race ./internal/framework/pipeline ./internal/framework/llm ./internal/framework/checkpoint ./internal/cli ./internal/modules/integration + go test ./... -shuffle=on -count=5 + ``` + +3. Generate one coverage report for investigation. Confirm the new checkpoint + tests execute source, extract, merge, normalize, manifest-validation, and + corruption paths. Do not compare the percentage to the review baseline and + do not add tests merely to increase it. +4. Check documentation links and `git diff --check`. Confirm no fixtures were + updated automatically and no secrets or external-service requirements were + introduced. + +Implementation is complete when: + +- every checkpoint stage has a real filesystem round trip and corrupted or + incompatible state is never silently reused; +- every deletion or rewrite in the feature roadmap is complete while its named + retained owner still passes; +- maintained examples have one example-contract owner plus one narrow default + production-composition smoke path; +- registrar and architecture checks permit legitimate extension without + weakening documented ownership or validator order; +- the full suite is deterministic, offline, credential-free, race-clean, and + diagnostically useful; +- internal documentation points to existing test owners; and +- all validation commands pass with no production behavior change. + +## Open Questions + +None. The feature roadmap contains enough evidence and policy decisions to +implement every stage without additional testing-policy choices. If a new test +reveals a production defect, that is a scope boundary rather than an open +planning question: stop the affected stage and request explicit authorization +before changing production behavior. diff --git a/docs/roadmap/tests.md b/docs/roadmap/tests.md index 9ab74ae..3334458 100644 --- a/docs/roadmap/tests.md +++ b/docs/roadmap/tests.md @@ -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. diff --git a/internal/framework/checkpoint/filesystem_test.go b/internal/framework/checkpoint/filesystem_test.go new file mode 100644 index 0000000..fd0dbf0 --- /dev/null +++ b/internal/framework/checkpoint/filesystem_test.go @@ -0,0 +1,415 @@ +package checkpoint + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "gitea.maximumdirect.net/eric/notarius/internal/core/source" + "gitea.maximumdirect.net/eric/notarius/internal/framework/contracts" + "gitea.maximumdirect.net/eric/notarius/internal/framework/pipeline" +) + +type filesystemCheckpointFixture struct { + root string + identity Identity + loader pipeline.CheckpointLoader + doc source.SourceDocument + extract pipeline.CheckpointArtifact + merge pipeline.CheckpointArtifact + normalize pipeline.CheckpointArtifact + dependencies []pipeline.CheckpointFingerprint + warnings []contracts.Warning + rejected []contracts.RejectedOutput +} + +func TestFilesystemCheckpointRoundTripsAllStages(t *testing.T) { + fixture := seedFilesystemCheckpoints(t) + + // Recording owns its inputs. These mutations must not change the durable values. + fixture.doc.Units[0].Text = "caller mutation" + fixture.doc.Metadata["owner"] = "caller mutation" + fixture.extract.Artifact.Content[0] = 'x' + fixture.extract.Artifact.Metadata["content"] = "caller mutation" + fixture.merge.Artifact.Content[0] = 'x' + fixture.normalize.Artifact.Content[0] = 'x' + fixture.warnings[0].Message = "caller mutation" + fixture.rejected[0].Message = "caller mutation" + + t.Run("source", func(t *testing.T) { + got, decision := fixture.loader.Source("source-module") + if !decision.Reused { + t.Fatalf("source decision = %#v", decision) + } + if got.Document == nil || got.Document.ID != "document-1" || got.Document.Units[0].Text != "original source" || got.Document.Metadata["owner"] != "fixture" { + t.Fatalf("source was not restored: %#v", got) + } + + got.Document.Units[0].Text = "loaded mutation" + got.Document.Metadata["owner"] = "loaded mutation" + reloaded, decision := fixture.loader.Source("source-module") + if !decision.Reused || reloaded.Document.Units[0].Text != "original source" || reloaded.Document.Metadata["owner"] != "fixture" { + t.Fatalf("source reload changed after loaded mutation: %#v decision=%#v", reloaded, decision) + } + }) + + t.Run("extract", func(t *testing.T) { + got, decision := fixture.loader.Extract("lane-a", "extract-module", fixture.dependencies) + if !decision.Reused || len(got.Outputs) != 1 || len(got.Rejected) != 1 || len(got.Warnings) != 1 { + t.Fatalf("extract result=%#v decision=%#v", got, decision) + } + output := got.Outputs[0] + if !bytes.Equal(output.Artifact.Content, []byte(`{"spell":"fire"}`)) || output.Artifact.Kind != "spell" || output.Artifact.Schema.ID != "spell-schema" || output.Artifact.Schema.Version != "1" || output.Artifact.MediaType != "application/json" || output.Artifact.Metadata["chunk"] != "chunk-a" || output.ChunkRef.StartUnitID != 1 || got.Warnings[0].ReasonCode != "partial" || got.Rejected[0].ReasonCode != "invalid_source" { + t.Fatalf("extract values were not restored: %#v", got) + } + manifest := readManifest[ExtractLaneManifest](t, filepath.Join(fixture.root, mustRelativePath(t, fixture.identity), "extract", "lane-a", "manifest.json")) + if manifest.Status != StatusSucceededWithRejections { + t.Fatalf("extract status = %q, want succeeded with rejections", manifest.Status) + } + + got.Outputs[0].Artifact.Content[0] = 'y' + got.Warnings[0].Message = "loaded mutation" + reloaded, decision := fixture.loader.Extract("lane-a", "extract-module", fixture.dependencies) + if !decision.Reused || !bytes.Equal(reloaded.Outputs[0].Artifact.Content, []byte(`{"spell":"fire"}`)) || reloaded.Warnings[0].Message != "partial output" { + t.Fatalf("extract reload changed after loaded mutation: %#v decision=%#v", reloaded, decision) + } + }) + + for _, tt := range []struct { + name string + load func() (pipeline.CheckpointArtifact, []contracts.Warning, pipeline.CheckpointDecision) + want []byte + }{ + {name: "merge", load: func() (pipeline.CheckpointArtifact, []contracts.Warning, pipeline.CheckpointDecision) { + got, decision := fixture.loader.Merge("lane-a", "merge-module", fixture.dependencies) + return got.Output, got.Warnings, decision + }, want: []byte(`{"spells":["fire"]}`)}, + {name: "normalize", load: func() (pipeline.CheckpointArtifact, []contracts.Warning, pipeline.CheckpointDecision) { + got, decision := fixture.loader.Normalize("lane-a", "normalize-module", fixture.dependencies) + return got.Output, got.Warnings, decision + }, want: []byte(`{"spells":["fire"],"normalized":true}`)}, + } { + t.Run(tt.name, func(t *testing.T) { + got, warnings, decision := tt.load() + if !decision.Reused || !bytes.Equal(got.Artifact.Content, tt.want) || got.Artifact.Kind != "spell" || got.Artifact.Schema.ID != "spell-schema" || got.Artifact.Schema.Version != "1" || got.Artifact.Metadata["lane"] != "lane-a" || len(warnings) != 1 || warnings[0].ReasonCode != "review" { + t.Fatalf("%s result=%#v warnings=%#v decision=%#v", tt.name, got, warnings, decision) + } + + got.Artifact.Content[0] = 'z' + reloaded, warnings, decision := tt.load() + if !decision.Reused || !bytes.Equal(reloaded.Artifact.Content, tt.want) || warnings[0].Message != "review manually" { + t.Fatalf("%s reload changed after loaded mutation: %#v warnings=%#v decision=%#v", tt.name, reloaded, warnings, decision) + } + }) + } + + if runtime.GOOS != "windows" { + t.Run("restrictive permissions", func(t *testing.T) { + root := filepath.Join(fixture.root, mustRelativePath(t, fixture.identity)) + err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + want := os.FileMode(0o600) + if info.IsDir() { + want = 0o700 + } + if got := info.Mode().Perm(); got != want { + t.Errorf("%s permissions = %o, want %o", path, got, want) + } + return nil + }) + if err != nil { + t.Fatal(err) + } + }) + } +} + +func TestFilesystemCheckpointRejectsMissingAndCorruptState(t *testing.T) { + for _, stage := range checkpointStages() { + t.Run(stage.name+" missing manifest", func(t *testing.T) { + fixture := seedFilesystemCheckpoints(t) + if err := os.Remove(stage.manifest(fixture)); err != nil { + t.Fatal(err) + } + assertStageNotReused(t, stage, fixture, "missing") + }) + + t.Run(stage.name+" malformed manifest", func(t *testing.T) { + fixture := seedFilesystemCheckpoints(t) + if err := os.WriteFile(stage.manifest(fixture), []byte("{"), 0o600); err != nil { + t.Fatal(err) + } + assertStageNotReused(t, stage, fixture, "decode") + }) + } +} + +func TestFilesystemCheckpointRejectsIncompatibleManifests(t *testing.T) { + for _, tt := range []struct { + name string + edit func(map[string]any) + want string + }{ + {"v1 schema", func(m map[string]any) { m["workspace_schema_version"] = WorkspaceSchemaVersionV1 }, "workspace schema"}, + {"unknown schema", func(m map[string]any) { m["workspace_schema_version"] = "notarius.workspace.future" }, "workspace schema"}, + {"identity", func(m map[string]any) { m["metadata"].(map[string]any)["checkpoint_identity_digest"] = "sha256:other" }, "identity"}, + {"stage", func(m map[string]any) { m["stage"] = string(StageMerge) }, "stage"}, + {"lane", func(m map[string]any) { m["lane_id"] = "lane-other" }, "lane"}, + {"module", func(m map[string]any) { m["module_key"] = "module-other" }, "module"}, + {"dependency", func(m map[string]any) { + m["dependency_fingerprints"] = []map[string]string{{"name": "input", "value": "other"}} + }, "dependency"}, + } { + t.Run(tt.name, func(t *testing.T) { + fixture := seedFilesystemCheckpoints(t) + editManifest(t, checkpointStages()[1].manifest(fixture), tt.edit) + assertStageNotReused(t, checkpointStages()[1], fixture, tt.want) + }) + } +} + +func TestFilesystemCheckpointRejectsNonTerminalStatuses(t *testing.T) { + for _, status := range []StageStatus{StatusRunning, StatusFailed, StatusPending, StatusInvalidated} { + t.Run(string(status), func(t *testing.T) { + fixture := seedFilesystemCheckpoints(t) + editManifest(t, checkpointStages()[1].manifest(fixture), func(m map[string]any) { m["status"] = string(status) }) + assertStageNotReused(t, checkpointStages()[1], fixture, "status") + }) + } +} + +func TestFilesystemCheckpointRejectsIncompleteArtifactsAndContent(t *testing.T) { + for _, tt := range []struct { + name string + edit func(map[string]any) + want string + }{ + {"artifact kind", func(m map[string]any) { m["outputs"].([]any)[0].(map[string]any)["artifact_kind"] = "" }, "artifact codec identity"}, + {"schema id", func(m map[string]any) { m["outputs"].([]any)[0].(map[string]any)["schema"].(map[string]any)["id"] = "" }, "artifact codec identity"}, + {"schema version", func(m map[string]any) { + m["outputs"].([]any)[0].(map[string]any)["schema"].(map[string]any)["version"] = "" + }, "artifact codec identity"}, + {"schema digest", func(m map[string]any) { m["outputs"].([]any)[0].(map[string]any)["schema_digest"] = "" }, "artifact codec identity"}, + {"base64", func(m map[string]any) { + m["outputs"].([]any)[0].(map[string]any)["content"].(map[string]any)["content_base64"] = "%" + }, "base64"}, + {"content digest", func(m map[string]any) { + m["outputs"].([]any)[0].(map[string]any)["content"].(map[string]any)["content_digest"] = "sha256:other" + }, "content digest"}, + } { + t.Run(tt.name, func(t *testing.T) { + fixture := seedFilesystemCheckpoints(t) + editJSON(t, filepath.Join(fixture.root, mustRelativePath(t, fixture.identity), "extract", "lane-a", "outputs.json"), tt.edit) + assertStageNotReused(t, checkpointStages()[1], fixture, tt.want) + }) + } +} + +func TestFilesystemCheckpointRejectsSourceAndOutputDigestMismatches(t *testing.T) { + t.Run("invalid source document", func(t *testing.T) { + fixture := seedFilesystemCheckpoints(t) + editJSON(t, filepath.Join(fixture.root, mustRelativePath(t, fixture.identity), "source", "source-document.json"), func(m map[string]any) { + m["document"].(map[string]any)["units"].([]any)[0].(map[string]any)["text"] = "" + }) + assertStageNotReused(t, checkpointStages()[0], fixture, "source checkpoint document") + }) + + t.Run("source output digest", func(t *testing.T) { + fixture := seedFilesystemCheckpoints(t) + editJSON(t, filepath.Join(fixture.root, mustRelativePath(t, fixture.identity), "source", "source-document.json"), func(m map[string]any) { + m["document"].(map[string]any)["digest"] = "sha256:other" + }) + assertStageNotReused(t, checkpointStages()[0], fixture, "output digest") + }) + + for _, stage := range checkpointStages()[1:] { + t.Run(stage.name+" output digest", func(t *testing.T) { + fixture := seedFilesystemCheckpoints(t) + editManifest(t, stage.manifest(fixture), func(m map[string]any) { + m["output_digests"].([]any)[0].(map[string]any)["value"] = "sha256:other" + }) + assertStageNotReused(t, stage, fixture, "output digest") + }) + } +} + +func TestFilesystemCheckpointReusesExtractWithRejections(t *testing.T) { + fixture := seedFilesystemCheckpoints(t) + result, decision := fixture.loader.Extract("lane-a", "extract-module", fixture.dependencies) + if !decision.Reused || len(result.Rejected) != 1 || result.Rejected[0].Message != "source reference is invalid" { + t.Fatalf("result=%#v decision=%#v", result, decision) + } +} + +type checkpointStage struct { + name string + manifest func(filesystemCheckpointFixture) string + load func(filesystemCheckpointFixture) pipeline.CheckpointDecision +} + +func checkpointStages() []checkpointStage { + return []checkpointStage{ + { + name: "source", + manifest: func(f filesystemCheckpointFixture) string { + return filepath.Join(f.root, mustRelativePathForTest(f.identity), "source", "manifest.json") + }, + load: func(f filesystemCheckpointFixture) pipeline.CheckpointDecision { + _, d := f.loader.Source("source-module") + return d + }, + }, + { + name: "extract", + manifest: func(f filesystemCheckpointFixture) string { + return filepath.Join(f.root, mustRelativePathForTest(f.identity), "extract", "lane-a", "manifest.json") + }, + load: func(f filesystemCheckpointFixture) pipeline.CheckpointDecision { + _, d := f.loader.Extract("lane-a", "extract-module", f.dependencies) + return d + }, + }, + { + name: "merge", + manifest: func(f filesystemCheckpointFixture) string { + return filepath.Join(f.root, mustRelativePathForTest(f.identity), "merge", "lane-a", "manifest.json") + }, + load: func(f filesystemCheckpointFixture) pipeline.CheckpointDecision { + _, d := f.loader.Merge("lane-a", "merge-module", f.dependencies) + return d + }, + }, + { + name: "normalize", + manifest: func(f filesystemCheckpointFixture) string { + return filepath.Join(f.root, mustRelativePathForTest(f.identity), "normalize", "lane-a", "manifest.json") + }, + load: func(f filesystemCheckpointFixture) pipeline.CheckpointDecision { + _, d := f.loader.Normalize("lane-a", "normalize-module", f.dependencies) + return d + }, + }, + } +} + +func seedFilesystemCheckpoints(t *testing.T) filesystemCheckpointFixture { + t.Helper() + root := t.TempDir() + identity, err := NewIdentity(representativeIdentityInput()) + if err != nil { + t.Fatal(err) + } + recorder, err := NewFilesystemRecorder(root, identity) + if err != nil { + t.Fatal(err) + } + fixture := filesystemCheckpointFixture{ + root: root, + identity: identity, + doc: checkpointDocument(), + extract: checkpointArtifact("extract", `{"spell":"fire"}`), + merge: checkpointArtifact("merge", `{"spells":["fire"]}`), + normalize: checkpointArtifact("normalize", `{"spells":["fire"],"normalized":true}`), + dependencies: []pipeline.CheckpointFingerprint{{Name: "source", Value: "sha256:source"}, {Name: "chunk-plan", Value: "sha256:plan"}}, + warnings: []contracts.Warning{{Scope: "extract", ReasonCode: "partial", Message: "partial output"}}, + rejected: []contracts.RejectedOutput{{Stage: "extract", LaneID: "lane-a", ModuleKey: "extract-module", ChunkID: "chunk-a", ValidatorName: "source_refs", ReasonCode: "invalid_source", Message: "source reference is invalid", AttemptCount: 1}}, + } + if err := recorder.SourceSucceeded("source-module", &fixture.doc); err != nil { + t.Fatal(err) + } + if err := recorder.ExtractSucceeded("lane-a", "extract-module", fixture.dependencies, []pipeline.CheckpointArtifact{fixture.extract}, fixture.rejected, fixture.warnings); err != nil { + t.Fatal(err) + } + mergeWarnings := []contracts.Warning{{Scope: "merge", ReasonCode: "review", Message: "review manually"}} + if err := recorder.MergeSucceeded("lane-a", "merge-module", fixture.dependencies, fixture.merge, mergeWarnings); err != nil { + t.Fatal(err) + } + if err := recorder.NormalizeSucceeded("lane-a", "normalize-module", fixture.dependencies, fixture.normalize, mergeWarnings); err != nil { + t.Fatal(err) + } + fixture.loader, err = NewFilesystemLoader(root, identity) + if err != nil { + t.Fatal(err) + } + return fixture +} + +func checkpointDocument() source.SourceDocument { + return source.SourceDocument{ + ID: "document-1", Kind: "transcript", Format: "text", Digest: "sha256:document", Metadata: map[string]any{"owner": "fixture", "number": float64(1)}, + Units: []source.SourceUnit{{ID: 1, Kind: "line", Text: "original source", Ref: source.SourceRef{SourceID: "document-1", StartUnitID: 1, EndUnitID: 1}, Metadata: map[string]any{"speaker": "narrator"}}}, + } +} + +func checkpointArtifact(module, content string) pipeline.CheckpointArtifact { + return pipeline.CheckpointArtifact{ + LaneID: "lane-a", ModuleKey: module, SourceID: "document-1", ChunkID: "chunk-a", ChunkIndex: 0, + ChunkRef: source.SourceRef{SourceID: "document-1", StartUnitID: 1, EndUnitID: 1}, SchemaDigest: "sha256:schema", + Artifact: contracts.SerializedArtifact{Kind: "spell", Schema: contracts.ArtifactSchema{ID: "spell-schema", Name: "Spell", Version: "1", JSONSchema: []byte(`{"type":"object"}`)}, MediaType: "application/json", Content: []byte(content), Metadata: map[string]any{"chunk": "chunk-a", "lane": "lane-a"}}, + } +} + +func assertStageNotReused(t *testing.T, stage checkpointStage, fixture filesystemCheckpointFixture, want string) { + t.Helper() + decision := stage.load(fixture) + if decision.Reused || !strings.Contains(strings.ToLower(decision.Reason), strings.ToLower(want)) { + t.Fatalf("%s decision=%#v, want non-reused reason containing %q", stage.name, decision, want) + } +} + +func editManifest(t *testing.T, path string, edit func(map[string]any)) { + t.Helper() + editJSON(t, path, edit) +} + +func editJSON(t *testing.T, path string, edit func(map[string]any)) { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + var value map[string]any + if err := json.Unmarshal(data, &value); err != nil { + t.Fatal(err) + } + edit(value) + data, err = json.Marshal(value) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatal(err) + } +} + +func readManifest[T any](t *testing.T, path string) T { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + var value T + if err := json.Unmarshal(data, &value); err != nil { + t.Fatal(err) + } + return value +} + +func mustRelativePath(t *testing.T, identity Identity) string { + t.Helper() + return mustRelativePathForTest(identity) +} + +func mustRelativePathForTest(identity Identity) string { + path, err := identity.RelativePath() + if err != nil { + panic(err) + } + return path +} diff --git a/internal/framework/checkpoint/identity_test.go b/internal/framework/checkpoint/identity_test.go new file mode 100644 index 0000000..669afb6 --- /dev/null +++ b/internal/framework/checkpoint/identity_test.go @@ -0,0 +1,202 @@ +package checkpoint + +import ( + "path/filepath" + "reflect" + "strings" + "testing" + + "gitea.maximumdirect.net/eric/notarius/internal/core/artifacts" + "gitea.maximumdirect.net/eric/notarius/internal/framework/pipeline" +) + +func TestNewIdentityNormalizesOrderAndEmptyValues(t *testing.T) { + base := representativeIdentityInput() + identity, err := NewIdentity(base) + if err != nil { + t.Fatal(err) + } + + for _, tt := range []struct { + name string + mutate func(*IdentityInput) + }{ + {"selected lanes", func(v *IdentityInput) { v.SelectedLanes = []string{"lane-a", "lane-b"} }}, + {"runtime fingerprints", func(v *IdentityInput) { + v.RuntimeOverrides = []Fingerprint{{Name: "model", Value: "large"}, {Name: "timeout", Value: "30s"}} + }}, + {"references", func(v *IdentityInput) { + v.References = []artifacts.ReferenceProvenance{v.References[1], v.References[0]} + }}, + {"provenance fingerprints", func(v *IdentityInput) { + v.ProvenanceFingerprints = []Fingerprint{{Name: "source", Value: "v2"}, {Name: "runner", Value: "v1"}} + }}, + {"resolved lanes", func(v *IdentityInput) { + v.Pipeline.ArtifactLanes = []pipeline.ResolvedArtifactLane{v.Pipeline.ArtifactLanes[1], v.Pipeline.ArtifactLanes[0]} + }}, + } { + t.Run(tt.name, func(t *testing.T) { + changed := cloneIdentityInput(base) + tt.mutate(&changed) + got, err := NewIdentity(changed) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(identity, got) { + t.Fatalf("reordered identity differs:\nbase=%#v\ngot=%#v", identity, got) + } + }) + } + + t.Run("duplicates and blanks are ignored", func(t *testing.T) { + changed := base + changed.SelectedLanes = []string{" ", "lane-b", "lane-a", "lane-a", ""} + changed.RuntimeOverrides = append(changed.RuntimeOverrides, Fingerprint{}, Fingerprint{Name: " ", Value: "ignored"}, Fingerprint{Name: "timeout", Value: "30s"}) + changed.ProvenanceFingerprints = append(changed.ProvenanceFingerprints, Fingerprint{}, Fingerprint{Name: "", Value: "ignored"}) + + got, err := NewIdentity(changed) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(identity, got) { + t.Fatalf("empty or duplicate values changed identity:\nbase=%#v\ngot=%#v", identity, got) + } + }) +} + +func TestNewIdentityChangesForMeaningfulInputs(t *testing.T) { + base := representativeIdentityInput() + original, err := NewIdentity(base) + if err != nil { + t.Fatal(err) + } + + cases := map[string]func(*IdentityInput){ + "pipeline id": func(v *IdentityInput) { v.Pipeline.ID = "another-pipeline" }, + "pipeline digest": func(v *IdentityInput) { v.Pipeline.Digest = "sha256:pipeline-digest-2" }, + "input key": func(v *IdentityInput) { v.InputKey = "another-input" }, + "raw input digest": func(v *IdentityInput) { v.RawInputDigest = "sha256:raw-input-2" }, + "source digest": func(v *IdentityInput) { v.SourceDigest = "sha256:source-2" }, + "selected lanes": func(v *IdentityInput) { v.SelectedLanes = []string{"lane-a"} }, + "runtime override": func(v *IdentityInput) { v.RuntimeOverrides[0].Value = "60s" }, + "reference digest": func(v *IdentityInput) { v.References[0].Digest = "sha256:reference-2" }, + "reference identity": func(v *IdentityInput) { v.References[0].OriginURI = "file:///other-reference" }, + "provenance": func(v *IdentityInput) { v.ProvenanceFingerprints[0].Value = "v3" }, + } + + for name, mutate := range cases { + t.Run(name, func(t *testing.T) { + changed := cloneIdentityInput(base) + mutate(&changed) + got, err := NewIdentity(changed) + if err != nil { + t.Fatal(err) + } + if got.Digest == original.Digest { + t.Fatalf("meaningful %s input did not change digest %q", name, got.Digest) + } + }) + } +} + +func TestNewIdentityUsesResolvedInputWhenKeyIsOmitted(t *testing.T) { + input := representativeIdentityInput() + input.InputKey = "" + + identity, err := NewIdentity(input) + if err != nil { + t.Fatal(err) + } + if identity.InputKey != input.Pipeline.Input.Module { + t.Fatalf("input key = %q, want resolved module %q", identity.InputKey, input.Pipeline.Input.Module) + } +} + +func TestNewIdentityRejectsMissingRequiredInputs(t *testing.T) { + cases := []struct { + name string + mutate func(*IdentityInput) + want string + }{ + {"pipeline id", func(v *IdentityInput) { v.Pipeline.ID = "" }, "pipeline id"}, + {"pipeline digest", func(v *IdentityInput) { v.Pipeline.Digest = "" }, "pipeline digest"}, + {"input key", func(v *IdentityInput) { v.InputKey = ""; v.Pipeline.Input = pipeline.Binding("") }, "input key"}, + {"input digests", func(v *IdentityInput) { v.RawInputDigest = ""; v.SourceDigest = "" }, "raw input digest or source digest"}, + } + + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + input := representativeIdentityInput() + tt.mutate(&input) + _, err := NewIdentity(input) + if err == nil || !strings.Contains(err.Error(), tt.want) { + t.Fatalf("error = %v, want category containing %q", err, tt.want) + } + }) + } +} + +func TestIdentityRelativePathIsDeterministicAndConfined(t *testing.T) { + identity, err := NewIdentity(representativeIdentityInput()) + if err != nil { + t.Fatal(err) + } + + first, err := identity.RelativePath() + if err != nil { + t.Fatal(err) + } + second, err := identity.RelativePath() + if err != nil { + t.Fatal(err) + } + if first != second { + t.Fatalf("relative path is not deterministic: %q != %q", first, second) + } + if filepath.IsAbs(first) || filepath.Clean(first) != first || filepath.ToSlash(first) != first { + t.Fatalf("path is not a clean relative slash-separated path: %q", first) + } + if strings.Contains(first, "../") || strings.HasPrefix(first, "../") || strings.Contains(first, `\\`) { + t.Fatalf("path escapes its root: %q", first) + } + parts := strings.Split(first, "/") + if len(parts) != 4 || parts[0] != "pipeline" || !strings.HasPrefix(parts[1], "input-") { + t.Fatalf("path does not contain the documented identity hierarchy: %q", first) + } + if !strings.Contains(parts[1], "source-digest") || !strings.Contains(parts[2], "pipeline-digest") || parts[3] == "" { + t.Fatalf("path omits digest-derived hierarchy: %q", first) + } +} + +func representativeIdentityInput() IdentityInput { + return IdentityInput{ + Pipeline: pipeline.ResolvedPipeline{ + ID: "pipeline", + Digest: "sha256:pipeline-digest-000000000000", + Input: pipeline.Binding("input"), + ArtifactLanes: []pipeline.ResolvedArtifactLane{ + {ID: "lane-b"}, + {ID: "lane-a"}, + }, + }, + InputKey: "input", + RawInputDigest: "sha256:raw-input-000000000000", + SourceDigest: "sha256:source-digest-000000000000", + SelectedLanes: []string{"lane-b", "lane-a"}, + RuntimeOverrides: []Fingerprint{{Name: "timeout", Value: "30s"}, {Name: "model", Value: "large"}}, + References: []artifacts.ReferenceProvenance{ + {Stage: "chunk", SlotName: "glossary", OriginURI: "file:///glossary", Digest: "sha256:reference-1"}, + {Stage: "extract", LaneID: "lane-a", SlotName: "party", OriginURI: "file:///party", Digest: "sha256:reference-2"}, + }, + ProvenanceFingerprints: []Fingerprint{{Name: "runner", Value: "v1"}, {Name: "source", Value: "v2"}}, + } +} + +func cloneIdentityInput(input IdentityInput) IdentityInput { + input.SelectedLanes = append([]string(nil), input.SelectedLanes...) + input.RuntimeOverrides = append([]Fingerprint(nil), input.RuntimeOverrides...) + input.References = append([]artifacts.ReferenceProvenance(nil), input.References...) + input.ProvenanceFingerprints = append([]Fingerprint(nil), input.ProvenanceFingerprints...) + input.Pipeline.ArtifactLanes = append([]pipeline.ResolvedArtifactLane(nil), input.Pipeline.ArtifactLanes...) + return input +}