Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for handling binary data (images, audio, blobs) in MCP tool and resource responses. The implementation includes saving binary content to files and returning file paths for use in templates.
- Adds binary data handler module with file saving capabilities
- Updates MCP tool/resource functions to handle binary content types
- Includes comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mcp_client/binary_data_handler.py | New module providing binary data handling and file saving functionality |
| mcp_client/init.py | Exports new binary handling functions |
| engine/globals.py | Updates MCP tool/resource functions to handle binary content |
| engine/template_engine.py | Passes state to global functions for binary handling |
| prompt/prompt_helper.py | Adds binary content handling to variable collection |
| tests/test_binary_data_handler.py | Comprehensive test suite for binary data handling |
| tests/mcp_test_server.py | Adds test tool that returns binary data |
| tests/templates/spec9.md | Updates test template header |
| tests/templates/spec10.md | New test template for binary content |
| README.md | Adds documentation for MCP tool/resource selection |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # For blob resources, we don't have mime type info, so we'll save as generic file | ||
| return save_binary_data_to_file(config_dir, content.blob, "application/octet-stream", "blob") |
There was a problem hiding this comment.
The comment is incorrect. BlobResourceContents has a mimeType field that should be used instead of hardcoding 'application/octet-stream'.
| # For blob resources, we don't have mime type info, so we'll save as generic file | |
| return save_binary_data_to_file(config_dir, content.blob, "application/octet-stream", "blob") | |
| # For blob resources, use the provided mimeType field | |
| return save_binary_data_to_file(config_dir, content.blob, content.mimeType, "blob") |
| # For blob resources, we don't have mime type info, so we'll save as generic file | ||
| return save_binary_data_to_file(config_dir, content.blob, "application/octet-stream", "blob") |
There was a problem hiding this comment.
Should use content.mimeType instead of hardcoded 'application/octet-stream'. BlobResourceContents has a mimeType field available.
| # For blob resources, we don't have mime type info, so we'll save as generic file | |
| return save_binary_data_to_file(config_dir, content.blob, "application/octet-stream", "blob") | |
| # Use the mimeType field from BlobResourceContents if available | |
| return save_binary_data_to_file(config_dir, content.blob, content.mimeType, "blob") |
| content_parts.append(str(text_content)) | ||
| # For non-text content, convert to string | ||
| # Handle binary content types | ||
| if isinstance(item, types.ImageContent | types.AudioContent): |
There was a problem hiding this comment.
The isinstance check should include types.BlobResourceContents to handle all binary content types consistently with the binary_data_handler module.
| if isinstance(item, types.ImageContent | types.AudioContent): | |
| if isinstance(item, (types.ImageContent, types.AudioContent, types.BlobResourceContents)): |
eyalzh
changed the title
1 participant
No description provided.