From 60b86dc40cdab156e548563070b3c5196f2aa7ed Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 17 Jul 2026 11:29:52 -0500 Subject: [PATCH] Finish the domain pipeline cleanup --- docs/internal/modules.md | 15 +- docs/internal/pipeline.md | 18 +- docs/operations.md | 4 +- docs/roadmap/domain.md | 348 --------- docs/roadmap/implementation.md | 723 ------------------ internal/cli/run_test.go | 5 +- internal/core/config/validation_test.go | 3 + internal/framework/contracts/artifact.go | 4 + .../pipeline/artifact_codec_registry.go | 21 +- .../pipeline/artifact_codec_registry_test.go | 56 +- internal/framework/pipeline/debug.go | 13 +- .../pipeline/default_modules_test.go | 5 +- internal/framework/pipeline/runner.go | 4 + .../pipeline/runner_terminal_debug_test.go | 28 + internal/modules/import_boundaries_test.go | 94 ++- .../seriatim/input/transcript/config_test.go | 3 + 16 files changed, 214 insertions(+), 1130 deletions(-) delete mode 100644 docs/roadmap/domain.md delete mode 100644 docs/roadmap/implementation.md diff --git a/docs/internal/modules.md b/docs/internal/modules.md index f8124a7..2ae7268 100644 --- a/docs/internal/modules.md +++ b/docs/internal/modules.md @@ -196,12 +196,15 @@ point for specializing reusable generic implementations, while the generic registrar composes only generic children. Core and framework production packages do not import production extensions. -CLI production code imports only exact family registrar packages. Compatibility -tests in the CLI, core, and framework trees may import roots and implementation -leaves directly. White-box tests within module families retain the production -family boundaries. `internal/modules/integration` is test infrastructure: its -black-box tests may compose multiple families, but it is not a production -module family or production dependency target. +CLI production code is the sole application composition root for extensions +and imports only exact family registrar packages. Other production packages, +including commands and newly introduced package trees, do not import module +packages directly. Compatibility tests in the CLI, core, and framework trees +may import roots and implementation leaves directly. Other non-module tests do +not receive that exemption. White-box tests within module families retain the +production family boundaries. `internal/modules/integration` is test +infrastructure: its black-box tests may compose multiple families, but it is +not a production module family or production dependency target. ## Adding An Extension diff --git a/docs/internal/pipeline.md b/docs/internal/pipeline.md index f9cb47c..ca9b40b 100644 --- a/docs/internal/pipeline.md +++ b/docs/internal/pipeline.md @@ -236,17 +236,17 @@ and terminal error text; failures before a candidate exists omit that payload. Only LLM calls made by the module operation belong to the module attempt. Validator calls retain independent scopes under `validate/` and are not duplicated into the module envelope. A failed terminal-envelope write is a -framework error and is joined with any primary attempt error. Debug data is -never used as a checkpoint source. Typed artifact debug envelopes are -domain-neutral, redact sensitive metadata and bytes through the common debug -policy, and record codec identity plus schema and content digests. +non-retryable framework error and is joined with any primary attempt error. +Debug data is never used as a checkpoint source. Typed artifact debug envelopes +are domain-neutral, redact sensitive metadata and bytes through the common +debug policy, and record codec identity plus schema and content digests. Merge and normalize attempts serialize their in-memory candidate with the -codec's candidate encoder before typed validation. Serialized validators and -attempt debug use that candidate representation, which carries the codec media -type and schema identity but is never checkpointed or passed downstream. Only -a validator-approved value is encoded through the strict final codec and made -eligible for a checkpoint or stage output. +codec's required candidate encoder before typed validation. Serialized +validators and attempt debug use that candidate representation, which carries +the codec media type and schema identity but is never checkpointed or passed +downstream. Only a validator-approved value is encoded through the strict final +codec and made eligible for a checkpoint or stage output. Checkpoint identity, physical layout, reuse behavior, and debug artifact handling are operator contracts in [Operations](../operations.md). Serialization diff --git a/docs/operations.md b/docs/operations.md index 49f3bd9..026172a 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -155,8 +155,8 @@ envelope recording acceptance, validator rejection, or a module, validator, candidate-serialization, or final-serialization error as applicable. It includes attempt-local warnings and any available candidate or rejection. A failure before a candidate exists has no candidate payload. If the envelope -cannot be persisted, the run reports that debug failure together with any -primary attempt error. +cannot be persisted, the run does not retry that module attempt and reports the +debug failure together with any primary attempt error. Checkpoint-reused chunk, extract, merge, and normalize work retains the stage-level input and output artifacts but has no retry-attempt artifacts diff --git a/docs/roadmap/domain.md b/docs/roadmap/domain.md deleted file mode 100644 index bd9489f..0000000 --- a/docs/roadmap/domain.md +++ /dev/null @@ -1,348 +0,0 @@ -# Domain-Typed Pipeline Feature Roadmap - -## Status - -Implemented on 2026-07-17. This roadmap records the design delivered for -[ADR-0002](../adr/0002-linear-pipes-and-filters-pipeline.md), -[ADR-0003](../adr/0003-typed-interfaces-with-two-zone-data-model.md), and -[ADR-0004](../adr/0004-package-modules-by-domain.md). Its implementation history -is summarized in the [completion record](implementation.md). - -This file is historical design context, not a current-behavior reference. -Implemented contracts and mechanics are defined by the architecture, -configuration, integration, operations, and internal documentation outside -`docs/roadmap/`. - -## User Intent - -Notarius should remain a small, explicit pipes-and-filters application while -making domain extensions safe to compose and straightforward to maintain. A -configured pipeline should fail before execution when its modules are -incompatible, should carry typed domain values rather than reparsed JSON between -artifact stages, and should preserve provenance and durable output contracts. - -The application must also enforce one configurable, process-wide ceiling on -in-flight LLM calls. Pipeline scheduling may impose stricter limits, but no -stage, lane, retry, validator, or future LLM-backed extension may bypass that -global ceiling. - -## Target Architecture - -### Pipeline and outcome model - -The topology remains: - -```text -input -> chunk -> extract -> merge -> normalize -> output -``` - -Input and chunk are pipeline-wide. Extract, merge, normalize, and their -validators operate per artifact lane. Output aggregates the terminal artifacts -from all lanes. The resolved pipeline retains explicit fields for those roles; -it does not become a general DAG or a heterogeneous ordered-stage list. - -Framework errors abort the run. Validator rejection is a recorded domain -outcome and does not abort unrelated work. Accepted extract results reach merge -in source-chunk order, regardless of execution completion order. Warnings, -rejections, artifacts, and reported errors are likewise ordered by stable -pipeline scope rather than goroutine completion time. - -### Engine-owned source model - -The engine owns `source.SourceDocument`, `source.SourceUnit`, `source.SourceRef`, -and `source.Chunk`. Domain modules may consume these types but must not redefine -their provenance semantics. - -- Every source unit has a canonical self-reference identifying its source and - unit range. -- A chunk contains ordered source units and one canonical reference spanning - its first through last unit. -- Chunk and unit references are validated for source identity, order, and - containment. -- Auxiliary reference material remains distinct from source provenance. -- Cloning, canonicalization, checkpointing, debugging, and digest computation - preserve the source references exactly. - -`source.Chunk.Ref` replaces duplicate start/end boundary fields. Because that -changes persisted workspace state, the workspace checkpoint schema advances to -`notarius.workspace.v2`. Existing v1 checkpoints are left intact but treated as -incompatible and recomputed; no in-place migration or deletion is required. - -### Typed artifact lanes - -Each lane has one canonical artifact type `T` from extraction through merge, -normalization, and typed validation. Module-facing Zone-B contracts are generic: - -- `Extractor[T]` produces typed per-chunk values plus framework-owned - provenance and diagnostics; -- `Merger[T]` combines accepted values in source-chunk order; -- `Normalizer[T]` canonicalizes the merged value; and -- `TypedValidator[T]` applies semantic checks at its configured artifact stage. - -The framework may use private erased adapters to keep heterogeneous lanes in -one resolved pipeline, but `any`, raw JSON, and a generic `Process(any)` API are -not module-facing handoffs. The extractor selected for a lane establishes its -artifact kind. Resolution uses that kind to select compatible merger, -normalizer, validator, and codec variants and rejects an incompatible lane -before any stage executes. - -Generic strategies remain reusable without knowing concrete domains. In -particular, append-order merge is parameterized by a typed combine function -provided during domain registration, and no-op normalization is instantiated -for the lane's concrete type. - -### Artifact identity and codecs - -Every typed artifact kind has exactly one registered `ArtifactCodec[T]`. An -artifact kind is a stable logical identifier, separate from a Go type name. A -codec owns: - -- artifact kind; -- schema identifier, name, and version; -- media type and JSON Schema bytes; and -- strict, deterministic encoding and decoding between `T` and the serialized - representation. - -Equal canonical values must encode to equal bytes. Those bytes are the basis -for artifact digests. Codec decoding rejects malformed or schema-incompatible -content. Domain validators continue to own semantic validity; codecs do not -replace them. - -`SerializedArtifact` is the Zone-C representation and includes the artifact -kind, schema metadata, media type, encoded content, and framework metadata. -Type erasure occurs through the codec after normalization for final output. -Intermediate checkpointing and opt-in debug recording may also use the codec, -but serialization for those side effects is not a stage handoff. - -Checkpoint metadata records artifact kind, schema identifier, schema version, -and schema digest. Reuse requires an exact compatible registered codec; -otherwise the checkpoint is safely invalidated. Output remains domain-neutral -and consumes serialized artifacts. - -Generic serialized validators remain supported for representation-level checks -such as valid JSON and JSON Schema validation. The framework encodes `T` through -its registered codec before invoking them. Domain validators receive `T` -directly. Chunk-stage validators remain in the source zone: semantic chunk -validators receive engine-owned chunks, while representation-level validators -receive the framework's canonical serialized chunk view. They do not force -source-zone values through a domain artifact codec. - -### Registration and resolution - -Registries expose typed registration helpers while privately retaining the Go -type identity needed to assemble erased lane executors. - -- Codecs are keyed by artifact kind, with exactly one codec per kind. -- Extractors are keyed by their existing module key and declare an artifact - kind. -- Mergers, normalizers, and validators are keyed by `(module key, artifact - kind)`, allowing stable generic keys such as `appendorder` and `noop` to have - multiple typed specializations. -- Resolved pipeline identity and dependency fingerprints include artifact kind - and schema identity, version, and digest. -- Duplicate or incompatible registrations and selections fail deterministically - during composition or resolution. - -The framework's public typed registration surface uses free generic functions, -because Go methods cannot declare their own type parameters. Private reflection -may verify and erase registered types, but it is not exposed to module authors. - -### Preparation, options, and dependencies - -Pipeline execution is split into resolution, preparation, and running. -Preparation constructs every selected module and validator before source input -begins and returns a prepared pipeline with explicit input, chunk, lane, and -output fields. - -- Construction receives framework-owned dependencies, including the one shared - scheduled structured-LLM client. -- Raw configured options are decoded once into implementation-owned option - structs during preparation. -- Missing dependencies, malformed options, unknown options, and incompatible - typed selections fail before stage execution. -- Configuration validation uses the same option decoders without requiring live - provider dependencies. -- Per-run data such as sources, chunks, references, session identity, lane - identity, and metadata remains in operation requests. -- Constructed implementations that can be scheduled concurrently are immutable - after preparation or otherwise explicitly concurrency-safe. - -Modules and validators must not construct provider clients, wrap their own -independent schedulers, or bypass the injected scheduled client. - -### D&D artifact model - -The D&D package root owns canonical `dnd.SpellList`, `dnd.SpellCast`, and -related evidence types, using engine-owned `source.SourceRef` values. The spell -extractor keeps its LLM response DTO and response schema private and maps the -canonicalized response to the domain model. - -The D&D spell codec separately owns the existing durable spell artifact schema. -The LLM response schema and durable artifact schema remain distinct contracts -even if their current JSON shapes are similar. Shape, source-reference, and -source-relatedness validators operate on the canonical typed model. The -validator-only duplicate spell model and inter-stage JSON reparsing disappear. - -The migration preserves the existing D&D spell payload, logical output bundle, -module and validator keys, prompt/schema identities, default validator chains, -warnings, rejection semantics, and manifest provenance unless a separate -compatibility decision explicitly changes one of those contracts. - -## Package Ownership - -The target production extension layout is: - -```text -internal/modules/dnd/ - types.go - codec/spells/ - chunk/scenes/ - extract/spells/ - validate/spells/shape/ - validate/spells/source_refs/ - validate/spells/source_relatedness/ - shared/ - register/ - -internal/modules/generic/ - chunk/units/ - merge/appendorder/ - normalize/noop/ - validate/always_accept/ - validate/always_reject/ - validate/valid_json/ - validate/valid_json_schema/ - output/json/ - register/ - -internal/modules/seriatim/ - input/transcript/ - register/ - -internal/framework/promptfs/ -``` - -Shared domain types live at the domain root. Registration lives in a sibling -`register` package so that the root never imports child implementations. Each -registrar exposes one composition entry point accepting the pipeline registry -set and LLM asset registry. The CLI composition root creates those registries -and invokes the generic, Seriatim, and D&D registrars. - -Concrete domain implementations do not import peer domains. Generic extensions -never import a concrete domain. A domain registrar may import generic packages -to register typed specializations for its domain. The application composition -root and designated black-box integration tests may compose multiple -registrars. Domain-neutral embedded prompt-asset filesystem support belongs to -the framework rather than a domain package. - -## Concurrency Policy - -### Configuration - -The existing `concurrency.total_llm` setting remains the application-wide -ceiling on actual provider calls. Version-2 configuration gains an extensible -stage-worker map: - -```yaml -concurrency: - total_llm: 4 - stage_workers: - extract: 4 -``` - -Initially, `extract` is the only recognized key. Unknown stage keys are rejected -so misspellings cannot silently alter scheduling. If omitted, the effective -extract worker count equals `total_llm`. Its valid range is -`1..concurrency.total_llm`. The environment override is -`NOTARIUS_STAGE_WORKERS_EXTRACT`; future stage overrides receive similarly -explicit names that map to the extensible file representation. - -The worker setting bounds framework jobs, not provider calls. Only an actual -LLM call consumes a permit from the shared scheduled client. The global -scheduled client remains authoritative even if future stages gain worker limits. - -### Scheduling - -After pipeline-wide chunking, all lanes may run concurrently. A central -dispatcher submits `(lane, chunk)` jobs to one run-wide extract worker pool in -round-robin order: source chunk first, then resolved lane order. This avoids one -unbounded goroutine per job and prevents an early lane from monopolizing the -queue. - -One job contains extraction, its stage-local retry behavior, and extract-stage -validation for that lane and chunk. Each lane begins its serial merge then -normalize continuation when all of its extract jobs reach a terminal state. -Different lanes' continuations may overlap, and any LLM-backed continuation or -validator still shares the global scheduled client. - -Workers publish immutable task results to a coordinator. Only the coordinator -mutates aggregate results, manifests, checkpoint indexes, warnings, and -rejections. Debug artifacts use attempt-specific paths and do not rely on -concurrent writes to shared files. - -Rejections do not cancel work. A framework error cancels the derived run -context, stops undispatched jobs, and waits for started jobs to finish or -observe cancellation. If the parent context was canceled, its error is -returned. Otherwise, internal cancellation errors are ignored when at least one -real framework error exists, and the primary returned error is selected from -all started-task framework errors by this stable ordering: - -1. stage order: extract, merge, then normalize; -2. resolved lane order; -3. source chunk index for chunk-scoped work; and -4. configured validator or operation order within that scope. - -The full per-task errors may be retained in opt-in diagnostics, but completion -timing never chooses the public error. Output runs only after every lane reaches -a successful or rejection-only terminal state and no framework error exists. - -Extractors and any validator instance callable by multiple workers must be safe -for concurrent use. Production implementations should normally satisfy this by -being immutable after preparation. - -## Compatibility and Safety - -- Existing production module keys, validator keys, profiles, default chains, - and maintained configurations continue to resolve. -- Existing durable D&D JSON content and logical output paths remain unchanged. -- Framework errors, validator rejections, retries, checkpoints, diagnostics, - and debug behavior retain their current semantics except for the explicitly - documented workspace-v2 compatibility boundary and deterministic concurrent - ordering. -- All provider calls pass through the shared global scheduler, across lanes, - stages, retries, and validators. -- Source text and LLM payloads remain subject to the existing opt-in debug and - sensitive-data handling policies. -- Package moves do not create user-visible key changes or concrete cross-domain - dependencies. - -## Non-Goals - -This feature does not introduce: - -- a general DAG or configurable stage topology; -- out-of-process plugins or an RPC extension protocol; -- cross-lane normalization; -- a new durable D&D spell schema merely to mirror internal Go types; -- per-stage LLM permit pools that could exceed or partition the global ceiling; - or -- concurrent work implemented through an unbounded goroutine per lane or chunk. - -## Completion Criteria - -The target state is reached when: - -- all production lanes use one typed artifact from extract through normalize - and typed validation; -- codecs own schema-aware serialization at every type-erasure, checkpoint, and - debug boundary; -- incompatible lane composition and invalid options fail before source work; -- source units and chunks carry validated canonical provenance; -- production extensions follow the domain-first package and registrar rules; -- extract scheduling is bounded, deterministic, concurrent across lanes, and - race-free; -- instrumented tests prove actual concurrent LLM calls never exceed - `concurrency.total_llm` across all callers; -- the maintained D&D example and compatibility baselines retain their durable - contracts; and -- current-behavior documentation is updated as each implemented boundary lands. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md deleted file mode 100644 index b515fbc..0000000 --- a/docs/roadmap/implementation.md +++ /dev/null @@ -1,723 +0,0 @@ -# Domain-Typed Pipeline Implementation Roadmap - -## Status - -The domain-typed pipeline and the first five remediation stages were completed -on 2026-07-17. A subsequent review identified three additional implementation -issues. Stages 1 through 5 are retained as the completed implementation record; -Stages 6 through 8 are the decision-complete plan for the remaining work. - -Implement the pending stages in order. Keep each stage independently reviewable -and leave the repository passing its full validation suite before beginning -the next stage. Unless a stage explicitly says otherwise, preserve public CLI, -configuration, module-key, validator-key, durable output, and checkpoint file -contracts. - -Current architectural policy is defined by -[Architecture](../policy/architecture.md). The design context remains in the -[feature roadmap](domain.md) and -[ADR-0002](../adr/0002-linear-pipes-and-filters-pipeline.md), -[ADR-0003](../adr/0003-typed-interfaces-with-two-zone-data-model.md), and -[ADR-0004](../adr/0004-package-modules-by-domain.md). - -## Delivered Baseline - -The existing implementation already provides: - -- domain-first production extensions and package-family registrars; -- engine-owned source provenance using `source.SourceRef` and workspace schema - v2; -- one domain-owned Go type through extraction, merge, normalization, and typed - validation; -- schema-aware artifact codecs at checkpoint, debug, and output boundaries; -- pre-execution typed resolution, option validation, and preparation; -- one shared scheduled LLM client; and -- bounded chunk-first, lane-second extraction with deterministic public - outcomes. - -Stages 1 through 5 corrected checkpoint identity, completed merge and normalize -retry debugging, removed kind-ambiguous registry lookup, strengthened the -initial architectural guard, and removed the obsolete sequential extraction -path. Stages 6 through 8 complete candidate-artifact handling, make attempt -debugging comprehensive across all stages, and close the remaining import-guard -loopholes. - -## Implementation Rules - -Apply these rules to every stage: - -1. Read the task-specific documents listed in - [Development](../development.md) before changing the affected subsystem. -2. Add focused regression tests that fail against the pre-stage code and pass - after the change. -3. Preserve deterministic behavior. Do not base digests, public ordering, - reference resolution, or reported errors on Go map iteration or goroutine - completion order. -4. Keep framework-owned type erasure private. Do not reintroduce module-facing - `any`, raw JSON handoffs, or a generic processing interface. -5. Update current-behavior documentation in the same stage when externally - observable or documented internal behavior changes. Follow - [Documentation Policy](../policy/documentation.md); do not describe a later - stage as already implemented. -6. Run focused tests while iterating. At the end of every stage, run: - - ```sh - go test ./... - go test -race ./... - go vet ./... - go build ./cmd/notarius - git diff --check - ``` - -## Completed Remediation Record - -The following five stages have been implemented and are retained to document -the decisions and completion criteria that produced the current baseline. - -## Stage 1: Include Resolved Validator Policy In Pipeline Identity - -### Goal - -Ensure that every effective validator-chain change alters the resolved -pipeline digest and therefore prevents reuse of checkpoints created under a -different validation policy. - -### Required Changes - -1. Extend `resolvedPipelineDigest` in - `internal/framework/pipeline/profile.go` to include the complete resolved - validator-chain collection. Include, for every chain: - - - stage, lane ID, and owning module key; - - validator order; - - each complete resolved binding, including module key, LLM profile, - retries, options, and references; - - validator execution class; - - validator target; and - - artifact kind. - -2. Hash the already canonical `ResolvedPipeline.ValidatorChains` order produced - by resolution. Do not independently sort validators or otherwise weaken - configured order. Continue excluding only the digest field itself. -3. Use the same deterministic JSON-and-SHA-256 mechanism as the existing - pipeline digest. Go map keys encoded inside bindings must retain the - deterministic ordering supplied by `encoding/json`. -4. Do not add validator fingerprints separately to individual stage - dependencies. Pipeline/workspace identity is the authoritative invalidation - boundary for validation-policy changes. -5. Do not advance the workspace schema version. The corrected digest naturally - selects a different checkpoint directory; existing checkpoints remain - intact and become reuse misses for the changed pipeline identity. - -### Tests - -Add focused tests under `internal/framework/pipeline` and, where useful, -`internal/core/workspace` proving that: - -- registering a different default chain changes the resolved pipeline digest; -- adding, removing, or reordering a default validator changes the digest; -- changing a resolved validator binding option or LLM profile changes the - digest; -- changing execution class, target, or artifact kind changes the digest; -- resolving the same pipeline and catalog repeatedly produces the same digest; -- an explicit empty override produces an empty resolved chain and differs from - a non-empty inherited default; and -- a changed validator-chain digest produces a different checkpoint identity, - while an unchanged chain preserves it. - -Retain the existing tests proving that the digest field itself is excluded and -that artifact schema identity participates in the digest. - -### Documentation - -Update the checkpoint invalidation description in `docs/operations.md` and the -identity description in `docs/internal/pipeline.md` to state concisely that the -effective resolved validator policy participates in pipeline identity. - -### Completion Gate - -A checkpoint accepted under one resolved default or explicit validator chain -must not be reusable after that chain changes. - -## Stage 2: Complete Attempt-Scoped Debugging For Merge And Normalize - -### Goal - -Give merge and normalize retries the same attempt-level observability and -nested LLM-call association already provided for chunk and extract attempts. - -### Required Changes - -1. In `internal/framework/pipeline/runner_typed.go`, create an attempt-specific - debug context before invoking each merge or normalize module. Pass that - context to both the module operation and its validation chain. -2. Use these stable paths: - - ```text - merge//attempt-.json - merge//attempt-/prompt-.json - merge//attempt-/response-.json - merge//attempt-/response-content-. - - normalize//attempt-.json - normalize//attempt-/prompt-.json - normalize//attempt-/response-.json - normalize//attempt-/response-content-. - ``` - - Continue using two-digit retry attempt numbers and the existing debug path - sanitization and LLM call numbering behavior. -3. Write one module-attempt envelope for every attempted merge and normalize - operation: - - - on success, record the codec-backed candidate artifact and warnings that - will be promoted if the attempt is accepted; - - on validator rejection, record the rejection without promoting discarded - warnings; - - on module, validation, serialization, or debug failure, record the error; - and - - in all cases, attach the LLM calls recorded in the module attempt scope. - -4. Keep validator-specific debug scopes under the existing `validate/...` - hierarchy. A validator's LLM calls remain linked to its validator attempt; - the merge or normalize module envelope links only calls made by that module - attempt. -5. Preserve the existing stage-level `input.json` and `output.json` artifacts. - Checkpoint reuse should continue to emit stage-level artifacts but should - not synthesize retry attempts that did not execute. -6. Factor common attempt-envelope behavior into a small private helper where it - prevents chunk, extract, merge, and normalize instrumentation from drifting. - Do not introduce a new public runner abstraction solely for debugging. -7. Treat any debug write failure as a framework error, consistent with current - debug policy. - -### Tests - -Add tests with instrumented LLM-backed fake mergers and normalizers proving -that: - -- first-attempt success writes the expected attempt and nested LLM artifacts; -- a failed first attempt followed by success writes two distinct attempt - envelopes and associates each call with the correct attempt; -- module errors, validator errors, and final rejection are represented in the - corresponding attempt envelope; -- validator LLM calls remain under validator paths rather than being attributed - to the module scope; -- discarded-attempt warnings are not promoted; -- checkpoint reuse produces no module-attempt files; and -- no merge or normalize call falls back to an unscoped stage-name debug path. - -Extend the CLI debug integration test only as needed to verify the public debug -directory layout. Keep most behavioral coverage in the pipeline package. - -### Documentation - -Update `docs/operations.md` if necessary to show the stable merge and normalize -attempt paths. Update `docs/internal/pipeline.md` only where its implementation -description needs clarification; its existing attempt-scoping guarantee should -become fully true rather than be weakened. - -### Completion Gate - -Every executed merge and normalize retry must have a distinct debug envelope, -and every LLM call made by that module attempt must be nested under and linked -from that attempt. - -## Stage 3: Make Typed Variant Spec Lookup Kind-Specific - -### Goal - -Ensure that CLI reference-target discovery and other behavior-sensitive lookup -select the merger or normalizer specification for the lane's actual artifact -kind, never an arbitrary map entry. - -### Required Changes - -1. Add explicit kind-specific lookup methods to `MergerRegistry` and - `NormalizerRegistry`, named: - - ```go - SpecForArtifact(key string, kind contracts.ArtifactKind) (ModuleSpec, bool) - ``` - - Normalize the key and artifact kind in the same way as typed registration - and return a cloned spec. -2. Change CLI reference-target discovery in `internal/cli/run.go` to retain the - selected extractor's declared artifact kind and use it for merger and - normalizer spec lookup in that lane. -3. A missing typed variant must produce a deterministic error naming the - pipeline, lane, stage, module key, requested artifact kind, and sorted - registered kinds, matching the quality of full pipeline resolution errors. -4. Keep `Spec(key)` for kind-neutral catalog inspection and compatibility with - existing callers, but remove its map-order dependence. Select the first - registered artifact kind in sorted order before cloning its spec. Add a - comment making clear that behavior-sensitive code must use - `SpecForArtifact`. -5. Do not require typed variants under one reusable key to expose identical - reference slots or capabilities. Their kind-specific specs are allowed to - differ, and resolution must consistently choose the matching variant. -6. Audit all merger and normalizer `Spec` callers. Convert any caller making a - lane-specific decision to `SpecForArtifact`; leave only catalog or display - callers on the kind-neutral method. - -### Tests - -Register at least two artifact-kind variants under the same merger key and the -same normalizer key with intentionally different reference slots. Prove that: - -- `SpecForArtifact` returns the correct cloned variant; -- lookup is stable regardless of registration order; -- CLI qualified and unqualified reference discovery uses the selected lane's - variant; -- a reference accepted by one variant is not incorrectly accepted for another; -- a missing variant reports sorted available kinds; and -- repeated `Spec(key)` calls return the same deterministic catalog result. - -Retain coverage for the existing single-variant D&D catalog behavior. - -### Documentation - -Update `docs/internal/pipeline.md` or `docs/internal/modules.md` only if either -currently describes registry lookup mechanics. No CLI or configuration syntax -change is intended. - -### Completion Gate - -No behavior-sensitive merger or normalizer lookup may depend on map iteration, -and CLI reference resolution must use the artifact kind selected by the lane's -extractor. - -## Stage 4: Enforce Domain Import Boundaries Generically - -### Goal - -Make the repository guard enforce ADR-0004 for present and future module -families without a hardcoded list of domain names or domain pairs. - -### Required Changes - -1. Refactor `internal/modules/import_boundaries_test.go` so that a module family - is derived from the first path segment under `internal/modules/`, rather than - recognized by a fixed `isDomain` list. -2. Treat `generic` as the domain-neutral reusable extension family. Treat every - other production family, including `dnd`, `seriatim`, and future families, - as concrete for import-boundary purposes. -3. Enforce these rules: - - - a family root package must not import its own child implementation or - registrar packages; - - child packages within the same concrete family may import the family root, - shared helpers, or sibling implementations when needed; - - a concrete family must not import another concrete family; - - generic packages must not import concrete families; - - concrete implementation packages must not import generic implementation - packages directly; - - `internal/modules//register` may import its own family packages and - generic packages to compose typed strategies; - - the generic registrar may import generic child packages; and - - the application composition root and designated black-box integration - tests may compose multiple families. - -4. Exclude `internal/modules/integration` from production-family discovery. - Preserve its exemption only for `_test.go` black-box composition files; do - not create a blanket exemption for production Go files. -5. Apply production import rules to white-box tests located in concrete module - packages. If an existing test composes concrete and generic implementations, - move that cross-family coverage to `internal/modules/integration` or replace - the foreign implementation with a package-local test double. Do not exempt - arbitrary `_test.go` files merely because they are tests. -6. Keep the test based on parsed Go imports. Do not add a new build tool or - external dependency for this guard. - -### Tests - -Expand the table-driven import tests to cover: - -- a hypothetical future concrete family, demonstrating that no code change is - needed to enforce its boundaries; -- generic-to-concrete rejection for both current and hypothetical families; -- concrete-to-peer-concrete rejection; -- concrete implementation-to-generic rejection; -- concrete registrar-to-generic acceptance; -- family-root-to-child rejection; -- child-to-family-root and same-family sibling acceptance; -- application composition-root acceptance; and -- black-box integration-test acceptance without exempting non-test files. - -Run the guard against the complete current repository and confirm that no -production package must be moved to satisfy it. - -### Documentation - -No ADR change is required. Update `docs/internal/modules.md` only if its package -boundary description needs to name the registrar-only generic composition -rule more clearly. - -### Completion Gate - -Adding a new directory under `internal/modules/` must automatically -receive the same cross-family and registrar enforcement as current production -families. - -## Stage 5: Remove The Obsolete Sequential Extraction Path - -### Goal - -Make the runner's structure match the implemented concurrent architecture: -the coordinator owns extraction, and a lane continuation begins from finalized -extract results and performs only merge and normalize work. - -### Required Changes - -1. Replace `runTypedLane` with a continuation-oriented private function whose - input explicitly contains the finalized extraction state needed by merge and - normalize: - - - accepted typed extract artifacts; - - serialized checkpoint artifacts; - - accepted warnings; - - rejected outputs; and - - the original extract checkpoint decision. - - Use a private struct if it keeps the call boundary clear and avoids a long - positional parameter list. -2. Change `continueLane` in `runner_concurrent.go` to pass the finalized state - directly. Do not make completed extraction look like checkpoint reuse. -3. Remove: - - - `completedExtractLoader`; - - the `RunInput.extractDecision` private override; - - the sequential extraction branch formerly contained in `runTypedLane`; - and - - duplicate extraction retry, validation, checkpoint, and debug logic made - unreachable by concurrent coordination. - -4. Keep extraction checkpoint loading and validation in `prepareLaneExtract`, - extract execution in `runExtractJob`, and deterministic final aggregation in - `finalizeLaneExtract`. -5. Preserve existing behavior exactly: - - - extract checkpoint events report the real loader decision; - - reused and freshly computed extract results enter continuation through the - same typed state; - - accepted artifacts reach merge in chunk-index order; - - warnings and rejections retain stable ordering and promotion semantics; - - a lane with no accepted extracts follows the current merge behavior; - - stage-level extract input/output debug artifacts remain unchanged; and - - failure classification and cancellation retain their deterministic scope. - -6. Keep merge and normalize serial within a lane and keep cross-lane - continuations bounded by the existing worker policy. Do not introduce a - goroutine per lane or chunk. - -### Tests - -Add or adjust focused runner tests proving parity for: - -- fresh extraction, fully reused extraction, and a mixture of accepted and - rejected chunk results; -- deterministic extract ordering under reverse completion; -- warning promotion across retries; -- real checkpoint decision reporting for reused and recomputed extracts; -- extract input/output and attempt debug paths; -- framework cancellation and deterministic error selection; and -- unchanged bounded worker and global LLM concurrency behavior. - -Use source search or a package-local compile-time assertion where practical to -confirm there is only one extraction execution path and no remaining -`completedExtractLoader` or `extractDecision` compatibility shim. - -### Documentation - -Update `docs/internal/pipeline.md` if its execution-flow description names the -old continuation mechanism. This stage is an internal refactor and must not -change operator or integration contracts. - -### Completion Gate - -The concurrent coordinator must be the only code path that executes extraction, -and lane continuation must consume finalized extraction state without routing -it through a synthetic checkpoint loader. - -## Pending Remediation Plan - -Implement Stages 6 through 8 in order. Do not perform final roadmap closeout -until all three stages and their documentation changes have landed. - -## Stage 6: Validate Merge And Normalize Candidates Before Final Encoding - -### Goal - -Preserve the two-zone artifact model at merge and normalize boundaries: a -module result remains a candidate artifact until typed validation accepts it, -and only an accepted artifact is encoded with the final artifact codec and -eligible for checkpointing or downstream use. - -### Required Changes - -1. Refactor the merge and normalize attempt paths in - `internal/framework/pipeline/runner_typed.go` so each attempt proceeds in - this order: - - 1. execute the typed module and collect its warnings; - 2. serialize the returned value with `ArtifactCodec.EncodeCandidate` for - attempt-debug capture; - 3. execute the resolved typed validator chain against the in-memory value; - 4. on validator rejection, record the rejected candidate and finish the - attempt without invoking `ArtifactCodec.Encode` or creating a - checkpoint artifact; and - 5. only after validator acceptance, invoke `ArtifactCodec.Encode` through - the normal checkpoint-artifact path. - -2. Use one private candidate-serialization helper for merge, normalize, and - any equivalent pre-validation debug path. The helper must preserve the - codec's media type and schema digest in the debug artifact, but its result - must never be used as a final checkpoint or downstream artifact. -3. Treat candidate-encoding and final-encoding failures as framework errors, - not validator rejections. Record the applicable attempt envelope before - returning the error using the existing attempt-debug behavior; Stage 7 will - consolidate that behavior across every retrying stage. -4. Preserve retry behavior: - - - a validator rejection may be retried according to the stage retry policy; - - warnings from rejected attempts remain attempt-local unless existing - warning-promotion policy says otherwise; - - a final accepted value and its serialized checkpoint artifact are - published only after the attempt has completed successfully; and - - checkpoint identity, checkpoint event reporting, and durable artifact - formats remain unchanged. - -5. Do not weaken a domain codec so that its final `Encode` accepts invalid - domain values. In particular, keep the D&D spell codec's semantic checks at - the final-artifact boundary and use `EncodeCandidate` for its intentionally - incomplete pre-validation representation. - -### Tests - -Add focused merge and normalize tests using an instrumented codec for which -`EncodeCandidate` accepts an invalid value but `Encode` rejects it. Prove that: - -- the validator, rather than final encoding, determines that the candidate is - rejected; -- rejected candidates never call final `Encode` and never produce checkpoint - artifacts; -- an accepted candidate calls final `Encode` exactly once and produces the - existing checkpoint representation; -- candidate-encoding and post-acceptance final-encoding failures are reported - as framework errors and have attempt debug records; and -- retry warnings, rejection promotion, and checkpoint event ordering remain - deterministic. - -Retain or add a D&D spell regression test demonstrating that an incomplete -candidate can reach validation without making the final spell codec permissive. - -### Documentation - -Update `docs/internal/pipeline.md` to state explicitly that merge and normalize -debug payloads may contain candidate encodings, while checkpoints and stage -outputs always contain validator-approved final encodings. Update -`docs/internal/artifacts.md` if it currently implies that every serialized -debug artifact is a final Domain Artifact Zone representation. - -### Completion Gate - -No merge or normalize code path may invoke final artifact encoding before typed -validation acceptance, and a validator-rejected candidate must remain -observable as a rejection even when the codec would refuse to encode it as a -final artifact. - -## Stage 7: Record Every Executed Attempt And Its Terminal Outcome - -### Goal - -Make attempt debugging complete and uniform across chunk, extract, merge, and -normalize so every executed retry has one terminal attempt envelope, including -validator rejection, module error, validator error, and serialization error. - -### Required Changes - -1. Introduce or consolidate a private attempt-terminal recorder used by all - four retrying stages. It must write exactly one envelope per executed - attempt and support these terminal outcomes: - - - accepted success; - - validator rejection; - - module execution error; - - validator execution error; - - candidate-serialization error; and - - final-serialization error where final encoding occurs within the attempt. - -2. Preserve the existing debug directory layout and envelope schema. Populate - the envelope consistently with the attempt number, warnings accumulated by - that attempt, any available candidate payload or rejection details, and the - terminal error text when an error occurred. A validator rejection without - an execution error remains a rejection and must not acquire a synthetic - error string. -3. Complete the extract path in - `internal/framework/pipeline/runner_concurrent.go`: - - - write an attempt envelope before returning a validator rejection or - validator error; - - write an attempt envelope before returning a final-serialization error; - and - - stop discarding errors returned while writing module-error attempt data. - -4. Complete the chunk path in `internal/framework/pipeline/runner.go` by - recording validator execution errors in the envelope's error field and by - propagating attempt-write failures on every terminal path. -5. Route the existing merge and normalize attempt handling through the same - terminal-recording behavior without changing their retry or validation - semantics. Coordinate this work with Stage 6 so rejected candidates use - candidate encoding and accepted values use final encoding. -6. Keep LLM-call ownership scoped to the operation that made the call: - - - module calls belong to the module attempt envelope; - - validator calls remain in their validator-specific debug scope; and - - attaching a call to an attempt must not duplicate it in another module - attempt or orphan it from the attempt that initiated it. - -7. Never silently discard a debug write failure. If another error already - exists, return an `errors.Join` result that preserves both the primary error - and the contextualized debug error. If no primary error exists, return the - contextualized debug error. Do not replace a validator rejection with a - framework error unless persisting its required debug record fails. -8. Keep debug failures subject to the retry boundary already surrounding the - relevant attempt; do not add a second retry loop specifically for debug - persistence. - -### Tests - -Add table-driven attempt-debug tests covering each terminal outcome for chunk -and extract, and retain equivalent merge and normalize coverage. At minimum, -prove that: - -- a first-attempt extract rejection followed by success writes both envelopes; -- a terminal extract rejection, validator error, module error, candidate-codec - error, and final-codec error each write an envelope with the correct fields; -- a chunk validator execution error appears in its attempt envelope; -- debug write failures are returned, and are joined with the primary error - when both occur; -- module LLM calls are attached to the correct attempt while validator LLM - calls remain isolated; and -- success, rejection, warning, and retry ordering remains deterministic. - -Use a shared assertion helper to verify the invariant that the number and -indices of attempt envelopes equal the attempts actually executed. - -### Documentation - -Update `docs/internal/pipeline.md` and `docs/operations.md` so their attempt -debug guarantees name all terminal outcomes and explain the separation between -module-attempt and validator-call scopes. Do not promise that a payload exists -when failure occurred before a candidate value was available. - -### Completion Gate - -For chunk, extract, merge, and normalize, every entered attempt must leave -exactly one terminal envelope or return an error that explicitly reports why -that envelope could not be persisted. No attempt-related debug write error may -be ignored. - -## Stage 8: Close Production Import-Guard Loopholes - -### Goal - -Make the automated import guard enforce the complete domain-first dependency -policy for production code, including the framework, the application -composition root, and the integration-test-only package. - -### Required Changes - -1. Extend `internal/modules/import_boundaries_test.go` so validation considers - both the importing file's repository-relative path and whether it is a test - file. Do not return early merely because the importer is outside - `internal/modules`. -2. Treat `internal/modules/integration` as test infrastructure, not as a module - family or a production dependency target: - - - reject every import of `internal/modules/integration` from a non-test Go - file; and - - continue allowing designated black-box tests under - `internal/modules/integration` to import concrete families for composition - coverage. - -3. Reject imports of any `internal/modules/**` package from non-test files - under `internal/framework/**` or `internal/core/**`. These packages define - inward framework and core layers and must remain independent of all module - implementations, including `generic`. -4. For non-test files under the production CLI composition root - `internal/cli/**`, allow module imports only when the target is the exact - family registrar package `internal/modules//register`. Reject direct - imports of family roots, domain leaves, adapters, and generic implementation - leaves. Apply the same rule automatically to newly added families. -5. Preserve test-only assembly allowances needed for black-box and - compatibility coverage. In particular, `_test.go` files in the CLI, - framework, and core trees may import module packages, and designated - integration black-box tests may compose concrete families. These allowances - must be based on the importing file being a test file, not on a directory - exemption that production files could inherit. -6. Retain all Stage 4 within-module rules: concrete peer isolation, - registrar-only concrete-to-generic composition, family-root direction, and - same-family child relationships. Keep path parsing structural so new module - families receive the rules without a hard-coded family list. -7. Improve failure messages to name the importing file, import target, and the - applicable boundary rule. - -### Tests - -Add table-driven fixtures proving that the guard: - -- rejects production imports of `internal/modules/integration` from module and - non-module packages; -- rejects production framework and core imports of concrete and generic module - packages; -- accepts an exact registrar import from production CLI code; -- rejects production CLI imports of a family root, concrete leaf, generic - leaf, or other non-registrar module package; -- accepts equivalent direct imports from `_test.go` compatibility tests; -- accepts designated black-box integration-test composition while rejecting a - production `.go` file in the same directory; and -- retains every cross-family, registrar, and family-root case covered by Stage - 4. - -Run the guard against the full repository and ensure all current production -imports comply without adding path-specific exemptions. - -### Documentation - -Update `docs/internal/modules.md` to distinguish production composition through -family registrars from test-only direct composition, and to state that -`internal/modules/integration` is not a production dependency target. Update -other current-behavior documentation only if it describes broader composition -permissions. - -### Completion Gate - -A new production file cannot bypass the domain-first dependency rules by being -placed outside `internal/modules`, under `internal/modules/integration`, or in -the CLI composition tree. Test-only allowances must remain explicit and -file-scoped. - -## Final Verification And Closeout - -After Stages 6 through 8: - -1. Run the full validation commands from this document on a clean worktree. -2. Exercise the maintained D&D production example with both a fresh workspace - and checkpoint resume. -3. Verify that a merge or normalize candidate rejected by typed validation is - not passed to final encoding or checkpoint creation. -4. Verify that every executed chunk, extract, merge, and normalize retry has a - terminal attempt envelope with correctly scoped LLM-call data. -5. Confirm current production imports satisfy the strengthened framework, - registrar-only CLI, integration-target, and module-family boundary rules. -6. Re-read current-behavior documentation for statements made true or obsolete - by these stages. -7. Replace this roadmap's status with a concise completion record only after all - stages and documentation updates have landed. - -## Open Questions - -None. The implementation choices required for Stages 6 through 8 are specified -above. Final roadmap closeout is intentionally deferred until those stages are -complete. diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 8cef149..e6d181f 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -3787,7 +3787,10 @@ func (fakeRunCodec) Kind() contracts.ArtifactKind { return fakeRunArtifactKind } func (fakeRunCodec) Schema() contracts.ArtifactSchema { return contracts.ArtifactSchema{ID: "fake.artifact", Name: "fake_artifact", Version: "v1", JSONSchema: []byte(`{"type":"object"}`)} } -func (fakeRunCodec) MediaType() string { return "application/json" } +func (fakeRunCodec) MediaType() string { return "application/json" } +func (fakeRunCodec) EncodeCandidate(v fakeRunArtifact) ([]byte, error) { + return json.Marshal(v) +} func (fakeRunCodec) Encode(v fakeRunArtifact) ([]byte, error) { return json.Marshal(v) } func (fakeRunCodec) Decode(b []byte) (fakeRunArtifact, error) { var v fakeRunArtifact diff --git a/internal/core/config/validation_test.go b/internal/core/config/validation_test.go index 1f0a00b..2bb945b 100644 --- a/internal/core/config/validation_test.go +++ b/internal/core/config/validation_test.go @@ -643,6 +643,9 @@ func (fakeArtifactCodec) Schema() contracts.ArtifactSchema { return contracts.ArtifactSchema{ID: "urn:notarius:test:artifact", Name: "Test artifact", Version: "1", JSONSchema: []byte(`{"type":"string"}`)} } func (fakeArtifactCodec) MediaType() string { return "application/json" } +func (fakeArtifactCodec) EncodeCandidate(value fakeArtifact) ([]byte, error) { + return []byte(fmt.Sprintf("%q", value)), nil +} func (fakeArtifactCodec) Encode(value fakeArtifact) ([]byte, error) { return []byte(fmt.Sprintf("%q", value)), nil } diff --git a/internal/framework/contracts/artifact.go b/internal/framework/contracts/artifact.go index 1d3c309..7981874 100644 --- a/internal/framework/contracts/artifact.go +++ b/internal/framework/contracts/artifact.go @@ -41,6 +41,10 @@ type ArtifactCodec[T any] interface { Kind() ArtifactKind Schema() ArtifactSchema MediaType() string + // EncodeCandidate serializes a stage result before semantic validation. It + // must not apply validity checks owned by typed validators; Encode remains + // the strict final-artifact boundary used after validation succeeds. + EncodeCandidate(T) ([]byte, error) Encode(T) ([]byte, error) Decode([]byte) (T, error) } diff --git a/internal/framework/pipeline/artifact_codec_registry.go b/internal/framework/pipeline/artifact_codec_registry.go index 45c0075..7d152f9 100644 --- a/internal/framework/pipeline/artifact_codec_registry.go +++ b/internal/framework/pipeline/artifact_codec_registry.go @@ -120,19 +120,16 @@ func RegisterArtifactCodec[T any](registry *ArtifactCodecRegistry, codec contrac return decoded, nil }, } - entry.encodeCandidate = entry.encode - if candidate, ok := any(codec).(interface{ EncodeCandidate(T) ([]byte, error) }); ok { - entry.encodeCandidate = func(value any) ([]byte, error) { - typed, err := exactTypedValue[T]("encode candidate artifact", value) - if err != nil { - return nil, err - } - content, err := candidate.EncodeCandidate(typed) - if err != nil { - return nil, &ArtifactCodecOperationError{Operation: "encode", Kind: spec.Kind, Err: err} - } - return append([]byte(nil), content...), nil + entry.encodeCandidate = func(value any) ([]byte, error) { + typed, err := exactTypedValue[T]("encode candidate artifact", value) + if err != nil { + return nil, err } + content, err := codec.EncodeCandidate(typed) + if err != nil { + return nil, &ArtifactCodecOperationError{Operation: "encode candidate", Kind: spec.Kind, Err: err} + } + return append([]byte(nil), content...), nil } if provider, ok := any(codec).(interface{ Metadata(T) map[string]any }); ok { entry.metadata = func(value any) map[string]any { diff --git a/internal/framework/pipeline/artifact_codec_registry_test.go b/internal/framework/pipeline/artifact_codec_registry_test.go index 4bced7b..ea75c0e 100644 --- a/internal/framework/pipeline/artifact_codec_registry_test.go +++ b/internal/framework/pipeline/artifact_codec_registry_test.go @@ -24,19 +24,28 @@ type codecScore struct { type codecNotesAlias codecNotes type testArtifactCodec[T any] struct { - kind contracts.ArtifactKind - schema contracts.ArtifactSchema - mediaType string - encodeFunc func(T) ([]byte, error) - decodeFunc func([]byte) (T, error) + kind contracts.ArtifactKind + schema contracts.ArtifactSchema + mediaType string + encodeFunc func(T) ([]byte, error) + candidateFunc func(T) ([]byte, error) + decodeFunc func([]byte) (T, error) } func (c testArtifactCodec[T]) Kind() contracts.ArtifactKind { return c.kind } func (c testArtifactCodec[T]) Schema() contracts.ArtifactSchema { return c.schema } func (c testArtifactCodec[T]) MediaType() string { return c.mediaType } +func (c testArtifactCodec[T]) EncodeCandidate(value T) ([]byte, error) { + if c.candidateFunc != nil { + return c.candidateFunc(value) + } + return c.encodeFunc(value) +} func (c testArtifactCodec[T]) Encode(value T) ([]byte, error) { return c.encodeFunc(value) } func (c testArtifactCodec[T]) Decode(content []byte) (T, error) { return c.decodeFunc(content) } +var _ contracts.ArtifactCodec[codecNotes] = testArtifactCodec[codecNotes]{} + func TestArtifactCodecRegistryStoresHeterogeneousExactTypes(t *testing.T) { registry := NewArtifactCodecRegistry() if err := RegisterArtifactCodec(registry, notesCodec()); err != nil { @@ -92,6 +101,43 @@ func TestArtifactCodecRegistryStoresHeterogeneousExactTypes(t *testing.T) { } } +func TestArtifactCodecRegistryKeepsCandidateAndFinalEncodingDistinct(t *testing.T) { + candidateCalls, finalCalls := 0, 0 + codec := notesCodec() + codec.candidateFunc = func(codecNotes) ([]byte, error) { + candidateCalls++ + return []byte(`{"items":["candidate"]}`), nil + } + codec.encodeFunc = func(codecNotes) ([]byte, error) { + finalCalls++ + return []byte(`{"items":["final"]}`), nil + } + registry := NewArtifactCodecRegistry() + if err := RegisterArtifactCodec(registry, codec); err != nil { + t.Fatalf("RegisterArtifactCodec() error = %v, want nil", err) + } + entry, _, err := registry.entry(codec.kind) + if err != nil { + t.Fatalf("entry() error = %v, want nil", err) + } + + candidate, err := serializeArtifact(entry, codecNotes{}, true) + if err != nil { + t.Fatalf("serialize candidate error = %v, want nil", err) + } + if string(candidate.Content) != `{"items":["candidate"]}` || candidateCalls != 1 || finalCalls != 0 { + t.Fatalf("candidate content = %s, calls = candidate %d, final %d", candidate.Content, candidateCalls, finalCalls) + } + + final, err := serializeArtifact(entry, codecNotes{}, false) + if err != nil { + t.Fatalf("serialize final error = %v, want nil", err) + } + if string(final.Content) != `{"items":["final"]}` || candidateCalls != 1 || finalCalls != 1 { + t.Fatalf("final content = %s, calls = candidate %d, final %d", final.Content, candidateCalls, finalCalls) + } +} + func TestArtifactCodecRegistryStoresValidatedSchemaMetadata(t *testing.T) { registry := NewArtifactCodecRegistry() codec := notesCodec() diff --git a/internal/framework/pipeline/debug.go b/internal/framework/pipeline/debug.go index a53fa3b..0d1a8fb 100644 --- a/internal/framework/pipeline/debug.go +++ b/internal/framework/pipeline/debug.go @@ -409,6 +409,17 @@ type attemptTerminalRecorder struct { envelope debugTimedEnvelope } +type attemptDebugPersistenceError struct { + label string + err error +} + +func (e *attemptDebugPersistenceError) Error() string { + return fmt.Sprintf("write %s attempt debug artifact: %v", e.label, e.err) +} + +func (e *attemptDebugPersistenceError) Unwrap() error { return e.err } + func newAttemptTerminalRecorder(recorder DebugRecorder, attemptPath, label string, scope *debugLLMScope, envelope debugTimedEnvelope) attemptTerminalRecorder { return attemptTerminalRecorder{recorder: recorder, path: attemptPath, label: label, scope: scope, envelope: envelope} } @@ -420,7 +431,7 @@ func (r attemptTerminalRecorder) record(payload any, terminalErr error) error { envelope.Error = terminalErr.Error() } if err := writeDebugAttempt(r.recorder, r.path, envelope, r.scope); err != nil { - debugErr := fmt.Errorf("write %s attempt debug artifact: %w", r.label, err) + debugErr := &attemptDebugPersistenceError{label: r.label, err: err} return errors.Join(terminalErr, debugErr) } return terminalErr diff --git a/internal/framework/pipeline/default_modules_test.go b/internal/framework/pipeline/default_modules_test.go index d15c98d..6e01a00 100644 --- a/internal/framework/pipeline/default_modules_test.go +++ b/internal/framework/pipeline/default_modules_test.go @@ -143,7 +143,10 @@ func (defaultArtifactCodec) Kind() contracts.ArtifactKind { return defaultArtifa func (defaultArtifactCodec) Schema() contracts.ArtifactSchema { return contracts.ArtifactSchema{ID: "urn:notarius:test:default", Name: "default", Version: "1", JSONSchema: []byte(`{"type":"object"}`)} } -func (defaultArtifactCodec) MediaType() string { return "application/json" } +func (defaultArtifactCodec) MediaType() string { return "application/json" } +func (defaultArtifactCodec) EncodeCandidate(value defaultArtifact) ([]byte, error) { + return json.Marshal(value) +} func (defaultArtifactCodec) Encode(value defaultArtifact) ([]byte, error) { return json.Marshal(value) } func (defaultArtifactCodec) Decode(content []byte) (defaultArtifact, error) { var value defaultArtifact diff --git a/internal/framework/pipeline/runner.go b/internal/framework/pipeline/runner.go index 7c52c23..c786faa 100644 --- a/internal/framework/pipeline/runner.go +++ b/internal/framework/pipeline/runner.go @@ -357,6 +357,10 @@ func runWithRetry(ctx context.Context, retries int, run func(attempt int) (bool, } accepted, rejection, err := run(attempt) if err != nil { + var debugErr *attemptDebugPersistenceError + if errors.As(err, &debugErr) { + return false, nil, fmt.Errorf("failed after %d attempt(s): %w", attempt, err) + } if attempt == attempts { return false, nil, fmt.Errorf("failed after %d attempt(s): %w", attempt, err) } diff --git a/internal/framework/pipeline/runner_terminal_debug_test.go b/internal/framework/pipeline/runner_terminal_debug_test.go index dda34fd..e801932 100644 --- a/internal/framework/pipeline/runner_terminal_debug_test.go +++ b/internal/framework/pipeline/runner_terminal_debug_test.go @@ -18,6 +18,7 @@ type terminalChunker struct { chunks []source.Chunk warnings []contracts.Warning err error + calls *int } func (c terminalChunker) Key() string { return c.key } @@ -25,6 +26,9 @@ func (c terminalChunker) Key() string { return c.key } func (terminalChunker) ReferenceSlots() []contracts.ReferenceSlot { return nil } func (c terminalChunker) Chunk(context.Context, contracts.ChunkRequest) (contracts.ChunkResult, error) { + if c.calls != nil { + (*c.calls)++ + } return contracts.ChunkResult{Chunks: cloneSourceChunks(c.chunks), Warnings: cloneWarnings(c.warnings)}, c.err } @@ -211,6 +215,30 @@ func TestRunnerJoinsPrimaryAndAttemptWriteErrors(t *testing.T) { }) } +func TestRunnerDoesNotRetryAfterTerminalAttemptWriteFailure(t *testing.T) { + prepared, chunks := preparedTerminalDebugPipeline(t) + prepared.resolved.Chunk.Retries = 1 + calls := 0 + prepared.chunker = terminalChunker{key: prepared.resolved.Chunk.Module, chunks: chunks, calls: &calls} + prepared.chunkValidators.validators = []preparedValidator{{ + resolved: ResolvedValidator{Binding: Binding("terminal/chunk-validator"), Target: ValidatorTargetChunk}, + chunk: terminalChunkValidator{result: contracts.ValidationResult{Approved: true}}, + }} + debug := newCapturedDebugRecorder() + debug.failPath = "chunk/attempt-01.json" + + _, err := New().Run(context.Background(), RunInput{Prepared: prepared, RawInput: []byte("input"), Debug: debug}) + if err == nil || !strings.Contains(err.Error(), "write chunk attempt debug artifact") || !strings.Contains(err.Error(), "debug recorder failure") { + t.Fatalf("Run() error = %v, want terminal attempt debug failure", err) + } + if calls != 1 { + t.Fatalf("chunk calls = %d, want one attempt without retry", calls) + } + if debug.has("chunk/attempt-02.json") { + t.Fatal("second chunk attempt envelope exists after non-retryable debug persistence failure") + } +} + func TestRunnerKeepsExtractModuleAndValidatorLLMCallsIsolated(t *testing.T) { prepared := preparedAttemptDebugPipeline(t) debug := newCapturedDebugRecorder() diff --git a/internal/modules/import_boundaries_test.go b/internal/modules/import_boundaries_test.go index 3187d04..2027807 100644 --- a/internal/modules/import_boundaries_test.go +++ b/internal/modules/import_boundaries_test.go @@ -189,6 +189,41 @@ func TestImportBoundaryRules(t *testing.T) { sourcePackage: "cli", importPath: moduleImportPrefix + "almanac/extract/events", }, + { + name: "command production cannot import registrar", + filename: "cmd/notarius/main.go", + sourcePackage: "main", + importPath: moduleImportPrefix + "almanac/register", + wantError: true, + }, + { + name: "command production cannot import concrete leaf", + filename: "cmd/notarius/main.go", + sourcePackage: "main", + importPath: moduleImportPrefix + "almanac/extract/events", + wantError: true, + }, + { + name: "unknown production package cannot import concrete leaf", + filename: "internal/application/bootstrap.go", + sourcePackage: "application", + importPath: moduleImportPrefix + "almanac/extract/events", + wantError: true, + }, + { + name: "unknown production package cannot import registrar", + filename: "internal/application/bootstrap.go", + sourcePackage: "application", + importPath: moduleImportPrefix + "almanac/register", + wantError: true, + }, + { + name: "unknown test package is not a compatibility root", + filename: "internal/application/bootstrap_test.go", + sourcePackage: "application", + importPath: moduleImportPrefix + "almanac/extract/events", + wantError: true, + }, { name: "framework production cannot import concrete module", filename: "internal/framework/pipeline/runner.go", @@ -324,35 +359,41 @@ func validateImport(filename string, sourcePackage string, importPath string) er } return importBoundaryViolation(filename, importPath, "module integration composition is allowed only in black-box tests") } - if !isTest && (strings.HasPrefix(filename, "internal/framework/") || strings.HasPrefix(filename, "internal/core/")) { + sourceFamily, sourceRoot, sourceRegistrar := moduleFamilyForFile(filename) + if sourceFamily != "" { + if sourceRoot && sourceFamily == target.family && target.child { + return importBoundaryViolation(filename, importPath, "family root must not import child packages") + } + if sourceFamily == target.family { + return nil + } + if sourceFamily == "generic" { + return importBoundaryViolation(filename, importPath, fmt.Sprintf("generic family must not import concrete family %q", target.family)) + } + if target.family == "generic" { + if sourceRegistrar { + return nil + } + return importBoundaryViolation(filename, importPath, fmt.Sprintf("concrete family %q may import generic implementations only from its registrar", sourceFamily)) + } + return importBoundaryViolation(filename, importPath, fmt.Sprintf("concrete family %q must not import concrete family %q", sourceFamily, target.family)) + } + if isTest { + if isCompatibilityTestFile(filename) { + return nil + } + return importBoundaryViolation(filename, importPath, "direct module imports from non-module tests are allowed only in CLI, core, and framework compatibility-test roots") + } + if strings.HasPrefix(filename, "internal/framework/") || strings.HasPrefix(filename, "internal/core/") { return importBoundaryViolation(filename, importPath, "core and framework production code must not import module implementations") } - if !isTest && strings.HasPrefix(filename, "internal/cli/") { + if strings.HasPrefix(filename, "internal/cli/") { if target.registrar { return nil } return importBoundaryViolation(filename, importPath, "CLI production code may import only exact module family registrar packages") } - sourceFamily, sourceRoot, sourceRegistrar := moduleFamilyForFile(filename) - if sourceFamily == "" { - return nil - } - if sourceRoot && sourceFamily == target.family && target.child { - return importBoundaryViolation(filename, importPath, "family root must not import child packages") - } - if sourceFamily == target.family { - return nil - } - if sourceFamily == "generic" { - return importBoundaryViolation(filename, importPath, fmt.Sprintf("generic family must not import concrete family %q", target.family)) - } - if target.family == "generic" { - if sourceRegistrar { - return nil - } - return importBoundaryViolation(filename, importPath, fmt.Sprintf("concrete family %q may import generic implementations only from its registrar", sourceFamily)) - } - return importBoundaryViolation(filename, importPath, fmt.Sprintf("concrete family %q must not import concrete family %q", sourceFamily, target.family)) + return importBoundaryViolation(filename, importPath, "production code outside module families may import modules only from the CLI composition root through exact registrar packages") } type moduleImportTarget struct { @@ -404,6 +445,15 @@ func isBlackBoxIntegrationTest(filename string, sourcePackage string) bool { return isIntegrationFile(filename) && strings.HasSuffix(filename, "_test.go") && sourcePackage == "integration_test" } +func isCompatibilityTestFile(filename string) bool { + if !strings.HasSuffix(filename, "_test.go") { + return false + } + return strings.HasPrefix(filename, "internal/cli/") || + strings.HasPrefix(filename, "internal/core/") || + strings.HasPrefix(filename, "internal/framework/") +} + func testRepositoryRoot(t *testing.T) string { t.Helper() _, filename, _, ok := runtime.Caller(0) diff --git a/internal/modules/seriatim/input/transcript/config_test.go b/internal/modules/seriatim/input/transcript/config_test.go index 8671de2..2314ee1 100644 --- a/internal/modules/seriatim/input/transcript/config_test.go +++ b/internal/modules/seriatim/input/transcript/config_test.go @@ -293,6 +293,9 @@ func (seriatimArtifactCodec) Schema() contracts.ArtifactSchema { return contracts.ArtifactSchema{ID: "fake.event", Name: "fake_event", Version: "v1", JSONSchema: []byte(`{"type":"object"}`)} } func (seriatimArtifactCodec) MediaType() string { return "application/json" } +func (seriatimArtifactCodec) EncodeCandidate(value seriatimArtifact) ([]byte, error) { + return json.Marshal(value) +} func (seriatimArtifactCodec) Encode(value seriatimArtifact) ([]byte, error) { return json.Marshal(value) }