diff --git a/docs/adr/0004-package-modules-by-domain.md b/docs/adr/0004-package-modules-by-domain.md index 3ddbd8e3..1e42c6d8 100644 --- a/docs/adr/0004-package-modules-by-domain.md +++ b/docs/adr/0004-package-modules-by-domain.md @@ -36,7 +36,8 @@ as `dnd.SpellList` without creating a Go import cycle. The `generic` tree is a peer extension family for reusable implementations that contain no concrete source-format or artifact-domain knowledge. Source-format and output-format families, such as Seriatim and JSON output, follow the same -domain-first organization even when they do not define a Zone-B artifact type. +domain-first organization even when they do not define a type in the +[domain artifact zone](0003-typed-interfaces-with-two-zone-data-model.md#domain-artifact-zone). Concrete domain implementation packages do not import another concrete domain. Generic extension packages never import concrete domains. A domain registrar diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index a1550ab0..46ad177d 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,48 +1,453 @@ -# Domain-Typed Pipeline Completion Record +# Domain-Typed Pipeline Implementation Roadmap ## Status -Implemented on 2026-07-17. +The domain-typed pipeline was implemented on 2026-07-17. A subsequent review +identified five follow-up issues. The stages below are the decision-complete +implementation plan for resolving them. -This file is a concise historical record. Current behavior is owned by -[Architecture](../policy/architecture.md), -[Configuration](../config.md), [Operations](../operations.md), -[Pipeline Internals](../internal/pipeline.md), -[Module Internals](../internal/modules.md), -[LLM Runtime Internals](../internal/llm.md), and the durable -[integration contracts](../integrations/). The accepted decisions remain in +Implement the 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 +## Delivered Baseline -- Production extensions use domain-first packages and package-family - registrars without changing selectable module or validator keys. -- Source units and chunks carry canonical engine-owned provenance using - `source.SourceRef`; workspace checkpoints use schema v2. -- Artifact lanes keep one domain-owned Go type through extraction, merge, - normalization, and typed validation. -- Artifact codecs own stable schema-aware checkpoint, debug, and output - serialization; generic representation validators consume serialized views. -- Resolution verifies artifact-kind, codec, typed variant, capability, - reference, validator, and option compatibility before source work. -- Preparation constructs the full run-local implementation set and injects one - shared scheduled LLM client. -- Extraction uses bounded chunk-first, lane-second dispatch. Lane continuations - are also bounded, public results are deterministic, and framework failures - cancel undispatched work without treating rejections as errors. -- Maintained D&D payloads, logical output paths, prompt and schema identities, - default validators, warnings, rejections, and manifest provenance remain - covered by compatibility tests. +The existing implementation already provides: -## Completion Evidence +- 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. -Focused tests cover durable compatibility, typed resolution and preparation, -codec boundaries, checkpoint invalidation, debug recording, bounded scheduling, -reverse completion, cancellation, rejection, retries, and independent extract -worker and provider-call limits. Maintained example configurations and input are -validated and exercised by the CLI test suite. +The following work corrects checkpoint identity, completes retry debugging, +removes kind-ambiguous registry lookup, strengthens architectural enforcement, +and removes the obsolete sequential extraction path. -Repository validation is defined in [Development](../development.md). The Go -race suite requires a CGO-capable toolchain in the execution environment. +## 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 + ``` + +## 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. + +## Final Verification And Closeout + +After all five stages: + +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 changing the registered default spell validator chain selects a + different checkpoint identity. +4. Verify merge and normalize attempt debug behavior with instrumented + LLM-backed test modules. +5. Confirm current production imports satisfy the generic boundary guard. +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 these five remediation stages +are specified above.