feat: deactivate built-in web_search tools when disabled to allow MCP overrides#4904
feat: deactivate built-in web_search tools when disabled to allow MCP overrides#4904
Conversation
… tools Co-authored-by: Soulter <37870767+Soulter@users.noreply.github.com>
Co-authored-by: Soulter <37870767+Soulter@users.noreply.github.com>
|
@MonkeyCode-AI review 一下 |
|
MonkeyCode-AI 正在分析任务... |
MonkeyCode-AI
left a comment
There was a problem hiding this comment.
我是 MonkeyCode AI 编程助手,你可以在 GitHub 仓库的 PR 中 at @MonkeyCode-AI 来呼唤我。
任务执行细节请参考: https://monkeycode-ai.com/tasks/public?id=fd1aacd2-6300-424d-86c1-4114a7609b4b
代码审查结果
变更总体达成“web_search 关闭时不占用工具名空间”的目标,但仍存在可能误禁用 MCP 覆盖工具与配置读取不一致等问题,需要修改后再合并。
✨ 代码亮点
- 通过集中封装 _set_tools_active 简化了对多个内置工具的统一启用/禁用操作
- edit_web_search_tools 中对 tool_set 的 remove/add 逻辑清晰,保持了 provider 选择行为不变
| 🚨 Critical | 💡 Suggestion | |
|---|---|---|
| 1 | 0 | 0 |
| func_tool_mgr = self.context.get_llm_tool_manager() | ||
| for tool_name in self.TOOLS: | ||
| tool = func_tool_mgr.get_func(tool_name) | ||
| if tool: | ||
| tool.active = active | ||
|
|
There was a problem hiding this comment.
Caution
🚨 _set_tools_active 可能误伤 MCP 覆盖的同名工具(以及其他来源的同名工具)
当前实现通过 func_tool_mgr.get_func(tool_name) 获取“当前注册表里该名字对应的工具”,然后直接写 tool.active。若 MCP 在 built-in 被禁用后注册了同名工具(PR 目标场景),get_func("web_search") 可能返回 MCP 的自定义工具;随后 edit_web_search_tools 每次请求都会调用 _set_tools_active(websearch_enable),当 web_search=false 时会把 MCP 工具也设置为 inactive,导致 MCP 工具不可用,反而违背“允许 MCP overrides”的目标。同样地,在 init 里禁用也可能提前把后续注册/已注册的同名非内置工具置为 inactive。要真正做到“内置工具不占名”,应只切换“本插件定义的工具实例”,而不是按名字从全局注册表取当前实例。
建议: 将本插件定义的工具对象在初始化阶段缓存为实例引用(例如 self._builtin_tool_instances = {name: <tool_obj>}),后续只对这些实例改 active;或者在工具对象上有 owner/module 标识时,先校验 tool 是否属于本插件再修改。至少应避免通过 get_func(name) 修改到已被覆盖的工具。
| func_tool_mgr = self.context.get_llm_tool_manager() | |
| for tool_name in self.TOOLS: | |
| tool = func_tool_mgr.get_func(tool_name) | |
| if tool: | |
| tool.active = active | |
| def __init__(self, context: star.Context) -> None: | |
| self.context = context | |
| self.tavily_key_index = 0 | |
| self.tavily_key_lock = asyncio.Lock() | |
| # 将 str 类型的 key 迁移至 list[str],并保存 | |
| cfg = self.context.get_config() | |
| provider_settings = cfg.get("provider_settings") | |
| if provider_settings: | |
| tavily_key = provider_settings.get("websearch_tavily_key") | |
| if isinstance(tavily_key, str): | |
| logger.info( | |
| "检测到旧版 websearch_tavily_key (字符串格式),自动迁移为列表格式并保存。", | |
| ) | |
| if tavily_key: | |
| provider_settings["websearch_tavily_key"] = [tavily_key] | |
| else: | |
| provider_settings["websearch_tavily_key"] = [] | |
| cfg.save_config() | |
| self.bing_search = Bing() | |
| self.sogo_search = Sogo() | |
| self.baidu_initialized = False | |
| # Cache built-in tool instances so we don't accidentally toggle MCP overrides | |
| func_tool_mgr = self.context.get_llm_tool_manager() | |
| self._builtin_tool_instances = { | |
| tool_name: func_tool_mgr.get_func(tool_name) for tool_name in self.TOOLS | |
| } | |
| # Deactivate built-in web search tools if web_search is disabled | |
| # This allows MCP to provide custom web_search tools | |
| cfg = self.context.get_config() | |
| websearch_enable = cfg.get("provider_settings", {}).get("web_search", False) | |
| if not websearch_enable: | |
| self._set_tools_active(False) | |
| def _set_tools_active(self, active: bool) -> None: | |
| """Set the active status of all built-in web search tools. | |
| Args: | |
| active: True to activate tools, False to deactivate them | |
| """ | |
| for tool_name, tool in getattr(self, "_builtin_tool_instances", {}).items(): | |
| if tool: | |
| tool.active = active |
Built-in web_search tools occupied the tool namespace even when
web_search: false, preventing MCP servers from registering custom tools with the same names.Changes
Added
_set_tools_active(bool)helper method - Togglesactiveflag on all web search tools (web_search,fetch_url,web_search_tavily,tavily_extract_web_page)Modified
__init__- Deactivates tools at plugin load whenweb_search: falsein configModified
edit_web_search_toolshook - Dynamically activates/deactivates tools on each LLM request based on current configBehavior
When
web_search: false:inactivefunc_tool_manager.pyL406, L414, L420)When
web_search: true:activeas beforeExample
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
api.soulter.top/home/REDACTED/work/AstrBot/AstrBot/.venv/bin/python3 /home/REDACTED/work/AstrBot/AstrBot/.venv/bin/python3 main.py(dns block)astrbot-registry.soulter.top/home/REDACTED/work/AstrBot/AstrBot/.venv/bin/python3 /home/REDACTED/work/AstrBot/AstrBot/.venv/bin/python3 main.py(dns block)models.dev/home/REDACTED/work/AstrBot/AstrBot/.venv/bin/python3 /home/REDACTED/work/AstrBot/AstrBot/.venv/bin/python3 main.py(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.