comments

Author: dbschmigelski

.codecov.yml

@@ -1,3 +1,11 @@
 coverage:
-  ignore:
-    - "src/strands/experimental/tools/tool_provider.py" # This is an interface, cannot meaningfully cover
+  status:
+    project:
+      default:
+        target: 90%    # overall coverage threshold
+    patch:
+      default:
+        target: 90%    # patch coverage threshold
+        base: auto
+        # Only post patch coverage on decreases
+        only_pulls: true

pyproject.toml

@@ -133,14 +133,15 @@ dependencies = [
     "pytest-cov>=7.0.0,<8.0.0",
     "pytest-asyncio>=1.0.0,<1.3.0",
     "pytest-xdist>=3.0.0,<4.0.0",
+    "pytest-timeout>=2.0.0,<3.0.0",
     "moto>=5.1.0,<6.0.0",
 ]
 
 [[tool.hatch.envs.hatch-test.matrix]]
 python = ["3.13", "3.12", "3.11", "3.10"]
 
 [tool.hatch.envs.hatch-test.scripts]
-run = "pytest{env:HATCH_TEST_ARGS:} {args}" # Run with: hatch test
+run = "pytest{env:HATCH_TEST_ARGS:} --timeout=10  {args}" # Run with: hatch test
 run-cov = "pytest{env:HATCH_TEST_ARGS:} {args} --cov --cov-config=pyproject.toml --cov-report html --cov-report xml {args}" # Run with: hatch test -c
 cov-combine = []
 cov-report = []

src/strands/agent/agent.py

@@ -541,7 +541,7 @@ def cleanup(self) -> None:
         Note: This method uses a "belt and braces" approach with automatic cleanup
         through __del__ as a fallback, but explicit cleanup is recommended.
         """
-        if self._cleanup_called:
+        if getattr(self, "_cleanup_called", False):
             return
 
         run_async(self.cleanup_async)
@@ -553,18 +553,22 @@ async def cleanup_async(self) -> None:
         such as MCP clients. It should be called when the agent is no longer needed
         to ensure proper resource cleanup.
 
-        Note: This method uses a "belt and braces" approach with automatic cleanup
-        through __del__ as a fallback, but explicit cleanup is recommended.
+        This method is idempotent and safe to call multiple times.
         """
-        if self._cleanup_called:
+        # Use getattr with False default: if _cleanup_called was deleted during garbage collection,
+        # we default to False (cleanup not called) to ensure cleanup still runs
+        if getattr(self, "_cleanup_called", False):
             return
 
-        logger.debug("agent_id=<%s> | cleaning up agent resources", self.agent_id)
+        agent_id = getattr(self, "agent_id", None)
+        logger.debug("agent_id=<%s> | cleaning up agent resources", agent_id)
 
-        await self.tool_registry.cleanup_async()
+        tool_registry = getattr(self, "tool_registry", None)
+        if tool_registry:
+            await tool_registry.cleanup_async()
 
         self._cleanup_called = True
-        logger.debug("agent_id=<%s> | agent cleanup complete", self.agent_id)
+        logger.debug("agent_id=<%s> | agent cleanup complete", agent_id)
 
     def __del__(self) -> None:
         """Automatic cleanup when agent is garbage collected.
@@ -574,7 +578,8 @@ def __del__(self) -> None:
         try:
             self.cleanup()
         except Exception as e:
-            logger.debug("agent_id=<%s>, error=<%s> | exception during __del__ cleanup", self.agent_id, e)
+            agent_id = getattr(self, "agent_id", None)
+            logger.debug("agent_id=<%s>, error=<%s> | exception during __del__ cleanup", agent_id, e)
 
     async def stream_async(
         self, prompt: AgentInput = None, *, invocation_state: dict[str, Any] | None = None, **kwargs: Any

src/strands/tools/mcp/mcp_client.py

@@ -210,15 +210,13 @@ async def load_tools(self, **kwargs: Any) -> Sequence[AgentTool]:
 
             while True:
                 logger.debug("page=<%d>, token=<%s> | fetching tools page", page_count, pagination_token)
-                paginated_tools = self.list_tools_sync(pagination_token)
+                paginated_tools = self.list_tools_sync(pagination_token, prefix=self._prefix)
 
                 # Process each tool as we get it
                 for tool in paginated_tools:
                     # Apply filters
                     if self._should_include_tool(tool):
-                        # Apply prefix if needed
-                        processed_tool = self._apply_prefix(tool)
-                        self._loaded_tools.append(processed_tool)
+                        self._loaded_tools.append(tool)
 
                 logger.debug(
                     "page=<%d>, page_tools=<%d>, total_filtered=<%d> | processed page",
@@ -313,12 +311,18 @@ async def _set_close_event() -> None:
         self._tool_provider_started = False
         self._consumers = set()
 
-    def list_tools_sync(self, pagination_token: Optional[str] = None) -> PaginatedList[MCPAgentTool]:
+    def list_tools_sync(
+        self, pagination_token: Optional[str] = None, prefix: Optional[str] = None
+    ) -> PaginatedList[MCPAgentTool]:
         """Synchronously retrieves the list of available tools from the MCP server.
 
         This method calls the asynchronous list_tools method on the MCP session
         and adapts the returned tools to the AgentTool interface.
 
+        Args:
+            pagination_token: Optional token for pagination
+            prefix: Optional prefix to apply to tool names
+
         Returns:
             List[AgentTool]: A list of available tools adapted to the AgentTool interface
         """
@@ -332,7 +336,16 @@ async def _list_tools_async() -> ListToolsResult:
         list_tools_response: ListToolsResult = self._invoke_on_background_thread(_list_tools_async()).result()
         self._log_debug_with_thread("received %d tools from MCP server", len(list_tools_response.tools))
 
-        mcp_tools = [MCPAgentTool(tool, self) for tool in list_tools_response.tools]
+        mcp_tools = []
+        for tool in list_tools_response.tools:
+            if prefix:
+                prefixed_name = f"{prefix}_{tool.name}"
+                mcp_tool = MCPAgentTool(tool, self, name_override=prefixed_name)
+                logger.debug("tool_rename=<%s->%s> | renamed tool", tool.name, prefixed_name)
+            else:
+                mcp_tool = MCPAgentTool(tool, self)
+            mcp_tools.append(mcp_tool)
+
         self._log_debug_with_thread("successfully adapted %d MCP tools", len(mcp_tools))
         return PaginatedList[MCPAgentTool](mcp_tools, token=list_tools_response.nextCursor)
 
@@ -670,31 +683,21 @@ def _should_include_tool(self, tool: MCPAgentTool) -> bool:
             if self._matches_patterns(tool, self._tool_filters["rejected"]):
                 return False
 
+        print(f"Returning true for {tool.mcp_tool.name} {tool.tool_name}")
         return True
 
-    def _apply_prefix(self, tool: MCPAgentTool) -> MCPAgentTool:
-        """Apply prefix to a single tool if needed."""
-        if not self._prefix:
-            return tool
-
-        # Create new tool with prefixed agent name but preserve original MCP name
-        old_name = tool.tool_name
-        new_agent_name = f"{self._prefix}_{tool.mcp_tool.name}"
-        new_tool = MCPAgentTool(tool.mcp_tool, tool.mcp_client, name_override=new_agent_name)
-        logger.debug("tool_rename=<%s->%s> | renamed tool", old_name, new_agent_name)
-        return new_tool
-
     def _matches_patterns(self, tool: MCPAgentTool, patterns: list[_ToolFilterPattern]) -> bool:
         """Check if tool matches any of the given patterns."""
         for pattern in patterns:
             if callable(pattern):
                 if pattern(tool):
                     return True
             elif isinstance(pattern, Pattern):
-                if pattern.match(tool.tool_name):
+                if pattern.match(tool.mcp_tool.name):
                     return True
             elif isinstance(pattern, str):
-                if pattern == tool.tool_name:
+                print(f"checking {pattern} against {tool.mcp_tool.name}")
+                if pattern == tool.mcp_tool.name:
                     return True
         return False
 

tests/strands/agent/test_agent.py

@@ -894,7 +894,7 @@ def test_agent_cleanup():
     """Test that agent cleanup method works correctly."""
     # Create a fresh agent to avoid fixture interference
     agent = Agent()
-    
+
     with unittest.mock.patch("strands.agent.agent.run_async") as mock_run_async:
         agent.cleanup()
 

tests/strands/tools/mcp/test_mcp_client_tool_provider.py

@@ -4,6 +4,7 @@
 from unittest.mock import MagicMock, patch
 
 import pytest
+from mcp.types import Tool as MCPTool
 
 from strands.tools.mcp import MCPClient
 from strands.tools.mcp.mcp_agent_tool import MCPAgentTool
@@ -41,12 +42,18 @@ def mock_agent_tool(mock_mcp_tool):
     return agent_tool
 
 
-def create_mock_tool(name: str) -> MagicMock:
+def create_mock_tool(tool_name: str, mcp_tool_name: str | None = None) -> MagicMock:
     """Helper to create mock tools with specific names."""
     tool = MagicMock(spec=MCPAgentTool)
-    tool.tool_name = name
-    tool.mcp_tool = MagicMock()
-    tool.mcp_tool.name = name
+    tool.tool_name = tool_name
+    tool.tool_spec = {
+        "name": tool_name,
+        "description": f"Description for {tool_name}",
+        "inputSchema": {"json": {"type": "object", "properties": {}}},
+    }
+    tool.mcp_tool = MagicMock(spec=MCPTool)
+    tool.mcp_tool.name = mcp_tool_name or tool_name
+    tool.mcp_tool.description = f"Description for {tool_name}"
     return tool
 
 
@@ -146,8 +153,8 @@ async def test_load_tools_handles_pagination(mock_transport):
         # Should have called list_tools_sync twice
         assert mock_list_tools.call_count == 2
         # First call with no token, second call with "page2" token
-        mock_list_tools.assert_any_call(None)
-        mock_list_tools.assert_any_call("page2")
+        mock_list_tools.assert_any_call(None, prefix=None)
+        mock_list_tools.assert_any_call("page2", prefix=None)
 
         assert len(tools) == 2
         assert tools[0] is tool1
@@ -236,31 +243,44 @@ async def test_rejected_filter_string_match(mock_transport):
 @pytest.mark.asyncio
 async def test_prefix_renames_tools(mock_transport):
     """Test that prefix properly renames tools."""
-    original_tool = create_mock_tool("original_name")
-    original_tool.mcp_client = MagicMock()
+    # Create a mock MCP tool (not MCPAgentTool)
+    mock_mcp_tool = MagicMock()
+    mock_mcp_tool.name = "original_name"
 
     client = MCPClient(mock_transport, prefix="prefix")
     client._tool_provider_started = True
 
+    # Mock the session active state
+    mock_thread = MagicMock()
+    mock_thread.is_alive.return_value = True
+    client._background_thread = mock_thread
+
     with (
-        patch.object(client, "list_tools_sync") as mock_list_tools,
+        patch.object(client, "_invoke_on_background_thread") as mock_invoke,
         patch("strands.tools.mcp.mcp_client.MCPAgentTool") as mock_agent_tool_class,
     ):
-        mock_list_tools.return_value = PaginatedList([original_tool])
+        # Mock the MCP server response
+        mock_list_tools_result = MagicMock()
+        mock_list_tools_result.tools = [mock_mcp_tool]
+        mock_list_tools_result.nextCursor = None
 
-        new_tool = MagicMock(spec=MCPAgentTool)
-        new_tool.tool_name = "prefix_original_name"
-        mock_agent_tool_class.return_value = new_tool
+        mock_future = MagicMock()
+        mock_future.result.return_value = mock_list_tools_result
+        mock_invoke.return_value = mock_future
 
-        tools = await client.load_tools()
+        # Mock MCPAgentTool creation
+        mock_agent_tool = MagicMock(spec=MCPAgentTool)
+        mock_agent_tool.tool_name = "prefix_original_name"
+        mock_agent_tool_class.return_value = mock_agent_tool
 
-        # Should create new MCPAgentTool with prefixed name
-        mock_agent_tool_class.assert_called_once_with(
-            original_tool.mcp_tool, original_tool.mcp_client, name_override="prefix_original_name"
-        )
+        # Call list_tools_sync directly to test prefix functionality
+        result = client.list_tools_sync(prefix="prefix")
 
-        assert len(tools) == 1
-        assert tools[0] is new_tool
+        # Should create MCPAgentTool with prefixed name
+        mock_agent_tool_class.assert_called_once_with(mock_mcp_tool, client, name_override="prefix_original_name")
+
+        assert len(result) == 1
+        assert result[0] is mock_agent_tool
 
 
 @pytest.mark.asyncio
@@ -318,3 +338,41 @@ async def test_remove_consumer_cleanup_failure(mock_transport):
 
         with pytest.raises(ToolProviderException, match="Failed to cleanup MCP client: Cleanup failed"):
             await client.remove_consumer("consumer1")
+
+
+def test_mcp_client_reuse_across_multiple_agents(mock_transport):
+    """Test that a single MCPClient can be used across multiple agents."""
+    from strands import Agent
+
+    tool1 = create_mock_tool(tool_name="shared_echo", mcp_tool_name="echo")
+    client = MCPClient(mock_transport, tool_filters={"allowed": ["echo"]}, prefix="shared")
+
+    with (
+        patch.object(client, "list_tools_sync") as mock_list_tools,
+        patch.object(client, "start") as mock_start,
+        patch.object(client, "stop") as mock_stop,
+    ):
+        mock_list_tools.return_value = PaginatedList([tool1])
+
+        # Create two agents with the same client
+        agent_1 = Agent(tools=[client])
+        agent_2 = Agent(tools=[client])
+
+        # Both agents should have the same tool
+        assert "shared_echo" in agent_1.tool_names
+        assert "shared_echo" in agent_2.tool_names
+        assert agent_1.tool_names == agent_2.tool_names
+
+        # Client should only be started once
+        mock_start.assert_called_once()
+
+        # First agent cleanup - client should remain active
+        agent_1.cleanup()
+        mock_stop.assert_not_called()  # Should not stop yet
+
+        # Second agent should still work
+        assert "shared_echo" in agent_2.tool_names
+
+        # Final cleanup when last agent is removed
+        agent_2.cleanup()
+        mock_stop.assert_called_once()  # Now it should stop

tests_integ/mcp/test_mcp_tool_provider.py

@@ -35,7 +35,7 @@ def short_names_only(tool) -> bool:
     agent = Agent(tools=[client])
     tool_names = agent.tool_names
 
-    assert "echo_with_delay" not in [name.replace("test_", "") for name in tool_names]
+    assert "test_echo_with_delay" not in [name for name in tool_names]
     assert all(name.startswith("test_") for name in tool_names)
 
     agent.cleanup()
@@ -91,29 +91,6 @@ def test_mcp_client_tool_provider_reuse():
     assert agent1.tool_names == agent2.tool_names
 
     agent1.cleanup()
-    agent2.cleanup()
-
-
-def test_mcp_client_reference_counting():
-    """Test that MCPClient uses reference counting - cleanup only happens when last consumer is removed."""
-    filters: ToolFilters = {"allowed": ["echo"]}
-    client = MCPClient(
-        lambda: stdio_client(StdioServerParameters(command="python", args=["tests_integ/mcp/echo_server.py"])),
-        tool_filters=filters,
-        prefix="ref",
-    )
-
-    # Create two agents with the same client
-    agent1 = Agent(tools=[client])
-    agent2 = Agent(tools=[client])
-
-    # Both should have the tool
-    assert "ref_echo" in agent1.tool_names
-    assert "ref_echo" in agent2.tool_names
-
-    # Agent 1 uses the tool
-    result1 = agent1.tool.ref_echo(to_echo="Agent 1 Test")
-    assert "Agent 1 Test" in str(result1)
 
     # Agent 1 cleans up - client should still be active for agent 2
     agent1.cleanup()
@@ -122,7 +99,6 @@ def test_mcp_client_reference_counting():
     result2 = agent2.tool.ref_echo(to_echo="Agent 2 Test")
     assert "Agent 2 Test" in str(result2)
 
-    # Agent 2 cleans up - now client should be fully cleaned up
     agent2.cleanup()