diff --git a/pkg/mcp/tools_config_envs.go b/pkg/mcp/tools_config_envs.go index 397b8c1eef..bf2a91567b 100644 --- a/pkg/mcp/tools_config_envs.go +++ b/pkg/mcp/tools_config_envs.go @@ -86,6 +86,9 @@ type ConfigEnvsAddInput struct { // constructed and forwarded to the CLI. Character-set validation is deferred to // func's own parser, keeping this layer as a thin pass-through. func (i ConfigEnvsAddInput) validate() error { + if i.Value == nil && i.SecretName == nil && i.ConfigMapName == nil { + return fmt.Errorf("at least one of value, secretName, or configMapName must be provided") + } if i.Value != nil && (i.SecretName != nil || i.ConfigMapName != nil) { return fmt.Errorf("value is mutually exclusive with secretName/configMapName; provide one or the other") } @@ -169,14 +172,13 @@ var configEnvsRemoveTool = &mcp.Tool{ } type ConfigEnvsRemoveInput struct { - Path string `json:"path" jsonschema:"required,Path to the function project directory"` - Name *string `json:"name,omitempty" jsonschema:"Name of the environment variable to remove"` - Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` + Path string `json:"path" jsonschema:"required,Path to the function project directory"` + Name string `json:"name" jsonschema:"required,Name of the environment variable to remove"` + Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` } func (i ConfigEnvsRemoveInput) Args() []string { - args := []string{"envs", "remove", "--path", i.Path} - args = appendStringFlag(args, "--name", i.Name) + args := []string{"envs", "remove", "--path", i.Path, "--name", i.Name} args = appendBoolFlag(args, "--verbose", i.Verbose) return args } @@ -186,6 +188,10 @@ type ConfigEnvsRemoveOutput struct { } func (s *Server) configEnvsRemoveHandler(ctx context.Context, r *mcp.CallToolRequest, input ConfigEnvsRemoveInput) (result *mcp.CallToolResult, output ConfigEnvsRemoveOutput, err error) { + if input.Name == "" { + err = fmt.Errorf("'name' must not be empty") + return + } out, err := s.executor.Execute(ctx, "config", input.Args()...) if err != nil { err = fmt.Errorf("%w\n%s", err, string(out)) diff --git a/pkg/mcp/tools_config_envs_test.go b/pkg/mcp/tools_config_envs_test.go index 327ab06f9a..3b9effa3cc 100644 --- a/pkg/mcp/tools_config_envs_test.go +++ b/pkg/mcp/tools_config_envs_test.go @@ -556,6 +556,99 @@ func TestTool_ConfigEnvsRemove(t *testing.T) { } } +// TestTool_ConfigEnvsAdd_BulkImport ensures the config_envs_add tool accepts calls without name +// for bulk-import syntax (e.g. value='{{ secret:mySecret }}'). +func TestTool_ConfigEnvsAdd_BulkImport(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + return []byte("Environment variables imported successfully\n"), nil + } + + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_envs_add", + Arguments: map[string]any{"path": ".", "value": "{{ secret:mySecret }}"}, + }) + if err != nil { + t.Fatal(err) + } + if result.IsError { + t.Fatalf("bulk-import without name must succeed: %v", result) + } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } +} + +// TestTool_ConfigEnvsAdd_MissingValue ensures the config_envs_add tool rejects calls without value. +func TestTool_ConfigEnvsAdd_MissingValue(t *testing.T) { + executor := mock.NewExecutor() + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_envs_add", + Arguments: map[string]any{"path": ".", "name": "API_KEY"}, + }) + // Schema validation may produce a protocol-level error or a tool-level error result. + if err == nil && !result.IsError { + t.Fatal("expected error when value is missing") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when required fields are absent") + } +} + +// TestTool_ConfigEnvsRemove_EmptyName ensures the config_envs_remove tool rejects an empty name. +func TestTool_ConfigEnvsRemove_EmptyName(t *testing.T) { + executor := mock.NewExecutor() + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_envs_remove", + Arguments: map[string]any{"path": ".", "name": ""}, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when name is empty") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when name is empty") + } +} + +// TestTool_ConfigEnvsRemove_MissingName ensures the config_envs_remove tool rejects calls without name. +func TestTool_ConfigEnvsRemove_MissingName(t *testing.T) { + executor := mock.NewExecutor() + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_envs_remove", + Arguments: map[string]any{"path": "."}, + }) + // Schema validation may produce a protocol-level error or a tool-level error result. + if err == nil && !result.IsError { + t.Fatal("expected error when name is missing") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when required fields are absent") + } +} + // TestTool_ConfigEnvsList_Error ensures the config_envs_list tool propagates executor errors. func TestTool_ConfigEnvsList_Error(t *testing.T) { executor := mock.NewExecutor() @@ -594,7 +687,7 @@ func TestTool_ConfigEnvsAdd_Error(t *testing.T) { result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ Name: "config_envs_add", - Arguments: map[string]any{"path": "."}, + Arguments: map[string]any{"path": ".", "name": "API_KEY", "value": "secret123"}, }) if err != nil { t.Fatal(err) @@ -602,6 +695,9 @@ func TestTool_ConfigEnvsAdd_Error(t *testing.T) { if !result.IsError { t.Fatal("expected error result, got success") } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } } // TestTool_ConfigEnvsRemove_Error ensures the config_envs_remove tool propagates executor errors. @@ -618,7 +714,7 @@ func TestTool_ConfigEnvsRemove_Error(t *testing.T) { result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ Name: "config_envs_remove", - Arguments: map[string]any{"path": "."}, + Arguments: map[string]any{"path": ".", "name": "API_KEY"}, }) if err != nil { t.Fatal(err) @@ -626,4 +722,7 @@ func TestTool_ConfigEnvsRemove_Error(t *testing.T) { if !result.IsError { t.Fatal("expected error result, got success") } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } } diff --git a/pkg/mcp/tools_config_labels.go b/pkg/mcp/tools_config_labels.go index 0c62e87290..ee99bd60f2 100644 --- a/pkg/mcp/tools_config_labels.go +++ b/pkg/mcp/tools_config_labels.go @@ -60,16 +60,14 @@ var configLabelsAddTool = &mcp.Tool{ } type ConfigLabelsAddInput struct { - Path string `json:"path" jsonschema:"required,Path to the function project directory"` - Name *string `json:"name,omitempty" jsonschema:"Name of the label"` - Value *string `json:"value,omitempty" jsonschema:"Value of the label"` - Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` + Path string `json:"path" jsonschema:"required,Path to the function project directory"` + Name string `json:"name" jsonschema:"required,Name of the label"` + Value string `json:"value" jsonschema:"required,Value of the label"` + Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` } func (i ConfigLabelsAddInput) Args() []string { - args := []string{"labels", "add", "--path", i.Path} - args = appendStringFlag(args, "--name", i.Name) - args = appendStringFlag(args, "--value", i.Value) + args := []string{"labels", "add", "--path", i.Path, "--name", i.Name, "--value", i.Value} args = appendBoolFlag(args, "--verbose", i.Verbose) return args } @@ -79,6 +77,14 @@ type ConfigLabelsAddOutput struct { } func (s *Server) configLabelsAddHandler(ctx context.Context, r *mcp.CallToolRequest, input ConfigLabelsAddInput) (result *mcp.CallToolResult, output ConfigLabelsAddOutput, err error) { + if input.Name == "" { + err = fmt.Errorf("'name' must not be empty") + return + } + if input.Value == "" { + err = fmt.Errorf("'value' must not be empty") + return + } out, err := s.executor.Execute(ctx, "config", input.Args()...) if err != nil { err = fmt.Errorf("%w\n%s", err, string(out)) @@ -103,14 +109,13 @@ var configLabelsRemoveTool = &mcp.Tool{ } type ConfigLabelsRemoveInput struct { - Path string `json:"path" jsonschema:"required,Path to the function project directory"` - Name *string `json:"name,omitempty" jsonschema:"Name of the label to remove"` - Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` + Path string `json:"path" jsonschema:"required,Path to the function project directory"` + Name string `json:"name" jsonschema:"required,Name of the label to remove"` + Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` } func (i ConfigLabelsRemoveInput) Args() []string { - args := []string{"labels", "remove", "--path", i.Path} - args = appendStringFlag(args, "--name", i.Name) + args := []string{"labels", "remove", "--path", i.Path, "--name", i.Name} args = appendBoolFlag(args, "--verbose", i.Verbose) return args } @@ -120,6 +125,10 @@ type ConfigLabelsRemoveOutput struct { } func (s *Server) configLabelsRemoveHandler(ctx context.Context, r *mcp.CallToolRequest, input ConfigLabelsRemoveInput) (result *mcp.CallToolResult, output ConfigLabelsRemoveOutput, err error) { + if input.Name == "" { + err = fmt.Errorf("'name' must not be empty") + return + } out, err := s.executor.Execute(ctx, "config", input.Args()...) if err != nil { err = fmt.Errorf("%w\n%s", err, string(out)) diff --git a/pkg/mcp/tools_config_labels_test.go b/pkg/mcp/tools_config_labels_test.go index e3d2a42c83..c693a86eb8 100644 --- a/pkg/mcp/tools_config_labels_test.go +++ b/pkg/mcp/tools_config_labels_test.go @@ -171,6 +171,138 @@ func TestTool_ConfigLabelsRemove(t *testing.T) { } } +// TestTool_ConfigLabelsAdd_EmptyName ensures the config_labels_add tool rejects an empty name. +func TestTool_ConfigLabelsAdd_EmptyName(t *testing.T) { + executor := mock.NewExecutor() + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_labels_add", + Arguments: map[string]any{"path": ".", "name": "", "value": "prod"}, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when name is empty") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when name is empty") + } +} + +// TestTool_ConfigLabelsAdd_EmptyValue ensures the config_labels_add tool rejects an empty value. +func TestTool_ConfigLabelsAdd_EmptyValue(t *testing.T) { + executor := mock.NewExecutor() + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_labels_add", + Arguments: map[string]any{"path": ".", "name": "environment", "value": ""}, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when value is empty") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when value is empty") + } +} + +// TestTool_ConfigLabelsRemove_EmptyName ensures the config_labels_remove tool rejects an empty name. +func TestTool_ConfigLabelsRemove_EmptyName(t *testing.T) { + executor := mock.NewExecutor() + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_labels_remove", + Arguments: map[string]any{"path": ".", "name": ""}, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when name is empty") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when name is empty") + } +} + +// TestTool_ConfigLabelsAdd_MissingName ensures the config_labels_add tool rejects calls without name. +func TestTool_ConfigLabelsAdd_MissingName(t *testing.T) { + executor := mock.NewExecutor() + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_labels_add", + Arguments: map[string]any{"path": ".", "value": "prod"}, + }) + // Schema validation may produce a protocol-level error or a tool-level error result. + if err == nil && !result.IsError { + t.Fatal("expected error when name is missing") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when required fields are absent") + } +} + +// TestTool_ConfigLabelsAdd_MissingValue ensures the config_labels_add tool rejects calls without value. +func TestTool_ConfigLabelsAdd_MissingValue(t *testing.T) { + executor := mock.NewExecutor() + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_labels_add", + Arguments: map[string]any{"path": ".", "name": "environment"}, + }) + // Schema validation may produce a protocol-level error or a tool-level error result. + if err == nil && !result.IsError { + t.Fatal("expected error when value is missing") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when required fields are absent") + } +} + +// TestTool_ConfigLabelsRemove_MissingName ensures the config_labels_remove tool rejects calls without name. +func TestTool_ConfigLabelsRemove_MissingName(t *testing.T) { + executor := mock.NewExecutor() + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_labels_remove", + Arguments: map[string]any{"path": "."}, + }) + // Schema validation may produce a protocol-level error or a tool-level error result. + if err == nil && !result.IsError { + t.Fatal("expected error when name is missing") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when required fields are absent") + } +} + // TestTool_ConfigLabelsList_Error ensures the config_labels_list tool propagates executor errors. func TestTool_ConfigLabelsList_Error(t *testing.T) { executor := mock.NewExecutor() @@ -209,7 +341,7 @@ func TestTool_ConfigLabelsAdd_Error(t *testing.T) { result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ Name: "config_labels_add", - Arguments: map[string]any{"path": "."}, + Arguments: map[string]any{"path": ".", "name": "environment", "value": "prod"}, }) if err != nil { t.Fatal(err) @@ -217,6 +349,9 @@ func TestTool_ConfigLabelsAdd_Error(t *testing.T) { if !result.IsError { t.Fatal("expected error result, got success") } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } } // TestTool_ConfigLabelsRemove_Error ensures the config_labels_remove tool propagates executor errors. @@ -233,7 +368,7 @@ func TestTool_ConfigLabelsRemove_Error(t *testing.T) { result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ Name: "config_labels_remove", - Arguments: map[string]any{"path": "."}, + Arguments: map[string]any{"path": ".", "name": "environment"}, }) if err != nil { t.Fatal(err) @@ -241,4 +376,7 @@ func TestTool_ConfigLabelsRemove_Error(t *testing.T) { if !result.IsError { t.Fatal("expected error result, got success") } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } } diff --git a/pkg/mcp/tools_config_volumes.go b/pkg/mcp/tools_config_volumes.go index 232d866d30..c41c404a56 100644 --- a/pkg/mcp/tools_config_volumes.go +++ b/pkg/mcp/tools_config_volumes.go @@ -111,14 +111,13 @@ var configVolumesRemoveTool = &mcp.Tool{ } type ConfigVolumesRemoveInput struct { - Path string `json:"path" jsonschema:"required,Path to the function project directory"` - MountPath *string `json:"mountPath,omitempty" jsonschema:"Mount path of the volume to remove"` - Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` + Path string `json:"path" jsonschema:"required,Path to the function project directory"` + MountPath string `json:"mountPath" jsonschema:"required,Mount path of the volume to remove"` + Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` } func (i ConfigVolumesRemoveInput) Args() []string { - args := []string{"volumes", "remove", "--path", i.Path} - args = appendStringFlag(args, "--mount-path", i.MountPath) + args := []string{"volumes", "remove", "--path", i.Path, "--mount-path", i.MountPath} args = appendBoolFlag(args, "--verbose", i.Verbose) return args } @@ -128,6 +127,10 @@ type ConfigVolumesRemoveOutput struct { } func (s *Server) configVolumesRemoveHandler(ctx context.Context, r *mcp.CallToolRequest, input ConfigVolumesRemoveInput) (result *mcp.CallToolResult, output ConfigVolumesRemoveOutput, err error) { + if input.MountPath == "" { + err = fmt.Errorf("'mountPath' must not be empty") + return + } out, err := s.executor.Execute(ctx, "config", input.Args()...) if err != nil { err = fmt.Errorf("%w\n%s", err, string(out)) diff --git a/pkg/mcp/tools_config_volumes_test.go b/pkg/mcp/tools_config_volumes_test.go index bc10965554..2fc8d874ea 100644 --- a/pkg/mcp/tools_config_volumes_test.go +++ b/pkg/mcp/tools_config_volumes_test.go @@ -175,6 +175,50 @@ func TestTool_ConfigVolumesRemove(t *testing.T) { } } +// TestTool_ConfigVolumesRemove_EmptyMountPath ensures the config_volumes_remove tool rejects an empty mountPath. +func TestTool_ConfigVolumesRemove_EmptyMountPath(t *testing.T) { + executor := mock.NewExecutor() + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_volumes_remove", + Arguments: map[string]any{"path": ".", "mountPath": ""}, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when mountPath is empty") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when mountPath is empty") + } +} + +// TestTool_ConfigVolumesRemove_MissingMountPath ensures the config_volumes_remove tool rejects calls without mountPath. +func TestTool_ConfigVolumesRemove_MissingMountPath(t *testing.T) { + executor := mock.NewExecutor() + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "config_volumes_remove", + Arguments: map[string]any{"path": "."}, + }) + // Schema validation may produce a protocol-level error or a tool-level error result. + if err == nil && !result.IsError { + t.Fatal("expected error when mountPath is missing") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when required fields are absent") + } +} + // TestTool_ConfigVolumesList_Error ensures the config_volumes_list tool propagates executor errors. func TestTool_ConfigVolumesList_Error(t *testing.T) { executor := mock.NewExecutor() @@ -237,7 +281,7 @@ func TestTool_ConfigVolumesRemove_Error(t *testing.T) { result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ Name: "config_volumes_remove", - Arguments: map[string]any{"path": "."}, + Arguments: map[string]any{"path": ".", "mountPath": "/workspace/secret"}, }) if err != nil { t.Fatal(err) @@ -245,4 +289,7 @@ func TestTool_ConfigVolumesRemove_Error(t *testing.T) { if !result.IsError { t.Fatal("expected error result, got success") } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } }