fix: disabling text content on mcp with option to enable it#3
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR modifies MCP tool responses to return ToolResult objects with structured content instead of plain dictionaries, while adding an optional flag to include text content alongside structured content.
- Replaced dictionary returns with ToolResult objects containing structured content
- Added
--text-contentCLI flag to optionally enable text content in MCP responses - Updated test assertions to work with the new ToolResult format
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| aiofmp/mcp_tools.py | Modified create_tool_response to return ToolResult with configurable text content |
| aiofmp/cli.py | Added --text-content CLI option and environment variable support |
| aiofmp/mcp_server.py | Added imports for ToolResult and ToolTransformConfig |
| tests/test_mcp_tools.py | Updated test assertions to validate ToolResult objects and structured content |
| README.md | Updated documentation to include the new --text-content option |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if include_text_content: | ||
| # Return both text and structured content | ||
| import json | ||
| text_content = json.dumps(response, indent=2) |
There was a problem hiding this comment.
The import json statement should be moved to the top of the file to avoid repeated imports on each function call when text content is enabled.
| assert response["data"] == data | ||
| assert response["message"] == "Test message" | ||
| # Check that it's a ToolResult object | ||
| from fastmcp.tools.tool import ToolResult |
There was a problem hiding this comment.
The import statement is repeated in multiple test methods. Consider moving this import to the top of the file to avoid redundant imports and improve readability.
| os.environ["MCP_PORT"] = str(port) | ||
|
|
||
| # Set text content flag | ||
| os.environ["MCP_INCLUDE_TEXT_CONTENT"] = str(text_content) |
There was a problem hiding this comment.
Converting boolean text_content to string will result in 'True' or 'False', but the environment variable check in mcp_tools.py expects 'true' or 'false' (lowercase). This will cause the flag to not work as expected.
| os.environ["MCP_INCLUDE_TEXT_CONTENT"] = str(text_content) | |
| os.environ["MCP_INCLUDE_TEXT_CONTENT"] = "true" if text_content else "false" |
No description provided.