narratio/docs/roadmap/remediation.md

# ConfigKit Migration Review — Required Remediations

Reviewed 2026-09-07 at `f7a40af`, against the changes after `1558c9c`, the
[feature roadmap](configkit-adoption.md), and the completed
[implementation plan](implementation.md).

The main migration is implemented. The following findings are reproducible gaps
in the intended behavior, not proposals to restore retired compatibility or
filesystem restrictions. P2 denotes a correctness issue to fix before release;
P3 denotes an operator-facing completeness issue also required by the accepted
plan. No application code was changed during this review.

## 1. Derive explicit publish destinations after family expansion

Status: Resolved — P2

Resolution: Missing destinations are now derived through one shared domain helper
both during ConfigKit resolution and again after concrete family expansion. Explicit
destinations remain unchanged, while unknown sources still fail read-only publish
validation. Regression coverage verifies resolved runtime and effective inspection
output for generated members, explicit overrides, and unknown sources.

**Evidence:** `internal/config/pipeline_configkit.go:208` derives missing publish
destinations during the ConfigKit resolver. A concrete family member does not
exist yet, so the resolver ignores the failed derivation. After family expansion,
`internal/config/artifact_families.go:141` calls the now-read-only publish
validator, which rejects the still-empty destination. The previous implementation
could derive it during that later validation.

**Reproduction:** In an otherwise valid pipeline, define a family `meta` over
`party.characters`, with output pattern `artifacts/{character_id}.md`. Select a
valid campaign whose party contains `alice`, and add:

```yaml
publish:
  outputs:
    - source: narratio.artifact.meta_alice
```

`config validate --config <pipeline> --campaign-file <campaign>` fails with
`pipeline.publish.outputs[0].dest is required when destination cannot be derived
from "narratio.artifact.meta_alice"`. Adding `dest: artifacts/alice.md` makes the
same configuration pass. The destination is therefore available and derivable;
the lifecycle omits the required final normalization step.

**Required change:** Complete publish destination resolution after concrete
artifacts exist, before publish validation and final projection/digest generation.
Share the domain derivation logic and keep validation read-only. Preserve
explicit destinations and generated family publish-rule collision checks. Do not
require redundant `dest` fields or revive mutation inside validators.

**Acceptance:** Test an explicit publish source naming a generated member with
omitted destination, an explicit destination override, and a genuinely unknown
source. Validate both effective inspection and resolved runtime configuration.

## 2. Stop re-defaulting an explicitly empty Notarius working directory

Status: Resolved — P2

Resolution: Removed both legacy blank-value fallbacks after ConfigKit resolution.
An omitted working directory is still derived by the presence-aware resolver, an
explicit relative directory is still rooted at the pipeline, and explicit empty or
whitespace-only values remain empty and fail enabled-Notarius validation. Existing
relocation-invariant digest coverage remains green.

**Evidence:** ConfigKit's resolver correctly uses presence-aware defaulting at
`internal/config/pipeline_configkit.go:180`. However,
`internal/config/load.go:454` subsequently replaces any blank working directory
with the config file's directory, irrespective of input presence.
`internal/config/effective_digest.go` also applies a blank-value fallback while
capturing logical paths. These retained legacy fallbacks bypass the new rule that
defaults apply only to absent fields.

**Reproduction:** Set `notarius.enabled: true`, a valid config path, pipeline ID
and output map, and `notarius.working_directory: ""`. With
`config_path: notarius/config.yml`, `config show` succeeds and reports
`working_directory: notarius`; validation never sees the explicit invalid blank.

**Required change:** Let the ConfigKit resolver own the absent-field default.
The subsequent runtime and logical-path conversions must preserve explicit
emptiness so enabled-Notarius validation can reject it. Remove duplicate
value-based defaulting rather than adding another private presence mechanism.

**Acceptance:** Omission still derives the expected directory; explicit empty
and whitespace-only values fail when Notarius is enabled; a configured relative
directory resolves from the pipeline root. Preserve digest relocation invariance.

## 3. Enforce unique base definitions even when a profile removes them

Status: Resolved — P2

Resolution: Loading now discovers base declarations through ConfigKit's public
historical-path enumeration and metadata history from the selected result,
without separately resolving the base (see finding 7). Duplicate ordinary or
family definitions therefore fail even when an overlay nulls their container or
all of Scriptorium. A single base owner
with a legitimate profile override remains supported.

**Evidence:** `rejectDuplicateBaseDefinitions` at
`internal/config/pipeline_configkit.go:354` enumerates only artifact/family keys
remaining in the final typed configuration. It inspects history for those keys
but never examines base declarations removed by an overlay.

**Reproduction:** Import `first.yml` defining
`scriptorium.artifacts.recap.prompt_id` and `second.yml` defining
`scriptorium.artifacts.recap.output_path`. Base validation correctly rejects the
two-file declaration. Add `profiles/clear.yml` containing:

```yaml
scriptorium:
  artifacts: null
```

The same root with `--profile clear` passes. A selected profile can thus hide the
base ownership conflict, contrary to the plan's unique base-definition rule.
This concerns disjoint contributions to one artifact, not ConfigKit's generic
conflicting-leaf checks, which still work.

**Required change:** Discover the complete base declaration set through public
ConfigKit metadata/history, including declarations displaced by an overlay, and
validate ownership independently of final effective-key survival. Keep legitimate
profile overrides and profile-only definitions supported. Do not reparse source
YAML or add a second composer.

**Acceptance:** Cover duplicate ordinary and family definitions hidden by a null
artifact/family container or null Scriptorium section. Base and selected-profile
loads must reject the same base conflict, while a single base declaration with a
valid overlay remains accepted.

## 4. Preserve null versus empty collections in effective YAML

Status: Resolved — P2

Resolution: Effective YAML is now emitted from the credential-safe JSON value
projection through an output-only YAML-node conversion. Explicit nulls, empty maps,
empty slices, booleans, strings, and exact JSON numbers retain their semantic types.
Public show/diff tests cover null and empty maps and slices, while digest,
credential-redaction, deterministic output, and exact-integer coverage remain intact.

**Evidence:** `internal/config/effective_pipeline.go:154` marshals the typed
projection directly with `yaml.Marshal`. The encoder emits nil slices as `[]`
and nil maps as `{}`, while the JSON projection used by diff and digest preserves
them as `null`. The shared input type does not guarantee shared output semantics.

**Reproduction:** Set `audita.modules: null` in a valid base and
`audita.modules: []` in `profiles/empty.yml`. Both `config show` invocations emit
`modules: []`, but `config diff default empty` reports:

```text
changed audita.modules null []
```

The commands disagree about the effective value. The same renderer also supplies
the resolved pipeline diagnostic written by prepare.

**Required change:** Render YAML from a credential-safe projection that explicitly
preserves nulls and empty collections, consistently with the semantic projection.
Keep exact integers and safe credential reference names. An output-only conversion
is appropriate; do not restore YAML decoding or a configuration parser. This
requires truthful inspection, not a guarantee that inspection output is reloadable.

**Acceptance:** Verify null versus empty maps and slices through the public show
boundary, and agreement with diff/digest distinctions. Retain credential-leakage
and exact-integer regression coverage.

## 5. Complete provenance for generated artifacts and inferred dependencies

Status: Resolved — P3

Resolution: Generated catalog records now collect the complete base/profile family
definition set from ConfigKit history. Effective source ownership for materialized
dependency lists is rebuilt narrowly from retained explicit-list and input-edge
origins, including mixed and generated-family dependencies. CLI tests verify a
profiled family catalog agrees with generated source attribution and that inferred
ordinary edges name their input contributor instead of the central default.

Two new reporting paths lose available declaration information:

- **Profile contributions to family members:**
  `internal/config/artifact_catalog.go:52` fills a generated member's definitions
  from the single `member.Source`. That source is chosen by the first matching
  non-default family field in `artifactFamilySource`, not the full set of effective
  contributors. Define family `meta` in the base and override its `prompt_id` in
  `profiles/testing.yml`. `config artifacts --profile testing` reports only the
  base file for `meta_alice`, even though `config sources` correctly attributes
  that member's prompt to the testing overlay.
- **Inferred ordinary dependencies:** With `second.inputs.prior.source` set to
  `narratio.artifact.first` and no explicit dependency list, `config sources`
  reports `scriptorium.artifacts.second.depends_on` as `default / default`.
  `finalizeArtifactDependencies` stores the input-edge origin, but the effective
  ownership projection retains the pre-inference default attribution from
  `internal/config/effective_digest.go:55` and does not consult that origin.

**Impact:** Operators inspecting a testing profile or following a dependency can
be directed to the wrong file or told a user-derived value is a central default.
Runtime ordering and generated artifact values were correct in these reproductions.

**Required change:** Use ConfigKit's contributing locations for family definition
sets, as for ordinary artifacts, and add narrow generated attribution for inferred
dependency lists using their retained edge origins. Preserve family/character/party
identity and deterministic ordering. Avoid introducing a second general provenance
store or choosing a file based on the first alphabetically visited field.

**Acceptance:** A profiled family catalog exposes both relevant files; an inferred
edge points back to its input declaration; mixed explicit/inferred dependencies
retain their relevant contributors. Verify catalog and sources agree after expansion.

## 6. Surface successful secrets bootstrap reports, including skipped empty variables

Status: Resolved — P3

Resolution: Successful bootstrap reports now flow once through the command
diagnostics channel, while direct internal command calls default that channel to
discard. ConfigKit's value-free report makes skipped existing empty variables
explicit and remains available before later typed-load failures; structured output
stays isolated on stdout. Command-level coverage verifies successful reporting,
skipped-empty reporting, value redaction, clean config-show output, and one report
for a two-profile diff.

**Evidence:** `internal/app/secrets_bootstrap.go:36` discards the report whenever
`LoadSecretsDir` returns no error. Reports are only rendered when directory
bootstrap itself fails. This does not implement the roadmap's safe bootstrap
reporting boundary or Stage 6's warning presentation requirement.

**Reproduction:** Place a synthetic key in `secrets/NARRATIO_REVIEW_KEY`, reference
it from `audita.llm_api_key`, and invoke config validation with
`NARRATIO_REVIEW_KEY=` in the environment. ConfigKit correctly skips the file
because the variable already exists, then typed loading fails with `secret.empty`.
Narratio shows only that load failure, omitting the bootstrap report's
`skipped_existing` and `ExistingEmpty` information. A successful load likewise
discards all bootstrap results.

**Required change:** Retain and render or summarize the safe report once per
invocation through a diagnostic output channel, including when a later typed load
fails. Make skipped-existing-empty outcomes explicit. Preserve process environment
precedence and the existing partial report on failure; keep values out of all
diagnostics and keep structured/YAML stdout uncontaminated. Do not add bootstrap
rollback, retries, or repeated directory loading.

**Acceptance:** Command-level tests demonstrate safe success/report output and a
useful skipped-empty diagnostic, without leaking a synthetic secret or polluting
show output. A two-profile diff still bootstraps and reports only once.

## 7. Do not resolve unused base credentials when loading a profile

Status: Resolved — P2

Resolution: Upgraded to released ConfigKit v0.1.2 and replaced the extra base load
with `Metadata.HistoricalPaths()` enumeration of declaration paths in the selected
result. Ownership checks still inspect base-layer history for both ordinary
artifacts and families, even when overlays remove them. Regression tests verify
that a profile can replace an unset production credential with a resolvable testing
reference or remove it with null. Existing hidden-duplicate and legitimate-override
tests remain intact. No local dependency replacement or second composer is needed.

Follow-up review: 2026-09-07 at `3ce1981`. The original six reproductions are
addressed, but the fix for finding 3 introduces this regression.

**Evidence:** `internal/config/pipeline_configkit.go:49-57` loads the base again
without a profile to check duplicate base definitions. This calls the complete
typed ConfigKit loader, including secret resolution, domain resolution, and
validation—not just declaration ownership inspection. Consequently, loading a
valid testing profile also requires credentials from the unselected base.

**Reproduction:** In an otherwise valid base, set `audita.llm_api_key` to
`NARRATIO_REVIEW_PROD_KEY`. In `profiles/testing.yml`, replace it with
`NARRATIO_REVIEW_TEST_KEY`. Leave the production variable unset and supply only
the testing variable with a synthetic value. `config validate --config <root>
--profile testing` fails with `secret.unset` at the base credential declaration.
Supplying both variables makes it pass. A profile that instead sets
`audita.llm_api_key: null` also fails when the unused production variable is
unset. Both failures were reproduced through the current CLI.

**Impact:** Operators cannot use an independent testing credential environment
or remove a production credential requirement through a profile. Commands fail
despite having all credentials required by their effective configuration. The
extra full load also unnecessarily repeats configuration processing.

**Required change:** Enforce unique base ownership without requiring the
unselected base to succeed as a separately resolved runtime configuration.
Inspect complete declaration history through ConfigKit's public metadata,
including declarations displaced by overlays. If additional upstream metadata
support is needed, address that explicitly rather than requiring unused secrets,
suppressing arbitrary load failures, reparsing YAML, or restoring a second
composer. Preserve the duplicate-definition protection added for finding 3.

**Acceptance:** Add public-boundary regression tests for a profile replacing an
unset production credential with an available testing credential, and for a
profile removing that credential with null. Both must load successfully without
the production variable. Retain failures for duplicate base ordinary/family
definitions hidden by null overlays, along with legitimate profile override and
profile-only definition coverage. Resolve secrets only for the selected effective
configuration.

## Follow-up verification

After adopting ConfigKit v0.1.2 and completing finding 7, the full
`scripts/check-release-candidate.sh v2.0.0` check passed: module hygiene,
formatting/whitespace, uncached tests and race tests, vet, build, documentation,
maintained examples, and Linux amd64/arm64 plus macOS arm64 release cross-builds.
`go mod verify` also passed. The new credential regressions failed before the
ownership-check fix and passed afterward. No local module replacements, release
tags, or pushes were used.

At `3ce1981`, `go test ./...`, `go test -race ./...`, `go vet ./...`, and
`go build ./...` passed. The new finding was reproduced with a freshly built CLI,
temporary configuration files, and synthetic credentials; no external adapters
or cloud services were invoked. That review pass changed only this review record.

## Original verification and scope

The following checks passed at the reviewed commit:

- `go test ./...`
- `go test -race ./...`
- `go vet ./...`
- `go build ./...`
- The release asset builder for `v2.0.0`, producing exactly Linux amd64, Linux
  arm64, and macOS arm64 assets; the Linux amd64 binary reported the expected version.

The existing full suites include documentation and maintained-example checks.
The findings above were additionally reproduced through a freshly built CLI with
isolated temporary YAML fixtures and synthetic credentials. No live downstream
tools or cloud services were invoked. Linux arm64 and macOS arm64 were cross-built,
not tested natively. No tag was created or pushed.

The migration removes a net 1,235 production Go lines and 843 test Go lines versus
`1558c9c` (physical diff lines, including comments and whitespace). ConfigKit owns
generic loading/composition; Narratio retains domain resolution, orchestration,
safe inspection, and adapter delivery. These findings require completing that
design, not reversing the simplification.