notarius/docs/roadmap/remediation.md

ConfigKit Adoption Review Findings

Scope And Assessment

Reviewed the adoption commits from c403a38 through 3010a91 against the feature roadmap and the implementation plan. The main architecture is implemented: ConfigKit v0.3.0 owns composition, decoding, provenance, and frozen inspection; Notarius owns document semantics, source-aware path policy, detached runtime conversion, and catalog resolution. The old YAML loader and operational environment-overlay layer are removed.

The following findings require remediation. No application code was changed during this review. Reproductions used temporary configuration files and the built production CLI, without provider calls.

Finding 1 — Record Normalization At Sequence Boundaries

Status: Addressed. Scoped normalization records now cover steps and validators; load-boundary regressions verify normalized runtime values and inspection.

Priority: P1. Valid configuration can fail loading.

Evidence: internal/core/config/pipeline_document.go:194 records individual step-field changes; validator normalization at the same file's binding traversal similarly records individual validator fields. ConfigKit audits these sequence changes at the enclosing sequence boundary. The special-case steps record in document_paths.go covers relative-reference resolution, but not other sequence normalization.

This otherwise valid pipeline fails plain config validate:

version: 5
pipelines:
  session:
    input: {module: seriatim}
    steps:
      - id: " first "
        artifacts:
          spells:
            extract: {module: dnd/spells}

Observed: resolve.unrecorded_change at /pipelines/session/steps. Likewise, a single-step extractor with validators: [{module: " generic/valid_json "}] fails at the validators sequence. Accepted whitespace normalization must not depend on the presence of an unrelated relative reference or enclosing map-key rename.

Required correction:

• Record legitimate sequence transformations at the covering boundary required by ConfigKit, including nested validators and ordered steps.
• Consolidate the relative-reference-specific workaround into a coherent normalization-recording approach. Do not disable the audit or record the entire configuration indiscriminately.
• Preserve original-path lookup and useful normalization reasons.

Acceptance: the two reproductions load and normalize correctly; nested module/profile/selector/key changes in ordered steps also work; effective values match runtime values and explanation retains declaration provenance. Add focused load-boundary regression tests without duplicating upstream tests.

Finding 2 — Preserve Explicit Presence In Document Validation

Status: Addressed. Plain loading rejects unsupported empty validator overrides and invalid reference branches; supported empty overrides retain their meaning.

Priority: P2. Plain validation and runtime conversion disagree about invalid configuration.

Two concrete gaps in internal/core/config/pipeline_document.go:

• At line 405, disallowed validators are checked with len > 0. input: {module: seriatim, validators: []} passes plain config validate, but selected-pipeline validation rejects it because the runtime override is explicitly set. Output bindings have the same gap.
• At line 428, reference source selection tests whether the path is nonblank, rather than whether the branch was supplied. A binding-local reference {path: "", artifact: {step: previous, lane: registry}} passes plain validation. Conversion in loaded_config.go then silently drops the explicitly invalid path branch and selects the artifact. Whitespace-only paths have the same problem.

Required correction:

• Reject an explicitly configured validator override on input/output, including an empty list, while retaining empty-list disabling on supported stages and the separately agreed empty lane-level legacy-list behavior.
• Validate reference union branch presence separately from branch content. Reject a supplied empty path and reject two supplied branches; retain existing null rejection and valid external/generated forms.
• Keep presence-only rules in the document boundary and share semantic predicates with runtime validation where practical. Do not add a second complete validation pass simply to compensate for these omissions.

Acceptance: both invalid examples fail plain loading with field-specific diagnostics before runtime conversion; valid empty validator overrides on chunk/extract/merge/normalize and valid reference forms remain accepted.

Finding 3 — Restore Worker-Key Normalization Without Losing Explicitness

Status: Addressed. Worker keys normalize before defaulting; original input presence preserves explicit counts. Regression tests cover defaults and collisions.

Priority: P2. The migration introduced an unplanned restriction.

The former normalizeStageWorkers trimmed keys and rejected normalized collisions. The new operational resolver does not normalize worker keys before deriveDocumentExtractWorkers; validateStageWorkerKey requires the literal extract. The following formerly supported input is now rejected:

version: 5
concurrency:
  stage_workers:
    " extract ": 2

Observed: validate.error reports the padded key as unsupported. The target retains whitespace-normalized worker identifiers.

Required correction:

• Normalize worker keys with deterministic collision checking and a recorded map transformation before applying derived defaults.
• Preserve the original declaration's explicitness: ConfigKit presence queries use entry-state paths, so checking only the newly canonical key would mistake a moved explicit worker count for an omitted count and overwrite it.
• Reuse the existing map/path machinery where useful rather than adding a parallel normalization framework.

Acceptance: the example retains a worker count of 2, omitted workers derive from effective total concurrency, explicit invalid counts still fail, and extract plus " extract " fails as a collision. Inspection explains the normalization instead of reporting a synthetic default for the explicit value.

Finding 4 — Include Source Filenames In Load Diagnostics

Priority: P2. Import/profile failures lack actionable file attribution.

internal/cli/configkit_load.go:29 calls WriteDiagnostics with diagnostics only and discards LoadError.Sources(). Errors display locations such as (source-1:6:7) without any filename or source-ID legend. With composed configurations, an operator cannot identify the file to edit from that output. Stage 8 explicitly called for safe source context using source records.

Required correction:

• Continue using ConfigKit's canonical diagnostic renderer and add a small, deterministic source-ID-to-file/label presentation using its public source records. A source legend is sufficient; do not rebuild a diagnostic framework.
• Quote/escape source labels safely, and keep all diagnostics on stderr. Do not print configuration contents or wrap the same failure repeatedly.
• Cover an error originating in an imported file or selected profile, not only a main-file failure.

Acceptance: stderr identifies the actual declaring filename alongside the diagnostic's source ID, line, and column; stdout remains empty on failure; no credentials, raw YAML, or unescaped terminal controls are added.

Finding 5 — Finish Current Documentation And Comment Migration

Priority: P2 for the unusable operator example; P3 for the stale comment.

docs/config.md:539 explains how to enable the combat semantic validator, but five entries in its YAML snippet still use scalar validator shorthand. Version 5 requires objects. Copying even its first scalar entry produces decode.type: “configuration value must be a mapping.”

internal/core/config/load.go:18 also still describes the production loader as under construction and claims commands use the deleted version 4 loader.

Required correction:

• Convert every validator in that current example to the canonical module: object form, preserving order and retry settings.
• Check other current snippets for the same migration omission, excluding intentional historical/migration examples.
• Replace the obsolete loader comment with its implemented responsibility.

Acceptance: a minimal v5 configuration containing the documented validator chain passes production selected-pipeline validation offline, and current comments no longer describe a dual-loader transition.

Verification

Already passed during review:

• go test ./...
• go vet ./...
• Production CLI build.
• Selected-pipeline validation of both maintained D&D examples, plus the complete example with the development configuration profile.
• Effective/explanation inspection smoke checks.

The existing green suite does not cover the failing edge cases above. After remediation, add the focused behavioral coverage described per finding, rerun the full checks, and validate the corrected documentation example. No live LLM tests are needed.