diff --git a/astrbot/core/agent/mcp_client.py b/astrbot/core/agent/mcp_client.py index 1646a21a9..f12ee7deb 100644 --- a/astrbot/core/agent/mcp_client.py +++ b/astrbot/core/agent/mcp_client.py @@ -1,5 +1,7 @@ import asyncio import logging +import os +import sys from contextlib import AsyncExitStack from datetime import timedelta from typing import Generic @@ -45,6 +47,22 @@ def _prepare_config(config: dict) -> dict: return config +def _prepare_stdio_env(config: dict) -> dict: + """Preserve Windows executable resolution for stdio subprocesses.""" + if sys.platform != "win32": + return config + + pathext = os.environ.get("PATHEXT") + if not pathext: + return config + + prepared = config.copy() + env = dict(prepared.get("env") or {}) + env.setdefault("PATHEXT", pathext) + prepared["env"] = env + return prepared + + async def _quick_test_mcp_connection(config: dict) -> tuple[bool, str]: """Quick test MCP server connectivity""" import aiohttp @@ -214,6 +232,7 @@ class MCPClient: ) else: + cfg = _prepare_stdio_env(cfg) server_params = mcp.StdioServerParameters( **cfg, ) diff --git a/astrbot/core/agent/tool.py b/astrbot/core/agent/tool.py index 42b7e8eb4..094530d56 100644 --- a/astrbot/core/agent/tool.py +++ b/astrbot/core/agent/tool.py @@ -94,11 +94,21 @@ class ToolSet: return len(self.tools) == 0 def add_tool(self, tool: FunctionTool) -> None: - """Add a tool to the set.""" - # 检查是否已存在同名工具 + """Add a tool to the set. + + If a tool with the same name already exists: + - Prefer the one that is active (active=True) + - If both have the same active state, use the new one (overwrite) + """ for i, existing_tool in enumerate(self.tools): if existing_tool.name == tool.name: - self.tools[i] = tool + # Use getattr with a default of True for compatibility with tools + # that may not define an `active` attribute (e.g., mocks). + existing_active = bool(getattr(existing_tool, "active", True)) + new_active = bool(getattr(tool, "active", True)) + # Overwrite if new tool is active, or if existing tool is not active + if new_active or not existing_active: + self.tools[i] = tool return self.tools.append(tool) diff --git a/astrbot/core/config/default.py b/astrbot/core/config/default.py index 901c6e3f6..72ce8c6e4 100644 --- a/astrbot/core/config/default.py +++ b/astrbot/core/config/default.py @@ -1091,7 +1091,7 @@ CONFIG_METADATA_2 = { "type": "list", # provider sources templates "config_template": { - "OpenAI": { + "OpenAI Compatible": { "id": "openai", "provider": "openai", "type": "openai_chat_completion", @@ -1271,6 +1271,7 @@ CONFIG_METADATA_2 = { "api_base": "http://127.0.0.1:11434/v1", "proxy": "", "custom_headers": {}, + "ollama_disable_thinking": False, }, "LM Studio": { "id": "lm_studio", @@ -1831,6 +1832,11 @@ CONFIG_METADATA_2 = { "items": {}, "hint": "此处添加的键值对将被合并到 OpenAI SDK 的 default_headers 中,用于自定义 HTTP 请求头｡值必须为字符串｡", }, + "ollama_disable_thinking": { + "description": "关闭思考模式", + "type": "bool", + "hint": "关闭 Ollama 思考模式。", + }, "custom_extra_body": { "description": "自定义请求体参数", "type": "dict", diff --git a/astrbot/core/provider/func_tool_manager.py b/astrbot/core/provider/func_tool_manager.py index 7290f055d..6b0d3a2d3 100644 --- a/astrbot/core/provider/func_tool_manager.py +++ b/astrbot/core/provider/func_tool_manager.py @@ -313,13 +313,30 @@ class FunctionToolManager: break def get_func(self, name) -> FuncTool | None: - for f in self.func_list: + # 优先返回已激活的工具（后加载的覆盖前面的，与 ToolSet.add_tool 保持一致） + # 使用 getattr(..., True) 与 ToolSet.add_tool 保持一致：没有 active 属性的工具视为已激活 + for f in reversed(self.func_list): + if f.name == name and getattr(f, "active", True): + return f + # 退化则拿最后一个同名工具 + for f in reversed(self.func_list): if f.name == name: return f + return None def get_full_tool_set(self) -> ToolSet: - """获取完整工具集""" - tool_set = ToolSet(self.func_list.copy()) + """获取完整工具集 + + 使用 ToolSet.add_tool 进行填充。对于同名工具，去重规则为： + - 优先保留 active=True 的工具； + - 当 active 状态相同时，后加载的工具会覆盖前面的工具。 + + 因此，后加载的 inactive 工具不会覆盖已激活的工具； + 同时，MCP 工具在需要时仍可覆盖被禁用的内置工具。 + """ + tool_set = ToolSet() + for tool in self.func_list: + tool_set.add_tool(tool) return tool_set @staticmethod diff --git a/astrbot/core/provider/sources/openai_source.py b/astrbot/core/provider/sources/openai_source.py index 33d4beb7c..90d427248 100644 --- a/astrbot/core/provider/sources/openai_source.py +++ b/astrbot/core/provider/sources/openai_source.py @@ -211,6 +211,26 @@ class ProviderOpenAIOfficial(Provider): self.reasoning_key = "reasoning_content" + def _ollama_disable_thinking_enabled(self) -> bool: + value = self.provider_config.get("ollama_disable_thinking", False) + if isinstance(value, str): + return value.strip().lower() in {"1", "true", "yes", "on"} + return bool(value) + + def _apply_provider_specific_extra_body_overrides( + self, extra_body: dict[str, Any] + ) -> None: + if self.provider_config.get("provider") != "ollama": + return + if not self._ollama_disable_thinking_enabled(): + return + + # Ollama's OpenAI-compatible endpoint reliably maps reasoning_effort=none + # to think=false, while direct think=false passthrough is not stable. + extra_body.pop("reasoning", None) + extra_body.pop("think", None) + extra_body["reasoning_effort"] = "none" + async def get_models(self): try: models_str = [] @@ -246,6 +266,7 @@ class ProviderOpenAIOfficial(Provider): custom_extra_body = self.provider_config.get("custom_extra_body", {}) if isinstance(custom_extra_body, dict): extra_body.update(custom_extra_body) + self._apply_provider_specific_extra_body_overrides(extra_body) model = payloads.get("model", "").lower() @@ -296,6 +317,7 @@ class ProviderOpenAIOfficial(Provider): to_del.append(key) for key in to_del: del payloads[key] + self._apply_provider_specific_extra_body_overrides(extra_body) stream = await self.client.chat.completions.create( **payloads, diff --git a/dashboard/src/composables/useProviderSources.ts b/dashboard/src/composables/useProviderSources.ts index c4dc51b89..1ada5e352 100644 --- a/dashboard/src/composables/useProviderSources.ts +++ b/dashboard/src/composables/useProviderSources.ts @@ -204,11 +204,10 @@ export function useProviderSources(options: UseProviderSourcesOptions) { const advancedSourceConfig = computed(() => { if (!editableProviderSource.value) return null - const excluded = ['id', 'key', 'api_base', 'enable', 'type', 'provider_type', 'provider'] + const excluded = new Set(['id', 'key', 'api_base', 'enable', 'type', 'provider_type', 'provider']) const advanced: Record = {} for (const key of Object.keys(editableProviderSource.value)) { - if (excluded.includes(key)) continue Object.defineProperty(advanced, key, { get() { return editableProviderSource.value![key] @@ -216,7 +215,7 @@ export function useProviderSources(options: UseProviderSourcesOptions) { set(val) { editableProviderSource.value![key] = val }, - enumerable: true + enumerable: !excluded.has(key) }) } @@ -347,7 +346,9 @@ export function useProviderSources(options: UseProviderSourcesOptions) { selectedProviderSource.value = source selectedProviderSourceOriginalId.value = source?.id || null suppressSourceWatch = true - editableProviderSource.value = source ? JSON.parse(JSON.stringify(source)) : null + editableProviderSource.value = source + ? ensureProviderSourceDefaults(JSON.parse(JSON.stringify(source))) + : null nextTick(() => { suppressSourceWatch = false }) @@ -356,6 +357,18 @@ export function useProviderSources(options: UseProviderSourcesOptions) { isSourceModified.value = false } + function ensureProviderSourceDefaults(source: any) { + if (!source || typeof source !== 'object') { + return source + } + + if (source.provider === 'ollama' && source.ollama_disable_thinking === undefined) { + source.ollama_disable_thinking = false + } + + return source + } + function extractSourceFieldsFromTemplate(template: Record) { const sourceFields: Record = {} const excludeKeys = ['id', 'enable', 'model', 'provider_source_id', 'modalities', 'custom_extra_body'] @@ -391,14 +404,14 @@ export function useProviderSources(options: UseProviderSourcesOptions) { } const newId = generateUniqueSourceId(template.id) - const newSource = { + const newSource = ensureProviderSourceDefaults({ ...extractSourceFieldsFromTemplate(template), id: newId, type: template.type, provider_type: template.provider_type, provider: template.provider, enable: true - } + }) providerSources.value.push(newSource) selectedProviderSource.value = newSource diff --git a/dashboard/src/i18n/locales/en-US/features/config-metadata.json b/dashboard/src/i18n/locales/en-US/features/config-metadata.json index 261ca7471..2c9fda243 100644 --- a/dashboard/src/i18n/locales/en-US/features/config-metadata.json +++ b/dashboard/src/i18n/locales/en-US/features/config-metadata.json @@ -1069,6 +1069,10 @@ "description": "Custom request headers", "hint": "Key/value pairs added here are merged into the OpenAI SDK default_headers for custom HTTP headers. Values must be strings." }, + "ollama_disable_thinking": { + "description": "Disable thinking mode", + "hint": "Close Ollama thinking mode." + }, "custom_extra_body": { "description": "Custom request body parameters", "hint": "Add extra parameters to requests, such as temperature, top_p, max_tokens, etc.", diff --git a/dashboard/src/i18n/locales/zh-CN/features/config-metadata.json b/dashboard/src/i18n/locales/zh-CN/features/config-metadata.json index edc96161f..fb7402528 100644 --- a/dashboard/src/i18n/locales/zh-CN/features/config-metadata.json +++ b/dashboard/src/i18n/locales/zh-CN/features/config-metadata.json @@ -1071,6 +1071,10 @@ "description": "自定义添加请求头", "hint": "此处添加的键值对将被合并到 OpenAI SDK 的 default_headers 中，用于自定义 HTTP 请求头。值必须为字符串。" }, + "ollama_disable_thinking": { + "description": "关闭思考模式", + "hint": "关闭 Ollama 思考模式。" + }, "custom_extra_body": { "description": "自定义请求体参数", "hint": "用于在请求时添加额外的参数，如 temperature、top_p、max_tokens 等。", diff --git a/tests/test_openai_source.py b/tests/test_openai_source.py index 2eea15d10..0040f0be6 100644 --- a/tests/test_openai_source.py +++ b/tests/test_openai_source.py @@ -1,6 +1,7 @@ from types import SimpleNamespace import pytest +from openai.types.chat.chat_completion import ChatCompletion from astrbot.core.provider.sources.groq_source import ProviderGroq from astrbot.core.provider.sources.openai_source import ProviderOpenAIOfficial @@ -447,3 +448,88 @@ async def test_handle_api_error_unknown_image_error_raises(): ) finally: await provider.terminate() + + +@pytest.mark.asyncio +async def test_apply_provider_specific_extra_body_overrides_disables_ollama_thinking(): + provider = _make_provider( + { + "provider": "ollama", + "ollama_disable_thinking": True, + } + ) + try: + extra_body = { + "reasoning": {"effort": "high"}, + "reasoning_effort": "low", + "think": True, + "temperature": 0.2, + } + + provider._apply_provider_specific_extra_body_overrides(extra_body) + + assert extra_body["reasoning_effort"] == "none" + assert "reasoning" not in extra_body + assert "think" not in extra_body + assert extra_body["temperature"] == 0.2 + finally: + await provider.terminate() + + +@pytest.mark.asyncio +async def test_query_injects_reasoning_effort_none_for_ollama(monkeypatch): + provider = _make_provider( + { + "provider": "ollama", + "ollama_disable_thinking": True, + "custom_extra_body": { + "reasoning": {"effort": "high"}, + "temperature": 0.1, + }, + } + ) + try: + captured_kwargs = {} + + async def fake_create(**kwargs): + captured_kwargs.update(kwargs) + return ChatCompletion.model_validate( + { + "id": "chatcmpl-test", + "object": "chat.completion", + "created": 0, + "model": "qwen3.5:4b", + "choices": [ + { + "index": 0, + "message": { + "role": "assistant", + "content": "ok", + }, + "finish_reason": "stop", + } + ], + "usage": { + "prompt_tokens": 1, + "completion_tokens": 1, + "total_tokens": 2, + }, + } + ) + + monkeypatch.setattr(provider.client.chat.completions, "create", fake_create) + + await provider._query( + payloads={ + "model": "qwen3.5:4b", + "messages": [{"role": "user", "content": "hello"}], + }, + tools=None, + ) + + extra_body = captured_kwargs["extra_body"] + assert extra_body["reasoning_effort"] == "none" + assert "reasoning" not in extra_body + assert extra_body["temperature"] == 0.1 + finally: + await provider.terminate() diff --git a/tests/unit/test_tool_conflict_resolution.py b/tests/unit/test_tool_conflict_resolution.py new file mode 100644 index 000000000..7fd04d448 --- /dev/null +++ b/tests/unit/test_tool_conflict_resolution.py @@ -0,0 +1,233 @@ +"""Tests for tool conflict resolution in ToolSet.add_tool and FunctionToolManager. + +This module tests the fix for issue #5821: when an MCP external tool shares a name +with a disabled built-in tool, the MCP tool should not be removed as collateral damage. +""" + +import pytest + +from astrbot.core.agent.tool import FunctionTool, ToolSet +from astrbot.core.provider.func_tool_manager import FunctionToolManager + + +def make_tool(name: str, active: bool = True) -> FunctionTool: + """Create a simple FunctionTool for testing.""" + return FunctionTool( + name=name, + description=f"Test tool {name}", + parameters={"type": "object", "properties": {}}, + active=active, + ) + + +class TestToolSetAddTool: + """Tests for ToolSet.add_tool conflict resolution.""" + + def test_new_tool_active_existing_inactive_overwrites(self): + """Active tool should overwrite inactive tool with same name.""" + toolset = ToolSet() + toolset.add_tool(make_tool("web_search", active=False)) + toolset.add_tool(make_tool("web_search", active=True)) + + assert len(toolset.tools) == 1 + assert toolset.tools[0].active is True + + def test_new_tool_inactive_existing_active_preserves_existing(self): + """Inactive tool should NOT overwrite active tool with same name.""" + toolset = ToolSet() + toolset.add_tool(make_tool("web_search", active=True)) + toolset.add_tool(make_tool("web_search", active=False)) + + 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() + first = make_tool("web_search", active=True) + second = make_tool("web_search", active=True) + second.description = "Second web search" + + toolset.add_tool(first) + toolset.add_tool(second) + + assert len(toolset.tools) == 1 + # The second tool should be the one kept + assert toolset.tools[0] is second + assert toolset.tools[0].description == "Second web search" + + def test_both_inactive_last_one_wins(self): + """When both tools are inactive, the new one should overwrite.""" + toolset = ToolSet() + toolset.add_tool(make_tool("web_search", active=False)) + toolset.add_tool(make_tool("web_search", active=False)) + + assert len(toolset.tools) == 1 + + def test_different_names_both_added(self): + """Tools with different names should both be added.""" + toolset = ToolSet() + toolset.add_tool(make_tool("web_search")) + toolset.add_tool(make_tool("code_search")) + + assert len(toolset.tools) == 2 + + def test_missing_active_attribute_defaults_to_true(self): + """Tools without 'active' attribute should be treated as active.""" + toolset = ToolSet() + + # Create a mock object without 'active' attribute + class MockTool: + name = "mock_tool" + description = "Mock" + parameters = {"type": "object"} + + mock_tool = MockTool() + toolset.add_tool(mock_tool) # type: ignore + + # Should be added successfully + assert len(toolset.tools) == 1 + + # Adding another tool without active should overwrite + mock_tool2 = MockTool() + toolset.add_tool(mock_tool2) # type: ignore + + assert len(toolset.tools) == 1 + + +class TestFunctionToolManagerGetFunc: + """Tests for FunctionToolManager.get_func with conflict resolution.""" + + def test_returns_last_active_tool(self): + """Should return the last active tool when multiple have same name.""" + manager = FunctionToolManager() + manager.func_list = [ + make_tool("web_search", active=True), + make_tool("web_search", active=True), + ] + + result = manager.get_func("web_search") + assert result is not None + # Should return the last one (reversed order) + assert result is manager.func_list[1] + + def test_returns_active_over_inactive(self): + """Should prefer active tool over inactive tool with same name.""" + manager = FunctionToolManager() + manager.func_list = [ + make_tool("web_search", active=False), + make_tool("web_search", active=True), + ] + + result = manager.get_func("web_search") + assert result is not None + assert result.active is True + assert result is manager.func_list[1] + + def test_inactive_cannot_override_active(self): + """Inactive tool after active should not be returned.""" + manager = FunctionToolManager() + manager.func_list = [ + make_tool("web_search", active=True), + make_tool("web_search", active=False), + ] + + result = manager.get_func("web_search") + assert result is not None + assert result.active is True + assert result is manager.func_list[0] + + def test_fallback_to_last_when_none_active(self): + """Should return last tool with matching name when none are active.""" + manager = FunctionToolManager() + manager.func_list = [ + make_tool("web_search", active=False), + make_tool("web_search", active=False), + ] + + result = manager.get_func("web_search") + assert result is not None + # Should return the last one (reversed order in fallback) + assert result is manager.func_list[1] + + def test_returns_none_when_not_found(self): + """Should return None when tool name not found.""" + manager = FunctionToolManager() + manager.func_list = [make_tool("other_tool")] + + result = manager.get_func("web_search") + assert result is None + + +class TestFunctionToolManagerGetFullToolSet: + """Tests for FunctionToolManager.get_full_tool_set.""" + + def test_deduplicates_by_name_using_add_tool(self): + """Should deduplicate tools using add_tool logic.""" + manager = FunctionToolManager() + manager.func_list = [ + make_tool("web_search", active=False), + make_tool("web_search", active=True), + make_tool("code_search", active=True), + ] + + toolset = manager.get_full_tool_set() + + # Should have 2 tools after deduplication + assert len(toolset.tools) == 2 + # web_search should be active (the MCP version) + web_search = toolset.get_tool("web_search") + assert web_search is not None + assert web_search.active is True + + def test_no_deepcopy_preserves_identity(self): + """Should not deep copy tools, preserving object identity.""" + manager = FunctionToolManager() + tool = make_tool("web_search") + manager.func_list = [tool] + + toolset = manager.get_full_tool_set() + + # Same object reference (no deepcopy) + assert toolset.tools[0] is tool + + def test_mcp_tool_overrides_disabled_builtin(self): + """ + Integration test: MCP tool should override disabled built-in tool. + This is the core scenario for issue #5821. + """ + manager = FunctionToolManager() + # Simulate: built-in tool registered first (disabled) + # Then MCP tool registered (enabled) + manager.func_list = [ + make_tool("web_search", active=False), # Built-in, disabled + make_tool("web_search", active=True), # MCP, enabled + ] + + # get_func should return the MCP tool (active one) + result = manager.get_func("web_search") + assert result is not None + assert result.active is True + assert result is manager.func_list[1] + + # get_full_tool_set should also keep the MCP tool + toolset = manager.get_full_tool_set() + assert len(toolset.tools) == 1 + assert toolset.tools[0].active is True + + def test_disabled_mcp_cannot_override_enabled_builtin(self): + """Disabled MCP tool should not override enabled built-in tool.""" + manager = FunctionToolManager() + manager.func_list = [ + make_tool("web_search", active=True), # Built-in, enabled + make_tool("web_search", active=False), # MCP, disabled + ] + + result = manager.get_func("web_search") + assert result is not None + assert result.active is True + assert result is manager.func_list[0] + + toolset = manager.get_full_tool_set() + assert len(toolset.tools) == 1 + assert toolset.tools[0].active is True