fix(mcp): prevent hang in config add/remove handlers when required fields are omitted#3761
Conversation
|
[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 MCP tool input validation by making previously optional arguments required for several config tools, and expands tests to verify missing required fields are rejected.
Changes:
- Make
mountPathrequired forconfig_volumes_remove. - Make
name/valuerequired forconfig_labels_addandnamerequired forconfig_labels_remove. - Make
valuerequired forconfig_envs_add(while keepingnameoptional for bulk-import) andnamerequired forconfig_envs_remove, adding/adjusting tests accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/mcp/tools_config_volumes.go | Make mountPath required and always include --mount-path in args |
| pkg/mcp/tools_config_volumes_test.go | Add missing-mountPath test; update remove error test inputs/assertions |
| pkg/mcp/tools_config_labels.go | Make label name/value required and include flags unconditionally |
| pkg/mcp/tools_config_labels_test.go | Add missing-field tests; update error test inputs/assertions |
| pkg/mcp/tools_config_envs.go | Require value, keep name optional for bulk-import; require name for remove |
| pkg/mcp/tools_config_envs_test.go | Add bulk-import + missing-field tests; update error test inputs/assertions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3761 +/- ##
==========================================
- Coverage 57.03% 56.98% -0.05%
==========================================
Files 182 181 -1
Lines 21376 21388 +12
==========================================
- Hits 12191 12189 -2
- Misses 7953 7962 +9
- Partials 1232 1237 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- 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.
Ankitsinghsisodya
force-pushed
the
fix/mcp-config-required-fields
branch
from
7634b93 to
c7c4474
Compare
Problem
The
config_envs_add,config_envs_remove,config_labels_add,config_labels_remove, andconfig_volumes_removeMCP tool handlerscould hang indefinitely when required fields were omitted by the caller.
The CLI commands these handlers invoke fall through to interactive survey
prompts (
runAddEnvsPrompt,runRemoveEnvsPrompt, etc.) when flags areabsent. 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 theJSON 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,
derived from the CLI flag-gating logic:
config_envs_add:value→ requiredstring.namestays*string(optional) because the bulk-import syntax intentionally omits it:
--value='{{ secret:mySecret }}'. Requiringvalueensuresvp != nil, which satisfies the CLI'snp != nil || vp != nilcondition and always skips the interactive path.
config_envs_remove:name→ requiredstring. No valid removecall exists without a name.
config_labels_add: bothnameandvalue→ requiredstring.The CLI condition is
np != nil && vp != nil(AND, not OR), soeither field missing triggers the interactive prompt. Labels have no
bulk-import use case.
config_labels_remove:name→ requiredstring. Same reasoningas envs remove.
config_volumes_remove:mountPath→ requiredstring. Volumes areidentified by mount path; omitting it always falls to interactive mode.
Args()methods are simplified accordingly — required string fields areappended directly instead of going through
appendStringFlag.Tests
_Errortests to supply all required fields so they correctlytest executor error propagation rather than schema rejection.
executor is never invoked when required fields are absent:
TestTool_ConfigEnvsAdd_MissingValue,TestTool_ConfigEnvsRemove_MissingName,TestTool_ConfigLabelsAdd_MissingName,TestTool_ConfigLabelsAdd_MissingValue,TestTool_ConfigLabelsRemove_MissingName,TestTool_ConfigVolumesRemove_MissingMountPath.TestTool_ConfigEnvsAdd_BulkImportconfirming theoptional-name bulk-import path still works.