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 be5a59c7c7..542ad71c17 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,96 @@ 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: 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(). +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) + if len(override.BuildWith) > 0 { + merged.BuildWith = slices.Clone(override.BuildWith) + } else { + merged.BuildWith = slices.Clone(rc.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() +} + +// 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: { @@ -175,12 +267,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..f298af2360 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,267 @@ 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() + + wantPackages := slices.Clone(RuntimeDefaults[TransportTypeNPX].AdditionalPackages) + + // 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" + + 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) { + 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) +} + +// 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() + + 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, WithOverrides, and IsEmpty, all of +// which enumerate every field individually. +func TestRuntimeConfigFieldCount(t *testing.T) { + t.Parallel() + + // 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()) + }) + } +} 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) }