From b0a16ccce8d5402793c6853dd6b5ee8359662643 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 6 Nov 2026 15:16:35 +0000 Subject: [PATCH] Simplify profile null fallback behavior --- docs/roadmap/implementation.md | 4 ++ internal/configuration/load.go | 23 ------------ internal/configuration/load_test.go | 8 +--- internal/configuration/targets.go | 37 +++++++++--------- internal/configuration/targets_test.go | 52 +++++++++++++++++++++++--- 5 files changed, 70 insertions(+), 54 deletions(-) diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index d7f7b88..1759b15 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -250,6 +250,10 @@ converters and callers may wait for later stages; do not add transitional fields ## Stage 2: Simplify Execution's Post-Handoff Null Semantics +**Completed:** Focused configuration tests pass with the locally selected Go +toolchain. Public resource and retained-provenance assertions for inherited nulls +remain scheduled with the inspection wiring and isolation checks. + **Scope:** Profile capture and its targeted configuration tests. 1. In `internal/configuration/targets.go`, implement the pointer-based behavior diff --git a/internal/configuration/load.go b/internal/configuration/load.go index 2cf8070..e026b60 100644 --- a/internal/configuration/load.go +++ b/internal/configuration/load.go @@ -119,7 +119,6 @@ const ( type loaded struct { result *configkit.Result[document] - profiles map[string]map[string]fieldState promptMetadata map[string][]promptInputPresence prompts map[string]*promptEntry registry *backend.Registry @@ -211,8 +210,6 @@ func indexedProfiles(config *document) map[string]profileConfig { var envName = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) -var profileFields = []string{"backend", "endpoint", "model", "temperature", "top_p", "max_tokens", "timeout_seconds", "service_tier", "reasoning_effort", "api_key_env", "api_key_required", "extra_params"} - // Load captures one immutable effective configuration for internal consumers. func Load(main configkit.Input, underlays []configkit.Underlay) (*loaded, error) { return load(main, underlays) @@ -235,7 +232,6 @@ func configurationDefinition(state *loaded) configkit.Definition[document] { Sensitive: secretProviderPath, Resolve: func(config *document, resolution *configkit.Resolution) error { for _, resources := range resourceSets(config) { - captureFields(resources.profiles, func(v profileConfig) string { return v.ID }, resources.bases.profiles, resources.namespace, profileFields, resolution, state.profiles) capturePromptMetadata(resources.prompts, resources.bases.prompts, resources.namespace, resolution, state.promptMetadata) } for _, resources := range resourceSets(config) { @@ -254,7 +250,6 @@ func configurationDefinition(state *loaded) configkit.Definition[document] { // load performs one ConfigKit lifecycle over the selected source tiers. func load(main configkit.Input, underlays []configkit.Underlay) (*loaded, error) { state := &loaded{ - profiles: make(map[string]map[string]fieldState), promptMetadata: make(map[string][]promptInputPresence), } result, err := configkit.LoadLayered(main, configkit.LayeredOptions[document]{ @@ -352,24 +347,6 @@ func captureFieldState(path configkit.Path, resolution *configkit.Resolution) fi } } -func captureFields[T any](entries []T, identity func(T) string, basePath configkit.Path, namespace string, fields []string, resolution *configkit.Resolution, destination map[string]map[string]fieldState) { - for index, entry := range entries { - states := make(map[string]fieldState, len(fields)) - for _, field := range fields { - info, exists := resolution.EntryInfo(basePath.Index(index).Key(field)) - switch { - case !exists || !info.Explicit: - states[field] = fieldAbsent - case info.Null: - states[field] = fieldNull - default: - states[field] = fieldValue - } - } - destination[indexedResourceID(namespace, identity(entry))] = states - } -} - // eligibleResourceList leaves invalid identities to the existing validation // boundaries, without mutating a list that cannot be recorded by identity. func eligibleResourceList[T any](entries []T, identity func(T) string) bool { diff --git a/internal/configuration/load_test.go b/internal/configuration/load_test.go index a1e392e..f6ba06d 100644 --- a/internal/configuration/load_test.go +++ b/internal/configuration/load_test.go @@ -488,9 +488,6 @@ func TestLoadComposesLiteralKeysAndEffectivePresence(t *testing.T) { if profile.Temperature == nil || *profile.Temperature != 0 || profile.TopP != nil || profile.APIKeyRequired == nil || *profile.APIKeyRequired { t.Fatalf("effective values were not retained: %+v", profile) } - if loaded.profiles["base/child.v1"]["temperature"] != fieldValue || loaded.profiles["base/child.v1"]["top_p"] != fieldNull || loaded.profiles["base/child.v1"]["api_key_required"] != fieldValue { - t.Fatalf("effective presence was lost: %+v", loaded.profiles["base/child.v1"]) - } provider := profile.ExtraParams["provider"].(map[string]any) if provider["first"] != int64(1) || provider["second"] != int64(3) { t.Fatalf("recursive overlay failed: %+v", provider) @@ -776,9 +773,8 @@ func TestLoadCapturesProfilePresence(t *testing.T) { if profile.Temperature == nil || *profile.Temperature != 0 || profile.TopP != nil || profile.APIKeyRequired == nil || *profile.APIKeyRequired { t.Fatalf("profile values were not preserved: %+v", profile) } - fields := loaded.profiles["local"] - if fields["temperature"] != fieldValue || fields["top_p"] != fieldNull || fields["api_key_required"] != fieldValue || fields["model"] != fieldAbsent { - t.Fatalf("profile presence was not preserved: %+v", fields) + if profile.Model != nil { + t.Fatalf("omitted model was not preserved: %+v", profile) } } diff --git a/internal/configuration/targets.go b/internal/configuration/targets.go index 16a3a59..06eb4f1 100644 --- a/internal/configuration/targets.go +++ b/internal/configuration/targets.go @@ -23,7 +23,7 @@ func (state *loaded) buildExecutionProfiles() error { state.indexedProfiles = make(map[string]domain.ExecutionProfile, len(profileConfigs)) for _, id := range sortedKeys(profileConfigs) { config := profileConfigs[id] - profile, err := captureExecutionProfile(id, config, state.profiles[id], state.registry) + profile, err := captureExecutionProfile(id, config, state.registry) if err != nil { return err } @@ -62,7 +62,7 @@ func captureBackend(id string, config backendConfig) domain.Backend { return definition } -func captureExecutionProfile(id string, config profileConfig, fields map[string]fieldState, registry *backend.Registry) (domain.ExecutionProfile, error) { +func captureExecutionProfile(id string, config profileConfig, registry *backend.Registry) (domain.ExecutionProfile, error) { target := defaults.ExecutionTargetDefault() if config.Backend != nil && *config.Backend != "" { selected, err := registry.GetBackend(*config.Backend) @@ -75,47 +75,44 @@ func captureExecutionProfile(id string, config profileConfig, fields map[string] target.ExtraParams = selected.ExtraParams } presence := domain.ExecutionTargetPresence{} - if fields["endpoint"] != fieldAbsent { + if config.Endpoint != nil { target.Endpoint = valueOrZero(config.Endpoint) } - if fields["model"] != fieldAbsent { + if config.Model != nil { target.Model = valueOrZero(config.Model) } - if fields["temperature"] != fieldAbsent { + if config.Temperature != nil { target.Temperature = valueOrZero(config.Temperature) - presence.Temperature = fields["temperature"] == fieldValue + presence.Temperature = true } - if fields["top_p"] != fieldAbsent { + if config.TopP != nil { target.TopP = valueOrZero(config.TopP) - presence.TopP = fields["top_p"] == fieldValue + presence.TopP = true } - if fields["max_tokens"] != fieldAbsent { + if config.MaxTokens != nil { target.MaxTokens = valueOrZero(config.MaxTokens) - presence.MaxTokens = fields["max_tokens"] == fieldValue + presence.MaxTokens = true } - if fields["timeout_seconds"] == fieldValue { + if config.TimeoutSeconds != nil { target.TimeoutSeconds = valueOrZero(config.TimeoutSeconds) presence.TimeoutSeconds = true } - if fields["service_tier"] != fieldAbsent { + if config.ServiceTier != nil { target.ServiceTier = valueOrZero(config.ServiceTier) } - if fields["reasoning_effort"] != fieldAbsent { + if config.ReasoningEffort != nil { target.ReasoningEffort = valueOrZero(config.ReasoningEffort) } - if fields["api_key_env"] != fieldAbsent { + if config.APIKeyEnv != nil { target.APIKeyEnv = valueOrZero(config.APIKeyEnv) } - if fields["api_key_required"] != fieldAbsent { + if config.APIKeyRequired != nil { target.APIKeyRequired = valueOrZero(config.APIKeyRequired) } - if target.APIKeyRequired && fields["api_key_env"] == fieldAbsent { + if target.APIKeyRequired && config.APIKeyEnv == nil { target.APIKeyEnv = "" } - switch fields["extra_params"] { - case fieldNull: - target.ExtraParams = nil - case fieldValue: + if config.ExtraParams != nil { merged, err := overlayProviderMap(target.ExtraParams, config.ExtraParams) if err != nil { return domain.ExecutionProfile{}, fmt.Errorf("profile %q extra parameters: %w", id, err) diff --git a/internal/configuration/targets_test.go b/internal/configuration/targets_test.go index fad5862..013811a 100644 --- a/internal/configuration/targets_test.go +++ b/internal/configuration/targets_test.go @@ -132,7 +132,7 @@ func TestLoadBuildsEffectiveTargetsAfterInheritance(t *testing.T) { target := child.ResolvedTarget if target.BackendID != "second" || target.Endpoint != "https://second.example/v1" || target.Model != "model" || target.Temperature != 0 || target.TopP != 0 || target.MaxTokens != 0 || - target.TimeoutSeconds != defaults.ExecutionDefaultTimeoutSeconds || target.APIKeyEnv != "" || target.APIKeyRequired { + target.TimeoutSeconds != defaults.ExecutionDefaultTimeoutSeconds || target.APIKeyEnv != "SECOND_KEY" || target.APIKeyRequired { t.Fatalf("effective child target is wrong: %+v", target) } if !child.TargetPresence.Temperature || !child.TargetPresence.MaxTokens || child.TargetPresence.TopP || child.TargetPresence.TimeoutSeconds { @@ -154,8 +154,8 @@ func TestLoadBuildsEffectiveTargetsAfterInheritance(t *testing.T) { } } noMap, _ := state.GetProfile(context.Background(), "no-provider-map") - if noMap.ResolvedTarget.ExtraParams != nil { - t.Fatalf("explicit null provider map did not suppress backend defaults: %+v", noMap.ResolvedTarget.ExtraParams) + if !reflect.DeepEqual(noMap.ResolvedTarget.ExtraParams, selected.ExtraParams) { + t.Fatalf("explicit null provider map did not inherit backend defaults: %+v", noMap.ResolvedTarget.ExtraParams) } noTimeout, _ := state.GetProfile(context.Background(), "no-timeout") if noTimeout.ResolvedTarget.TimeoutSeconds != 0 || !noTimeout.TargetPresence.TimeoutSeconds { @@ -183,8 +183,8 @@ func TestLoadBuildsEffectiveTargetsAfterInheritance(t *testing.T) { t.Fatalf("endpoint-only profile was not retained: %+v", endpointOnly.ResolvedTarget) } clearedEndpoint, _ := state.GetProfile(context.Background(), "cleared-endpoint") - if clearedEndpoint.ResolvedTarget.Endpoint != "" || clearedEndpoint.ResolvedTarget.BackendID != "first" { - t.Fatalf("explicit null recovered backend endpoint: %+v", clearedEndpoint.ResolvedTarget) + if clearedEndpoint.ResolvedTarget.Endpoint != "https://first.example/v1" || clearedEndpoint.ResolvedTarget.BackendID != "first" { + t.Fatalf("explicit null did not recover backend endpoint: %+v", clearedEndpoint.ResolvedTarget) } child.ResolvedTarget.ExtraParams["source"] = "mutated" again, _ := state.GetProfile(context.Background(), "child") @@ -193,6 +193,48 @@ func TestLoadBuildsEffectiveTargetsAfterInheritance(t *testing.T) { } } +func TestProfileNullFallbackAndExplicitValues(t *testing.T) { + state, err := load(mapping("profile fallback", map[string]any{ + "backends": []any{map[string]any{"id": "backend", "endpoint": "https://backend.example/v1", "api_key_env": "BACKEND_KEY", "extra_params": map[string]any{"backend": true}}}, + "profiles": []any{ + map[string]any{"id": "parent", "backend": "backend", "endpoint": "https://parent.example/v1", "extra_params": map[string]any{"parent": true}}, + map[string]any{"id": "inherited-null", "extends": map[string]any{"id": "parent"}, "endpoint": nil, "extra_params": nil, "api_key_env": nil}, + map[string]any{"id": "omitted", "backend": "backend"}, + map[string]any{"id": "null", "backend": "backend", "endpoint": nil, "api_key_env": nil, "extra_params": nil}, + map[string]any{"id": "empty", "backend": "backend", "endpoint": "", "api_key_env": "", "extra_params": map[string]any{}}, + map[string]any{"id": "required-null", "backend": "backend", "api_key_required": true, "api_key_env": nil}, + map[string]any{"id": "required-omitted", "backend": "backend", "api_key_required": true}, + map[string]any{"id": "required-explicit", "backend": "backend", "api_key_required": true, "api_key_env": "PROFILE_KEY"}, + }, + }), nil) + if err != nil { + t.Fatal(err) + } + for _, id := range []string{"inherited-null", "omitted", "null", "empty", "required-null", "required-omitted", "required-explicit"} { + profile, err := state.GetProfile(context.Background(), id) + if err != nil { + t.Fatal(err) + } + target := profile.ResolvedTarget + wantEndpoint, wantKey := "https://backend.example/v1", "BACKEND_KEY" + switch id { + case "empty": + wantEndpoint, wantKey = "", "" + case "required-null", "required-omitted": + wantKey = "" + case "required-explicit": + wantKey = "PROFILE_KEY" + } + if target.Endpoint != wantEndpoint || target.APIKeyEnv != wantKey || !reflect.DeepEqual(target.ExtraParams, map[string]any{"backend": true}) { + t.Errorf("%s target = %+v", id, target) + } + } + child := state.result.Config.Profiles[1] + if child.Endpoint != nil || child.ExtraParams != nil || child.APIKeyEnv != nil { + t.Fatalf("inherited nulls were not retained in resolved values: %+v", child) + } +} + func TestLoadedZeroControlsReachBuiltInRequest(t *testing.T) { payloads := make(chan map[string]any, 1) server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {