narratio/docs/roadmap/remediation.md

ConfigKit Migration Review — Required Remediations

Reviewed 2026-09-07 at f7a40af, against the changes after 1558c9c, the feature roadmap, and the completed implementation plan.

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:

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:

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:

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.