notarius/docs/roadmap/remediation.md

# ConfigKit Adoption Review Findings

## Scope And Assessment

Reviewed the adoption commits from `c403a38` through `3010a91` against
[the feature roadmap](configkit.md) and [the implementation plan](implementation.md).
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`:

```yaml
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:

```yaml
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

Status: Addressed. CLI load errors include a quoted source legend; command tests
cover imported/profile failures, source coordinates, and terminal-control escaping.

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

Status: Addressed. The validator example uses canonical objects and passes
production selected-pipeline validation; the loader comment describes current behavior.

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.

### Remediation Verification

All five findings are addressed. Added load-boundary regressions cover sequence
normalization, explicit branch validity, and worker-key presence; CLI tests cover
source legends. After these changes, the full Go tests, vet, and build passed,
as did production selected-pipeline validation of the corrected documentation
snippet and the minimal, complete, and development-profile examples.