-
Notifications
You must be signed in to change notification settings - Fork 17
fix: mcp type errors #402
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
fix: mcp type errors #402
Conversation
🦋 Changeset detectedLatest commit: 45645aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @Adebesin-Cell, 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 primarily focuses on resolving type-related issues within the Model Context Protocol (MCP) integration by updating the core SDK and refining how MCP tools and content are handled. The changes aim to enhance the stability and robustness of the MCP client and tool adapters, ensuring better compatibility with the updated SDK and more reliable data conversion processes. 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 focuses on fixing type errors, primarily by updating the @modelcontextprotocol/sdk dependency and improving type safety across the codebase. The changes are solid, particularly the more robust handling of different data structures and property existence checks in create-tool.ts and sampling-handler.ts. The refactoring of convertMcpContentToADKParts to manage various content types is a significant improvement. I've included a few suggestions to further enhance correctness and type safety.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Adebesin Tolulope (Lope) <[email protected]>
|
@claude review |
|
Claude finished @MarvelNwachukwu's task —— View job Code Review of PR #402: fix: mcp type errorsI've completed a comprehensive review of the MCP type error fixes in this PR. Overall, the changes improve type safety and error handling, though there are some areas that could benefit from additional improvements. Todo List
Review SummaryThis PR successfully addresses several type safety issues in the MCP integration and upgrades the SDK to v1.23.0. The changes are well-structured and improve the robustness of the codebase. Positive Changes1. SDK Update (package.json:38)✅ Good: Updated 2. Transport Improvement (client.ts:3,126)✅ Good: Replaced 3. Enhanced Error Handling✅ Good: Comprehensive error handling in Issues and Improvements Needed1. Type Safety Issue in McpToolAdapter (create-tool.ts:73-77)Issue: The metadata extraction logic has potential type safety issues: if ("metadata" in mcpTool && typeof mcpTool.metadata === "object") {
metadata = mcpTool.metadata as McpToolMetadata;
} else if (mcpTool._meta && typeof mcpTool._meta === "object") {
metadata = mcpTool._meta as McpToolMetadata;
}Problems:
Recommended Fix: if (mcpTool.metadata && typeof mcpTool.metadata === "object") {
metadata = mcpTool.metadata as McpToolMetadata;
} else if (mcpTool._meta && typeof mcpTool._meta === "object") {
metadata = mcpTool._meta as McpToolMetadata;
}2. Incomplete Data Handling in sampling-handler.ts (lines 224-238)Issue: The code for handling case "image":
case "audio": {
// ... validation code shows missing data problem
if (!hasData) {
this.logger.warn(`Missing or invalid 'data' for ${mcpContent.type} content.`);
return [...parts, { text: `[${mcpContent.type.toUpperCase()} CONTENT MISSING DATA]` }];
}Assessment: ✅ Good - This was actually handled correctly! The code properly validates data exists and provides a meaningful fallback when it's missing. I see this was already addressed in response to the review comments. 3. Debugging Information (sampling-handler.ts:187)Minor Issue: The log message could be more informative: `Converted MCP message - role: ${mcpMessage.role} -> ${adkRole}, content type: ${Array.isArray(mcpMessage.content) ? "array" : (mcpMessage.content as any)?.type ?? "unknown"}`,This provides better debugging information than logging the object directly. Security Assessment✅ Security: No security concerns identified. The code follows defensive programming practices with proper input validation and error handling. Type Safety AssessmentOverall: Good improvement in type safety with a few minor issues to address.
Performance Considerations✅ Performance: The changes should have minimal performance impact. The StreamableHTTPClientTransport should actually improve performance for streaming scenarios. Final RecommendationAPPROVE with minor suggestions. This PR successfully addresses the stated type errors and improves the robustness of the MCP integration. The main issue to address is the metadata extraction logic in The code follows good TypeScript practices, includes comprehensive error handling, and the SDK update brings valuable improvements to the MCP integration. |
MarvelNwachukwu
left a comment
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.
MCP Example runs fine locally, might be worth checking Claude's comments for minor suggestions before merging
Most of the issues are resolved already |
Closes #406 when this PR merged
This PR focuses on resolving type-related issues within the Model Context Protocol (MCP) integration by updating the core SDK and refining the handling of MCP tools and content. The changes enhance the stability and robustness of the MCP client and tool adapters, ensuring better compatibility with the updated SDK and more reliable data conversion processes.
Key Changes:
@modelcontextprotocol/sdkto version^1.23.0to leverage the latest features and fixes.SSEClientTransportwithStreamableHTTPClientTransportto improve streaming capabilities and type handling in HTTP communication.McpToolAdapterto safely handle tool metadata (metadataand_meta) and ensure theexecutemethod exists and is a function.McpSamplingHandlerto better convert various MCP content types (arrays,tool_use,tool_result) to ADK parts, along with asafeTexthelper for improved type safety.clean-locksscript to the rootpackage.jsonfor easily removing lock files (pnpm-lock.yaml,package-lock.json,yarn.lock) across the repository.Related Issue
Type of Change
How Has This Been Tested?
clean-locksscript to ensure proper removal of lock files.Checklist
Additional Notes
These changes primarily target type safety, data conversion reliability, and SDK compatibility for the MCP integration.