-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(mcp): Make tool schemas more Gemini-compatible. #8637
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8637 +/- ##
==========================================
+ Coverage 51.04% 51.07% +0.03%
==========================================
Files 431 431
Lines 30986 31001 +15
Branches 6367 6368 +1
==========================================
+ Hits 15818 15835 +17
+ Misses 13752 13751 -1
+ Partials 1416 1415 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Is this an temporary thing or a "semi-permanent"?
Do other LLM run into similar issues?
} else { | ||
const rtext = await response.text(); | ||
try { | ||
console.dir(JSON.parse(rtext), { depth: null }); |
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.
How do we know if the response conforms to what the tool expect?
Do we need to do some sort of zod validation?
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 point of the smoke test is to make sure the Gemini API doesn't throw errors based on any of the tool input schemas provided - it's not testing actual tool calling. This is more of a sanity check than a complete solution, but is better than nothing.
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.
Interesting, what kind of error it may throw?
Does the Gemini API do any validation to make sure the response match the spec?
We can probably add that validation here later.
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.
If the tool schemas don't conform to the limited schema supported by Gemini, the request will 400 even if it's only one tool out of 35 that are broken. That's bad, so this smoke test ensures that all of our tools are vaild as far as the API is concerned.
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.
Really cool, script! Thanks for codify it!
Feel like we should add some manual test instructions to CONTRIBUTIONS.md of mcp.
Thank you for this fix! Looking forward to using :) |
Had a report from a fellow Googler that the MCP server was throwing errors when used with ADK. While I had done testing with Genkit, we do some manual massaging of JSON schemas in Genkit to work around Gemini limitations.
This brings some of those workarounds directly to the MCP server. I also added a smoke test that can be run from
scripts/mcp-tests
to load all tool definitions into a simple Gemini model call so we can easily verify this going forward (automated would be great but I don't have time to figure that out today).