From 444031688dd6d4fa033fd4ea699639a62a6f38f1 Mon Sep 17 00:00:00 2001 From: "Grapette.L" <90967079+Lesereingrape@users.noreply.github.com> Date: Mon, 12 Oct 2026 20:45:47 +0800 Subject: [PATCH 1/3] Python: Reject a non-positive MCP request timeout instead of disabling it --- python/packages/core/agent_framework/_mcp.py | 7 ++-- python/packages/core/tests/core/test_mcp.py | 34 ++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/python/packages/core/agent_framework/_mcp.py b/python/packages/core/agent_framework/_mcp.py index 56f68cf3c3b..a532802a75a 100644 --- a/python/packages/core/agent_framework/_mcp.py +++ b/python/packages/core/agent_framework/_mcp.py @@ -950,7 +950,8 @@ def __init__( individual ``FunctionTool`` instances. session: An existing MCP client session to use. The caller retains ownership; closing or resetting this wrapper does not close or replace that session. - request_timeout: Timeout in seconds for MCP requests. + request_timeout: Timeout in seconds for MCP requests. Must be positive, or None to + leave requests without a timeout. client: A chat client for sampling callbacks. sampling_approval_callback: Optional gate invoked before each server-initiated ``sampling/createMessage`` request is forwarded to ``client``. It receives the @@ -997,6 +998,8 @@ def __init__( ) if max_host_payload_size_bytes is not None and max_host_payload_size_bytes <= 0: raise ValueError("max_host_payload_size_bytes must be positive or None.") + if request_timeout is not None and request_timeout <= 0: + raise ValueError("request_timeout must be positive or None.") allowed_modes = { "structured_first", "content_first", @@ -2004,7 +2007,7 @@ async def _connect_on_owner( read_stream=transport[0], write_stream=transport[1], read_timeout_seconds=( - timedelta(seconds=self.request_timeout) if self.request_timeout else None + timedelta(seconds=self.request_timeout) if self.request_timeout is not None else None ), message_handler=self.message_handler, logging_callback=self.logging_callback, diff --git a/python/packages/core/tests/core/test_mcp.py b/python/packages/core/tests/core/test_mcp.py index 367bdcdc7f5..bb770b3b903 100644 --- a/python/packages/core/tests/core/test_mcp.py +++ b/python/packages/core/tests/core/test_mcp.py @@ -1051,6 +1051,40 @@ def test_mcp_host_payload_size_limit_must_be_positive_or_none() -> None: assert unlimited.max_host_payload_size_bytes is None +def test_mcp_request_timeout_must_be_positive_or_none() -> None: + with pytest.raises(ValueError, match="positive or None"): + MCPTool(name="invalid", request_timeout=0) # type: ignore[abstract] # ty: ignore[call-non-callable] + + with pytest.raises(ValueError, match="positive or None"): + MCPStdioTool(name="invalid", command="test-command", request_timeout=-1) + + unlimited = MCPTool(name="unlimited", request_timeout=None) # type: ignore[abstract] # ty: ignore[call-non-callable] + assert unlimited.request_timeout is None + + +async def test_connect_passes_positive_request_timeout_to_session() -> None: + tool = MCPStdioTool(name="test", command="test-command", request_timeout=5, load_tools=False, load_prompts=False) + + mock_transport = (Mock(), Mock()) + mock_context_manager = Mock() + mock_context_manager.__aenter__ = AsyncMock(return_value=mock_transport) + mock_context_manager.__aexit__ = AsyncMock(return_value=None) + tool.get_mcp_client = Mock(return_value=mock_context_manager) # type: ignore[method-assign] + + with patch("mcp.client.session.ClientSession") as mock_session_class: + mock_session = AsyncMock() + mock_session._request_id = 1 + + session_cm = AsyncMock() + session_cm.__aenter__ = AsyncMock(return_value=mock_session) + session_cm.__aexit__ = AsyncMock(return_value=None) + mock_session_class.return_value = session_cm + + await tool.connect() + + assert mock_session_class.call_args.kwargs["read_timeout_seconds"] == timedelta(seconds=5) + + def test_mcp_host_payload_size_boundary_uri_serialization_and_early_abort( monkeypatch: pytest.MonkeyPatch, ) -> None: From d53fc23390d47947caaccbf0abcbb51b605fe662 Mon Sep 17 00:00:00 2001 From: "Grapette.L" <90967079+Lesereingrape@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:35:52 +0800 Subject: [PATCH 2/3] test: drop the timeout test that poked the removed session path --- python/packages/core/tests/core/test_mcp.py | 23 --------------------- 1 file changed, 23 deletions(-) diff --git a/python/packages/core/tests/core/test_mcp.py b/python/packages/core/tests/core/test_mcp.py index adeb887f7eb..89b3d31414b 100644 --- a/python/packages/core/tests/core/test_mcp.py +++ b/python/packages/core/tests/core/test_mcp.py @@ -1431,29 +1431,6 @@ def test_mcp_request_timeout_must_be_positive_or_none() -> None: assert unlimited.request_timeout is None -async def test_connect_passes_positive_request_timeout_to_session() -> None: - tool = MCPStdioTool(name="test", command="test-command", request_timeout=5, load_tools=False, load_prompts=False) - - mock_transport = (Mock(), Mock()) - mock_context_manager = Mock() - mock_context_manager.__aenter__ = AsyncMock(return_value=mock_transport) - mock_context_manager.__aexit__ = AsyncMock(return_value=None) - tool.get_mcp_client = Mock(return_value=mock_context_manager) # type: ignore[method-assign] - - with patch("mcp.client.session.ClientSession") as mock_session_class: - mock_session = AsyncMock() - mock_session._request_id = 1 - - session_cm = AsyncMock() - session_cm.__aenter__ = AsyncMock(return_value=mock_session) - session_cm.__aexit__ = AsyncMock(return_value=None) - mock_session_class.return_value = session_cm - - await tool.connect() - - assert mock_session_class.call_args.kwargs["read_timeout_seconds"] == timedelta(seconds=5) - - def test_mcp_host_payload_size_boundary_uri_serialization_and_early_abort( monkeypatch: pytest.MonkeyPatch, ) -> None: From 69b0b89aac1528bcf07fd931b4182383fcd1c83f Mon Sep 17 00:00:00 2001 From: "Grapette.L" <90967079+Lesereingrape@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:32:18 +0800 Subject: [PATCH 3/3] fix: revalidate request_timeout before opening the MCP session --- python/packages/core/agent_framework/_mcp.py | 3 +++ python/packages/core/tests/core/test_mcp.py | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/python/packages/core/agent_framework/_mcp.py b/python/packages/core/agent_framework/_mcp.py index 2793c2ea232..fc7467f7bda 100644 --- a/python/packages/core/agent_framework/_mcp.py +++ b/python/packages/core/agent_framework/_mcp.py @@ -2315,6 +2315,7 @@ async def _connect_on_owner( reset_discovery: If True, removes state derived from the previous session after its transport closes. Raises: + ValueError: If ``request_timeout`` was set to a non-positive value after construction. ToolException: If connection or session initialization fails. """ log_level = _to_mcp_logging_level(logger.level) @@ -2333,6 +2334,8 @@ async def _connect_on_owner( self._exit_stack = AsyncExitStack() connection = self._connection if connection is None: + if self.request_timeout is not None and self.request_timeout <= 0: + raise ValueError("request_timeout must be positive or None.") try: try: from mcp import Client, types diff --git a/python/packages/core/tests/core/test_mcp.py b/python/packages/core/tests/core/test_mcp.py index 89b3d31414b..7f6ac11b2d3 100644 --- a/python/packages/core/tests/core/test_mcp.py +++ b/python/packages/core/tests/core/test_mcp.py @@ -5426,6 +5426,25 @@ async def test_connect_preserves_request_timeout_over_24_hours() -> None: assert mock_client_class.call_args.kwargs["read_timeout_seconds"] == 90_000 +async def test_connect_revalidates_request_timeout_cleared_after_construction() -> None: + tool = MCPStdioTool( + name="test", + command="test-command", + load_tools=False, + load_prompts=False, + request_timeout=60, + ) + tool.request_timeout = 0 + + with patch("mcp.Client") as mock_client_class: + mock_client_class.return_value = _mock_sdk_client(protocol_version="2026-07-28") + + with pytest.raises(ValueError, match="request_timeout must be positive or None."): + await tool.connect() + + assert mock_client_class.call_count == 0 + + async def test_connect_retains_and_close_clears_sdk_client() -> None: """Test a framework-owned Client and its session share one lifecycle.""" tool = MCPStdioTool(name="test", command="test-command", load_tools=False, load_prompts=False)