|
|
|
|
@@ -0,0 +1,231 @@
|
|
|
|
|
# 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: Open — P2
|
|
|
|
|
|
|
|
|
|
**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: Open — P2
|
|
|
|
|
|
|
|
|
|
**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: Open — P2
|
|
|
|
|
|
|
|
|
|
**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: Open — P3
|
|
|
|
|
|
|
|
|
|
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: Open — P3
|
|
|
|
|
|
|
|
|
|
**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.
|
|
|
|
|
|
|
|
|
|
## 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.
|
