From 884a62e8a6b675889a2cca1e8b3851ebe7a10de5 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 7 Sep 2026 11:52:57 +0000 Subject: [PATCH] Derive family member publish destinations --- docs/roadmap/remediation.md | 231 ++++++++++++++++++++++ internal/config/artifact_families.go | 1 + internal/config/artifact_families_test.go | 76 +++++++ internal/config/pipeline_configkit.go | 18 +- internal/config/validate_publish.go | 30 +++ 5 files changed, 342 insertions(+), 14 deletions(-) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..f4eb86d --- /dev/null +++ b/docs/roadmap/remediation.md @@ -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 --campaign-file ` 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. diff --git a/internal/config/artifact_families.go b/internal/config/artifact_families.go index ca05e2a..de67ca0 100644 --- a/internal/config/artifact_families.go +++ b/internal/config/artifact_families.go @@ -124,6 +124,7 @@ func expandPipelineArtifactFamilies(cfg *PipelineConfig, party ResolvedParty) er if err := expandFamilyPublishRules(cfg, catalog); err != nil { return err } + applyMissingPublishDestinations(cfg.Publish, cfg.Scriptorium, cfg.Notarius) cfg.Scriptorium.ArtifactFamilies = nil cfg.familyCatalog = catalog cfg.resolution.artifactFamiliesExpanded = true diff --git a/internal/config/artifact_families_test.go b/internal/config/artifact_families_test.go index d5a2d2b..4910198 100644 --- a/internal/config/artifact_families_test.go +++ b/internal/config/artifact_families_test.go @@ -334,6 +334,82 @@ func TestFamilyPublishRulesExpandToConcreteArtifacts(t *testing.T) { } } +func TestExplicitFamilyMemberPublishDestinationResolution(t *testing.T) { + dir := t.TempDir() + writePartyResolutionFile(t, filepath.Join(dir, "party.yml"), `schema_version: narratio.party.v1 +characters: + alice: + player: {name: Alice Player} + character: + name: Alice + classes: [{name: wizard}] +`) + + for _, test := range []struct { + name string + source string + dest string + wantDest string + wantErr string + }{ + { + name: "derive generated member output", + source: "narratio.artifact.meta_alice", + wantDest: "artifacts/alice.md", + }, + { + name: "preserve explicit override", + source: "narratio.artifact.meta_alice", + dest: "published/alice-summary.md", + wantDest: "published/alice-summary.md", + }, + { + name: "reject unknown source", + source: "narratio.artifact.missing", + wantErr: "is unsupported", + }, + } { + t.Run(test.name, func(t *testing.T) { + publishDestination := "" + if test.dest != "" { + publishDestination = " dest: " + test.dest + "\n" + } + pipeline := artifactFamilyPipelineYAML(` + meta: + enabled: true + for_each: party.characters + prompt_id: dnd.meta + profile_id: production + output_path_pattern: artifacts/{character_id}.md +`) + ` +publish: + outputs: + - source: ` + test.source + "\n" + publishDestination + pipelinePath, campaignPath, sessionPath := writeArtifactFamilyConfig(t, dir, pipeline) + cfg, err := LoadWithSessionOptions(pipelinePath, campaignPath, sessionPath, SessionLoadOptions{}) + if test.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), test.wantErr) { + t.Fatalf("LoadWithSessionOptions() error = %v, want %q", err, test.wantErr) + } + return + } + if err != nil { + t.Fatalf("LoadWithSessionOptions() error = %v", err) + } + if got := cfg.Pipeline.Publish.Outputs[0].Dest; got != test.wantDest { + t.Fatalf("resolved runtime destination = %q, want %q", got, test.wantDest) + } + effective, err := MarshalEffectivePipeline(cfg.Pipeline) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(effective), "dest: "+test.wantDest+"\n") { + t.Fatalf("effective pipeline omitted resolved destination:\n%s", effective) + } + }) + } +} + const validFamilyYAML = ` character_meta: enabled: true diff --git a/internal/config/pipeline_configkit.go b/internal/config/pipeline_configkit.go index d50e27f..505eba3 100644 --- a/internal/config/pipeline_configkit.go +++ b/internal/config/pipeline_configkit.go @@ -10,7 +10,6 @@ import ( "strings" configkit "gitea.maximumdirect.net/eric/ConfigKit" - "gitea.maximumdirect.net/eric/narratio/internal/artifactpolicy" ) type pipelineResolutionMetadata struct { @@ -206,19 +205,10 @@ func resolvePipelineConfig(cfg *PipelineConfig, resolution *configkit.Resolution resolution.Record(configkit.Keys("notarius"), configkit.ChangeNormalization, "canonical Notarius configuration") } if cfg.Publish != nil { - configuredOutputs := scriptoriumOutputPathMap(cfg.Scriptorium) - extractionOutputs := notariusOutputKeySet(cfg.Notarius) - for index := range cfg.Publish.Outputs { - output := cfg.Publish.Outputs[index] - if strings.TrimSpace(output.Dest) != "" { - continue - } - destination, err := artifactpolicy.ResolvePublishedDestinationWithExtractions(strings.TrimSpace(output.Source), "", configuredOutputs, extractionOutputs) - if err != nil { - continue - } - configkit.Normalize(resolution, configkit.Keys("publish", "outputs").Index(index).Key("dest"), &output.Dest, destination, "derived from publish source") - cfg.Publish.Outputs[index] = output + for _, derived := range deriveMissingPublishDestinations(cfg.Publish, cfg.Scriptorium, cfg.Notarius) { + output := cfg.Publish.Outputs[derived.index] + configkit.Normalize(resolution, configkit.Keys("publish", "outputs").Index(derived.index).Key("dest"), &output.Dest, derived.dest, "derived from publish source") + cfg.Publish.Outputs[derived.index] = output } } return nil diff --git a/internal/config/validate_publish.go b/internal/config/validate_publish.go index dff6d81..353e48e 100644 --- a/internal/config/validate_publish.go +++ b/internal/config/validate_publish.go @@ -9,6 +9,36 @@ import ( "gitea.maximumdirect.net/eric/narratio/internal/pathsafe" ) +type derivedPublishDestination struct { + index int + dest string +} + +func deriveMissingPublishDestinations(cfg *PublishConfig, scriptorium *ScriptoriumConfig, notarius *NotariusConfig) []derivedPublishDestination { + if cfg == nil { + return nil + } + configuredOutputs := scriptoriumOutputPathMap(scriptorium) + extractionOutputs := notariusOutputKeySet(notarius) + derived := make([]derivedPublishDestination, 0) + for index, output := range cfg.Outputs { + if strings.TrimSpace(output.Dest) != "" { + continue + } + dest, err := artifactpolicy.ResolvePublishedDestinationWithExtractions(strings.TrimSpace(output.Source), "", configuredOutputs, extractionOutputs) + if err == nil { + derived = append(derived, derivedPublishDestination{index: index, dest: dest}) + } + } + return derived +} + +func applyMissingPublishDestinations(cfg *PublishConfig, scriptorium *ScriptoriumConfig, notarius *NotariusConfig) { + for _, derived := range deriveMissingPublishDestinations(cfg, scriptorium, notarius) { + cfg.Outputs[derived.index].Dest = derived.dest + } +} + func validateStorage(cfg StorageConfig) error { backend := strings.ToLower(strings.TrimSpace(cfg.Backend)) switch backend {