Added the strictSchemeValidation setting for MCP server parameters. #6961
Added the strictSchemeValidation setting for MCP server parameters. #6961blouflab wants to merge 1 commit into
Conversation
…efaults to false; if true, it enforces strict validation rules for the tools.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Summary of Changes
Hello @blouflab, 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 introduces a new configuration option, strictSchemeValidation, for Model Context Protocol (MCP) server parameters. This setting provides granular control over the strictness of schema validation for tools discovered from an MCP server. By default, the system maintains its current validation behavior, but enabling this option will enforce stricter rules, specifically by skipping tools whose subschemas do not explicitly define types. This enhancement aims to improve the robustness and predictability of tool integration by allowing users to opt into more rigorous schema checks.
Highlights
- New Configuration Setting: A new optional boolean setting,
strictSchemeValidation, has been introduced in theMCPServerConfigto control the strictness of schema validation for MCP server parameters. - Conditional Strict Schema Validation: The core schema validation logic in
hasValidTypeshas been updated to conditionally enforce stricter rules. WhenstrictSchemeValidationistrue, tools will be skipped if their subschemas lack defined types; otherwise, the existing, more lenient validation applies. - Configuration Propagation: The
strictSchemeValidationsetting is now propagated through the tool discovery and instantiation process, ensuring that the chosen validation policy is consistently applied when interacting with MCP tools.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces a strictSchemeValidation setting for MCP servers to control schema validation strictness. The implementation correctly adds the configuration and modifies the validation logic in hasValidTypes. However, there are two main issues. First, the unit tests for hasValidTypes are not updated, causing them to fail with the new logic. This is a critical issue that needs to be addressed. Second, the strictSchemeValidation flag is passed down to DiscoveredMCPTool but is never used, which is misleading and adds dead code. I've provided comments with suggestions to fix these issues.
| * @visiblefortesting | ||
| */ | ||
| export function hasValidTypes(schema: unknown): boolean { | ||
| export function hasValidTypes(schema: unknown, mcpServerConfig?: MCPServerConfig): boolean { |
There was a problem hiding this comment.
The signature and logic of hasValidTypes have been changed, but the corresponding unit tests in packages/core/src/tools/mcp-client.test.ts have not been updated. Specifically, the test 'should return false for a schema with no type and no subschemas' will now fail because the default behavior (non-strict) has changed to return true.
Please update the unit tests for hasValidTypes to reflect the new behavior and add test cases for both strict and non-strict validation modes to ensure the feature is correctly tested.
| override readonly parameterSchema: unknown, | ||
| readonly timeout?: number, | ||
| readonly trust?: boolean, | ||
| readonly strictSchemeValidation?: boolean, |
There was a problem hiding this comment.
The strictSchemeValidation property is added to the DiscoveredMCPTool constructor but it is never used within the class or its invocations. This makes the code misleading as it suggests that the tool itself performs some form of strict validation, when this logic is actually handled upstream in mcp-client.ts before the tool is created.
This unused property adds dead code and can cause confusion for future maintenance. It should be removed from this class, the asFullyQualifiedTool method, and the constructor call in packages/core/src/tools/mcp-client.ts.
There was a problem hiding this comment.
please consider this concern from the bot and comment on whether it is valid and then I will review.
|
Also please sign the CLA before I can review your code |
| } | ||
|
|
||
| // | ||
| const useStrictValidation = mcpServerConfig?.strictSchemeValidation ?? false; |
There was a problem hiding this comment.
Came from the issue you created, #6632.
Just curious, from a behavior change perspective, would it be safer to default to strict checking and loosen up the check with a deliberate setting.
There was a problem hiding this comment.
That's a good question regarding the default behavior. The current default of false for strictSchemeValidation aligns with the default behavior specified in the MCP Protocol itself. Changing this to true by default might introduce breaking changes for existing MCP servers or tools that rely on the protocol's non-strict default. The intent here is to provide an explicit opt-in for stricter validation, rather than forcing it by default, to maintain compatibility with the protocol's established behavior.
There was a problem hiding this comment.
We don't have this flag before. We are changing from:
if (!hasSubSchema) return false;
to
if (!hasSubSchema && useStrictValidation) return false;
So before is equivalent to useStrictValidation always set to true, setting the default to true is more consistent with the original behavior.
There was a problem hiding this comment.
Default is false. In that case it uses the default of the Mcp protocol which is to not skip a tool because there is no type in a subschema.
True is the same of what you have actually, which is not a default of the protocol itself and the cause of issue related.
There was a problem hiding this comment.
if (!hasSubSchema && useStrictValidation) return false;So before is equivalent to
useStrictValidationalways set to true, setting the default totrueis more consistent with the original behavior.
That's where it hurts. It's not the original behavior, this check has been added to makes it compliant with 1 aws mcp. See the threads for reference.
This is a question of both common sense and ethics. Is it Google's place to redefine the standard for the MCP protocol?
| } | ||
|
|
||
| if (!hasValidTypes(funcDecl.parametersJsonSchema)) { | ||
| if (!hasValidTypes(funcDecl.parametersJsonSchema,mcpServerConfig )) { |
There was a problem hiding this comment.
We should add something to the warning here mentioning the configuration option to loosen the checks so people can be notified of the way to disable the check if they so choose.
Added the strictSchemeValidation setting for MCP server parameters.
Defaults to false; if true, it enforces strict validation rules for the tools (current rules)
strictSchemeValidation:
"my_mcp": { "url": "https://xxx", "strictSchemeValidation" : false/true }Linked issues / bugs
Resolves #6632