fix: prevent accidental removal of MCP external tools due to name collisions with disabled built-in tools

[whatevertogo]

Fixes #5821

Modifications / 改动点

问题根因：
当 MCP 外接工具（如 web_search）与内置工具重名时，如果内置工具被禁用（active=False），get_full_tool_set() 会将所有同名工具都加入 ToolSet，随后 remove_tool() 会把所有同名工具全部移除，导致 MCP 工具也被"连坐"清除。

修复内容：

1. func_tool_manager.py - get_func 方法

   • 优先返回已激活的工具
   • 退化时返回最后一个同名工具（通常是后加载的 MCP 工具）

2. func_tool_manager.py - get_full_tool_set 方法

   • 使用 add_tool() 进行填充，而非直接复制列表
   • 确保同名工具只保留一个

3. tool.py - add_tool 方法

   • 同名工具冲突时优先保留激活状态的工具
   • 避免禁用工具覆盖激活工具

改动文件：

• astrbot/core/agent/tool.py - 修改 add_tool 逻辑

• astrbot/core/provider/func_tool_manager.py - 修改 get_func 和 get_full_tool_set 逻辑

• This is NOT a breaking change. / 这不是一个破坏性变更。

Screenshots or Test Results / 运行截图或测试结果

$ uv run pytest tests/ -v --tb=short -k "tool"
========== 58 passed, 587 deselected, 6 warnings in 62.80s ==========

场景验证：

场景	内置工具	MCP 工具	结果
内置禁用，MCP启用	web_search(active=False)	web_search(active=True)	✅ 保留 MCP
内置启用，MCP启用	web_search(active=True)	web_search(active=True)	✅ 保留 MCP
内置启用，MCP禁用	web_search(active=True)	web_search(active=False)	✅ 保留内置

---

Checklist / 检查清单

• 😊 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。/ If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
• 👀 我的更改经过了良好的测试，并已在上方提供了"验证步骤"和"运行截图"。/ My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
• 🤓 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到了 requirements.txt 和 pyproject.toml 文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in requirements.txt and pyproject.toml.
• 😮 我的更改没有引入恶意代码。/ My changes do not introduce malicious code.

由 Sourcery 提供的摘要

通过调整工具选择和聚合逻辑，防止当 MCP 外部工具与被禁用的内置工具同名时被意外移除。

错误修复：

• 确保在清理或更新工具集时，与被禁用的内置工具同名的 MCP 工具不会被移除。

增强内容：

• 在工具查找和工具集处理中出现同名冲突时，优先考虑活跃工具，并始终让后加载的工具覆盖先加载的工具。

Original summary in English

Summary by Sourcery

解决 MCP 外部工具与同名内置工具之间的冲突，确保活跃工具能够被正确保留。

Bug 修复：

• 防止在工具集清理和重建过程中，当 MCP 工具与已禁用的内置工具同名时，MCP 工具被错误移除的问题。

增强：

• 统一工具冲突解决策略：优先保留处于激活状态的工具；当工具的激活状态相同时，后加载的工具覆盖先加载的工具。
• 通过 ToolSet.add_tool 构建完整的函数工具集，以便在所有工具上统一应用去重和冲突处理规则。

测试：

• 添加全面的单元测试，覆盖 ToolSet.add_tool 和 FunctionToolManager 在各种名称与激活状态冲突场景下的行为，包括 MCP 工具与内置工具之间的交互。

Original summary in English

Summary by Sourcery

Resolve conflicts between MCP external tools and built-in tools with the same name to ensure active tools are retained correctly.

Bug Fixes:

• Prevent MCP tools from being removed when they share a name with disabled built-in tools during tool set cleanup and reconstruction.

Enhancements:

• Unify tool conflict resolution by preferring active tools and letting later-loaded tools overwrite earlier ones when activation state matches.
• Build the full function tool set via ToolSet.add_tool so that deduplication and conflict rules are applied consistently across all tools.

Tests:

• Add comprehensive unit tests covering ToolSet.add_tool and FunctionToolManager behavior for various name and active-state conflict scenarios, including MCP and built-in tool interactions.

Bug 修复：

• 防止在清理和重建工具集时，当 MCP 工具与被禁用的内置工具同名时，MCP 工具被移除。

增强：

• 统一工具冲突解决策略：优先选择处于激活状态的工具；当工具的激活状态相同时，后加载的工具会覆盖先加载的工具。
• 确保 FunctionToolManager 通过 ToolSet.add_tool 构建完整工具集，从而在所有工具上统一应用去重规则。

测试：

• 增加全面的单元测试，覆盖 ToolSet.add_tool 和 FunctionToolManager 在各种名称和激活状态冲突场景下的行为，包括 MCP 工具与内置工具的交互。

Original summary in English

Summary by Sourcery

解决 MCP 外部工具与同名内置工具之间的冲突，确保活跃工具能够被正确保留。

Bug 修复：

• 防止在工具集清理和重建过程中，当 MCP 工具与已禁用的内置工具同名时，MCP 工具被错误移除的问题。

增强：

• 统一工具冲突解决策略：优先保留处于激活状态的工具；当工具的激活状态相同时，后加载的工具覆盖先加载的工具。
• 通过 ToolSet.add_tool 构建完整的函数工具集，以便在所有工具上统一应用去重和冲突处理规则。

测试：

• 添加全面的单元测试，覆盖 ToolSet.add_tool 和 FunctionToolManager 在各种名称与激活状态冲突场景下的行为，包括 MCP 工具与内置工具之间的交互。

Original summary in English

Summary by Sourcery

Resolve conflicts between MCP external tools and built-in tools with the same name to ensure active tools are retained correctly.

Bug Fixes:

• Prevent MCP tools from being removed when they share a name with disabled built-in tools during tool set cleanup and reconstruction.

Enhancements:

• Unify tool conflict resolution by preferring active tools and letting later-loaded tools overwrite earlier ones when activation state matches.
• Build the full function tool set via ToolSet.add_tool so that deduplication and conflict rules are applied consistently across all tools.

Tests:

• Add comprehensive unit tests covering ToolSet.add_tool and FunctionToolManager behavior for various name and active-state conflict scenarios, including MCP and built-in tool interactions.

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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!

此拉取请求旨在解决当 MCP 外部工具与内置工具重名时，内置工具被禁用会导致 MCP 工具也一并被错误移除的问题。通过优化工具管理逻辑，特别是 add_tool、get_func 和 get_full_tool_set 方法，确保系统能够正确识别并优先保留激活状态的工具，从而允许 MCP 工具在与内置工具重名时能够正常工作，避免了不必要的“连坐”效应。

Highlights

• 工具冲突解决: 修复了 MCP 外部工具与内置工具重名时，因清洗规则导致 MCP 工具失效的问题。
• get_func 方法优化: func_tool_manager.py 中的 get_func 方法现在优先返回已激活的工具，并在没有激活工具时退化返回最后一个同名工具。
• get_full_tool_set 方法改进: func_tool_manager.py 中的 get_full_tool_set 方法现在使用 add_tool() 进行工具集填充，确保同名工具只保留一个。
• add_tool 逻辑更新: tool.py 中的 add_tool 方法已更新，在处理同名工具冲突时，优先保留激活状态的工具，避免禁用工具覆盖激活工具。

Changelog

• astrbot/core/agent/tool.py
  • 调整了 add_tool 方法的逻辑，使其在遇到同名工具时，优先保留已激活的工具，并防止已禁用的工具覆盖已激活的工具。
• astrbot/core/provider/func_tool_manager.py
  • 修改了 get_func 方法，使其首先查找并返回已激活的同名工具，如果不存在则返回列表中最后一个同名工具。
  • 更新了 get_full_tool_set 方法，使其通过调用 add_tool 来构建工具集，以确保同名工具的正确处理和优先级。

Activity

• 目前没有记录到任何活动。

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[sourcery-ai]

Hey - 我给出了一些整体性的反馈：

• get_full_tool_set 的文档字符串目前仍然描述的是简单的「最后一个获胜」行为，但在新的 add_tool 逻辑下，实际规则变成了「优先使用激活的工具，否则采用最后一个」；建议更新该文档字符串，使其准确反映现在的行为，避免让后续维护者产生困惑。
• FuncToolManager.func_list 的顺序、get_func 和 ToolSet.add_tool 之间的交互，现在隐含了比较微妙的优先级规则（激活 vs 未激活、内置 vs MCP、较早 vs 较晚）；可以考虑把这一决策逻辑集中到一个小的辅助函数中，或者在注释中更明确地说明优先级规则，以降低将来修改 add_tool 时出现回归问题的风险。

面向 AI 代理的提示

Please address the comments from this code review:

## Overall Comments
- The docstring for `get_full_tool_set` still describes a simple "last one wins" behavior, but with the new `add_tool` logic the actual rule is "prefer active tools, otherwise last one wins"; consider updating the docstring to accurately reflect this to avoid confusion for future maintainers.
- The interaction between `FuncToolManager.func_list` ordering, `get_func`, and `ToolSet.add_tool` now encodes fairly subtle precedence rules (active vs inactive, built-in vs MCP, earlier vs later); it may help to centralize this resolution logic in a small helper or make the precedence rules more explicit in comments to reduce the risk of regressions if `add_tool` is changed later.

---

Sourcery 对开源项目是免费的——如果你觉得我们的评审有帮助，欢迎分享给更多人 ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的评审。}

Original comment in English

Hey - I've left some high level feedback:

• The docstring for get_full_tool_set still describes a simple "last one wins" behavior, but with the new add_tool logic the actual rule is "prefer active tools, otherwise last one wins"; consider updating the docstring to accurately reflect this to avoid confusion for future maintainers.
• The interaction between FuncToolManager.func_list ordering, get_func, and ToolSet.add_tool now encodes fairly subtle precedence rules (active vs inactive, built-in vs MCP, earlier vs later); it may help to centralize this resolution logic in a small helper or make the precedence rules more explicit in comments to reduce the risk of regressions if add_tool is changed later.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The docstring for `get_full_tool_set` still describes a simple "last one wins" behavior, but with the new `add_tool` logic the actual rule is "prefer active tools, otherwise last one wins"; consider updating the docstring to accurately reflect this to avoid confusion for future maintainers.
- The interaction between `FuncToolManager.func_list` ordering, `get_func`, and `ToolSet.add_tool` now encodes fairly subtle precedence rules (active vs inactive, built-in vs MCP, earlier vs later); it may help to centralize this resolution logic in a small helper or make the precedence rules more explicit in comments to reduce the risk of regressions if `add_tool` is changed later.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[gemini-code-assist]

Code Review

此 PR 旨在解决当 MCP 工具与禁用的内置工具重名时，MCP 工具被意外移除的问题。通过修改 add_tool 的逻辑以优先保留激活的工具，以及调整 get_full_tool_set 使用 add_tool 来处理同名工具，从根源上解决了该问题。同时，get_func 也被优化，以优先返回激活的工具。

然而，本次修改引入了一个重要的安全隐患：工具选择逻辑存在不一致性。具体来说，用于执行工具的 get_func 方法和用于 LLM 展示工具的 get_full_tool_set 方法在处理同名且激活的工具时，采用了不同的冲突解决策略。这种不一致可能导致 LLM 看到的工具定义与实际执行的工具不符，从而引发意外行为或潜在的“工具劫持”问题。建议将 get_func 的逻辑与 ToolSet.add_tool 中使用的“后激活者胜出”策略对齐，以确保一致性。此外，我还提出了一些关于代码可读性和效率的改进建议。

[Copilot]

Pull request overview

该 PR 旨在修复 MCP 外接工具与内置工具重名时，由于工具集合构建与清洗（remove）逻辑导致 同名工具被“连坐”全部移除、进而使 MCP 工具彻底失效的问题，确保工具集合在输出给 LLM 前能正确保留可用工具。

Changes:

• 调整 FunctionToolManager.get_func()：优先返回已激活工具，并在退化场景下选择后加载的同名工具。
• 调整 FunctionToolManager.get_full_tool_set()：通过 ToolSet.add_tool() 构建工具集以去重同名工具。
• 调整 ToolSet.add_tool()：同名冲突时优先保留 active=True 的工具，避免禁用工具覆盖启用工具。

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
astrbot/core/provider/func_tool_manager.py	调整工具检索与完整工具集构建方式，尝试避免同名工具在清洗时被整体移除
astrbot/core/agent/tool.py	更新 ToolSet.add_tool() 的同名覆盖策略，使其优先保留激活工具

[whatevertogo]

已根据评审意见修复

感谢 @sourcery-ai、@gemini-code-assist 和 @copilot 的评审意见。已修复以下问题：

安全问题

• get_func 与 get_full_tool_set 逻辑不一致：get_func 现在使用 reversed() 遍历，与 ToolSet.add_tool 的"后激活者胜出"策略保持一致，避免潜在的"工具劫持"问题

性能问题

• 移除不必要的 deepcopy：MCPTool 上的运行时对象不应被深拷贝，直接使用 add_tool(tool) 即可

代码质量

• 可读性改进：将双重否定条件 if not (existing_tool.active and not tool.active) 简化为 if new_active or not existing_active
• 兼容性改进：使用 getattr(tool, "active", True) 兼容可能缺少 active 属性的工具对象
• 文档更新：get_full_tool_set() 的 docstring 现在准确描述了实际的工具解析行为

所有测试通过 ✅

[gemini-code-assist]

感谢您的更新，whatevertogo！很高兴看到您根据评审意见进行了修复。我已仔细审阅了您的更改，并确认以下问题已得到妥善解决：

• 安全问题：get_func 和 get_full_tool_set 的逻辑现在与 ToolSet.add_tool 的策略保持一致，通过优先处理激活状态的工具并使用 reversed() 遍历，有效避免了潜在的“工具劫持”问题。
• 性能问题：通过优化 get_full_tool_set 的实现，避免了不必要的 deepcopy 操作，提升了效率。
• 代码质量：
  • 双重否定条件 if not (existing_tool.active and not tool.active) 已成功简化为更清晰的 if new_active or not existing_active。
  • getattr(tool, "active", True) 的使用增强了代码的兼容性，能够更好地处理缺少 active 属性的工具对象。
  • get_full_tool_set() 的文档字符串也已更新，准确反映了其新的工具解析行为。

这些改进使得工具管理逻辑更加健壮、高效且易于理解。做得非常棒！

[whatevertogo]

如果要更好的修复方式的话要引入命名空间，所以我未采用

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - 我发现了 2 个问题

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="astrbot/core/provider/func_tool_manager.py" line_range="314-315" />
<code_context>

     def get_func(self, name) -> FuncTool | None:
-        for f in self.func_list:
+        # 优先返回已激活的工具（后加载的覆盖前面的，与 ToolSet.add_tool 保持一致）
+        for f in reversed(self.func_list):
+            if f.name == name and getattr(f, "active", False):
+                return f
</code_context>
<issue_to_address>
**issue (bug_risk):** 将 `get_func` 与 `ToolSet.add_tool` 中对 `active` 的默认处理方式对齐，以避免产生不一致的行为。

这里 `getattr(f, "active", False)` 会将没有 `active` 属性的工具视为未激活，而 `ToolSet.add_tool` 使用的是 `getattr(..., True)`，在冲突处理时会将它们视为已激活。这种不一致意味着，当存在一个同名且 `active=False` 的工具时，一个工具在集合中可能被视为已激活，但在 `get_func` 中却会被跳过。请在这两个位置对齐默认值，并相应调整注释/行为，以保证冲突处理与查找逻辑保持一致。
</issue_to_address>

### Comment 2
<location path="tests/unit/test_tool_conflict_resolution.py" line_range="44-52" />
<code_context>
+        assert len(toolset.tools) == 1
+        assert toolset.tools[0].active is True
+
+    def test_both_active_last_one_wins(self):
+        """When both tools are active, the new one should overwrite."""
+        toolset = ToolSet()
+        toolset.add_tool(make_tool("web_search", active=True))
+        toolset.add_tool(make_tool("web_search", active=True))
+
+        assert len(toolset.tools) == 1
+        # The second tool should be the one kept
+        assert toolset.tools[0].description == "Test tool web_search"
+
+    def test_both_inactive_last_one_wins(self):
</code_context>
<issue_to_address>
**issue (testing):** 该测试实际上并不能证明被保留的是第二个处于激活状态的工具。

由于这两个工具的创建方式完全相同，即使保留的是第一个工具，这里的断言依然会通过。为了真正验证覆盖行为，需要将两个实例区分开，并断言第二个实例被保留下来，例如：

```python
first = make_tool("web_search", active=True)
second = make_tool("web_search", active=True)
second.description = "Second web search"

toolset = ToolSet()
toolset.add_tool(first)
toolset.add_tool(second)

assert len(toolset.tools) == 1
assert toolset.tools[0] is second
assert toolset.tools[0].description == "Second web search"
```

如果实现实际上保留的是第一个工具而不是最后一个，这个测试就会失败。
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{请帮我变得更有用！请对每条评论点击 👍 或 👎，我会根据这些反馈改进后续的代码审查。}

Original comment in English

Hey - I've found 2 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="astrbot/core/provider/func_tool_manager.py" line_range="314-315" />
<code_context>

     def get_func(self, name) -> FuncTool | None:
-        for f in self.func_list:
+        # 优先返回已激活的工具（后加载的覆盖前面的，与 ToolSet.add_tool 保持一致）
+        for f in reversed(self.func_list):
+            if f.name == name and getattr(f, "active", False):
+                return f
</code_context>
<issue_to_address>
**issue (bug_risk):** Align default `active` handling between `get_func` and `ToolSet.add_tool` to avoid inconsistent behavior.

Here `getattr(f, "active", False)` treats tools without an `active` attribute as inactive, while `ToolSet.add_tool` uses `getattr(..., True)`, treating them as active in conflict resolution. This mismatch means a tool can be active in the set but skipped by `get_func` when a same-name tool has `active=False`. Please align the default in both places and adjust comments/behavior so conflict resolution and lookup remain consistent.
</issue_to_address>

### Comment 2
<location path="tests/unit/test_tool_conflict_resolution.py" line_range="44-52" />
<code_context>
+        assert len(toolset.tools) == 1
+        assert toolset.tools[0].active is True
+
+    def test_both_active_last_one_wins(self):
+        """When both tools are active, the new one should overwrite."""
+        toolset = ToolSet()
+        toolset.add_tool(make_tool("web_search", active=True))
+        toolset.add_tool(make_tool("web_search", active=True))
+
+        assert len(toolset.tools) == 1
+        # The second tool should be the one kept
+        assert toolset.tools[0].description == "Test tool web_search"
+
+    def test_both_inactive_last_one_wins(self):
</code_context>
<issue_to_address>
**issue (testing):** This test does not actually prove that the second active tool is the one kept.

Because both tools are created identically, this assertion would still pass if the first tool were kept. To really validate the overwrite behavior, differentiate the two instances and assert that the second is the one retained, for example:

```python
first = make_tool("web_search", active=True)
second = make_tool("web_search", active=True)
second.description = "Second web search"

toolset = ToolSet()
toolset.add_tool(first)
toolset.add_tool(second)

assert len(toolset.tools) == 1
assert toolset.tools[0] is second
assert toolset.tools[0].description == "Second web search"
```

This will fail if the implementation keeps the first tool instead of the last.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[zouyonghe]

@sourcery-ai review

[zouyonghe]

你可以自己跑一下，如果只有重构建议就没什么问题了

[sourcery-ai]

Hey - 我在这里给出一些整体性的反馈：

• 在 FunctionToolManager.get_func 中，你可以通过一次自后向前遍历来避免对 func_list 进行两次迭代：在反向遍历时记录最后一次遇到的工具，并在找到第一个处于激活状态的匹配项时立即返回；如果没有处于激活状态的匹配项，则回退到最后一次遇到的匹配项。这样可以在保持相同语义的同时，让逻辑更简单，也稍微更高效一些。

给 AI Agent 的提示词

Please address the comments from this code review:

## Overall Comments
- In `FunctionToolManager.get_func`, you can avoid iterating `func_list` twice by doing a single reverse pass that tracks the last-seen tool and immediately returns on the first active match, falling back to the last-seen match if none are active, which keeps the same semantics with simpler and slightly more efficient logic.

---

Sourcery 对开源项目是免费的——如果你觉得这次代码审查有帮助，欢迎分享 ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续的代码审查。}

Original comment in English

Hey - I've left some high level feedback:

• In FunctionToolManager.get_func, you can avoid iterating func_list twice by doing a single reverse pass that tracks the last-seen tool and immediately returns on the first active match, falling back to the last-seen match if none are active, which keeps the same semantics with simpler and slightly more efficient logic.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `FunctionToolManager.get_func`, you can avoid iterating `func_list` twice by doing a single reverse pass that tracks the last-seen tool and immediately returns on the first active match, falling back to the last-seen match if none are active, which keeps the same semantics with simpler and slightly more efficient logic.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}