mcp: split config tools by action for accurate tool annotations#3661
mcp: split config tools by action for accurate tool annotations#3661Ankitsinghsisodya wants to merge 3 commits intoknative:mainfrom
Conversation
- Introduced separate tools for managing environment variables, labels, and volumes: `config_envs_list`, `config_envs_add`, `config_envs_remove`, `config_labels_list`, `config_labels_add`, `config_labels_remove`, `config_volumes_list`, `config_volumes_add`, and `config_volumes_remove`. - Updated corresponding handler functions to support new tool functionalities. - Enhanced test cases to validate the new tools and their respective actions. - Improved argument handling and output structures for better clarity and usability.
|
[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/XL
|
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
This PR updates the MCP “config” tools to avoid incorrect static tool annotations by splitting the previously multi-action tools into separate single-action tools, so clients can correctly treat list as read-only while still flagging remove as destructive.
Changes:
- Split
config_{envs,labels,volumes}into*_list,*_add, and*_removetools with action-appropriate MCP annotations. - Remove the
actionfield from config tool inputs and hardcode the correct subcommand in each tool’sArgs(). - Replace the prior combined tests with dedicated list/add/remove tests per resource.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/mcp/tools_config_volumes.go | Splits volumes config into list/add/remove tools, each with distinct input/output structs and annotations. |
| pkg/mcp/tools_config_volumes_test.go | Updates tests to call config_volumes_{list,add,remove} and validates the resulting CLI args. |
| pkg/mcp/tools_config_labels.go | Splits labels config into list/add/remove tools with action-specific annotations and args. |
| pkg/mcp/tools_config_labels_test.go | Updates tests to target the new labels tools and validates expected args. |
| pkg/mcp/tools_config_envs.go | Splits env var config into list/add/remove tools with action-specific annotations and args. |
| pkg/mcp/tools_config_envs_test.go | Updates tests to call the new envs tools and validates expected args. |
| pkg/mcp/mcp.go | Registers the new tools with the MCP server instead of the old combined tools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var configEnvsListTool = &mcp.Tool{ | ||
| Name: "config_envs_list", | ||
| Title: "Config Environment Variables List", | ||
| Description: "Lists the environment variables configured for a function.", | ||
| Annotations: &mcp.ToolAnnotations{ | ||
| Title: "Config Environment Variables", | ||
| ReadOnlyHint: false, | ||
| DestructiveHint: ptr(true), | ||
| IdempotentHint: false, // Adding the same environment variable twice or removing a non-existent one will fail. | ||
| Title: "Config Environment Variables List", | ||
| ReadOnlyHint: true, | ||
| IdempotentHint: true, | ||
| }, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3661 +/- ##
==========================================
+ Coverage 53.49% 56.85% +3.35%
==========================================
Files 181 181
Lines 20732 20788 +56
==========================================
+ Hits 11091 11818 +727
+ Misses 8567 7761 -806
- Partials 1074 1209 +135
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:
|
- Updated instructions for `config_*` tools to specify new commands: `config_envs_list`, `config_envs_add`, `config_envs_remove`, `config_labels_list`, `config_labels_add`, `config_labels_remove`, `config_volumes_list`, `config_volumes_add`, and `config_volumes_remove`. - Clarified usage instructions, including the requirement for absolute paths and the effects of add/remove operations on local `func.yaml` files. - Added authoritative usage references for environment variables, labels, and volumes.
- Implemented tests for `config_envs_list`, `config_envs_add`, and `config_envs_remove` to ensure proper propagation of executor errors. - Added similar tests for `config_labels_list`, `config_labels_add`, and `config_labels_remove`. - Included tests for `config_volumes_list`, `config_volumes_add`, and `config_volumes_remove` to validate error handling. - Each test verifies that the expected error result is returned when the executor encounters an error during execution.
|
cc @gauron99 |
gauron99
requested review from
lkingland
and removed request for
dsimansk and
jrangelramos
Problem
config_envs,config_labels, andconfig_volumeseach accepted anactionparameter (
list,add,remove) but declaredDestructiveHint: trueat thetool level. MCP tool annotations are static metadata evaluated at discovery time,
before any invocation arguments exist. A client that respects hints treats the tool
as destructive on every call — including a read-only
list.Changes
Split each combined tool into three single-action tools, each carrying annotations
that accurately describe that action's semantics:
config_{envs,labels,volumes}_listconfig_{envs,labels,volumes}_addconfig_{envs,labels,volumes}_removeThe
actionfield is removed from all input structs. Each tool'sArgs()methodhardcodes the correct subcommand.
IdempotentHintis set totrueon list tools.Tests
Nine dedicated tests (list/add/remove per resource) replace the previous six combined
tests. All pass.