fix(mcp): prevent hang in config add/remove handlers when required fields are omitted#3827
Conversation
- Implemented tests to ensure `config_envs_add` and `config_envs_remove` tools reject calls with missing required fields (name and value). - Added similar validation tests for `config_labels_add` and `config_labels_remove` tools to check for missing name and value. - Enhanced `config_volumes_remove` tool to validate the presence of mountPath. - Updated input structures for `config_envs` and `config_labels` to enforce required fields. These changes improve the robustness of the tools by ensuring proper error handling for invalid inputs.
- Implemented tests for `config_envs_remove`, `config_labels_add`, and `config_labels_remove` tools to ensure they reject empty names and values. - Added validation in the handlers for `config_labels_add`, `config_labels_remove`, and `config_volumes_remove` to return errors when required fields are empty. - These changes enhance error handling and improve the robustness of the configuration tools.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankitsinghsisodya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
knative-prow
Bot
added
size/L
|
Hi @Ankitsinghsisodya. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR tightens input validation for several MCP “config_*” tools by making key parameters required, adding explicit empty-string guards, and expanding tests to cover missing/empty argument cases.
Changes:
- Make previously optional inputs (e.g.,
mountPath,name,value) required in tool input schemas and CLI arg construction. - Add handler-level validation to reject empty strings for required fields.
- Add/adjust tests to cover empty/missing arguments and to assert executor invocation behavior on propagation errors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/mcp/tools_config_volumes.go | Makes mountPath required and rejects empty mountPath before invoking the executor. |
| pkg/mcp/tools_config_volumes_test.go | Adds tests for empty/missing mountPath and adjusts error-path test args/assertions. |
| pkg/mcp/tools_config_labels.go | Makes name/value required and rejects empty strings in add/remove handlers. |
| pkg/mcp/tools_config_labels_test.go | Adds tests for missing/empty label fields and strengthens executor-invocation assertions. |
| pkg/mcp/tools_config_envs.go | Adds validation requiring at least one source for env values; makes config_envs_remove name required + non-empty. |
| pkg/mcp/tools_config_envs_test.go | Adds tests for bulk import, missing value, and missing/empty remove name; updates error-path tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err == nil && !result.IsError { | ||
| t.Fatal("expected error when mountPath is missing") | ||
| } |
| 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") |
| 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"` |
| 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") | ||
| } |
lkingland
added
ok-to-test
Problem
The
config_envs_add,config_envs_remove,config_labels_add,config_labels_remove, andconfig_volumes_removeMCP tool handlers could hang indefinitely when required fields were omitted by the caller.The CLI commands these handlers invoke fall through to interactive survey prompts when flags are absent. These prompts block on stdin. The MCP subprocess has no TTY, so the call deadlocks and never returns.
Root cause
The affected fields were declared as
*stringwithomitemptyin the JSON schema, making them appear optional to MCP clients. The CLI, however, requires them to operate non-interactively.Fix
Correct the schema to reflect the actual requirements of each command:
config_envs_add:value→ requiredstring.namestays*string(optional) for bulk-import syntax.config_envs_remove:name→ requiredstring.config_labels_add: bothnameandvalue→ requiredstring.config_labels_remove:name→ requiredstring.config_volumes_remove:mountPath→ requiredstring.Tests
_Errortests to supply all required fieldsTestTool_ConfigEnvsAdd_MissingValue,TestTool_ConfigEnvsRemove_MissingName,TestTool_ConfigLabelsAdd_MissingName,TestTool_ConfigLabelsAdd_MissingValue,TestTool_ConfigLabelsRemove_MissingName,TestTool_ConfigVolumesRemove_MissingMountPathTestTool_ConfigEnvsAdd_BulkImportconfirming the optional-name bulk-import path still works