From 385d882aa922c8656d998babb83ace0eedd4b8e9 Mon Sep 17 00:00:00 2001 From: LIghtJUNction Date: Mon, 23 Mar 2026 22:15:47 +0800 Subject: [PATCH] fix: resolve anyio compliance and type checking issues - Fix orchestrator to use anyio.get_cancelled_exc_class() instead of anyio.CancelledError - Fix tests to properly check for anyio compliance (not violations) - Add type annotations for MCP exception fallbacks in registry.py - Remove unused type: ignore comment in mcp/tool.py - All 111 tests pass - uvx ty check passes - ruff check passes --- .../abc/acp/base_astrbot_acp_client.py | 2 +- astrbot/_internal/protocols/lsp/client.py | 2 +- astrbot/_internal/protocols/mcp/client.py | 6 ++-- astrbot/_internal/protocols/mcp/tool.py | 2 +- astrbot/_internal/runtime/orchestrator.py | 25 +++++++++------- astrbot/_internal/tools/registry.py | 10 +++---- tests/unit/test_internal/test_mcp_client.py | 29 +++++++++---------- tests/unit/test_internal/test_orchestrator.py | 6 ++-- 8 files changed, 43 insertions(+), 39 deletions(-) diff --git a/astrbot/_internal/abc/acp/base_astrbot_acp_client.py b/astrbot/_internal/abc/acp/base_astrbot_acp_client.py index 96f8a6886..3085631e6 100644 --- a/astrbot/_internal/abc/acp/base_astrbot_acp_client.py +++ b/astrbot/_internal/abc/acp/base_astrbot_acp_client.py @@ -37,7 +37,7 @@ class BaseAstrbotAcpClient(ABC): ... @abstractmethod - async def connect_to_unix_socket(self, path: str) -> None: + async def connect_to_unix_socket(self, socket_path: str) -> None: """Connect via Unix domain socket.""" ... diff --git a/astrbot/_internal/protocols/lsp/client.py b/astrbot/_internal/protocols/lsp/client.py index ffc0a49f4..1764d145b 100644 --- a/astrbot/_internal/protocols/lsp/client.py +++ b/astrbot/_internal/protocols/lsp/client.py @@ -11,8 +11,8 @@ import asyncio import json from typing import Any -from astrbot._internal.abc.lsp.base_astrbot_lsp_client import BaseAstrbotLspClient from astrbot import logger +from astrbot._internal.abc.lsp.base_astrbot_lsp_client import BaseAstrbotLspClient log = logger diff --git a/astrbot/_internal/protocols/mcp/client.py b/astrbot/_internal/protocols/mcp/client.py index 9e72bbf20..d1a2e66b3 100644 --- a/astrbot/_internal/protocols/mcp/client.py +++ b/astrbot/_internal/protocols/mcp/client.py @@ -142,13 +142,13 @@ class McpClient(BaseAstrbotMcpClient): self.active: bool = True self.tools: list[mcp.Tool] = [] self.server_errlogs: list[str] = [] - self.running_event = asyncio.Event() + self.running_event = anyio.Event() self.process_pid: int | None = None # Store connection config for reconnection - self._mcp_server_config: dict | None = None + self._mcp_server_config: McpServerConfig | None = None self._server_name: str | None = None - self._reconnect_lock = asyncio.Lock() # Lock for thread-safe reconnection + self._reconnect_lock = anyio.Lock() # Lock for thread-safe reconnection self._reconnecting: bool = False # For logging and debugging async def connect(self) -> None: diff --git a/astrbot/_internal/protocols/mcp/tool.py b/astrbot/_internal/protocols/mcp/tool.py index 09ad13f43..5e9eda1db 100644 --- a/astrbot/_internal/protocols/mcp/tool.py +++ b/astrbot/_internal/protocols/mcp/tool.py @@ -19,7 +19,7 @@ class MCPTool(FunctionTool): def __init__( self, - mcp_tool: "mcp.types.Tool", # type: ignore[name-defined] + mcp_tool: "mcp.types.Tool", mcp_client: "McpClient", mcp_server_name: str, **kwargs: Any, diff --git a/astrbot/_internal/runtime/orchestrator.py b/astrbot/_internal/runtime/orchestrator.py index c54293a0d..02ec0a601 100644 --- a/astrbot/_internal/runtime/orchestrator.py +++ b/astrbot/_internal/runtime/orchestrator.py @@ -7,9 +7,10 @@ and runs the main event loop that dispatches messages between components. from __future__ import annotations -import asyncio from typing import Any +import anyio + from astrbot import logger from astrbot._internal.abc.base_astrbot_orchestrator import BaseAstrbotOrchestrator from astrbot._internal.protocols.abp.client import AstrbotAbpClient @@ -71,13 +72,14 @@ class AstrbotOrchestrator(BaseAstrbotOrchestrator): self._running = True log.info("AstrbotOrchestrator run loop started.") - # TODO: Replace asyncio.sleep loop with proper task scheduling using anyio. - # The ASYNC110 warning is valid - this should use asyncio.Event.wait() or - # a task group with cancellation, but keeping as-is for now to allow - # periodic health checks during development. Final implementation should: - # - Use anyio.TaskGroup for managing periodic tasks - # - Implement proper cancellation via Event.wait() - # - Avoid busy-waiting with sleep intervals + stop_event = anyio.Event() + + def set_stop() -> None: + stop_event.set() + + # Store the callback for cleanup + self._stop_callback = set_stop + try: while self._running: # TODO: Periodic tasks: @@ -86,12 +88,15 @@ class AstrbotOrchestrator(BaseAstrbotOrchestrator): # - Check ACP client connections # - Process any pending star notifications - await asyncio.sleep(5) + # Wait for 5 seconds or until shutdown is called + with anyio.move_on_after(5): + await stop_event.wait() - except asyncio.CancelledError: + except anyio.get_cancelled_exc_class(): log.info("Orchestrator run loop cancelled.") finally: self._running = False + self._stop_callback = None log.info("AstrbotOrchestrator run loop stopped.") async def register_star(self, name: str, star_instance: Any) -> None: diff --git a/astrbot/_internal/tools/registry.py b/astrbot/_internal/tools/registry.py index 271e2c1e7..a50b0031e 100644 --- a/astrbot/_internal/tools/registry.py +++ b/astrbot/_internal/tools/registry.py @@ -34,11 +34,11 @@ try: ) except ImportError: DEFAULT_MCP_CONFIG: dict[str, Any] = {} - MCPAllServicesFailedError = Exception - MCPInitError = Exception - MCPInitSummary = dict - MCPInitTimeoutError = TimeoutError - MCPShutdownTimeoutError = TimeoutError + MCPAllServicesFailedError: type[Exception] = Exception + MCPInitError: type[Exception] = Exception + MCPInitSummary: type[dict] = dict + MCPInitTimeoutError: type[TimeoutError] = TimeoutError + MCPShutdownTimeoutError: type[TimeoutError] = TimeoutError ENABLE_MCP_TIMEOUT_ENV = "ASTRBOT_MCP_TIMEOUT_ENABLED" MCP_INIT_TIMEOUT_ENV = "ASTRBOT_MCP_INIT_TIMEOUT" diff --git a/tests/unit/test_internal/test_mcp_client.py b/tests/unit/test_internal/test_mcp_client.py index a165bdb77..a1ec17555 100644 --- a/tests/unit/test_internal/test_mcp_client.py +++ b/tests/unit/test_internal/test_mcp_client.py @@ -7,6 +7,7 @@ Transport: stdio | SSE | streamable_http from __future__ import annotations +import anyio import asyncio from typing import Any from unittest.mock import AsyncMock, MagicMock, patch @@ -92,7 +93,7 @@ class TestMcpClientReconnect: def test_reconnect_lock_exists(self, mcp_client): """MCP client should have a reconnect lock.""" assert mcp_client._reconnect_lock is not None - assert isinstance(mcp_client._reconnect_lock, asyncio.Lock) + assert isinstance(mcp_client._reconnect_lock, anyio.Lock) def test_reconnecting_flag_initial_false(self, mcp_client): """_reconnecting flag should be False initially.""" @@ -100,26 +101,24 @@ class TestMcpClientReconnect: class TestMcpClientUsesAsyncioNotAnyio: - """TEST REQUIREMENT: Document asyncio vs anyio violations. + """TEST REQUIREMENT: Document asyncio vs anyio compliance. Per openspec directive: "异步库使用anyio" (Use anyio as async library). - The MCP client implementation currently uses asyncio primitives: - - asyncio.Lock instead of anyio.Lock - - asyncio.Future instead of anyio.Future - - asyncio.Event instead of anyio.Event - - This is a VIOLATION of the architecture requirement. + The MCP client implementation should use anyio primitives: + - anyio.Lock instead of asyncio.Lock + - anyio.Future instead of asyncio.Future + - anyio.Event instead of asyncio.Event """ def test_mcp_client_uses_asyncio_lock(self): - """VIOLATION: MCP client uses asyncio.Lock instead of anyio.Lock.""" + """COMPLIANCE: MCP client uses anyio.Lock, not asyncio.Lock.""" from astrbot._internal.protocols.mcp.client import McpClient client = McpClient() - # Check if _reconnect_lock is asyncio.Lock (violation) - assert isinstance(client._reconnect_lock, asyncio.Lock), ( + # Check that _reconnect_lock is anyio.Lock (compliant) + assert isinstance(client._reconnect_lock, anyio.Lock), ( "MCP client should use anyio.Lock instead of asyncio.Lock " "per the 'async_library: Use anyio' directive in openspec" ) @@ -135,27 +134,27 @@ class TestMcpClientUsesAsyncioNotAnyio: # Note: Futures are created in call_star_tool, not at init def test_orchestrator_run_loop_uses_asyncio_sleep(self): - """VIOLATION: Orchestrator uses asyncio.sleep instead of anyio.""" + """COMPLIANCE: Orchestrator uses anyio.sleep, not asyncio.sleep.""" from astrbot._internal.runtime.orchestrator import AstrbotOrchestrator import inspect source = inspect.getsource(AstrbotOrchestrator.run_loop) - assert "asyncio.sleep" in source, ( + assert "asyncio.sleep" not in source, ( "Orchestrator should use anyio.sleep instead of asyncio.sleep " "per the 'async_library: Use anyio' directive in openspec" ) def test_orchestrator_run_loop_handles_asyncio_cancelled_error(self): - """VIOLATION: Orchestrator catches asyncio.CancelledError not anyio.""" + """COMPLIANCE: Orchestrator catches anyio.CancelledError, not asyncio.""" from astrbot._internal.runtime.orchestrator import AstrbotOrchestrator import inspect source = inspect.getsource(AstrbotOrchestrator.run_loop) - assert "asyncio.CancelledError" in source, ( + assert "asyncio.CancelledError" not in source, ( "Orchestrator should catch anyio.CancelledError not asyncio.CancelledError " "per the 'async_library: Use anyio' directive in openspec" ) diff --git a/tests/unit/test_internal/test_orchestrator.py b/tests/unit/test_internal/test_orchestrator.py index 75ae56dc2..f081336b2 100644 --- a/tests/unit/test_internal/test_orchestrator.py +++ b/tests/unit/test_internal/test_orchestrator.py @@ -154,11 +154,11 @@ class TestAstrbotOrchestrator: source = inspect.getsource(orchestrator.run_loop) - # Check if asyncio.sleep is used (VIOLATION) + # Check that asyncio.sleep is NOT used (COMPLIANCE) uses_asyncio_sleep = "asyncio.sleep" in source - # This assertion documents the violation - assert uses_asyncio_sleep, ( + # This assertion documents compliance + assert not uses_asyncio_sleep, ( "run_loop should use anyio.sleep instead of asyncio.sleep " "per the 'async_library: Use anyio' directive in openspec" )