From c00b116179849d0126889ce9c3e7c2fa71411921 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 29 Jul 2026 20:31:02 +0200 Subject: [PATCH 1/2] Move RuntimeConfig copy/merge onto the type The field-by-field copies of templates.RuntimeConfig rot as the struct grows: they were complete when written, then RuntimeEnv and BuildWith were added and each one silently stopped being carried. Clone and WithOverrides put the copy/merge logic on the type itself, so the enumeration of all four fields lives in one file next to the struct declaration instead of being reimplemented at each call site. A field can still be forgotten in Clone or WithOverrides when a new one is added, but a guard test now fails the moment RuntimeConfig's field count changes, forcing that update to happen. WithOverrides (renamed from MergedWith, base.WithOverrides(override) instead of a symmetric-sounding name that doesn't say which side wins) starts from a copy of the base struct, so an unhandled future field defaults to base-wins rather than a zero value. Clone starts the same way. Both guard against a nil receiver instead of panicking. GetDefaultRuntimeConfig now returns a value already detached from the package-global RuntimeDefaults map (via Clone internally), retiring the whole class of aliasing bugs at the source instead of requiring every caller to remember to clone what they get back. The build-constraint check (BuildWith is only supported for uvx builds) moves into the templates package as RuntimeConfig.ValidateFor, next to Validate and the defaults it needs. loadRuntimeConfig now runs every runtime config it returns - override, config-file, and default fallback alike - through ValidateFor, so the constraint can't be silently skipped on one of the three paths the way a caller-side check could be forgotten on a fourth. Also rename the build-constraint rejection message from --build-with to build_with. The check lives in pkg/ and is reachable from the REST API, the TUI and the user config file, so naming a CLI flag misleads every non-CLI caller. Co-Authored-By: Claude Opus 5 --- pkg/container/templates/runtime_config.go | 94 +++++++- .../templates/runtime_config_test.go | 216 ++++++++++++++++++ pkg/runner/protocol.go | 75 +----- pkg/runner/protocol_test.go | 116 +--------- 4 files changed, 329 insertions(+), 172 deletions(-) diff --git a/pkg/container/templates/runtime_config.go b/pkg/container/templates/runtime_config.go index be5a59c7c7..b6e4aa39c5 100644 --- a/pkg/container/templates/runtime_config.go +++ b/pkg/container/templates/runtime_config.go @@ -6,7 +6,9 @@ package templates import ( "errors" "fmt" + "maps" "regexp" + "slices" "strings" nameref "github.com/google/go-containerregistry/pkg/name" @@ -159,6 +161,80 @@ func (rc *RuntimeConfig) Validate() error { return errors.Join(errs...) } +// Clone returns a deep copy of rc, safe for the caller to mutate without +// affecting the original — including RuntimeDefaults entries, whose slices +// are package-global and would otherwise be aliased by a shallow copy. +// A nil receiver returns nil. +func (rc *RuntimeConfig) Clone() *RuntimeConfig { + if rc == nil { + return nil + } + clone := *rc + clone.AdditionalPackages = slices.Clone(rc.AdditionalPackages) + clone.BuildWith = slices.Clone(rc.BuildWith) + clone.RuntimeEnv = maps.Clone(rc.RuntimeEnv) + return &clone +} + +// WithOverrides returns a new RuntimeConfig with rc as the base and override +// layered on top: +// - BuilderImage: override wins if non-empty, else falls back to rc's. +// - AdditionalPackages: the union, rc's entries first, then any override +// entries not already present. +// - RuntimeEnv: merged, with override's value winning on a shared key. +// - BuildWith: taken from override as-is; no defaults exist for it. +// +// WithOverrides(nil) returns rc.Clone() — a distinct object, never rc itself. +// A nil receiver returns override.Clone(). +func (rc *RuntimeConfig) WithOverrides(override *RuntimeConfig) *RuntimeConfig { + if rc == nil { + return override.Clone() + } + if override == nil { + return rc.Clone() + } + + // Start from a copy of the base so any future field defaults to + // base-wins rather than a zero value. + merged := *rc + if override.BuilderImage != "" { + merged.BuilderImage = override.BuilderImage + } + + seen := make(map[string]bool, len(rc.AdditionalPackages)) + merged.AdditionalPackages = append([]string(nil), rc.AdditionalPackages...) + for _, pkg := range rc.AdditionalPackages { + seen[pkg] = true + } + for _, pkg := range override.AdditionalPackages { + if !seen[pkg] { + merged.AdditionalPackages = append(merged.AdditionalPackages, pkg) + seen[pkg] = true + } + } + + merged.RuntimeEnv = mergeEnvMaps(rc.RuntimeEnv, override.RuntimeEnv) + merged.BuildWith = slices.Clone(override.BuildWith) + + return &merged +} + +// ValidateFor rejects a non-empty BuildWith for transports whose builder +// doesn't support build-time dependency constraints — only the uvx builder +// currently supports them — and otherwise validates rc via Validate. +// A nil receiver returns nil. +func (rc *RuntimeConfig) ValidateFor(transportType TransportType) error { + if rc == nil { + return nil + } + if transportType != TransportTypeUVX && len(rc.BuildWith) > 0 { + return fmt.Errorf( + "build_with is not supported for %s:// builds (only uvx://)", transportType, + ) + } + return rc.Validate() +} + // RuntimeDefaults provides default configurations for each runtime type var RuntimeDefaults = map[TransportType]RuntimeConfig{ TransportTypeGO: { @@ -175,12 +251,26 @@ var RuntimeDefaults = map[TransportType]RuntimeConfig{ }, } -// GetDefaultRuntimeConfig returns the default runtime configuration for a given transport type +// GetDefaultRuntimeConfig returns the default runtime configuration for a given +// transport type. The result is a deep copy detached from RuntimeDefaults, so +// callers may freely mutate it without affecting the package-global defaults. func GetDefaultRuntimeConfig(transportType TransportType) RuntimeConfig { config, ok := RuntimeDefaults[transportType] if !ok { // Return empty config if transport type not found return RuntimeConfig{} } - return config + return *config.Clone() +} + +// mergeEnvMaps merges two environment variable maps without mutating either +// input. Entries in override take precedence over entries in base. +func mergeEnvMaps(base, override map[string]string) map[string]string { + if len(base) == 0 && len(override) == 0 { + return nil + } + merged := make(map[string]string, len(base)+len(override)) + maps.Copy(merged, base) + maps.Copy(merged, override) + return merged } diff --git a/pkg/container/templates/runtime_config_test.go b/pkg/container/templates/runtime_config_test.go index b69a87a7be..7a58670ab2 100644 --- a/pkg/container/templates/runtime_config_test.go +++ b/pkg/container/templates/runtime_config_test.go @@ -4,6 +4,8 @@ package templates import ( + "reflect" + "slices" "strings" "testing" @@ -529,3 +531,217 @@ func TestUVXTemplateWithoutBuildWithIsUnchanged(t *testing.T) { "no --with arguments should appear when BuildWith is empty") assert.NotContains(t, dockerfile, "--with") } + +func TestRuntimeConfigClone_Nil(t *testing.T) { + t.Parallel() + + var rc *RuntimeConfig + assert.Nil(t, rc.Clone()) +} + +func TestRuntimeConfigClone_Detached(t *testing.T) { + t.Parallel() + + src := &RuntimeConfig{ + BuilderImage: "golang:1.26-alpine", + AdditionalPackages: []string{"git"}, + BuildWith: []string{"mcp<2"}, + RuntimeEnv: map[string]string{"FOO": "bar"}, + } + clone := src.Clone() + assert.Equal(t, src, clone) + + // Mutating the source afterwards must not affect the clone. + src.AdditionalPackages[0] = "mutated" + src.BuildWith[0] = "mutated" + src.RuntimeEnv["FOO"] = "mutated" + assert.Equal(t, "git", clone.AdditionalPackages[0]) + assert.Equal(t, "mcp<2", clone.BuildWith[0]) + assert.Equal(t, "bar", clone.RuntimeEnv["FOO"]) + + // Mutating the clone must not affect the source. + clone2 := src.Clone() + clone2.AdditionalPackages[0] = "mutated-again" + clone2.RuntimeEnv["FOO"] = "mutated-again" + assert.Equal(t, "mutated", src.AdditionalPackages[0]) + assert.Equal(t, "mutated", src.RuntimeEnv["FOO"]) +} + +func TestRuntimeConfigClone_DoesNotAliasRuntimeDefaults(t *testing.T) { + t.Parallel() + + original := GetDefaultRuntimeConfig(TransportTypeNPX) + wantPackages := slices.Clone(original.AdditionalPackages) + + clone := original.Clone() + clone.AdditionalPackages[0] = "mutated" + + fresh := GetDefaultRuntimeConfig(TransportTypeNPX) + assert.Equal(t, wantPackages, fresh.AdditionalPackages, + "mutating a Clone() must not reach RuntimeDefaults") +} + +func TestRuntimeConfigWithOverrides(t *testing.T) { + t.Parallel() + + base := &RuntimeConfig{ + BuilderImage: "python:3.14-slim", + AdditionalPackages: []string{"ca-certificates", "git"}, + RuntimeEnv: map[string]string{"BASE_KEY": "base-value", "SHARED_KEY": "base-value"}, + } + + tests := []struct { + name string + override *RuntimeConfig + wantImage string + wantPackages []string + wantEnv map[string]string + wantBuild []string + }{ + { + name: "nil override behaves like Clone", + override: nil, + wantImage: "python:3.14-slim", + wantPackages: []string{"ca-certificates", "git"}, + wantEnv: map[string]string{"BASE_KEY": "base-value", "SHARED_KEY": "base-value"}, + }, + { + name: "empty override builder image falls back to base", + override: &RuntimeConfig{}, + wantImage: "python:3.14-slim", + wantPackages: []string{"ca-certificates", "git"}, + wantEnv: map[string]string{"BASE_KEY": "base-value", "SHARED_KEY": "base-value"}, + }, + { + name: "non-empty override builder image wins", + override: &RuntimeConfig{BuilderImage: "python:3.11-slim"}, + wantImage: "python:3.11-slim", + wantPackages: []string{"ca-certificates", "git"}, + wantEnv: map[string]string{"BASE_KEY": "base-value", "SHARED_KEY": "base-value"}, + }, + { + name: "packages dedupe, base first", + override: &RuntimeConfig{AdditionalPackages: []string{"ca-certificates", "curl"}}, + wantImage: "python:3.14-slim", + wantPackages: []string{"ca-certificates", "git", "curl"}, + wantEnv: map[string]string{"BASE_KEY": "base-value", "SHARED_KEY": "base-value"}, + }, + { + name: "runtime env: override wins on shared key, base-only and override-only keys survive", + override: &RuntimeConfig{ + RuntimeEnv: map[string]string{"SHARED_KEY": "override-value", "OVERRIDE_KEY": "override-value"}, + }, + wantImage: "python:3.14-slim", + wantPackages: []string{"ca-certificates", "git"}, + wantEnv: map[string]string{ + "BASE_KEY": "base-value", "SHARED_KEY": "override-value", "OVERRIDE_KEY": "override-value", + }, + }, + { + name: "build_with taken from override as-is, no defaults", + override: &RuntimeConfig{BuildWith: []string{"mcp<2"}}, + wantImage: "python:3.14-slim", + wantPackages: []string{"ca-certificates", "git"}, + wantEnv: map[string]string{"BASE_KEY": "base-value", "SHARED_KEY": "base-value"}, + wantBuild: []string{"mcp<2"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got := base.WithOverrides(tt.override) + assert.Equal(t, tt.wantImage, got.BuilderImage) + assert.Equal(t, tt.wantPackages, got.AdditionalPackages) + assert.Equal(t, tt.wantEnv, got.RuntimeEnv) + assert.Equal(t, tt.wantBuild, got.BuildWith) + }) + } +} + +func TestRuntimeConfigWithOverrides_RuntimeEnvNilWhenBothEmpty(t *testing.T) { + t.Parallel() + + // Base has no RuntimeEnv and override sets none either: the merged + // result must stay nil, not an empty map, so `omitempty` still omits + // runtime_env from serialized output. + base := &RuntimeConfig{AdditionalPackages: []string{"git"}} + assert.Nil(t, base.WithOverrides(nil).RuntimeEnv) + assert.Nil(t, base.WithOverrides(&RuntimeConfig{}).RuntimeEnv) +} + +func TestRuntimeConfigWithOverrides_NilEqualsClone(t *testing.T) { + t.Parallel() + + base := &RuntimeConfig{ + BuilderImage: "node:24-alpine", + AdditionalPackages: []string{"git"}, + } + merged := base.WithOverrides(nil) + cloned := base.Clone() + + assert.Equal(t, cloned, merged) + assert.NotSame(t, base, merged) +} + +func TestRuntimeConfigWithOverrides_DoesNotMutateInputs(t *testing.T) { + t.Parallel() + + base := &RuntimeConfig{ + AdditionalPackages: []string{"git"}, + RuntimeEnv: map[string]string{"FOO": "base"}, + } + override := &RuntimeConfig{ + AdditionalPackages: []string{"curl"}, + RuntimeEnv: map[string]string{"FOO": "override"}, + } + + got := base.WithOverrides(override) + got.AdditionalPackages[0] = "mutated" + got.RuntimeEnv["FOO"] = "mutated" + + assert.Equal(t, []string{"git"}, base.AdditionalPackages) + assert.Equal(t, map[string]string{"FOO": "base"}, base.RuntimeEnv) + assert.Equal(t, []string{"curl"}, override.AdditionalPackages) + assert.Equal(t, map[string]string{"FOO": "override"}, override.RuntimeEnv) +} + +// TestRuntimeConfigWithOverrides_OutputIsDetachedFromInputs covers the opposite +// direction from TestRuntimeConfigWithOverrides_DoesNotMutateInputs: mutating an +// input slice/map *after* WithOverrides must not change the already-returned +// result. BuildWith in particular was passed through as override.BuildWith +// directly (no clone), so it aliased the caller's slice. +func TestRuntimeConfigWithOverrides_OutputIsDetachedFromInputs(t *testing.T) { + t.Parallel() + + base := &RuntimeConfig{ + AdditionalPackages: []string{"git"}, + RuntimeEnv: map[string]string{"FOO": "base"}, + } + override := &RuntimeConfig{ + AdditionalPackages: []string{"curl"}, + RuntimeEnv: map[string]string{"FOO": "override"}, + BuildWith: []string{"mcp<2"}, + } + + got := base.WithOverrides(override) + + override.AdditionalPackages[0] = "mutated" + override.RuntimeEnv["FOO"] = "mutated" + override.BuildWith[0] = "mutated" + + assert.Equal(t, []string{"git", "curl"}, got.AdditionalPackages) + assert.Equal(t, "override", got.RuntimeEnv["FOO"]) + assert.Equal(t, []string{"mcp<2"}, got.BuildWith) +} + +// TestRuntimeConfigFieldCount guards against a field being added to +// RuntimeConfig without updating Clone and WithOverrides, both of which +// enumerate every field individually. +func TestRuntimeConfigFieldCount(t *testing.T) { + t.Parallel() + + // Adding a field? Update Clone and WithOverrides, then bump this. + require.Equal(t, 4, reflect.TypeOf(RuntimeConfig{}).NumField()) +} diff --git a/pkg/runner/protocol.go b/pkg/runner/protocol.go index 7f4be18f7b..44bda3a4f9 100644 --- a/pkg/runner/protocol.go +++ b/pkg/runner/protocol.go @@ -7,7 +7,6 @@ import ( "context" "fmt" "log/slog" - "maps" "os" "path/filepath" "strings" @@ -149,15 +148,6 @@ func createTemplateData( } templateData.RuntimeConfig = runtimeConfig - // Build-time dependency constraints are interpreted per package - // ecosystem; only the uvx builder currently supports them. Reject - // rather than silently ignore for the others. - if transportType != templates.TransportTypeUVX && len(runtimeConfig.BuildWith) > 0 { - return templateData, fmt.Errorf( - "--build-with is not supported for %s:// builds (only uvx://)", transportType, - ) - } - return templateData, nil } @@ -173,10 +163,12 @@ func loadRuntimeConfig( transportType templates.TransportType, override *templates.RuntimeConfig, ) (*templates.RuntimeConfig, error) { + defaults := templates.GetDefaultRuntimeConfig(transportType) + // If override is provided, merge with defaults before validating if override != nil { - merged := mergeRuntimeConfig(transportType, override) - if err := merged.Validate(); err != nil { + merged := defaults.WithOverrides(override) + if err := merged.ValidateFor(transportType); err != nil { return nil, fmt.Errorf("invalid runtime config override: %w", err) } return merged, nil @@ -185,63 +177,20 @@ func loadRuntimeConfig( // Try loading from user config (merge with defaults, then validate) provider := config.NewProvider() if userConfig, err := provider.GetRuntimeConfig(string(transportType)); err == nil && userConfig != nil { - merged := mergeRuntimeConfig(transportType, userConfig) - if err := merged.Validate(); err != nil { + merged := defaults.WithOverrides(userConfig) + if err := merged.ValidateFor(transportType); err != nil { return nil, fmt.Errorf("invalid runtime config in config file for %s: %w", transportType, err) } return merged, nil } - // Fall back to defaults - defaultConfig := templates.GetDefaultRuntimeConfig(transportType) - return &defaultConfig, nil -} - -// mergeRuntimeConfig merges an override RuntimeConfig with the defaults for the -// given transport type. Empty BuilderImage falls back to the default, and -// AdditionalPackages are merged (defaults first, then unique override entries). -func mergeRuntimeConfig(transportType templates.TransportType, override *templates.RuntimeConfig) *templates.RuntimeConfig { - defaults := templates.GetDefaultRuntimeConfig(transportType) - - merged := &templates.RuntimeConfig{ - BuilderImage: override.BuilderImage, - } - if merged.BuilderImage == "" { - merged.BuilderImage = defaults.BuilderImage - } - - // Start with default packages, then append any override packages not - // already present. - seen := make(map[string]bool, len(defaults.AdditionalPackages)) - merged.AdditionalPackages = append(merged.AdditionalPackages, defaults.AdditionalPackages...) - for _, pkg := range defaults.AdditionalPackages { - seen[pkg] = true - } - for _, pkg := range override.AdditionalPackages { - if !seen[pkg] { - merged.AdditionalPackages = append(merged.AdditionalPackages, pkg) - seen[pkg] = true - } - } - - merged.RuntimeEnv = mergeEnvMaps(defaults.RuntimeEnv, override.RuntimeEnv) - - // BuildWith has no defaults; the override's specifiers are used as-is. - merged.BuildWith = override.BuildWith - - return merged -} - -// mergeEnvMaps merges two environment variable maps without mutating either -// input. Entries in override take precedence over entries in base. -func mergeEnvMaps(base, override map[string]string) map[string]string { - if len(base) == 0 && len(override) == 0 { - return nil + // Fall back to defaults. GetDefaultRuntimeConfig already returns a value + // detached from the package-global RuntimeDefaults, so no further clone + // is needed here. + if err := defaults.ValidateFor(transportType); err != nil { + return nil, fmt.Errorf("invalid default runtime config for %s: %w", transportType, err) } - merged := make(map[string]string, len(base)+len(override)) - maps.Copy(merged, base) - maps.Copy(merged, override) - return merged + return &defaults, nil } // addBuildEnvToTemplate loads build environment variables from config and adds them to template data. diff --git a/pkg/runner/protocol_test.go b/pkg/runner/protocol_test.go index 6585af3a8a..1cf61d6141 100644 --- a/pkg/runner/protocol_test.go +++ b/pkg/runner/protocol_test.go @@ -335,7 +335,7 @@ func TestBuildFromProtocolSchemeWithNameDryRun(t *testing.T) { } } -func TestMergeRuntimeConfig(t *testing.T) { +func TestLoadRuntimeConfigMergesPerTransportDefaults(t *testing.T) { t.Parallel() tests := []struct { name string @@ -346,7 +346,7 @@ func TestMergeRuntimeConfig(t *testing.T) { wantRuntimeEnv map[string]string }{ { - name: "only packages override, no image — image falls back to default", + name: "only packages override, no image (--runtime-add-package without --runtime-image) — image falls back to default", transport: templates.TransportTypeNPX, override: &templates.RuntimeConfig{ BuilderImage: "", @@ -436,7 +436,8 @@ func TestMergeRuntimeConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := mergeRuntimeConfig(tt.transport, tt.override) + got, err := loadRuntimeConfig(tt.transport, tt.override) + require.NoError(t, err) if got.BuilderImage != tt.wantImage { t.Errorf("BuilderImage = %q, want %q", got.BuilderImage, tt.wantImage) @@ -457,90 +458,6 @@ func TestMergeRuntimeConfig(t *testing.T) { } } -func TestMergeEnvMaps(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - base map[string]string - override map[string]string - want map[string]string - }{ - { - name: "both nil", - base: nil, - override: nil, - want: nil, - }, - { - name: "base only", - base: map[string]string{"FOO": "bar"}, - override: nil, - want: map[string]string{"FOO": "bar"}, - }, - { - name: "override only", - base: nil, - override: map[string]string{"FOO": "bar"}, - want: map[string]string{"FOO": "bar"}, - }, - { - name: "override wins on shared key", - base: map[string]string{"FOO": "base-value", "BAR": "base-bar"}, - override: map[string]string{"FOO": "override-value"}, - want: map[string]string{"FOO": "override-value", "BAR": "base-bar"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - got := mergeEnvMaps(tt.base, tt.override) - assert.Equal(t, tt.want, got) - }) - } -} - -func TestMergeEnvMapsDoesNotMutateInputs(t *testing.T) { - t.Parallel() - - base := map[string]string{"FOO": "base-value"} - override := map[string]string{"BAR": "override-value"} - - got := mergeEnvMaps(base, override) - require.NotNil(t, got) - - // Mutate the returned map and confirm neither input map is affected. - got["FOO"] = "mutated" - got["BAZ"] = "new" - - assert.Equal(t, map[string]string{"FOO": "base-value"}, base) - assert.Equal(t, map[string]string{"BAR": "override-value"}, override) -} - -func TestLoadRuntimeConfigMergesOverrideWithDefaults(t *testing.T) { - t.Parallel() - - // Simulate the bug: --runtime-add-package without --runtime-image - override := &templates.RuntimeConfig{ - BuilderImage: "", - AdditionalPackages: []string{"curl"}, - } - - got, err := loadRuntimeConfig(templates.TransportTypeNPX, override) - if err != nil { - t.Fatalf("loadRuntimeConfig() error = %v", err) - } - - if got.BuilderImage == "" { - t.Error("loadRuntimeConfig() returned empty BuilderImage — should fall back to default") - } - if got.BuilderImage != "node:24-alpine" { - t.Errorf("BuilderImage = %q, want %q", got.BuilderImage, "node:24-alpine") - } -} - func TestCreateTemplateData(t *testing.T) { t.Parallel() tests := []struct { @@ -703,35 +620,20 @@ func TestLoadRuntimeConfig_MergesBaseConfigWithOverride(t *testing.T) { assert.Equal(t, expectedPackages, got.AdditionalPackages) } -func TestLoadRuntimeConfig_UsesOverrideBuilderImage(t *testing.T) { - t.Parallel() - - base, err := loadRuntimeConfig(templates.TransportTypeGO, nil) - require.NoError(t, err) - require.NotNil(t, base) - - customImage := "golang:1.24-alpine" - got, err := loadRuntimeConfig(templates.TransportTypeGO, &templates.RuntimeConfig{ - BuilderImage: customImage, - }) - require.NoError(t, err) - require.NotNil(t, got) - assert.Equal(t, customImage, got.BuilderImage) - assert.Equal(t, base.AdditionalPackages, got.AdditionalPackages) -} - -func TestMergeRuntimeConfigCarriesBuildWith(t *testing.T) { +func TestLoadRuntimeConfigCarriesBuildWith(t *testing.T) { t.Parallel() - got := mergeRuntimeConfig(templates.TransportTypeUVX, &templates.RuntimeConfig{ + got, err := loadRuntimeConfig(templates.TransportTypeUVX, &templates.RuntimeConfig{ BuildWith: []string{"mcp<2"}, }) + require.NoError(t, err) if len(got.BuildWith) != 1 || got.BuildWith[0] != "mcp<2" { t.Errorf("BuildWith = %v, want [mcp<2]", got.BuildWith) } // No override specifiers: merged config must not invent any. - got = mergeRuntimeConfig(templates.TransportTypeUVX, &templates.RuntimeConfig{}) + got, err = loadRuntimeConfig(templates.TransportTypeUVX, &templates.RuntimeConfig{}) + require.NoError(t, err) if len(got.BuildWith) != 0 { t.Errorf("BuildWith = %v, want empty", got.BuildWith) } From b7e8ead3b83d3e2b07a2fab0f49f171742d6b121 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 4 Aug 2026 22:18:44 +0200 Subject: [PATCH 2/2] Honor all runtime_config fields over the API The REST API's request type advertises all four RuntimeConfig fields (builder_image, additional_packages, build_with, runtime_env) and publishes them in swagger, but the service layer only ever copied the first two. A caller POSTing runtime_config.build_with got 201 Created and a workload built with unconstrained dependencies, silently - exactly the failure build_with exists to prevent (#6108). runtime_env was dropped the same way. Naively plumbing the two missing fields would trade that silent drop for an opaque 500: the build-constraint rejection lived deep in the imageRetriever path, and pkg/api/errors/handler.go scrubs any >=500 body down to bare status text. So this also moves where validation happens. runtimeConfigForImageBuild now merges the request onto the transport's base config with WithOverrides and validates the result with ValidateFor before it ever reaches the retriever, and that error is wrapped in retriever.ErrInvalidRunConfig, which is coded 400 and returned to the client intact. runtimeConfigFromRequest now clones the request's RuntimeConfig and normalizes it in place instead of copying it field by field, so a future field is carried automatically instead of needing a new branch. Deleted validateRuntimeConfig and isValidRuntimePackageName in favor of templates.RuntimeConfig.Validate(), which is strictly stronger: it reports every problem instead of the first, and closes a gap where ".foo"/"_foo" package names were accepted by the API but rejected at build time. The emptiness short-circuit in runtimeConfigFromRequest was itself still a hand-enumeration of all four fields, one line below the fix - a fifth field would be dropped there exactly as build_with was. Added templates.RuntimeConfig.IsEmpty() next to Clone and WithOverrides, and extended the field-count guard test to cover all three. WithOverrides also discarded the base's BuildWith unconditionally, which is fine for the CLI's static defaults (which never set it) but wrong for the API's base, which is the user's config file: a request setting only an unrelated field would silently drop a globally pinned build_with. BuildWith now falls back to the base when the override has none, matching BuilderImage's "override wins if set" rule. Carrying these fields through to responses exposed a round trip that was already broken for builder_image and additional_packages: a workload built from a protocol scheme persists the built image, not the uvx:// URI it came from, so GET returns a runtime_config that PUT then rejects with 400 as "only supported for protocol-scheme images". A client doing GET, edit, PUT could not save an existing protocol-built workload back. The update path now recognizes an inert echo - nothing to rebuild - only when the request's image, URL, and runtime_config all exactly match what is already persisted. The persisted config is threaded into BuildFullRunConfig so the echo is skipped for the retriever/build input alone: the request's runtime_config is never cleared, so the RunConfig the policy gate evaluates always carries it and a policy cannot be bypassed by echoing an unchanged config back. Anything else - a different image, a different URL, or a runtime_config that doesn't match - still returns 400, so a genuine attempt to configure a plain image, or to redirect an existing workload elsewhere, is not silently discarded. The rule is documented on the update endpoint, since swaggo drops descriptions on $ref fields and clients could not otherwise discover it. Loading the persisted state to check for an echo can itself fail, and that failure was being swallowed. A missing state file falls through to the existing protocol-scheme rejection - the workload exists, only its state file doesn't, so 400 is still the right answer - but any other load error (a corrupt file, a cancelled context) is now returned directly instead of silently disabling the echo check and producing a misleading 400 that hides the real cause. The req.URL == "" guard on WithRuntimeConfig meant an accepted echo of a remote workload's runtime_config was silently dropped from the rebuilt RunConfig even though runtimeConfigForImageBuild had already decided the request was an inert match - BuildFullRunConfig only attached the override when the request carried no URL. Removed that guard: a non-nil override here is either a protocol-scheme build (already validated above) or an accepted echo on an otherwise-rejected image/URL, and both must reach the RunConfig regardless of whether the workload is remote. The echo comparison normalized only the request's side before calling reflect.DeepEqual against the persisted value, so a config that reached storage before whitespace-trimming existed, or with a nil-vs-empty collection difference, could fail to match its own unchanged echo and be rejected as if it were a real change. Extracted the trim-and-filter logic out of runtimeConfigFromRequest into a shared normalizeRuntimeConfig helper and applied it to the persisted side of the comparison too. The state-load guard for echo detection checked the request's raw RuntimeConfig field instead of its normalized form, so a semantically empty "runtime_config": {} triggered a state read - and a failure on that read - for a request where no echo comparison was ever going to happen. Gated the LoadState call on the normalized value instead. The create endpoint's swagger annotation now documents the same protocol-scheme restriction, so callers aren't left to discover the 400 by trial and error. Co-Authored-By: Claude Opus 5 --- docs/server/docs.go | 4 +- docs/server/swagger.json | 4 +- docs/server/swagger.yaml | 13 +- pkg/api/v1/workload_service.go | 198 +++++--- pkg/api/v1/workload_service_test.go | 202 +++++++- pkg/api/v1/workload_types.go | 20 +- pkg/api/v1/workloads.go | 7 + pkg/api/v1/workloads_test.go | 435 +++++++++++++++++- pkg/api/v1/workloads_types_test.go | 8 + pkg/container/templates/runtime_config.go | 20 +- .../templates/runtime_config_test.go | 70 ++- 11 files changed, 849 insertions(+), 132 deletions(-) diff --git a/docs/server/docs.go b/docs/server/docs.go index dfa6533e6d..794ce6f841 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -8157,7 +8157,7 @@ const docTemplate = `{ ] }, "post": { - "description": "Create and start a new workload", + "description": "Create and start a new workload\nruntime_config is only accepted for protocol-scheme images\n(uvx://, npx://, go://); supplying it with an ordinary image\nreference or a remote url is rejected with 400.", "requestBody": { "content": { "application/json": { @@ -8522,7 +8522,7 @@ const docTemplate = `{ }, "/api/v1beta/workloads/{name}/edit": { "post": { - "description": "Update an existing workload configuration", + "description": "Update an existing workload configuration\nruntime_config on a non-protocol-scheme image is accepted only when it\nexactly matches the workload's persisted config and the image and url\nare unchanged (an inert echo, e.g. from a prior GET); otherwise it is\nrejected with 400.", "parameters": [ { "description": "Workload name", diff --git a/docs/server/swagger.json b/docs/server/swagger.json index a9625fbec9..a469eed67a 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -8150,7 +8150,7 @@ ] }, "post": { - "description": "Create and start a new workload", + "description": "Create and start a new workload\nruntime_config is only accepted for protocol-scheme images\n(uvx://, npx://, go://); supplying it with an ordinary image\nreference or a remote url is rejected with 400.", "requestBody": { "content": { "application/json": { @@ -8515,7 +8515,7 @@ }, "/api/v1beta/workloads/{name}/edit": { "post": { - "description": "Update an existing workload configuration", + "description": "Update an existing workload configuration\nruntime_config on a non-protocol-scheme image is accepted only when it\nexactly matches the workload's persisted config and the image and url\nare unchanged (an inert echo, e.g. from a prior GET); otherwise it is\nrejected with 400.", "parameters": [ { "description": "Workload name", diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 0969382edb..60cbf74417 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -6449,7 +6449,11 @@ paths: tags: - workloads post: - description: Create and start a new workload + description: |- + Create and start a new workload + runtime_config is only accepted for protocol-scheme images + (uvx://, npx://, go://); supplying it with an ordinary image + reference or a remote url is rejected with 400. requestBody: content: application/json: @@ -6544,7 +6548,12 @@ paths: - workloads /api/v1beta/workloads/{name}/edit: post: - description: Update an existing workload configuration + description: |- + Update an existing workload configuration + runtime_config on a non-protocol-scheme image is accepted only when it + exactly matches the workload's persisted config and the image and url + are unchanged (an inert echo, e.g. from a prior GET); otherwise it is + rejected with 400. parameters: - description: Workload name in: path diff --git a/pkg/api/v1/workload_service.go b/pkg/api/v1/workload_service.go index bc179ab7f6..19d3e9e067 100644 --- a/pkg/api/v1/workload_service.go +++ b/pkg/api/v1/workload_service.go @@ -9,11 +9,10 @@ import ( "fmt" "log/slog" "net/http" + "reflect" "strings" "time" - nameref "github.com/google/go-containerregistry/pkg/name" - "github.com/stacklok/toolhive-core/httperr" regtypes "github.com/stacklok/toolhive-core/registry/types" groupval "github.com/stacklok/toolhive-core/validation/group" @@ -32,6 +31,7 @@ import ( "github.com/stacklok/toolhive/pkg/transport" "github.com/stacklok/toolhive/pkg/transport/types" "github.com/stacklok/toolhive/pkg/workloads" + wterrors "github.com/stacklok/toolhive/pkg/workloads/types/errors" ) const ( @@ -40,24 +40,6 @@ const ( imageRetrievalTimeout = 10 * time.Minute ) -func isValidRuntimePackageName(pkg string) bool { - if pkg == "" { - return false - } - for i, r := range pkg { - switch { - case r >= 'a' && r <= 'z': - case r >= 'A' && r <= 'Z': - case r >= '0' && r <= '9': - case r == '.', r == '_': - case (r == '+' || r == '-') && i > 0: - default: - return false - } - } - return true -} - // WorkloadService handles business logic for workload operations type WorkloadService struct { workloadManager workloads.Manager @@ -94,8 +76,9 @@ func NewWorkloadService( // CreateWorkloadFromRequest creates a workload from a request func (s *WorkloadService) CreateWorkloadFromRequest(ctx context.Context, req *createRequest) (*runner.RunConfig, error) { - // Build the full run config (no existing port, so pass 0) - runConfig, err := s.BuildFullRunConfig(ctx, req, 0) + // Build the full run config (no existing port, so pass 0; no persisted + // workload exists yet, so pass nil) + runConfig, err := s.BuildFullRunConfig(ctx, req, 0, nil) if err != nil { return nil, err } @@ -130,8 +113,48 @@ func (s *WorkloadService) UpdateWorkloadFromRequest(ctx context.Context, name st slog.Debug("reusing existing port", "port", existingPort, "name", name) } + // A workload built from a protocol-scheme image (uvx://, npx://, go://) + // persists Image as the *built* image, which is no longer a protocol + // scheme, plus the RuntimeConfig used to build it. GET echoes both back + // (see runtimeConfigForResponse), so PUT-ing that response unchanged + // would otherwise hit runtimeConfigForImageBuild's protocol-scheme guard. + // The persisted config is threaded through to BuildFullRunConfig so + // runtimeConfigForImageBuild can recognize an exact echo - same image, + // same URL, same runtime_config - and skip only the retriever/build + // input for it. req.RuntimeConfig itself is never touched here, so + // runConfig.RuntimeConfig is always populated from the request before + // the policy gate below evaluates it - a policy can't be bypassed by + // echoing an unchanged config back. + // + // Only load state when an echo is even possible, to keep the extra I/O + // off requests that don't need it. Missing state falls through with + // persisted left nil - the workload exists (the handler already + // checked), only its state file doesn't, so this can't be an echo and + // the protocol-scheme guard below is the correct 400, not a 404. Any + // other load error (corrupt file, cancelled context) is surfaced + // instead of silently disabling the echo check and producing a + // misleading 400. + var persisted *runner.RunConfig + // runtimeConfigFromRequest is called again in BuildFullRunConfig; it's + // pure (Clone-then-normalize), so the duplicate call just decides whether + // a state read is needed at all. Gating on it rather than the raw + // req.RuntimeConfig != nil means a request with an empty/whitespace-only + // runtime_config (which normalizes away to nothing) never reads state. + if runtimeConfigFromRequest(req) != nil && (req.URL != "" || !runner.IsImageProtocolScheme(req.Image)) { + p, err := runner.LoadState(ctx, name) + switch { + case err == nil: + persisted = p + case errors.Is(err, wterrors.ErrRunConfigNotFound): + // No persisted state to echo against; fall through to the + // protocol-scheme guard. + default: + return nil, fmt.Errorf("failed to load persisted state for workload %q: %w", name, err) + } + } + // Build the full run config - runConfig, err := s.BuildFullRunConfig(ctx, req, existingPort) + runConfig, err := s.BuildFullRunConfig(ctx, req, existingPort, persisted) if err != nil { return nil, fmt.Errorf("failed to build workload config: %w", err) } @@ -145,11 +168,14 @@ func (s *WorkloadService) UpdateWorkloadFromRequest(ctx context.Context, name st return runConfig, nil } -// BuildFullRunConfig builds a complete RunConfig +// BuildFullRunConfig builds a complete RunConfig. persisted is the +// workload's existing RunConfig on update, or nil on create; it is used +// only to recognize an unchanged runtime_config echo on a non-protocol +// image (see runtimeConfigForImageBuild). // //nolint:gocyclo // TODO: refactor this into shorter functions func (s *WorkloadService) BuildFullRunConfig( - ctx context.Context, req *createRequest, existingPort int, + ctx context.Context, req *createRequest, existingPort int, persisted *runner.RunConfig, ) (*runner.RunConfig, error) { // If registry+server specified, resolve from registry and fill defaults. // The returned metadata is assigned to the local variables so the rest of @@ -242,7 +268,7 @@ func (s *WorkloadService) BuildFullRunConfig( } runtimeConfigOverride := runtimeConfigFromRequest(req) - retrievalRuntimeConfig, err := runtimeConfigForImageBuild(req, runtimeConfigOverride) + retrievalRuntimeConfig, err := runtimeConfigForImageBuild(req, runtimeConfigOverride, persisted) if err != nil { return nil, fmt.Errorf("%w: %w", retriever.ErrInvalidRunConfig, err) } @@ -355,8 +381,10 @@ func (s *WorkloadService) BuildFullRunConfig( runner.WithRegistryServerName(regServerName), } - // Runtime overrides only apply to protocol-scheme image builds. - if runtimeConfigOverride != nil && req.URL == "" { + // A non-nil override here is either a protocol-scheme build (validated + // above) or an accepted echo on an otherwise-rejected image/URL - both + // must be preserved on the RunConfig, so no req.URL guard here. + if runtimeConfigOverride != nil { options = append(options, runner.WithRuntimeConfig(runtimeConfigOverride)) } @@ -503,60 +531,85 @@ func createRequestToRemoteAuthConfig( return remoteAuthConfig } +// runtimeConfigFromRequest normalizes the request's runtime config in place +// on a clone, rather than copying it field by field, so a future field rides +// along without needing a new branch here. Returns nil if there is no +// runtime config, or if every field is empty after normalization — callers +// must treat nil as "no override" and not build one, since a nil result +// skips runtimeConfigForImageBuild's protocol-scheme guard downstream. func runtimeConfigFromRequest(req *createRequest) *templates.RuntimeConfig { - if req == nil || req.RuntimeConfig == nil { + if req == nil { return nil } - runtimeConfig := &templates.RuntimeConfig{} - if builderImage := strings.TrimSpace(req.RuntimeConfig.BuilderImage); builderImage != "" { - runtimeConfig.BuilderImage = builderImage - } - if len(req.RuntimeConfig.AdditionalPackages) > 0 { - for _, pkg := range req.RuntimeConfig.AdditionalPackages { - if trimmedPkg := strings.TrimSpace(pkg); trimmedPkg != "" { - runtimeConfig.AdditionalPackages = append(runtimeConfig.AdditionalPackages, trimmedPkg) - } - } - } - if runtimeConfig.BuilderImage == "" && len(runtimeConfig.AdditionalPackages) == 0 { + rc := normalizeRuntimeConfig(req.RuntimeConfig) + if rc == nil || rc.IsEmpty() { return nil } - return runtimeConfig + return rc } -func validateRuntimeConfig(runtimeConfig *templates.RuntimeConfig) error { - if runtimeConfig == nil { +// normalizeRuntimeConfig returns a deep copy of rc with fields trimmed and +// emptied consistently, so two configs that differ only in incidental +// whitespace or a nil-vs-empty collection compare equal. Used both to +// normalize an incoming request (runtimeConfigFromRequest) and to normalize +// a persisted config before comparing it against an already-normalized +// override for an echo (runtimeConfigForImageBuild) - a stored config +// persisted before this normalization existed (e.g. an untrimmed +// builder_image) must still compare equal to its own unchanged echo. +// Returns nil for a nil input. +func normalizeRuntimeConfig(rc *templates.RuntimeConfig) *templates.RuntimeConfig { + if rc == nil { return nil } - - if runtimeConfig.BuilderImage != "" { - if _, err := nameref.ParseReference(runtimeConfig.BuilderImage); err != nil { - return fmt.Errorf("runtime_config.builder_image must be a valid container image reference") - } + out := rc.Clone() + out.BuilderImage = strings.TrimSpace(out.BuilderImage) + out.AdditionalPackages = trimAndFilterEmpty(out.AdditionalPackages) + out.BuildWith = trimAndFilterEmpty(out.BuildWith) + if len(out.RuntimeEnv) == 0 { + out.RuntimeEnv = nil } + return out +} - for _, pkg := range runtimeConfig.AdditionalPackages { - if !isValidRuntimePackageName(pkg) { - return fmt.Errorf("runtime_config.additional_packages contains invalid package name %q", pkg) +// trimAndFilterEmpty trims whitespace from each entry and drops any that +// become empty, without mutating the input slice. +func trimAndFilterEmpty(entries []string) []string { + if len(entries) == 0 { + return nil + } + var out []string + for _, entry := range entries { + if trimmed := strings.TrimSpace(entry); trimmed != "" { + out = append(out, trimmed) } } - - return nil + return out } +// persisted is the workload's existing RunConfig on update (nil on create). +// It is used solely to recognize an unchanged runtime_config echo on a +// non-protocol image, in which case this returns nil, nil instead of +// rejecting: nothing needs to be (re)built. This only suppresses the value +// fed to the retriever - the caller populates runConfig.RuntimeConfig from +// runtimeConfigOverride regardless, so the policy gate still sees it. func runtimeConfigForImageBuild( req *createRequest, runtimeConfigOverride *templates.RuntimeConfig, + persisted *runner.RunConfig, ) (*templates.RuntimeConfig, error) { if runtimeConfigOverride == nil || req == nil { return nil, nil } - if err := validateRuntimeConfig(runtimeConfigOverride); err != nil { - return nil, err - } if req.URL != "" || !runner.IsImageProtocolScheme(req.Image) { + // An exact echo of the persisted workload - same image, same URL, + // same runtime_config - is inert: nothing to rebuild, so it isn't + // rejected even though the image isn't a protocol scheme. + if persisted != nil && req.Image == persisted.Image && req.URL == persisted.RemoteURL && + reflect.DeepEqual(runtimeConfigOverride, normalizeRuntimeConfig(persisted.RuntimeConfig)) { + return nil, nil + } return nil, fmt.Errorf("runtime_config is only supported for protocol-scheme images") } @@ -565,16 +618,17 @@ func runtimeConfigForImageBuild( return nil, err } - baseConfig := getBaseRuntimeConfig(transportType) - merged := &templates.RuntimeConfig{ - BuilderImage: baseConfig.BuilderImage, - AdditionalPackages: append([]string{}, baseConfig.AdditionalPackages...), - } - if runtimeConfigOverride.BuilderImage != "" { - merged.BuilderImage = runtimeConfigOverride.BuilderImage - } - if len(runtimeConfigOverride.AdditionalPackages) > 0 { - merged.AdditionalPackages = append(merged.AdditionalPackages, runtimeConfigOverride.AdditionalPackages...) + base := getBaseRuntimeConfig(transportType) + merged := base.WithOverrides(runtimeConfigOverride) + // Validating here, before the merged config ever reaches the builder, is + // load-bearing, not incidental: the caller wraps this error in + // retriever.ErrInvalidRunConfig, which is coded 400 and returned to the + // client intact. The same failure surfacing inside imageRetriever instead + // would be >=500, and pkg/api/errors/handler.go scrubs those down to a + // bare "Internal Server Error" — silently reintroducing the bug this + // design exists to close. + if err := merged.ValidateFor(transportType); err != nil { + return nil, fmt.Errorf("runtime_config: %w", err) } return merged, nil @@ -583,17 +637,11 @@ func runtimeConfigForImageBuild( func getBaseRuntimeConfig(transportType templates.TransportType) *templates.RuntimeConfig { provider := config.NewProvider() if userConfig, err := provider.GetRuntimeConfig(string(transportType)); err == nil && userConfig != nil { - return &templates.RuntimeConfig{ - BuilderImage: userConfig.BuilderImage, - AdditionalPackages: append([]string{}, userConfig.AdditionalPackages...), - } + return userConfig.Clone() } defaultConfig := templates.GetDefaultRuntimeConfig(transportType) - return &templates.RuntimeConfig{ - BuilderImage: defaultConfig.BuilderImage, - AdditionalPackages: append([]string{}, defaultConfig.AdditionalPackages...), - } + return defaultConfig.Clone() } // GetWorkloadNamesFromRequest gets workload names from either the names field or group diff --git a/pkg/api/v1/workload_service_test.go b/pkg/api/v1/workload_service_test.go index b03823aac1..b8294e3150 100644 --- a/pkg/api/v1/workload_service_test.go +++ b/pkg/api/v1/workload_service_test.go @@ -8,8 +8,10 @@ import ( "errors" "net/http" "os" + "path/filepath" "testing" + "github.com/adrg/xdg" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -23,6 +25,7 @@ import ( "github.com/stacklok/toolhive/pkg/runner" "github.com/stacklok/toolhive/pkg/runner/retriever" "github.com/stacklok/toolhive/pkg/secrets" + "github.com/stacklok/toolhive/pkg/state" workloadsmocks "github.com/stacklok/toolhive/pkg/workloads/mocks" ) @@ -204,7 +207,7 @@ func TestBuildFullRunConfig_ThreadsImageVerification(t *testing.T) { updateRequest: updateRequest{Image: testImage}, } - _, err := service.BuildFullRunConfig(context.Background(), req, 0) + _, err := service.BuildFullRunConfig(context.Background(), req, 0, nil) require.NoError(t, err) assert.Equal(t, retriever.VerifyImageDisabled, observed, "imageRetriever must receive s.imageVerification verbatim") @@ -261,7 +264,7 @@ func TestBuildFullRunConfig_AppliesOtelFromConfig(t *testing.T) { updateRequest: updateRequest{Image: testImage}, } - runConfig, err := service.BuildFullRunConfig(context.Background(), req, 0) + runConfig, err := service.BuildFullRunConfig(context.Background(), req, 0, nil) require.NoError(t, err) require.NotNil(t, runConfig.TelemetryConfig, "TelemetryConfig must be populated from config.OTEL when set — workloads created via the API would otherwise drop the endpoint") @@ -315,7 +318,7 @@ func TestBuildFullRunConfig_ThreadsAllowDockerGateway(t *testing.T) { }, } - runConfig, err := service.BuildFullRunConfig(context.Background(), req, 0) + runConfig, err := service.BuildFullRunConfig(context.Background(), req, 0, nil) require.NoError(t, err) assert.True(t, runConfig.AllowDockerGateway) } @@ -355,7 +358,7 @@ func TestBuildFullRunConfig_NoOtelConfigLeavesTelemetryNil(t *testing.T) { updateRequest: updateRequest{Image: testImage}, } - runConfig, err := service.BuildFullRunConfig(context.Background(), req, 0) + runConfig, err := service.BuildFullRunConfig(context.Background(), req, 0, nil) require.NoError(t, err) assert.Nil(t, runConfig.TelemetryConfig, "TelemetryConfig must remain nil when config.OTEL has no endpoint or prometheus path") @@ -473,6 +476,24 @@ func TestRuntimeConfigFromRequest(t *testing.T) { req.RuntimeConfig.AdditionalPackages[0] = "curl" assert.Equal(t, []string{"git"}, result.AdditionalPackages) }) + + t.Run("trims and filters build_with, carries runtime_env", func(t *testing.T) { + t.Parallel() + + req := &createRequest{ + updateRequest: updateRequest{ + RuntimeConfig: &templates.RuntimeConfig{ + BuildWith: []string{" mcp<2 ", "", " "}, + RuntimeEnv: map[string]string{"NODE_ENV": "production"}, + }, + }, + } + + result := runtimeConfigFromRequest(req) + require.NotNil(t, result) + assert.Equal(t, []string{"mcp<2"}, result.BuildWith) + assert.Equal(t, map[string]string{"NODE_ENV": "production"}, result.RuntimeEnv) + }) } func TestRuntimeConfigForImageBuild(t *testing.T) { @@ -484,6 +505,7 @@ func TestRuntimeConfigForImageBuild(t *testing.T) { result, err := runtimeConfigForImageBuild( &createRequest{updateRequest: updateRequest{Image: "go://github.com/example/server"}}, nil, + nil, ) require.NoError(t, err) assert.Nil(t, result) @@ -495,6 +517,7 @@ func TestRuntimeConfigForImageBuild(t *testing.T) { result, err := runtimeConfigForImageBuild( &createRequest{updateRequest: updateRequest{Image: "nginx:latest"}}, &templates.RuntimeConfig{BuilderImage: "golang:1.24-alpine"}, + nil, ) require.Error(t, err) assert.Nil(t, result) @@ -507,6 +530,7 @@ func TestRuntimeConfigForImageBuild(t *testing.T) { result, err := runtimeConfigForImageBuild( &createRequest{updateRequest: updateRequest{URL: "https://example.com"}}, &templates.RuntimeConfig{BuilderImage: "golang:1.24-alpine"}, + nil, ) require.Error(t, err) assert.Nil(t, result) @@ -519,10 +543,11 @@ func TestRuntimeConfigForImageBuild(t *testing.T) { result, err := runtimeConfigForImageBuild( &createRequest{updateRequest: updateRequest{Image: "go://github.com/example/server"}}, &templates.RuntimeConfig{BuilderImage: "not a valid image ref"}, + nil, ) require.Error(t, err) assert.Nil(t, result) - assert.Contains(t, err.Error(), "runtime_config.builder_image must be a valid container image reference") + assert.Contains(t, err.Error(), "runtime_config: invalid builder_image") }) t.Run("rejects invalid additional package names", func(t *testing.T) { @@ -531,10 +556,11 @@ func TestRuntimeConfigForImageBuild(t *testing.T) { result, err := runtimeConfigForImageBuild( &createRequest{updateRequest: updateRequest{Image: "go://github.com/example/server"}}, &templates.RuntimeConfig{AdditionalPackages: []string{"curl;rm -rf /"}}, + nil, ) require.Error(t, err) assert.Nil(t, result) - assert.Contains(t, err.Error(), "runtime_config.additional_packages contains invalid package name") + assert.Contains(t, err.Error(), "runtime_config: invalid package name") }) t.Run("rejects option like additional package names", func(t *testing.T) { @@ -543,10 +569,26 @@ func TestRuntimeConfigForImageBuild(t *testing.T) { result, err := runtimeConfigForImageBuild( &createRequest{updateRequest: updateRequest{Image: "go://github.com/example/server"}}, &templates.RuntimeConfig{AdditionalPackages: []string{"--allow-untrusted"}}, + nil, ) require.Error(t, err) assert.Nil(t, result) - assert.Contains(t, err.Error(), "runtime_config.additional_packages contains invalid package name") + assert.Contains(t, err.Error(), "runtime_config: invalid package name") + }) + + t.Run("rejects additional packages starting with . or _", func(t *testing.T) { + t.Parallel() + + for _, pkg := range []string{".foo", "_foo"} { + result, err := runtimeConfigForImageBuild( + &createRequest{updateRequest: updateRequest{Image: "go://github.com/example/server"}}, + &templates.RuntimeConfig{AdditionalPackages: []string{pkg}}, + nil, + ) + require.Error(t, err, "package %q should be rejected", pkg) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "runtime_config: invalid package name") + } }) t.Run("merges override with base defaults for protocol images", func(t *testing.T) { @@ -559,6 +601,7 @@ func TestRuntimeConfigForImageBuild(t *testing.T) { result, err := runtimeConfigForImageBuild( &createRequest{updateRequest: updateRequest{Image: "go://github.com/example/server"}}, override, + nil, ) require.NoError(t, err) require.NotNil(t, result) @@ -572,6 +615,23 @@ func TestRuntimeConfigForImageBuild(t *testing.T) { override.AdditionalPackages[0] = "git" assert.Equal(t, expectedPackages, result.AdditionalPackages) }) + + t.Run("build_with and runtime_env survive the merge for uvx", func(t *testing.T) { + t.Parallel() + + result, err := runtimeConfigForImageBuild( + &createRequest{updateRequest: updateRequest{Image: "uvx://arxiv-mcp-server"}}, + &templates.RuntimeConfig{ + BuildWith: []string{"mcp<2"}, + RuntimeEnv: map[string]string{"NODE_ENV": "production"}, + }, + nil, + ) + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, []string{"mcp<2"}, result.BuildWith) + assert.Equal(t, map[string]string{"NODE_ENV": "production"}, result.RuntimeEnv) + }) } // testDenyPolicyGate is a test helper that always blocks server creation with @@ -633,6 +693,134 @@ func TestCreateWorkloadFromRequest_PolicyGateDenied(t *testing.T) { require.ErrorIs(t, err, sentinel) } +// testCapturePolicyGate snapshots the RuntimeConfig it was asked to check, +// so a test can assert what the policy gate actually evaluated at call time +// - not just what the same object looks like later. Storing the +// *runner.RunConfig pointer instead would be wrong: a caller that mutates it +// after this call returns (e.g. a restore-after-the-fact) would make a later +// inspection of the pointer lie about what the gate actually saw. +type testCapturePolicyGate struct { + runner.NoopPolicyGate + called bool + snapshot *templates.RuntimeConfig +} + +func (g *testCapturePolicyGate) CheckCreateServer(_ context.Context, rc *runner.RunConfig) error { + g.called = true + g.snapshot = rc.RuntimeConfig.Clone() + return nil +} + +// TestBuildFullRunConfig_EchoedRuntimeConfigVisibleToPolicyGate guards the +// echo fix's synchronous policy gate: EnforcePolicyAndPullImage runs inside +// BuildFullRunConfig, so for an unchanged runtime_config echo on a +// non-protocol image, the gate must still see the real runtime_config, not +// nil. A clear-then-restore-after-BuildFullRunConfig-returns approach (an +// earlier version of this fix) would let a policy restricting builder +// images, packages, build constraints or runtime env silently approve the +// edit, since the gate runs before the restore. Threading persisted straight +// into BuildFullRunConfig, so req.RuntimeConfig is never touched, closes +// that gap: the retriever/build input is suppressed for the echo, but +// runConfig.RuntimeConfig - what the policy gate and the caller both see - +// is always populated from the request. +// +//nolint:paralleltest // Mutates the global policy gate. +func TestBuildFullRunConfig_EchoedRuntimeConfigVisibleToPolicyGate(t *testing.T) { + const testImage = "toolhivelocal/uvx-arxiv-mcp-server:20260101000000" + + gate := &testCapturePolicyGate{} + original := runner.ActivePolicyGate() + runner.RegisterPolicyGate(gate) + t.Cleanup(func() { runner.RegisterPolicyGate(original) }) + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockGroupManager := groupsmocks.NewMockManager(ctrl) + mockGroupManager.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) + + echoedConfig := &templates.RuntimeConfig{ + BuilderImage: "python:3.14-slim", + AdditionalPackages: []string{"ca-certificates"}, + BuildWith: []string{"mcp<2"}, + RuntimeEnv: map[string]string{"NODE_ENV": "production"}, + } + persisted := &runner.RunConfig{Image: testImage, RuntimeConfig: echoedConfig} + + mockRetriever := func( + _ context.Context, _ string, _ string, _ string, _ string, rc *templates.RuntimeConfig, + ) (string, regtypes.ServerMetadata, error) { + // The retriever/build input is suppressed for an echo - nothing to build. + assert.Nil(t, rc) + return testImage, ®types.ImageMetadata{Image: testImage}, nil + } + + service := &WorkloadService{ + groupManager: mockGroupManager, + imageRetriever: mockRetriever, + imagePuller: func(_ context.Context, _ string) error { return nil }, + configProvider: config.NewDefaultProvider(), + imageVerification: retriever.VerifyImageWarn, + } + + req := &createRequest{ + updateRequest: updateRequest{ + Image: testImage, + RuntimeConfig: echoedConfig, + }, + } + + runConfig, err := service.BuildFullRunConfig(context.Background(), req, 0, persisted) + require.NoError(t, err) + require.NotNil(t, runConfig.RuntimeConfig, + "echoed runtime_config must survive on the built RunConfig, not just be accepted") + assert.Equal(t, echoedConfig, runConfig.RuntimeConfig) + + require.True(t, gate.called, "policy gate must have been invoked") + require.NotNil(t, gate.snapshot, + "policy gate must evaluate the real runtime_config, not nil, AT CALL TIME - a policy "+ + "restricting builder images, packages, build constraints or runtime env must not be "+ + "bypassable by echoing an unchanged config back") + assert.Equal(t, echoedConfig, gate.snapshot) +} + +// TestUpdateWorkloadFromRequest_CorruptStateSurfacesError guards the +// not-found-vs-failure distinction in UpdateWorkloadFromRequest's echo check: +// a corrupt or unreadable state file must surface as an error, not be +// silently treated the same as "no persisted state" - which would let the +// protocol-scheme guard produce a misleading 400 that hides the real cause. +// +//nolint:paralleltest // Uses process-wide XDG state settings; keep sequential. +func TestUpdateWorkloadFromRequest_CorruptStateSurfacesError(t *testing.T) { + t.Cleanup(xdg.Reload) + t.Setenv("XDG_STATE_HOME", t.TempDir()) + xdg.Reload() + + const workloadName = "test-workload" + const testImage = "toolhivelocal/uvx-arxiv-mcp-server:20260101000000" + + // Write a corrupt state file directly, bypassing SaveState, so LoadState + // fails with something other than "not found". + dir := filepath.Join(xdg.StateHome, state.DefaultAppName, state.RunConfigsDir) + require.NoError(t, os.MkdirAll(dir, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(dir, workloadName+state.FileExtension), []byte("{not valid json"), 0o600)) + + service := &WorkloadService{configProvider: config.NewDefaultProvider()} + req := &createRequest{ + updateRequest: updateRequest{ + Image: testImage, + RuntimeConfig: &templates.RuntimeConfig{BuilderImage: "python:3.14-slim"}, + }, + } + + _, err := service.UpdateWorkloadFromRequest(context.Background(), workloadName, req, 0) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to load persisted state for workload") + assert.NotContains(t, err.Error(), "runtime_config is only supported for protocol-scheme images", + "a corrupt state file must not be mistaken for 'no persisted state' and fall through "+ + "to the unrelated protocol-scheme rejection") +} + func TestApplyImageDefaults(t *testing.T) { t.Parallel() diff --git a/pkg/api/v1/workload_types.go b/pkg/api/v1/workload_types.go index e11162cff5..5602dd3b98 100644 --- a/pkg/api/v1/workload_types.go +++ b/pkg/api/v1/workload_types.go @@ -42,10 +42,13 @@ type workloadStatusResponse struct { type updateRequest struct { // Docker image to use Image string `json:"image"` - // RuntimeConfig is only accepted on create/update when image is a protocol - // URI such as go://, npx://, or uvx://. - // GET responses may include runtime_config for existing workloads, but - // clients should not send it back with a built/non-protocol image. + // RuntimeConfig is accepted on create/update when image is a protocol + // URI such as go://, npx://, or uvx://. GET responses may include + // runtime_config for existing workloads; on update it may be sent back + // unchanged even with a built/non-protocol image, as long as it exactly + // matches the persisted config and the image and URL are unchanged - it + // is then preserved rather than applied to a rebuild. Any other + // runtime_config on a non-protocol image is rejected. RuntimeConfig *templates.RuntimeConfig `json:"runtime_config,omitempty"` // Host to bind to Host string `json:"host"` @@ -392,14 +395,11 @@ func runConfigToCreateRequest(runConfig *runner.RunConfig) *createRequest { } func runtimeConfigForResponse(runConfig *runner.RunConfig) *templates.RuntimeConfig { - if runConfig == nil || runConfig.RuntimeConfig == nil { + if runConfig == nil { return nil } - - return &templates.RuntimeConfig{ - BuilderImage: runConfig.RuntimeConfig.BuilderImage, - AdditionalPackages: append([]string{}, runConfig.RuntimeConfig.AdditionalPackages...), - } + // Clone's nil-receiver handling covers a nil runConfig.RuntimeConfig too. + return runConfig.RuntimeConfig.Clone() } // validateHeaderForwardConfig validates the header forward configuration. diff --git a/pkg/api/v1/workloads.go b/pkg/api/v1/workloads.go index 5956318909..47df648f01 100644 --- a/pkg/api/v1/workloads.go +++ b/pkg/api/v1/workloads.go @@ -325,6 +325,9 @@ func (s *WorkloadRoutes) deleteWorkload(w http.ResponseWriter, r *http.Request) // // @Summary Create a new workload // @Description Create and start a new workload +// @Description runtime_config is only accepted for protocol-scheme images +// @Description (uvx://, npx://, go://); supplying it with an ordinary image +// @Description reference or a remote url is rejected with 400. // @Tags workloads // @Accept json // @Produce json @@ -401,6 +404,10 @@ func (s *WorkloadRoutes) createWorkload(w http.ResponseWriter, r *http.Request) // // @Summary Update workload // @Description Update an existing workload configuration +// @Description runtime_config on a non-protocol-scheme image is accepted only when it +// @Description exactly matches the workload's persisted config and the image and url +// @Description are unchanged (an inert echo, e.g. from a prior GET); otherwise it is +// @Description rejected with 400. // @Tags workloads // @Accept json // @Produce json diff --git a/pkg/api/v1/workloads_test.go b/pkg/api/v1/workloads_test.go index 6091879908..ae1a37b6f4 100644 --- a/pkg/api/v1/workloads_test.go +++ b/pkg/api/v1/workloads_test.go @@ -4,6 +4,7 @@ package v1 import ( + "bytes" "context" "fmt" "net" @@ -12,11 +13,11 @@ import ( "strings" "testing" + "github.com/adrg/xdg" "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" - "golang.org/x/sync/errgroup" regtypes "github.com/stacklok/toolhive-core/registry/types" apierrors "github.com/stacklok/toolhive/pkg/api/errors" @@ -28,6 +29,7 @@ import ( groupsmocks "github.com/stacklok/toolhive/pkg/groups/mocks" "github.com/stacklok/toolhive/pkg/runner" "github.com/stacklok/toolhive/pkg/runner/retriever" + "github.com/stacklok/toolhive/pkg/workloads" workloadsmocks "github.com/stacklok/toolhive/pkg/workloads/mocks" wt "github.com/stacklok/toolhive/pkg/workloads/types" ) @@ -97,8 +99,20 @@ func TestGetWorkload(t *testing.T) { } } +// TestCreateWorkload cannot call t.Parallel() at its own level: two cases +// below compute their expectation via getBaseRuntimeConfig, which reads the +// process-wide config singleton (config.NewProvider() -> getSingletonConfig). +// Fixing that singleton to a known-empty config makes both the test's +// expectation and the production code path deterministic regardless of the +// developer's real ~/.config/toolhive - a configured additional_packages or +// runtime_env would otherwise make the expectation wrong on that machine. +// Subtests still run in parallel with each other; they just all observe the +// same fixed singleton, so there's nothing to race on. +// +//nolint:paralleltest,tparallel // Mutates the process-global config singleton; see comment above. func TestCreateWorkload(t *testing.T) { - t.Parallel() + config.SetSingletonConfig(&config.Config{}) + t.Cleanup(config.ResetSingleton) tests := []struct { name string @@ -137,8 +151,9 @@ func TestCreateWorkload(t *testing.T) { expectedBody: "Invalid proxy_mode", }, { - name: "with runtime config override", - requestBody: `{"name": "test-workload", "image": "go://github.com/example/server", "runtime_config": {"builder_image": "golang:1.24-alpine", "additional_packages": ["ca-certificates"]}}`, + name: "with runtime config override", + requestBody: `{"name": "test-workload", "image": "go://github.com/example/server", ` + + `"runtime_config": {"builder_image": "golang:1.24-alpine", "additional_packages": ["curl"]}}`, setupMock: func(_ *testing.T, wm *workloadsmocks.MockManager, _ *runtimemocks.MockRuntime, gm *groupsmocks.MockManager) { wm.EXPECT().DoesWorkloadExist(gomock.Any(), "test-workload").Return(false, nil) gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) @@ -146,15 +161,21 @@ func TestCreateWorkload(t *testing.T) { DoAndReturn(func(_ context.Context, runConfig *runner.RunConfig) error { assert.NotNil(t, runConfig.RuntimeConfig) assert.Equal(t, "golang:1.24-alpine", runConfig.RuntimeConfig.BuilderImage) - assert.Equal(t, []string{"ca-certificates"}, runConfig.RuntimeConfig.AdditionalPackages) + assert.Equal(t, []string{"curl"}, runConfig.RuntimeConfig.AdditionalPackages) return nil }) }, expectedRuntimeConfig: func() *templates.RuntimeConfig { base := getBaseRuntimeConfig(templates.TransportTypeGO) + // "curl" is not a Go default on any machine's config, so it + // must survive the merge regardless of local overrides in + // ~/.config/toolhive — proves the override is genuinely + // applied rather than the assertion being self-referential. + // Dedupe itself is covered hermetically in + // pkg/container/templates/runtime_config_test.go. return &templates.RuntimeConfig{ BuilderImage: "golang:1.24-alpine", - AdditionalPackages: append(append([]string{}, base.AdditionalPackages...), "ca-certificates"), + AdditionalPackages: append(append([]string{}, base.AdditionalPackages...), "curl"), } }(), expectedServerOrImage: "go://github.com/example/server", @@ -187,6 +208,68 @@ func TestCreateWorkload(t *testing.T) { expectedStatus: http.StatusBadRequest, expectedBody: "runtime_config is only supported for protocol-scheme images", }, + { + // build_with is only supported for uvx builds; npx must be + // rejected with an actionable 400, not a scrubbed 500 (the + // bug this design closes — see pkg/api/errors/handler.go). + name: "npx build_with is rejected with 400, not a scrubbed 500", + requestBody: `{"name": "test-workload", "image": "npx://some-pkg", "runtime_config": {"build_with": ["mcp<2"]}}`, + setupMock: func(_ *testing.T, wm *workloadsmocks.MockManager, _ *runtimemocks.MockRuntime, gm *groupsmocks.MockManager) { + wm.EXPECT().DoesWorkloadExist(gomock.Any(), "test-workload").Return(false, nil) + gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) + }, + expectedStatus: http.StatusBadRequest, + expectedBody: "build_with is not supported for npx:// builds", + }, + { + // runtime_env-only requests must not slip past the emptiness + // short-circuit: a request carrying only runtime_env against a + // non-protocol image must still hit the protocol-scheme guard, + // not be silently discarded and accepted. + name: "runtime_env only, non protocol image is rejected", + requestBody: `{"name": "test-workload", "image": "nginx:latest", "runtime_config": {"runtime_env": {"NODE_ENV": "production"}}}`, + setupMock: func(_ *testing.T, wm *workloadsmocks.MockManager, _ *runtimemocks.MockRuntime, gm *groupsmocks.MockManager) { + wm.EXPECT().DoesWorkloadExist(gomock.Any(), "test-workload").Return(false, nil) + gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) + }, + expectedStatus: http.StatusBadRequest, + expectedBody: "runtime_config is only supported for protocol-scheme images", + }, + { + // The headline bug: build_with (and runtime_env) must actually + // reach the build, not just be accepted. Pins both sinks fed by + // runtimeConfigFromRequest: the retriever config (via + // expectedRuntimeConfig, coming from runtimeConfigForImageBuild's + // WithOverrides merge) and the persisted config (via the + // RunWorkloadDetached closure, coming from runtimeConfigFromRequest + // unmerged) — these are two different functions. + name: "uvx build_with and runtime_env reach both the build and the persisted config", + requestBody: `{"name": "test-workload", "image": "uvx://arxiv-mcp-server", ` + + `"runtime_config": {"build_with": ["mcp<2"], "runtime_env": {"NODE_ENV": "production"}}}`, + setupMock: func(_ *testing.T, wm *workloadsmocks.MockManager, _ *runtimemocks.MockRuntime, gm *groupsmocks.MockManager) { + wm.EXPECT().DoesWorkloadExist(gomock.Any(), "test-workload").Return(false, nil) + gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) + wm.EXPECT().RunWorkloadDetached(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, runConfig *runner.RunConfig) error { + assert.NotNil(t, runConfig.RuntimeConfig) + assert.Equal(t, []string{"mcp<2"}, runConfig.RuntimeConfig.BuildWith) + assert.Equal(t, map[string]string{"NODE_ENV": "production"}, runConfig.RuntimeConfig.RuntimeEnv) + return nil + }) + }, + expectedRuntimeConfig: func() *templates.RuntimeConfig { + base := getBaseRuntimeConfig(templates.TransportTypeUVX) + return &templates.RuntimeConfig{ + BuilderImage: base.BuilderImage, + AdditionalPackages: base.AdditionalPackages, + BuildWith: []string{"mcp<2"}, + RuntimeEnv: map[string]string{"NODE_ENV": "production"}, + } + }(), + expectedServerOrImage: "uvx://arxiv-mcp-server", + expectedStatus: http.StatusCreated, + expectedBody: "test-workload", + }, { name: "with tool filters", requestBody: `{"name": "test-workload", "image": "test-image", "tools": ["filter1", "filter2"]}`, @@ -371,10 +454,10 @@ func TestUpdateWorkload(t *testing.T) { Return(core.Workload{Name: "test-workload"}, nil) gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) wm.EXPECT().UpdateWorkload(gomock.Any(), "test-workload", gomock.Any()). - DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (*errgroup.Group, error) { + DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (workloads.CompletionFunc, error) { assert.Equal(t, toolsFilter, runConfig.ToolsFilter, "Tools filter should be equal") assert.Equal(t, toolsOverride, runConfig.ToolsOverride, "Tools override should be equal") - return &errgroup.Group{}, nil + return nil, nil }) }, expectedStatus: http.StatusOK, @@ -397,10 +480,10 @@ func TestUpdateWorkload(t *testing.T) { Return(core.Workload{Name: "test-workload"}, nil) gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) wm.EXPECT().UpdateWorkload(gomock.Any(), "test-workload", gomock.Any()). - DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (*errgroup.Group, error) { + DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (workloads.CompletionFunc, error) { assert.Equal(t, toolsFilter, runConfig.ToolsFilter, "Tools filter should be equal") assert.Equal(t, toolsOverride, runConfig.ToolsOverride, "Tools override should be equal") - return &errgroup.Group{}, nil + return nil, nil }) }, expectedStatus: http.StatusOK, @@ -423,10 +506,10 @@ func TestUpdateWorkload(t *testing.T) { Return(core.Workload{Name: "test-workload"}, nil) gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) wm.EXPECT().UpdateWorkload(gomock.Any(), "test-workload", gomock.Any()). - DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (*errgroup.Group, error) { + DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (workloads.CompletionFunc, error) { assert.Equal(t, toolsFilter, runConfig.ToolsFilter, "Tools filter should be equal") assert.Equal(t, toolsOverride, runConfig.ToolsOverride, "Tools override should be equal") - return &errgroup.Group{}, nil + return nil, nil }) }, expectedStatus: http.StatusOK, @@ -454,9 +537,9 @@ func TestUpdateWorkload(t *testing.T) { Return(core.Workload{Name: "test-workload"}, nil) gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) wm.EXPECT().UpdateWorkload(gomock.Any(), "test-workload", gomock.Any()). - DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (*errgroup.Group, error) { + DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (workloads.CompletionFunc, error) { assert.Nil(t, runConfig.RuntimeConfig) - return &errgroup.Group{}, nil + return nil, nil }) }, expectedStatus: http.StatusOK, @@ -524,6 +607,314 @@ func TestUpdateWorkload(t *testing.T) { } } +// TestUpdateWorkload_ProtocolBuiltRuntimeConfigRoundTrip guards the GET-edit-PUT +// regression: a workload built from a protocol-scheme image (uvx://, npx://, +// go://) persists Image as the *built* image (no longer a protocol scheme) +// plus the RuntimeConfig used to build it. GET echoes both back, and PUT-ing +// that response unchanged must succeed rather than 400 on the protocol-scheme +// guard in runtimeConfigForImageBuild. A genuinely different runtime_config on +// the same non-protocol image must still be rejected. +// +//nolint:paralleltest // SaveState/LoadState use process-wide XDG state settings; keep sequential. +func TestUpdateWorkload_ProtocolBuiltRuntimeConfigRoundTrip(t *testing.T) { + t.Cleanup(xdg.Reload) + t.Setenv("XDG_STATE_HOME", t.TempDir()) + xdg.Reload() + + ctx := context.Background() + const workloadName = "test-workload" + builtImage := "toolhivelocal/uvx-arxiv-mcp-server:20260101000000" + + persisted := runner.NewRunConfig() + persisted.Name = workloadName + persisted.BaseName = workloadName + persisted.ContainerName = workloadName + persisted.Image = builtImage + persisted.RuntimeConfig = &templates.RuntimeConfig{ + BuilderImage: "python:3.14-slim", + AdditionalPackages: []string{"ca-certificates"}, + BuildWith: []string{"mcp<2"}, + RuntimeEnv: map[string]string{"NODE_ENV": "production"}, + } + require.NoError(t, persisted.SaveState(ctx)) + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockWorkloadManager := workloadsmocks.NewMockManager(ctrl) + mockRuntime := runtimemocks.NewMockRuntime(ctrl) + mockGroupManager := groupsmocks.NewMockManager(ctrl) + + routes := &WorkloadRoutes{ + workloadManager: mockWorkloadManager, + containerRuntime: mockRuntime, + groupManager: mockGroupManager, + workloadService: &WorkloadService{ + groupManager: mockGroupManager, + workloadManager: mockWorkloadManager, + imagePuller: func(_ context.Context, _ string) error { return nil }, + configProvider: config.NewDefaultProvider(), + imageVerification: retriever.VerifyImageWarn, + }, + } + + // GET: fetch the persisted config as JSON, exactly as a client would. + mockWorkloadManager.EXPECT().GetWorkload(gomock.Any(), workloadName). + Return(core.Workload{Name: workloadName}, nil) + + getReq := httptest.NewRequest("GET", "/"+workloadName, nil) + getRctx := chi.NewRouteContext() + getRctx.URLParams.Add("name", workloadName) + getReq = getReq.WithContext(context.WithValue(getReq.Context(), chi.RouteCtxKey, getRctx)) + getW := httptest.NewRecorder() + apierrors.ErrorHandler(routes.getWorkload).ServeHTTP(getW, getReq) + require.Equal(t, http.StatusOK, getW.Code, getW.Body.String()) + getBody := getW.Body.Bytes() + + t.Run("PUT the GET response back unchanged succeeds and preserves the config", func(t *testing.T) { + mockWorkloadManager.EXPECT().GetWorkload(gomock.Any(), workloadName). + Return(core.Workload{Name: workloadName}, nil) + mockGroupManager.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) + mockWorkloadManager.EXPECT().UpdateWorkload(gomock.Any(), workloadName, gomock.Any()). + DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (workloads.CompletionFunc, error) { + assert.NotNil(t, runConfig.RuntimeConfig) + assert.Equal(t, "python:3.14-slim", runConfig.RuntimeConfig.BuilderImage) + assert.Equal(t, []string{"ca-certificates"}, runConfig.RuntimeConfig.AdditionalPackages) + assert.Equal(t, []string{"mcp<2"}, runConfig.RuntimeConfig.BuildWith) + assert.Equal(t, map[string]string{"NODE_ENV": "production"}, runConfig.RuntimeConfig.RuntimeEnv) + return nil, nil + }) + // The runtime_config is an exact echo, so it's suppressed from the + // retriever/builder input only (nothing to rebuild); the request's + // runtime_config itself is never cleared, so runConfig.RuntimeConfig + // (asserted above) is populated from the request throughout. + routes.workloadService.imageRetriever = makeMockRetriever(t, builtImage, ®types.ImageMetadata{Image: builtImage}, nil) + + putReq := httptest.NewRequest("POST", "/"+workloadName+"/edit", bytes.NewReader(getBody)) + putReq.Header.Set("Content-Type", "application/json") + rctx := chi.NewRouteContext() + rctx.URLParams.Add("name", workloadName) + putReq = putReq.WithContext(context.WithValue(putReq.Context(), chi.RouteCtxKey, rctx)) + + putW := httptest.NewRecorder() + apierrors.ErrorHandler(routes.updateWorkload).ServeHTTP(putW, putReq) + assert.Equal(t, http.StatusOK, putW.Code, putW.Body.String()) + }) + + t.Run("genuinely different runtime_config on the same non-protocol image still 400s", func(t *testing.T) { + mockWorkloadManager.EXPECT().GetWorkload(gomock.Any(), workloadName). + Return(core.Workload{Name: workloadName}, nil) + mockGroupManager.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) + + body := fmt.Sprintf(`{"image": %q, "runtime_config": {"build_with": ["mcp>=3"]}}`, builtImage) + putReq := httptest.NewRequest("POST", "/"+workloadName+"/edit", strings.NewReader(body)) + putReq.Header.Set("Content-Type", "application/json") + rctx := chi.NewRouteContext() + rctx.URLParams.Add("name", workloadName) + putReq = putReq.WithContext(context.WithValue(putReq.Context(), chi.RouteCtxKey, rctx)) + + putW := httptest.NewRecorder() + apierrors.ErrorHandler(routes.updateWorkload).ServeHTTP(putW, putReq) + assert.Equal(t, http.StatusBadRequest, putW.Code) + assert.Contains(t, putW.Body.String(), "runtime_config is only supported for protocol-scheme images") + }) + + t.Run("changed image with echoed runtime_config still 400s", func(t *testing.T) { + mockWorkloadManager.EXPECT().GetWorkload(gomock.Any(), workloadName). + Return(core.Workload{Name: workloadName}, nil) + mockGroupManager.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) + + // Same runtime_config as persisted, but a different image - not an + // echo of the source, so the guard must still fire. + body := `{"image": "nginx:latest", "runtime_config": {"builder_image": "python:3.14-slim", ` + + `"additional_packages": ["ca-certificates"], "build_with": ["mcp<2"], ` + + `"runtime_env": {"NODE_ENV": "production"}}}` + putReq := httptest.NewRequest("POST", "/"+workloadName+"/edit", strings.NewReader(body)) + putReq.Header.Set("Content-Type", "application/json") + rctx := chi.NewRouteContext() + rctx.URLParams.Add("name", workloadName) + putReq = putReq.WithContext(context.WithValue(putReq.Context(), chi.RouteCtxKey, rctx)) + + putW := httptest.NewRecorder() + apierrors.ErrorHandler(routes.updateWorkload).ServeHTTP(putW, putReq) + assert.Equal(t, http.StatusBadRequest, putW.Code) + assert.Contains(t, putW.Body.String(), "runtime_config is only supported for protocol-scheme images") + }) + + t.Run("changed url with echoed runtime_config still 400s", func(t *testing.T) { + mockWorkloadManager.EXPECT().GetWorkload(gomock.Any(), workloadName). + Return(core.Workload{Name: workloadName}, nil) + mockGroupManager.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) + + // Same runtime_config as persisted, but a URL where the persisted + // workload had none - not an echo of the source. + body := `{"url": "https://example.com", "runtime_config": {"builder_image": "python:3.14-slim", ` + + `"additional_packages": ["ca-certificates"], "build_with": ["mcp<2"], ` + + `"runtime_env": {"NODE_ENV": "production"}}}` + putReq := httptest.NewRequest("POST", "/"+workloadName+"/edit", strings.NewReader(body)) + putReq.Header.Set("Content-Type", "application/json") + rctx := chi.NewRouteContext() + rctx.URLParams.Add("name", workloadName) + putReq = putReq.WithContext(context.WithValue(putReq.Context(), chi.RouteCtxKey, rctx)) + + putW := httptest.NewRecorder() + apierrors.ErrorHandler(routes.updateWorkload).ServeHTTP(putW, putReq) + assert.Equal(t, http.StatusBadRequest, putW.Code) + assert.Contains(t, putW.Body.String(), "runtime_config is only supported for protocol-scheme images") + }) +} + +// TestUpdateWorkload_RemoteEchoPreservesRuntimeConfig guards against the +// req.URL == "" guard on WithRuntimeConfig silently dropping an accepted +// echo's RuntimeConfig for remote workloads. thv run --remote-url with +// --runtime-image persists a RuntimeConfig on a remote workload today +// (configureRuntimeOptions in cmd/thv/app/run_flags.go does not exclude +// remote workloads), so an unchanged GET-edit-PUT of such a workload must +// round-trip the config, not silently lose it. +// +//nolint:paralleltest // Uses process-wide XDG state settings; keep sequential. +func TestUpdateWorkload_RemoteEchoPreservesRuntimeConfig(t *testing.T) { + t.Cleanup(xdg.Reload) + t.Setenv("XDG_STATE_HOME", t.TempDir()) + xdg.Reload() + + ctx := context.Background() + const workloadName = "test-remote-workload" + + persisted := runner.NewRunConfig() + persisted.Name = workloadName + persisted.BaseName = workloadName + persisted.ContainerName = workloadName + persisted.RemoteURL = "https://mcp.example.com/mcp" + persisted.RuntimeConfig = &templates.RuntimeConfig{BuilderImage: "python:3.14-slim"} + require.NoError(t, persisted.SaveState(ctx)) + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockWorkloadManager := workloadsmocks.NewMockManager(ctrl) + mockRuntime := runtimemocks.NewMockRuntime(ctrl) + mockGroupManager := groupsmocks.NewMockManager(ctrl) + + routes := &WorkloadRoutes{ + workloadManager: mockWorkloadManager, + containerRuntime: mockRuntime, + groupManager: mockGroupManager, + workloadService: &WorkloadService{ + groupManager: mockGroupManager, + workloadManager: mockWorkloadManager, + configProvider: config.NewDefaultProvider(), + imageVerification: retriever.VerifyImageWarn, + }, + } + + mockWorkloadManager.EXPECT().GetWorkload(gomock.Any(), workloadName). + Return(core.Workload{Name: workloadName}, nil) + + getReq := httptest.NewRequest("GET", "/"+workloadName, nil) + getRctx := chi.NewRouteContext() + getRctx.URLParams.Add("name", workloadName) + getReq = getReq.WithContext(context.WithValue(getReq.Context(), chi.RouteCtxKey, getRctx)) + getW := httptest.NewRecorder() + apierrors.ErrorHandler(routes.getWorkload).ServeHTTP(getW, getReq) + require.Equal(t, http.StatusOK, getW.Code, getW.Body.String()) + getBody := getW.Body.Bytes() + + mockWorkloadManager.EXPECT().GetWorkload(gomock.Any(), workloadName). + Return(core.Workload{Name: workloadName}, nil) + mockGroupManager.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) + mockWorkloadManager.EXPECT().UpdateWorkload(gomock.Any(), workloadName, gomock.Any()). + DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (workloads.CompletionFunc, error) { + require.NotNil(t, runConfig.RuntimeConfig) + assert.Equal(t, "python:3.14-slim", runConfig.RuntimeConfig.BuilderImage) + return nil, nil + }) + + putReq := httptest.NewRequest("POST", "/"+workloadName+"/edit", bytes.NewReader(getBody)) + putReq.Header.Set("Content-Type", "application/json") + rctx := chi.NewRouteContext() + rctx.URLParams.Add("name", workloadName) + putReq = putReq.WithContext(context.WithValue(putReq.Context(), chi.RouteCtxKey, rctx)) + + putW := httptest.NewRecorder() + apierrors.ErrorHandler(routes.updateWorkload).ServeHTTP(putW, putReq) + assert.Equal(t, http.StatusOK, putW.Code, putW.Body.String()) +} + +// TestUpdateWorkload_PaddedBuilderImageEchoRoundTrips guards normalization +// being applied to the persisted side of the echo comparison, not just the +// request side: a builder_image persisted with surrounding whitespace +// (reachable via --runtime-image, a plain StringVar with no trim-on-store) +// must still compare equal to its own unchanged echo. +// +//nolint:paralleltest // Uses process-wide XDG state settings; keep sequential. +func TestUpdateWorkload_PaddedBuilderImageEchoRoundTrips(t *testing.T) { + t.Cleanup(xdg.Reload) + t.Setenv("XDG_STATE_HOME", t.TempDir()) + xdg.Reload() + + ctx := context.Background() + const workloadName = "test-padded-workload" + builtImage := "toolhivelocal/uvx-arxiv-mcp-server:20260101000000" + + persisted := runner.NewRunConfig() + persisted.Name = workloadName + persisted.BaseName = workloadName + persisted.ContainerName = workloadName + persisted.Image = builtImage + persisted.RuntimeConfig = &templates.RuntimeConfig{BuilderImage: " golang:1.24-alpine "} + require.NoError(t, persisted.SaveState(ctx)) + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockWorkloadManager := workloadsmocks.NewMockManager(ctrl) + mockRuntime := runtimemocks.NewMockRuntime(ctrl) + mockGroupManager := groupsmocks.NewMockManager(ctrl) + + routes := &WorkloadRoutes{ + workloadManager: mockWorkloadManager, + containerRuntime: mockRuntime, + groupManager: mockGroupManager, + workloadService: &WorkloadService{ + groupManager: mockGroupManager, + workloadManager: mockWorkloadManager, + imageRetriever: makeMockRetriever(t, builtImage, ®types.ImageMetadata{Image: builtImage}, nil), + imagePuller: func(_ context.Context, _ string) error { return nil }, + configProvider: config.NewDefaultProvider(), + imageVerification: retriever.VerifyImageWarn, + }, + } + + mockWorkloadManager.EXPECT().GetWorkload(gomock.Any(), workloadName). + Return(core.Workload{Name: workloadName}, nil) + + getReq := httptest.NewRequest("GET", "/"+workloadName, nil) + getRctx := chi.NewRouteContext() + getRctx.URLParams.Add("name", workloadName) + getReq = getReq.WithContext(context.WithValue(getReq.Context(), chi.RouteCtxKey, getRctx)) + getW := httptest.NewRecorder() + apierrors.ErrorHandler(routes.getWorkload).ServeHTTP(getW, getReq) + require.Equal(t, http.StatusOK, getW.Code, getW.Body.String()) + getBody := getW.Body.Bytes() + + mockWorkloadManager.EXPECT().GetWorkload(gomock.Any(), workloadName). + Return(core.Workload{Name: workloadName}, nil) + mockGroupManager.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) + mockWorkloadManager.EXPECT().UpdateWorkload(gomock.Any(), workloadName, gomock.Any()). + Return(nil, nil) + + putReq := httptest.NewRequest("POST", "/"+workloadName+"/edit", bytes.NewReader(getBody)) + putReq.Header.Set("Content-Type", "application/json") + rctx := chi.NewRouteContext() + rctx.URLParams.Add("name", workloadName) + putReq = putReq.WithContext(context.WithValue(putReq.Context(), chi.RouteCtxKey, rctx)) + + putW := httptest.NewRecorder() + apierrors.ErrorHandler(routes.updateWorkload).ServeHTTP(putW, putReq) + assert.Equal(t, http.StatusOK, putW.Code, putW.Body.String()) +} + // TestUpdateWorkload_PortReuse tests the port reuse logic when editing workloads func TestUpdateWorkload_PortReuse(t *testing.T) { t.Parallel() @@ -549,9 +940,9 @@ func TestUpdateWorkload_PortReuse(t *testing.T) { Return(core.Workload{Name: "test-workload", Port: 8080}, nil) gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) wm.EXPECT().UpdateWorkload(gomock.Any(), "test-workload", gomock.Any()). - DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (*errgroup.Group, error) { + DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (workloads.CompletionFunc, error) { assert.Equal(t, 8080, runConfig.Port, "Port should be reused from existing workload") - return &errgroup.Group{}, nil + return nil, nil }) }, expectedStatus: http.StatusOK, @@ -569,9 +960,9 @@ func TestUpdateWorkload_PortReuse(t *testing.T) { Return(core.Workload{Name: "test-workload", Port: 8080}, nil) gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) wm.EXPECT().UpdateWorkload(gomock.Any(), "test-workload", gomock.Any()). - DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (*errgroup.Group, error) { + DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (workloads.CompletionFunc, error) { assert.Equal(t, 8080, runConfig.Port, "Port should remain the same") - return &errgroup.Group{}, nil + return nil, nil }) }, expectedStatus: http.StatusOK, @@ -589,9 +980,9 @@ func TestUpdateWorkload_PortReuse(t *testing.T) { Return(core.Workload{Name: "test-workload", Port: 8080}, nil) gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) wm.EXPECT().UpdateWorkload(gomock.Any(), "test-workload", gomock.Any()). - DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (*errgroup.Group, error) { + DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (workloads.CompletionFunc, error) { assert.Equal(t, 8080, runConfig.Port, "Port should default to existing port") - return &errgroup.Group{}, nil + return nil, nil }) }, expectedStatus: http.StatusOK, @@ -674,9 +1065,9 @@ func TestUpdateWorkload_PortReuse(t *testing.T) { Return(core.Workload{Name: "test-workload", Port: 8080}, nil) mockGroupManager.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) mockWorkloadManager.EXPECT().UpdateWorkload(gomock.Any(), "test-workload", gomock.Any()). - DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (*errgroup.Group, error) { + DoAndReturn(func(_ context.Context, _ string, runConfig *runner.RunConfig) (workloads.CompletionFunc, error) { assert.Equal(t, freePort, runConfig.Port, "Port should be set to explicitly requested port") - return &errgroup.Group{}, nil + return nil, nil }) mockRetriever := makeMockRetriever(t, diff --git a/pkg/api/v1/workloads_types_test.go b/pkg/api/v1/workloads_types_test.go index 8aeafd5701..34facd4ed8 100644 --- a/pkg/api/v1/workloads_types_test.go +++ b/pkg/api/v1/workloads_types_test.go @@ -413,6 +413,8 @@ func TestRunConfigToCreateRequest(t *testing.T) { RuntimeConfig: &templates.RuntimeConfig{ BuilderImage: "node:20-alpine", AdditionalPackages: []string{"git"}, + BuildWith: []string{"mcp<2"}, + RuntimeEnv: map[string]string{"NODE_ENV": "production"}, }, } @@ -422,6 +424,8 @@ func TestRunConfigToCreateRequest(t *testing.T) { require.NotNil(t, result.RuntimeConfig) assert.Equal(t, "node:20-alpine", result.RuntimeConfig.BuilderImage) assert.Equal(t, []string{"git"}, result.RuntimeConfig.AdditionalPackages) + assert.Equal(t, []string{"mcp<2"}, result.RuntimeConfig.BuildWith) + assert.Equal(t, map[string]string{"NODE_ENV": "production"}, result.RuntimeConfig.RuntimeEnv) }) t.Run("preserves runtime config for non protocol image", func(t *testing.T) { @@ -433,6 +437,8 @@ func TestRunConfigToCreateRequest(t *testing.T) { RuntimeConfig: &templates.RuntimeConfig{ BuilderImage: "node:20-alpine", AdditionalPackages: []string{"git"}, + BuildWith: []string{"mcp<2"}, + RuntimeEnv: map[string]string{"NODE_ENV": "production"}, }, } @@ -442,6 +448,8 @@ func TestRunConfigToCreateRequest(t *testing.T) { require.NotNil(t, result.RuntimeConfig) assert.Equal(t, "node:20-alpine", result.RuntimeConfig.BuilderImage) assert.Equal(t, []string{"git"}, result.RuntimeConfig.AdditionalPackages) + assert.Equal(t, []string{"mcp<2"}, result.RuntimeConfig.BuildWith) + assert.Equal(t, map[string]string{"NODE_ENV": "production"}, result.RuntimeConfig.RuntimeEnv) }) t.Run("nil runConfig", func(t *testing.T) { diff --git a/pkg/container/templates/runtime_config.go b/pkg/container/templates/runtime_config.go index b6e4aa39c5..542ad71c17 100644 --- a/pkg/container/templates/runtime_config.go +++ b/pkg/container/templates/runtime_config.go @@ -182,7 +182,10 @@ func (rc *RuntimeConfig) Clone() *RuntimeConfig { // - AdditionalPackages: the union, rc's entries first, then any override // entries not already present. // - RuntimeEnv: merged, with override's value winning on a shared key. -// - BuildWith: taken from override as-is; no defaults exist for it. +// - BuildWith: override wins if non-empty, else falls back to rc's — the +// same "override wins if set" rule as BuilderImage. Fallback rather than +// union: unioning two constraint sets could hand PEP 508 contradictory +// specifiers to uv. // // WithOverrides(nil) returns rc.Clone() — a distinct object, never rc itself. // A nil receiver returns override.Clone(). @@ -214,7 +217,11 @@ func (rc *RuntimeConfig) WithOverrides(override *RuntimeConfig) *RuntimeConfig { } merged.RuntimeEnv = mergeEnvMaps(rc.RuntimeEnv, override.RuntimeEnv) - merged.BuildWith = slices.Clone(override.BuildWith) + if len(override.BuildWith) > 0 { + merged.BuildWith = slices.Clone(override.BuildWith) + } else { + merged.BuildWith = slices.Clone(rc.BuildWith) + } return &merged } @@ -235,6 +242,15 @@ func (rc *RuntimeConfig) ValidateFor(transportType TransportType) error { return rc.Validate() } +// IsEmpty reports whether rc has no field set. A nil receiver is empty. +func (rc *RuntimeConfig) IsEmpty() bool { + if rc == nil { + return true + } + return rc.BuilderImage == "" && len(rc.AdditionalPackages) == 0 && + len(rc.BuildWith) == 0 && len(rc.RuntimeEnv) == 0 +} + // RuntimeDefaults provides default configurations for each runtime type var RuntimeDefaults = map[TransportType]RuntimeConfig{ TransportTypeGO: { diff --git a/pkg/container/templates/runtime_config_test.go b/pkg/container/templates/runtime_config_test.go index 7a58670ab2..f298af2360 100644 --- a/pkg/container/templates/runtime_config_test.go +++ b/pkg/container/templates/runtime_config_test.go @@ -570,15 +570,18 @@ func TestRuntimeConfigClone_Detached(t *testing.T) { func TestRuntimeConfigClone_DoesNotAliasRuntimeDefaults(t *testing.T) { t.Parallel() - original := GetDefaultRuntimeConfig(TransportTypeNPX) - wantPackages := slices.Clone(original.AdditionalPackages) + wantPackages := slices.Clone(RuntimeDefaults[TransportTypeNPX].AdditionalPackages) - clone := original.Clone() - clone.AdditionalPackages[0] = "mutated" + // Mutate the value GetDefaultRuntimeConfig hands back directly - not a + // Clone() of it - so this fails if GetDefaultRuntimeConfig ever reverts + // to a shallow `return config` instead of `return *config.Clone()`. + original := GetDefaultRuntimeConfig(TransportTypeNPX) + original.AdditionalPackages[0] = "mutated" - fresh := GetDefaultRuntimeConfig(TransportTypeNPX) - assert.Equal(t, wantPackages, fresh.AdditionalPackages, - "mutating a Clone() must not reach RuntimeDefaults") + assert.Equal(t, wantPackages, RuntimeDefaults[TransportTypeNPX].AdditionalPackages, + "mutating a GetDefaultRuntimeConfig() result must not reach RuntimeDefaults") + assert.Equal(t, wantPackages, GetDefaultRuntimeConfig(TransportTypeNPX).AdditionalPackages, + "a fresh GetDefaultRuntimeConfig() call must not observe the earlier mutation") } func TestRuntimeConfigWithOverrides(t *testing.T) { @@ -671,6 +674,29 @@ func TestRuntimeConfigWithOverrides_RuntimeEnvNilWhenBothEmpty(t *testing.T) { assert.Nil(t, base.WithOverrides(&RuntimeConfig{}).RuntimeEnv) } +// TestRuntimeConfigWithOverrides_BuildWithFallsBackToBase pins the API's +// exact scenario: getBaseRuntimeConfig reads the user's config file as the +// base, so a global runtime_configs.uvx.build_with pin must survive a +// request whose runtime_config sets unrelated fields only. +func TestRuntimeConfigWithOverrides_BuildWithFallsBackToBase(t *testing.T) { + t.Parallel() + + base := &RuntimeConfig{ + BuilderImage: "python:3.14-slim", + BuildWith: []string{"mcp<2"}, + } + override := &RuntimeConfig{BuilderImage: "python:3.11-slim"} + + got := base.WithOverrides(override) + assert.Equal(t, []string{"mcp<2"}, got.BuildWith) + + // Detachment: the merge must clone on the fallback path too, not just + // the override-wins path — otherwise merged.BuildWith aliases base's + // slice via the `merged := *rc` struct copy. + base.BuildWith[0] = "mutated" + assert.Equal(t, []string{"mcp<2"}, got.BuildWith) +} + func TestRuntimeConfigWithOverrides_NilEqualsClone(t *testing.T) { t.Parallel() @@ -737,11 +763,35 @@ func TestRuntimeConfigWithOverrides_OutputIsDetachedFromInputs(t *testing.T) { } // TestRuntimeConfigFieldCount guards against a field being added to -// RuntimeConfig without updating Clone and WithOverrides, both of which -// enumerate every field individually. +// RuntimeConfig without updating Clone, WithOverrides, and IsEmpty, all of +// which enumerate every field individually. func TestRuntimeConfigFieldCount(t *testing.T) { t.Parallel() - // Adding a field? Update Clone and WithOverrides, then bump this. + // Adding a field? Update Clone, WithOverrides, and IsEmpty, then bump this. require.Equal(t, 4, reflect.TypeOf(RuntimeConfig{}).NumField()) } + +func TestRuntimeConfigIsEmpty(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rc *RuntimeConfig + want bool + }{ + {name: "nil receiver", rc: nil, want: true}, + {name: "zero value", rc: &RuntimeConfig{}, want: true}, + {name: "builder image set", rc: &RuntimeConfig{BuilderImage: "golang:1.26-alpine"}, want: false}, + {name: "additional packages set", rc: &RuntimeConfig{AdditionalPackages: []string{"git"}}, want: false}, + {name: "build with set", rc: &RuntimeConfig{BuildWith: []string{"mcp<2"}}, want: false}, + {name: "runtime env set", rc: &RuntimeConfig{RuntimeEnv: map[string]string{"FOO": "bar"}}, want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, tt.rc.IsEmpty()) + }) + } +}