From ed4623bc415901134c765fe9cf06d59aa7891500 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 7 Sep 2026 13:10:29 +0000 Subject: [PATCH] Fix profile loading with ConfigKit history --- docs/releases/v2.0.0.md | 10 +-- docs/roadmap/remediation.md | 77 ++++++++++++++++++++-- go.mod | 2 +- go.sum | 4 +- internal/config/pipeline_configkit.go | 53 ++++++--------- internal/config/pipeline_configkit_test.go | 29 ++++++++ 6 files changed, 130 insertions(+), 45 deletions(-) diff --git a/docs/releases/v2.0.0.md b/docs/releases/v2.0.0.md index c08e7e2..4931e0e 100644 --- a/docs/releases/v2.0.0.md +++ b/docs/releases/v2.0.0.md @@ -6,7 +6,7 @@ their effective dependency graph directly inspectable. ## Summary -Pipeline roots now use ConfigKit v0.1.1 for strict YAML loading, explicit +Pipeline roots now use ConfigKit v0.1.2 for strict YAML loading, explicit imports, fixed-path named profiles, provenance, defaults, and typed credential references. The recommended production layout keeps production values in the base, defines each ordinary artifact and artifact family in its own imported @@ -32,8 +32,10 @@ notes remain historical descriptions of their releases. Inspection commands still avoid session, manifest, workspace, adapter, and remote-service activity, but they are not credential-free loaders: every -explicit typed credential reference must resolve. Pipelines containing artifact -families also require a campaign so the canonical party can be expanded. +explicit typed credential reference in the selected effective configuration must +resolve; references replaced or removed by that profile need not resolve. +Pipelines containing artifact families also require a campaign so the canonical +party can be expanded. `config show` is a secret-safe review projection, not a promised reloadable authoring file. @@ -77,7 +79,7 @@ guides. ## Changes -- Adopted ConfigKit v0.1.1 across pipeline, campaign, session, and party loading +- Adopted ConfigKit v0.1.2 across pipeline, campaign, session, and party loading while keeping Narratio domain validation and post-campaign expansion explicit. - Added ConfigKit imports and fixed `profiles/.yml` pipeline overlays, unified safe effective show/diff/digest projection, and source provenance. diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index decf27a..e7c0129 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -89,10 +89,11 @@ directory resolves from the pipeline root. Preserve digest relocation invariance Status: Resolved — P2 -Resolution: Named-profile loading now validates the independently resolved base -through the same ConfigKit definition and public metadata history before accepting -the selected result. Duplicate ordinary or family definitions therefore fail even -when an overlay nulls their container or all of Scriptorium. A single base owner +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 @@ -240,7 +241,73 @@ rollback, retries, or repeated directory loading. 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 +## 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 +--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: diff --git a/go.mod b/go.mod index 4c7871d..20fb89e 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module gitea.maximumdirect.net/eric/narratio go 1.26.0 require ( - gitea.maximumdirect.net/eric/ConfigKit v0.1.1 + gitea.maximumdirect.net/eric/ConfigKit v0.1.2 github.com/aws/aws-sdk-go-v2/config v1.32.17 github.com/aws/aws-sdk-go-v2/credentials v1.19.16 github.com/aws/aws-sdk-go-v2/service/s3 v1.101.0 diff --git a/go.sum b/go.sum index 2ce74c7..9c06607 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -gitea.maximumdirect.net/eric/ConfigKit v0.1.1 h1:/JXi95QDhgRtiQN7R/+6cGFcfc0+gfl+D44nKXPd52k= -gitea.maximumdirect.net/eric/ConfigKit v0.1.1/go.mod h1:tYWpsrbLyu05r8+1MapWp5fN6ZrX9I8hU2xEPedYmYk= +gitea.maximumdirect.net/eric/ConfigKit v0.1.2 h1:uZ5Mvf271SLItXEvUpQYfAvCOVYj9RaoG0auCcbCcSU= +gitea.maximumdirect.net/eric/ConfigKit v0.1.2/go.mod h1:tYWpsrbLyu05r8+1MapWp5fN6ZrX9I8hU2xEPedYmYk= github.com/aws/aws-sdk-go-v2 v1.41.7 h1:DWpAJt66FmnnaRIOT/8ASTucrvuDPZASqhhLey6tLY8= github.com/aws/aws-sdk-go-v2 v1.41.7/go.mod h1:4LAfZOPHNVNQEckOACQx60Y8pSRjIkNZQz1w92xpMJc= github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.7.10 h1:gx1AwW1Iyk9Z9dD9F4akX5gnN3QZwUB20GGKH/I+Rho= diff --git a/internal/config/pipeline_configkit.go b/internal/config/pipeline_configkit.go index 023e817..f510631 100644 --- a/internal/config/pipeline_configkit.go +++ b/internal/config/pipeline_configkit.go @@ -46,16 +46,6 @@ func loadPipelineWithConfigKit(path string, opts PipelineLoadOptions) (*Pipeline if err != nil { return nil, describePipelineLoadError(err) } - if profile != "" { - base, err := loadPipelineConfigKitResult(path, "") - if err != nil { - return nil, describePipelineLoadError(err) - } - baseConfig := base.Config - if err := rejectDuplicateBaseDefinitions(&baseConfig, base.Metadata(), base.Sources()); err != nil { - return nil, fmt.Errorf("load pipeline config: %w", err) - } - } cfg := result.Config metadata, err := pipelineMetadataFromConfigKit(&cfg, result, opts) if err != nil { @@ -266,7 +256,7 @@ func validatePipelineConfigKit(cfg *PipelineConfig, validation *configkit.Valida func pipelineMetadataFromConfigKit(cfg *PipelineConfig, result *configkit.Result[PipelineConfig], opts PipelineLoadOptions) (*pipelineResolutionMetadata, error) { sources := result.Sources() metadata := result.Metadata() - if err := rejectDuplicateBaseDefinitions(cfg, metadata, sources); err != nil { + if err := rejectDuplicateBaseDefinitions(metadata, sources); err != nil { return nil, err } resolved := &pipelineResolutionMetadata{ @@ -345,34 +335,30 @@ func ownershipSources(origin configkit.Origin, pathsByID map[string]string) []st return sources } -func rejectDuplicateBaseDefinitions(cfg *PipelineConfig, metadata configkit.Metadata, sources []configkit.SourceInfo) error { +func rejectDuplicateBaseDefinitions(metadata configkit.Metadata, sources []configkit.SourceInfo) error { pathsByID := make(map[string]string, len(sources)) for _, source := range sources { pathsByID[source.ID] = source.Path } - var declarations []struct { - kind string - key string - path configkit.Path - } - if cfg.Scriptorium != nil { - for key := range cfg.Scriptorium.Artifacts { - declarations = append(declarations, struct { - kind, key string - path configkit.Path - }{"artifact", key, configkit.Keys("scriptorium", "artifacts", key)}) + // Enumerate retained declarations, not just surviving effective keys. Loading + // the base separately would also resolve credentials removed by the profile. + // ConfigKit returns canonical JSON Pointers in deterministic order. + for _, path := range metadata.HistoricalPaths() { + parts := strings.Split(path.String(), "/") + if len(parts) != 4 || parts[1] != "scriptorium" { + continue } - for key := range cfg.Scriptorium.ArtifactFamilies { - declarations = append(declarations, struct { - kind, key string - path configkit.Path - }{"artifact family", key, configkit.Keys("scriptorium", "artifact_families", key)}) + var kind string + switch parts[2] { + case "artifacts": + kind = "artifact" + case "artifact_families": + kind = "artifact family" + default: + continue } - } - sort.Slice(declarations, func(i, j int) bool { return declarations[i].path.String() < declarations[j].path.String() }) - for _, declaration := range declarations { seen := map[string]struct{}{} - for _, change := range metadata.History(declaration.path) { + for _, change := range metadata.History(path) { if change.Origin.Layer != configkit.LayerBase { continue } @@ -386,7 +372,8 @@ func rejectDuplicateBaseDefinitions(cfg *PipelineConfig, metadata configkit.Meta claims = append(claims, pathsByID[sourceID]) } sort.Strings(claims) - return fmt.Errorf("pipeline Scriptorium %s %q is declared by multiple base files: %s", declaration.kind, declaration.key, strings.Join(claims, ", ")) + key := strings.NewReplacer("~1", "/", "~0", "~").Replace(parts[3]) + return fmt.Errorf("pipeline Scriptorium %s %q is declared by multiple base files: %s", kind, key, strings.Join(claims, ", ")) } } return nil diff --git a/internal/config/pipeline_configkit_test.go b/internal/config/pipeline_configkit_test.go index 3a6f7f2..d829784 100644 --- a/internal/config/pipeline_configkit_test.go +++ b/internal/config/pipeline_configkit_test.go @@ -88,6 +88,35 @@ whisperx: } } +func TestPipelineProfileDoesNotRequireUnusedBaseCredentials(t *testing.T) { + const production = "NARRATIO_PROFILE_UNUSED_PROD_KEY" + const testingKey = "NARRATIO_PROFILE_TEST_KEY" + t.Setenv(production, "") + if err := os.Unsetenv(production); err != nil { + t.Fatal(err) + } + t.Setenv(testingKey, "synthetic-test-key") + for _, credential := range []string{testingKey, "null"} { + t.Run(credential, func(t *testing.T) { + dir := t.TempDir() + root := writePipelineFixture(t, dir, "pipeline.yml", "whisperx:\n transcribe_url: https://transcription.example.test/transcribe\naudita:\n llm_api_key: "+production+"\n") + writePipelineFixture(t, dir, "profiles/testing.yml", "audita:\n llm_api_key: "+credential+"\n") + profile := "testing" + cfg, err := LoadPipelineWithOptions(root, PipelineLoadOptions{Profile: &profile}) + if err != nil { + t.Fatal(err) + } + if credential == "null" { + if cfg.Audita.LLMAPIKey != nil { + t.Fatal("profile did not remove the base credential") + } + } else if cfg.Audita.LLMAPIKey == nil || cfg.Audita.LLMAPIKey.EnvName() != testingKey || !cfg.Audita.LLMAPIKey.IsResolved() { + t.Fatal("profile credential was not resolved") + } + }) + } +} + func TestPipelineConfigKitAllowsOneBaseDeclarationWithProfileOverride(t *testing.T) { dir := t.TempDir() writePipelineFixture(t, dir, "pipeline.yml", `configkit: