Repository navigation
Conversation
Summary of ChangesHello, 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 an issue where MCP tool parameter schemas might not conform to the JSON Schema specification, specifically regarding the 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a normalizeToolSchema function to ensure tool parameter schemas are valid, which is a valuable improvement for robustness. The implementation is mostly correct, but I've identified a potential issue where an invalid schema could still be passed through under certain conditions. My review includes a suggestion to make the normalization more comprehensive.
| const fallback: Record<string, unknown> = { type: 'object', properties: {} }; | ||
| if (!schema || typeof schema !== 'object' || Array.isArray(schema)) { | ||
| return fallback; | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- narrowed by typeof+Array guards above | ||
| const obj = schema as Record<string, unknown>; | ||
| if (obj['type'] !== 'object') { | ||
| return { ...obj, type: 'object', properties: obj['properties'] ?? {} }; | ||
| } | ||
| return obj; |
There was a problem hiding this comment.
The current implementation of normalizeToolSchema can allow an invalid schema to pass through if type is already 'object' but the properties field is invalid (e.g., null or not an object). This could lead to downstream errors when the schema is consumed by stricter APIs.
The proposed change makes the normalization more robust by ensuring that type is always 'object' and properties is always a valid object. This better fulfills the function's goal of ensuring a valid tool parameter schema.
| const fallback: Record<string, unknown> = { type: 'object', properties: {} }; | |
| if (!schema || typeof schema !== 'object' || Array.isArray(schema)) { | |
| return fallback; | |
| } | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- narrowed by typeof+Array guards above | |
| const obj = schema as Record<string, unknown>; | |
| if (obj['type'] !== 'object') { | |
| return { ...obj, type: 'object', properties: obj['properties'] ?? {} }; | |
| } | |
| return obj; | |
| const fallback: Record<string, unknown> = { type: 'object', properties: {} }; | |
| if (!schema || typeof schema !== 'object' || Array.isArray(schema)) { | |
| return fallback; | |
| } | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- narrowed by typeof+Array guards above | |
| const obj = schema as Record<string, unknown>; | |
| const properties = obj['properties']; | |
| const hasValidProperties = | |
| properties && typeof properties === 'object' && !Array.isArray(properties); | |
| return { | |
| ...obj, | |
| type: 'object', | |
| properties: hasValidProperties ? properties : {}, | |
| }; |
References
- Ensuring a robust and valid JSON schema structure is a foundational step towards ensuring that JSON schemas for configuration can accurately match their corresponding TypeScript interfaces.
There was a problem hiding this comment.
Good catch — adopted the suggested approach. The function now always spreads to enforce both type: 'object' and valid properties, regardless of whether the original type was already correct. Added 3 test cases covering null, primitive, and array properties when type is already 'object'. Fixed in 3eae5ba.
gemini-cli
Bot
added
the
area/agent
MCP servers can advertise tool input schemas with missing `type`, non-object `type`, or completely malformed structures. This causes hard failures when the Gemini API validates strictly or when downstream providers (Anthropic, OpenAI-compatible) reject the request. Add `normalizeToolSchema()` that ensures every tool parameter schema has `type:'object'` at root level: - Wraps undefined/null/primitive/array schemas with a valid fallback - Injects `type:'object'` when missing from object schemas - Preserves valid schemas unchanged Applied at two layers for defense-in-depth: 1. DiscoveredMCPTool constructor (MCP tools) 2. DiscoveredTool registration in tool-registry (extension tools) Made-with: Cursor
Address review feedback: ensure properties is always a valid object even when type is already 'object'. Schemas with null, primitive, or array properties now get corrected to an empty object. Made-with: Cursor
3eae5ba to
34a39eb
Compare
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we have updated our contribution policy (see Discussion #17383). We only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. All other community pull requests are subject to closure after 14 days if they do not align with our current focus areas. For this reason, we strongly recommend that contributors only submit pull requests against issues explicitly labeled as 'help-wanted'. This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding and for being part of our community! |
Fixes #23382
What
Add
normalizeToolSchema()to ensure every MCP and extension-discovered tool parameter schema hastype: 'object'at root level before it reaches the API.Why
MCP servers from the ecosystem (Zapier, Linear, custom servers) can advertise schemas that are
undefined,null, missingtype, or have a non-object type at root. The Gemini API is lenient today, but:This is a zero-risk normalization — valid schemas pass through unchanged.
Changes
packages/core/src/tools/mcp-tool.tsnormalizeToolSchema(), apply inDiscoveredMCPToolconstructorpackages/core/src/tools/tool-registry.tsnormalizeToolSchema()atDiscoveredToolregistrationpackages/core/src/tools/mcp-tool.test.tsTest Plan
npx vitest run packages/core/src/tools/mcp-tool.test.ts— 66 tests passMade with Cursor