fix(budprompt): resolve RenamedToolset error in response formatters #1272
Conversation
Make get_tool_list() self-contained so it creates its own MCP connection and strips gateway slugs from tool names. This fixes the error where formatters called load_tools() (returns RenamedToolset) then tried to call list_tools() on it, which doesn't exist on RenamedToolset. Changes: - get_tool_list() now takes tool_config and creates own MCP connection - load_tools() calls mcp_server.list_tools() directly for original names - Formatters call get_tool_list(tool_config) directly - Fixed server_label validation error by defaulting to "unknown" Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @vsraccubits, 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 resolves a critical issue affecting tool listing for MCP tools, particularly when gateway slugs are in use. The core problem stemmed from an incompatibility with Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
![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 effectively resolves the RenamedToolset error by refactoring get_tool_list and correctly addresses a validation error by providing a default value for server_label. However, it introduces a potential Server-Side Request Forgery (SSRF) and credential leakage vulnerability in the MCPToolLoader.get_tool_list method due to unvalidated input in URL construction and the inclusion of a sensitive API key. Additionally, there are opportunities to improve maintainability by addressing code duplication in services/budprompt/budprompt/executors/v4/tool_loaders.py through helper methods.
| mcp_url = f"{self.base_url}servers/{tool_config.server_url}/mcp" | ||
| headers = {} | ||
| if self.api_key: | ||
| headers["Authorization"] = f"Bearer {self.api_key}" | ||
|
|
||
| mcp_server = MCPServerStreamableHTTP(url=mcp_url, headers=headers if headers else None) |
There was a problem hiding this comment.
This block constructs an MCP server URL using tool_config.server_url without validation, leading to a potential Server-Side Request Forgery (SSRF) and credential leakage vulnerability. An attacker could manipulate server_url for path traversal, redirecting requests with the sensitive Authorization header to arbitrary endpoints. Strict validation for tool_config.server_url is crucial to prevent this. Additionally, this code block is duplicated; consider extracting it into a private helper method to improve maintainability and adhere to the DRY principle.
| mcp_url = f"{self.base_url}servers/{tool_config.server_url}/mcp" | |
| headers = {} | |
| if self.api_key: | |
| headers["Authorization"] = f"Bearer {self.api_key}" | |
| mcp_server = MCPServerStreamableHTTP(url=mcp_url, headers=headers if headers else None) | |
| if not all(c.isalnum() or c in "-_" for c in tool_config.server_url): | |
| logger.error(f"Invalid server_url: {tool_config.server_url}") | |
| return {"server_label": server_label, "tools": [], "error": "invalid server_url"} | |
| mcp_url = f"{self.base_url}servers/{tool_config.server_url}/mcp" | |
| headers = {"Authorization": f"Bearer {self.api_key}"} if self.api_key else None | |
| mcp_server = MCPServerStreamableHTTP(url=mcp_url, headers=headers) |
| for tool in tools_list: | ||
| short_name = tool.name | ||
| for slug in gateway_slugs: | ||
| prefix = f"{slug}-" | ||
| if tool.name.startswith(prefix): | ||
| short_name = tool.name[len(prefix) :] | ||
| break |
There was a problem hiding this comment.
The logic inside this for loop to find a tool's short name by stripping a gateway slug prefix is very similar to the logic within the loop in load_tools (lines 101-107). To avoid this code duplication and make the logic reusable, you could extract it into a helper method.
For example:
def _get_short_name(self, original_name: str, gateway_slugs: list[str]) -> str:
"""Strips the first matching gateway slug prefix from a tool name."""
for slug in gateway_slugs:
prefix = f"{slug}-"
if original_name.startswith(prefix):
return original_name[len(prefix):]
return original_nameThis helper could then be used here as short_name = self._get_short_name(tool.name, gateway_slugs) and would also simplify the implementation in load_tools.
2 participants
Summary
Problem
After the MCP tool name shortening feature was added, both non-streaming and streaming formatters started failing with:
"error": "Failed to list tools from MCP server '...': 'RenamedToolset' object has no attribute 'list_tools'"
Root cause: When gateway_slugs are present, load_tools() returns a RenamedToolset (via mcp_server.renamed(name_map)). The formatters then called get_tool_list(RenamedToolset) which tried to invoke list_tools() - a method that doesn't
exist on RenamedToolset.
Solution
Made get_tool_list() self-contained:
This ensures formatters get the correct tool list without depending on load_tools() output.