From 442469b9f57e65ce17f09059609dd1a37cf8c3b6 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 21 Sep 2026 03:06:13 +0000 Subject: [PATCH] Fix derived publish destinations with explicit requiredness --- docs/releases/v2.1.1.md | 33 ++++ ...x-publish-output-destination-resolution.md | 171 ++++++++++++++++++ internal/config/pipeline_configkit.go | 8 +- internal/config/storage_publish_test.go | 48 +++++ 4 files changed, 259 insertions(+), 1 deletion(-) create mode 100644 docs/releases/v2.1.1.md create mode 100644 docs/roadmap/fix-publish-output-destination-resolution.md diff --git a/docs/releases/v2.1.1.md b/docs/releases/v2.1.1.md new file mode 100644 index 0000000..d2b121f --- /dev/null +++ b/docs/releases/v2.1.1.md @@ -0,0 +1,33 @@ +# Narratio v2.1.1 + +This release fixes configuration loading for publish rules that explicitly set +requiredness while relying on a derived destination. + +## Summary + +Publish rules with a derivable source and omitted `dest` now load correctly +whether `required` is omitted, explicitly true, or explicitly false. Previously, +explicit requiredness could trigger ConfigKit's `resolve.unrecorded_change` +diagnostic instead of accepting valid configuration. + +## Compatibility + +This is a backward-compatible bookkeeping correction. Publish schemas, +destination derivation, requiredness defaults, and YAML syntax support are +unchanged. Explicit destinations and explicit false values remain intact. +No ConfigKit upgrade or publication/storage behavior change is involved. + +## Upgrade + +No special upgrade action or configuration migration is required. Operators +need not add a derivable `dest`, remove explicit `required` values, or change +flow-style YAML to block-style YAML. See the +[configuration reference](../config.md#publish-configuration-summary). + +## Changes + +- Recorded derived publish destinations at ConfigKit's atomic sequence boundary + while retaining field-level resolution records. +- Added real load/validation and safe effective-configuration regression coverage + for built-in and configured-artifact sources, explicit requiredness, and an + explicit destination that must remain unchanged. diff --git a/docs/roadmap/fix-publish-output-destination-resolution.md b/docs/roadmap/fix-publish-output-destination-resolution.md new file mode 100644 index 0000000..cfa35a6 --- /dev/null +++ b/docs/roadmap/fix-publish-output-destination-resolution.md @@ -0,0 +1,171 @@ +# Bugfix Request: Preserve Publish Destination Derivation with Explicit Rules + +Status: Implemented; release target v2.1.1 + +## Summary + +Narratio should successfully derive a publish destination when a valid +`publish.outputs` rule omits `dest`, regardless of whether that rule explicitly +sets `required`. Narratio v2.1.0 currently rejects this valid configuration in +some cases with a ConfigKit `resolve.unrecorded_change` diagnostic. + +This is a configuration-resolution bookkeeping defect, not a YAML formatting +restriction. The fix should preserve the existing public configuration +contract: operators may omit a derivable destination and may independently set +`required` to either `true` or `false`. + +## Observed Behavior + +A pipeline containing a rule equivalent to the following fails during native +configuration loading: + +```yaml +publish: + outputs: [{"source": "narratio.transcript.final", "required": true}] +``` + +Narratio reports: + +```text +error [resolve.unrecorded_change] /publish/outputs: application resolution changed a value without a covering record +``` + +The same failure is possible with ordinary block-style YAML when `required` is +explicit and `dest` is omitted. Flow-style YAML is valid input and is not the +cause of the defect. + +The existing destination-derivation tests do not expose this case because +their rules also omit `required`. Narratio then supplies the default value for +`required` and records a change covering the complete `publish.outputs` +sequence. That broader record happens to cover the destination mutation as +well, masking the missing resolution record that occurs when `required` is +already explicit. + +## Expected Contract + +The current configuration contract remains unchanged: + +- `publish.outputs[].source` is required; +- `publish.outputs[].dest` may be omitted when Narratio can derive it from the + selected built-in or configured artifact source; +- `publish.outputs[].required` defaults to `true`, but an operator may set it + explicitly to either `true` or `false`; and +- equivalent valid YAML representations have equivalent configuration + semantics. + +For example, both rules below should load and validate, retain their explicit +requiredness, and receive the canonical derived destination: + +```yaml +publish: + outputs: + - source: narratio.transcript.final + required: true + - source: narratio.transcript.final_markdown + required: false +``` + +## Root Cause + +ConfigKit intentionally treats sequences as atomic when comparing values +before and after application resolution. Changing a field within one +`publish.outputs` entry is therefore observed as a change to the +`/publish/outputs` sequence. + +Narratio currently derives each missing destination by recording a +normalization at the individual `publish.outputs[index].dest` path. That child +record does not cover the atomic parent-sequence change. A separate +container-level record is currently created only when Narratio also defaults +an omitted `required` value, so configurations with explicit `required` values +lose that incidental coverage. + +## Recommended Fix + +Make Narratio's publish-output resolution bookkeeping conform to ConfigKit's +atomic-sequence contract. Whenever resolution derives one or more destinations +inside `publish.outputs`, the recorded resolution changes must cover the +`/publish/outputs` container. Destination derivation must not depend on an +unrelated `required` default being applied in the same list. + +The implementation may retain more specific provenance where useful, but it +must provide valid container-level coverage for the sequence mutation. Do not +change ConfigKit's sequence comparison semantics merely to accommodate this +call site. + +## Scope + +- Correct destination derivation for publish rules that explicitly set + `required` and omit a derivable `dest`. +- Preserve explicit `required: true` and `required: false` values. +- Preserve canonical destination derivation for built-in transcript sources + and configured artifact sources. +- Add focused regression coverage through Narratio's real configuration load + and validation boundary. +- Prepare a backward-compatible patch release after the fix passes the normal + repository validation. + +## Non-goals + +- Changing the `publish.outputs` schema, defaults, source vocabulary, or + destination derivation rules. +- Requiring operators or configuration generators to supply `dest` when it is + derivable. +- Requiring operators to omit explicit `required: true` values. +- Treating flow-style YAML or JSON-compatible YAML values as unsupported. +- Changing ConfigKit's documented atomic treatment of sequences. +- Adding storage, publication, or network integration behavior. + +## Regression Coverage + +Add one focused package-level behavioral test that loads and validates a +pipeline with omitted destinations and explicit requiredness. A representative +list should include both `required: true` and `required: false`, then assert +that: + +- loading and validation succeed; +- each destination is derived from its source; and +- each explicit `required` value is preserved. + +Using the compact flow-style representation that exposed the integration +failure is reasonable, while the existing block-style cases continue to cover +omitted `required`. The test should exercise the public configuration behavior, +not ConfigKit call choreography or Narratio's private recording mechanism. + +No network service, object store, credential, or test double is required. + +## Target End State + +All valid publish rules with derivable sources behave consistently across +these cases: + +| `dest` | `required` | Result | +| --- | --- | --- | +| omitted | omitted | destination derived; required defaults to `true` | +| omitted | `true` | destination derived; explicit `true` retained | +| omitted | `false` | destination derived; explicit `false` retained | +| explicit | either value | explicit destination and requiredness retained | + +`narratio config validate`, `narratio config show`, and ordinary pipeline +loading accept the corrected cases without an unrecorded-change diagnostic. +No operator configuration migration or workaround is required. + +## Release + +This is a backward-compatible correction to documented configuration behavior +and should be released as Narratio `v2.1.1`. The release note should identify +the corrected explicit-required/derived-destination interaction and state that +no special upgrade action is required. + +## Acceptance Criteria + +- The focused regression test fails against v2.1.0 behavior and passes after + the fix. +- Publish rules with explicit `required: true` or `required: false` and omitted + derivable destinations load and validate successfully. +- Derived destinations and explicit requiredness appear correctly in the safe + effective configuration. +- Existing destination derivation, publish validation, imports, profiles, and + configuration inspection tests remain green. +- The complete repository validation and release-candidate checks pass. +- The correction is published as the immutable `v2.1.1` tag with the required + release note. diff --git a/internal/config/pipeline_configkit.go b/internal/config/pipeline_configkit.go index f510631..c274553 100644 --- a/internal/config/pipeline_configkit.go +++ b/internal/config/pipeline_configkit.go @@ -209,11 +209,17 @@ func resolvePipelineConfig(cfg *PipelineConfig, resolution *configkit.Resolution resolution.Record(configkit.Keys("notarius"), configkit.ChangeNormalization, "canonical Notarius configuration") } if cfg.Publish != nil { - for _, derived := range deriveMissingPublishDestinations(cfg.Publish, cfg.Scriptorium, cfg.Notarius) { + destinations := deriveMissingPublishDestinations(cfg.Publish, cfg.Scriptorium, cfg.Notarius) + for _, derived := range destinations { 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 } + if len(destinations) > 0 { + // ConfigKit compares sequences atomically; child records alone do not + // cover the list mutation when requiredness was already explicit. + resolution.Record(configkit.Keys("publish", "outputs"), configkit.ChangeNormalization, "derived publish destinations") + } } return nil } diff --git a/internal/config/storage_publish_test.go b/internal/config/storage_publish_test.go index a083d76..6082dad 100644 --- a/internal/config/storage_publish_test.go +++ b/internal/config/storage_publish_test.go @@ -3,6 +3,8 @@ package config import ( "strings" "testing" + + "gopkg.in/yaml.v3" ) func TestStorageS3DefaultsAndValidation(t *testing.T) { @@ -549,6 +551,52 @@ publish: } } +func TestPublishDerivedDestinationsPreserveExplicitRequiredness(t *testing.T) { + pipelinePath, sessionPath := writeConfigFiles(t, testPipelineBaseYAML+` +scriptorium: + artifacts: + session_recap: + enabled: true + prompt_id: dnd.session_recap + output_path: artifacts/session_recap.md +publish: + outputs: + - {source: narratio.transcript.final, required: true} + - {source: narratio.transcript.final_markdown, required: false} + - {source: narratio.artifact.session_recap, required: false} + - {source: narratio.transcript.final_trimmed, dest: custom/trimmed.json, required: true} +`, testSessionBaseYAML) + cfg, err := Load(pipelinePath, sessionPath) + if err != nil { + t.Fatal(err) + } + if err := Validate(cfg); err != nil { + t.Fatal(err) + } + effective, err := MarshalEffectivePipeline(cfg.Pipeline) + if err != nil { + t.Fatal(err) + } + var projected struct { + Publish PublishConfig `yaml:"publish"` + } + if err := yaml.Unmarshal(effective, &projected); err != nil { + t.Fatal(err) + } + wantDest := []string{"transcripts/final.json", "transcripts/final.md", "artifacts/session_recap.md", "custom/trimmed.json"} + wantRequired := []bool{true, false, false, true} + for label, publish := range map[string]*PublishConfig{"loaded": cfg.Pipeline.Publish, "effective": &projected.Publish} { + if len(publish.Outputs) != len(wantDest) { + t.Fatalf("%s output count = %d, want %d", label, len(publish.Outputs), len(wantDest)) + } + for i, output := range publish.Outputs { + if output.Dest != wantDest[i] || output.Required == nil || *output.Required != wantRequired[i] { + t.Errorf("%s output %d = %#v, want destination %q and required %t", label, i, output, wantDest[i], wantRequired[i]) + } + } + } +} + func TestScriptoriumOutputPathsIgnoreInvalidKeys(t *testing.T) { scriptorium := &ScriptoriumConfig{Artifacts: map[string]ScriptoriumArtifactConfig{ " artifact": {OutputPath: "artifacts/invalid.md"},