From 62a4a205296f9888a236b3c025fdda2ac4b4bf4b Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 3 Oct 2026 01:21:48 +0000 Subject: [PATCH] Preserve message resource origins and profile credential lookup names --- construction_contract_test.go | 57 ++++++++++++++++++++++++++ docs/internal/sources.md | 5 +++ docs/roadmap/implementation.md | 13 +++++- docs/roadmap/remediation.md | 20 ++++++++- internal/configuration/load.go | 3 -- internal/configuration/prompts.go | 40 ++++-------------- internal/configuration/prompts_test.go | 19 ++++++++- 7 files changed, 118 insertions(+), 39 deletions(-) diff --git a/construction_contract_test.go b/construction_contract_test.go index e130db2..48e38f2 100644 --- a/construction_contract_test.go +++ b/construction_contract_test.go @@ -3,8 +3,11 @@ package promptkit_test import ( "context" "errors" + "io" + "net/http" "os" "path/filepath" + "strings" "testing" "testing/fstest" @@ -12,6 +15,60 @@ import ( "gitea.maximumdirect.net/eric/promptkit" ) +func TestProfileEnvironmentLookupAcceptsNonShellNames(t *testing.T) { + const name = "promptkit.test-api-key" + for _, test := range []struct { + name, value, direct, want string + unset, clear bool + }{ + {name: "set", value: " synthetic-key ", want: "Bearer synthetic-key"}, + {name: "empty"}, + {name: "whitespace", value: " "}, + {name: "unset", unset: true}, + {name: "direct precedence", value: "synthetic-key", direct: "direct-key", want: "Bearer direct-key"}, + {name: "explicit clear", value: "synthetic-key", clear: true}, + } { + t.Run(test.name, func(t *testing.T) { + t.Setenv(name, test.value) + if test.unset { + if err := os.Unsetenv(name); err != nil { + t.Fatal(err) + } + } + cfg := simpleConfig("hello") + cfg.Sources.Underlays = []configkit.Underlay{{Inputs: []configkit.Input{*cfg.Sources.Main}}} + lowerName := name + cfg.Sources.Underlays = append(cfg.Sources.Underlays, configkit.Underlay{Inputs: []configkit.Input{{ + Source: promptkit.ProfileSource("default credentials", map[string]promptkit.ProfileConfig{ + "profile": {APIKeyEnv: &lowerName}, + }), + }}}) + env := " " + name + " " + if test.clear { + env = "" + } + cfg.Sources.Main = &configkit.Input{Source: promptkit.ProfileSource("credentials", map[string]promptkit.ProfileConfig{ + "profile": {APIKeyEnv: &env}, + })} + var calls int + var auth string + cfg.HTTPClient = &http.Client{Transport: roundTripFunc(func(request *http.Request) (*http.Response, error) { + calls++ + auth = request.Header.Get("Authorization") + return &http.Response{StatusCode: http.StatusOK, Header: make(http.Header), Body: io.NopCloser(strings.NewReader(`{"choices":[{"message":{"content":"ok"}}]}`))}, nil + })} + engine, err := promptkit.NewEngine(cfg) + if err != nil { + t.Fatal(err) + } + _, err = engine.Run(context.Background(), promptkit.RunRequest{PromptID: "prompt", APIKey: test.direct}) + if err != nil || calls != 1 || auth != test.want { + t.Fatalf("run: err=%v calls=%d authorization=%q, want %q", err, calls, auth, test.want) + } + }) + } +} + func TestPublicConstructionWithLocalBackendAndProgrammaticProfile(t *testing.T) { backendID, model := "workstation", "offline-model" prompt := configkit.Input{Source: configkit.Mapping("prompt", map[string]any{ diff --git a/docs/internal/sources.md b/docs/internal/sources.md index 3861dfd..e36b1cf 100644 --- a/docs/internal/sources.md +++ b/docs/internal/sources.md @@ -38,6 +38,11 @@ at construction. It stores captured content, compiled templates, declared input metadata, and a definition hash. It closes content-file handles before returning and retains no source capability for public inspection. +Role and cache-control normalization is applied to execution-owned message +copies. The retained ConfigKit configuration keeps effective input message +metadata and its resource origins: recording a sequence-wide normalization in +ConfigKit v0.5.0 would replace origins of unchanged content-file fields. + The runner selects from this index for preparation and `InspectPrompt`. Neither path rescans configuration or reopens message files. `InspectPrompt` returns declared metadata without rendering or resolving profile, input, or diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 8a3a26c..c7c20da 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -199,8 +199,12 @@ Unknown fields are ConfigKit errors, including old `id`, `version`, and Literal definition keys remain exact; require nonblank identity but do not normalize/rekey maps after ConfigKit has resolved relationships. Update selection to use exact nonblank IDs/versions rather than silently trimming them into a -different identity. Existing value normalization (for example role lowercase or -endpoint trimming) is recorded through ConfigKit's resolution callbacks. Field +different identity. Scalar normalization such as endpoint trimming is recorded +through ConfigKit's resolution callbacks when +mutating its typed configuration. Message roles and cache control are normalized +on execution-owned copies instead: ConfigKit v0.5.0 treats sequence mutations as +whole-value replacements, losing unchanged content_file origins. Inspection +retains the effective input message metadata and its source context. Field references to definition IDs remain exact as well. Profile field rules after configuration overlay/inheritance: @@ -217,6 +221,11 @@ Profile field rules after configuration overlay/inheritance: | `api_key_required` | False | Explicit boolean, including false overriding true | False | | `extra_params` | Backend defaults | ConfigKit-composed profile map, recursively overlaid on backend map | Suppress entire configured provider map | +Profile API-key environment names follow the existing lookup contract, including +host-supported non-shell-identifier names. Do not extend the backend's naming +regular expression to profiles. Trimming, explicit clearing, optional lookup, +and direct request-key precedence remain unchanged. + Backend registry defaults are applied once during snapshot construction, after effective profile inheritance. Keep them out of the inheritance input: a parent's derived endpoint/key/defaults must not follow a child that changes backend. diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 21cbb0d..2dbe493 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -13,7 +13,8 @@ resources/templates, and builds execution-owned indexes. Catalog preflight and legacy profile resolution are removed. Operator inspection delegates to the retained ConfigKit result separately from ordinary execution. -Two findings require remediation. This review changes no implementation code. +Both findings have been addressed. The original review evidence is retained +below; the resolution section records the changes and regression coverage. ## 1. P2 — Message Normalization Loses Content-File Resource Origins @@ -106,3 +107,20 @@ Required outcome: The passing suite does not cover the two failing cases above. No provider requests were made. Beyond these findings, the reviewed implementation did not show another actionable ownership-boundary violation or strengthened guarantee. + +## Resolution + +1. Message role/cache-control normalization now occurs on execution-owned copies + during materialization. ConfigKit's captured message sequence and its origins + remain intact. The imported/inherited content regression now includes role + and cache-control normalization and asserts the unchanged field's source in + inspection metadata. This also removes duplicate cache-control normalization. +2. Removed the profile environment-name regular expression check, leaving backend + validation unchanged. A public engine/HTTP-transport regression covers a + non-shell environment name, unset/empty/whitespace values, explicit clearing, + trimming, and direct-key precedence. The implementation plan now states this + distinction explicitly. + +Both regressions failed against the reviewed implementation before the fixes. +After remediation, the full ordinary and race-enabled suites, affected packages +on Go 1.25.5, `go vet ./...`, and documentation/whitespace checks passed. diff --git a/internal/configuration/load.go b/internal/configuration/load.go index e539e15..c2ad0f5 100644 --- a/internal/configuration/load.go +++ b/internal/configuration/load.go @@ -275,9 +275,6 @@ func validateEffective(config *document, validation *configkit.Validation) { if profile.Model != nil && *profile.Model != "" && strings.TrimSpace(*profile.Model) == "" { validation.Error(base.Key("model"), "model must not be blank") } - if profile.APIKeyEnv != nil && *profile.APIKeyEnv != "" && !envName.MatchString(*profile.APIKeyEnv) { - validation.Error(base.Key("api_key_env"), "environment variable name is invalid") - } if profile.Temperature != nil { if err := domain.ValidateExecutionTargetSettings(domain.ExecutionTarget{Temperature: *profile.Temperature}); err != nil { validation.Error(base.Key("temperature"), err.Error()) diff --git a/internal/configuration/prompts.go b/internal/configuration/prompts.go index b8f0f89..7606830 100644 --- a/internal/configuration/prompts.go +++ b/internal/configuration/prompts.go @@ -66,37 +66,9 @@ func resolvePrompts(prompts map[string]map[string]promptConfig, resolution *conf if inputsChanged { resolution.Record(base.Key("inputs"), configkit.ChangeNormalization, "normalize input metadata") } - messagesChanged := false - for index := range definition.Messages { - message := &definition.Messages[index] - messagePath := base.Key("messages").Index(index) - if message.Role != nil { - role, err := domain.NormalizeMessageRole(*message.Role) - if err != nil { - resolution.Error(messagePath.Key("role"), err.Error()) - } else { - messagesChanged = normalizeString(&message.Role, messagePath.Key("role"), resolution, role) || messagesChanged - } - } - if control := message.CacheControl; control != nil && control.Type != nil { - value := &domain.CacheControl{Type: domain.CacheControlType(*control.Type)} - if control.TTL != nil { - value.TTL = *control.TTL - } - normalized, err := domain.NormalizeCacheControl(value) - if err != nil { - resolution.Error(messagePath.Key("cache_control"), err.Error()) - } else { - messagesChanged = normalizeString(&control.Type, messagePath.Key("cache_control").Key("type"), resolution, string(normalized.Type)) || messagesChanged - if control.TTL != nil { - messagesChanged = normalizeString(&control.TTL, messagePath.Key("cache_control").Key("ttl"), resolution, normalized.TTL) || messagesChanged - } - } - } - } - if messagesChanged { - resolution.Record(base.Key("messages"), configkit.ChangeNormalization, "normalize message metadata") - } + // Normalize message metadata on execution-owned copies below. ConfigKit + // records sequence mutations as whole-value replacements, which would + // discard the effective source contexts of unchanged content_file fields. if definition.Output != nil && definition.Output.SchemaPath != nil { normalizeString(&definition.Output.SchemaPath, base.Key("output").Key("schema_path"), resolution, strings.TrimSpace(*definition.Output.SchemaPath)) } @@ -180,6 +152,10 @@ func (state *loaded) materializePrompt(id, version string, raw promptConfig, loo if message.Role == nil { return nil, fmt.Errorf("%s: role is required", path.Key("role")) } + role, err := domain.NormalizeMessageRole(*message.Role) + if err != nil { + return nil, fmt.Errorf("%s: %w", path.Key("role"), err) + } if (message.Content != nil) == (message.ContentFile != nil) { return nil, fmt.Errorf("%s: exactly one of content or content_file is required", path) } @@ -214,7 +190,7 @@ func (state *loaded) materializePrompt(id, version string, raw promptConfig, loo } entry.messages = append(entry.messages, compiled) definition.Templates = append(definition.Templates, domain.PromptMessageTemplate{ - Role: *message.Role, Content: content, CacheControl: control, + Role: role, Content: content, CacheControl: control, }) } if raw.Output == nil { diff --git a/internal/configuration/prompts_test.go b/internal/configuration/prompts_test.go index d8cbfc8..44f332b 100644 --- a/internal/configuration/prompts_test.go +++ b/internal/configuration/prompts_test.go @@ -15,7 +15,7 @@ import ( func TestLoadCapturesInheritedImportedContent(t *testing.T) { files := fstest.MapFS{ "app/main.yaml": {Data: []byte("configkit:\n imports:\n - path: ../catalog/base.yaml\nprompts:\n child:\n extends: base\n")}, - "catalog/base.yaml": {Data: []byte("prompts:\n base:\n v1:\n messages:\n - role: user\n content_file: ./body.md\n output:\n format: text\n validation_mode: basic\n")}, + "catalog/base.yaml": {Data: []byte("prompts:\n base:\n v1:\n messages:\n - role: ' USER '\n cache_control:\n type: ' ephemeral '\n ttl: ' 1h '\n content_file: ./body.md\n output:\n format: text\n validation_mode: basic\n")}, "catalog/body.md": {Data: []byte("Captured {{.name}}")}, } namespace := configkit.NewFilesystem("embedded", files) @@ -37,6 +37,23 @@ func TestLoadCapturesInheritedImportedContent(t *testing.T) { if definition.Templates[0].Content != "Captured {{.name}}" || inspection.PromptHash == "" { t.Fatalf("inherited file content was not captured: %+v, %+v", definition, inspection) } + message := definition.Templates[0] + if message.Role != "user" || message.CacheControl == nil || message.CacheControl.Type != "ephemeral" || message.CacheControl.TTL != "1h" { + t.Fatalf("message metadata was not normalized: %+v", message) + } + field, ok := state.result.Metadata().Lookup(configkit.Keys("prompts", "child", "v1", "messages").Index(0).Key("content_file")) + if !ok || len(field.Origin.Locations) != 1 { + t.Fatalf("content origin was lost during normalization: %+v", field) + } + var originPath string + for _, source := range state.result.Sources() { + if source.ID == field.Origin.Locations[0].SourceID { + originPath = source.Path + } + } + if originPath != "catalog/base.yaml" { + t.Fatalf("content origin = %q, want catalog/base.yaml", originPath) + } files["catalog/body.md"].Data = []byte("Changed") again, againInspection, err := state.prompt("child", "v1") if err != nil || again.Templates[0].Content != "Captured {{.name}}" || againInspection.PromptHash != inspection.PromptHash {