-
Notifications
You must be signed in to change notification settings - Fork 259
Feat mcp tool pre parameter exclusion #667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feat mcp tool pre parameter exclusion #667
Conversation
- Modify McpTool.convertMcpSchemaToParameters to accept excludeParams parameter - Update McpClientManager to pass preset parameter exclusions to McpTool - Add defensive copying of required list to prevent UnsupportedOperationException - Include comprehensive tests for parameter exclusion functionality - Update existing tests to adapt new interface - Fix code formatting issues
- Create defensive copy of properties map to prevent mutation of original schema - Simplify parameter exclusion logic using properties.keySet().removeAll() - Improve code clarity and maintainability - Maintain backward compatibility
Summary of ChangesHello @xingmengxian, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where preset, runtime-injected parameters were incorrectly exposed in the JSON schema for tools registered with Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly implements the exclusion of preset parameters from the tool schema sent to the LLM. The changes in McpClientManager and McpTool are logical and well-executed. The new method signature in McpTool and the associated filtering logic are sound. The accompanying tests in McpToolTest are comprehensive, covering various scenarios including null and empty exclusion sets. I've added one suggestion to improve the robustness of the new tests by also checking collection sizes. Overall, this is a solid contribution that effectively resolves the stated issue.
| assertTrue(resultProperties.containsKey("param1")); | ||
| assertFalse(resultProperties.containsKey("param2")); | ||
| assertFalse(resultProperties.containsKey("param3")); | ||
|
|
||
| // Check that excluded required fields are removed | ||
| List<String> resultRequired = (List<String>) result.get("required"); | ||
| assertTrue(resultRequired.contains("param1")); | ||
| assertFalse(resultRequired.contains("param2")); | ||
| assertFalse(resultRequired.contains("param3")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertions for checking excluded parameters can be made more robust and concise. By also checking the size of the collections, you can be sure that no extra parameters are leaking through and that the collections have the exact expected size. Then, you only need to assert the presence of the expected parameter(s).
A similar improvement can be applied to the other new tests (testConvertMcpSchemaToParameters_WithExcludeParams_EmptyExcludes, ..._NullExcludes, and ..._NonExistentParams) by asserting the expected size of the collections.
| assertTrue(resultProperties.containsKey("param1")); | |
| assertFalse(resultProperties.containsKey("param2")); | |
| assertFalse(resultProperties.containsKey("param3")); | |
| // Check that excluded required fields are removed | |
| List<String> resultRequired = (List<String>) result.get("required"); | |
| assertTrue(resultRequired.contains("param1")); | |
| assertFalse(resultRequired.contains("param2")); | |
| assertFalse(resultRequired.contains("param3")); | |
| assertEquals(1, resultProperties.size()); | |
| assertTrue(resultProperties.containsKey("param1")); | |
| // Check that excluded required fields are removed | |
| List<String> resultRequired = (List<String>) result.get("required"); | |
| assertEquals(1, resultRequired.size()); | |
| assertTrue(resultRequired.contains("param1")); |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
When registering tools via
Mcptool, preset parameters (e.g.,agent_id,session_id, or other context-bound values injected at runtime) are not excluded from the JSON schema generated for LLM consumption. As a result, these parameters appear in the tool’s description sent to the LLM — causing the LLM to attempt setting them explicitly in its tool call arguments.This leads to:
io.agentscope.core.tool.Toolkit#registerToolMethod, which correctly omits preset parameters from the exposed schema