From f7044649835f37ac55598194eb8889e3cac14037 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 1 Aug 2026 07:27:48 +0900 Subject: [PATCH 1/5] fix: redact direct MCP cleanup transport errors --- src/agents/mcp/server.py | 32 +++++++++++++++++++++--------- tests/mcp/test_server_errors.py | 35 +++++++++++++++++++++------------ 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/src/agents/mcp/server.py b/src/agents/mcp/server.py index 35cf10ace8..ee97cfba1d 100644 --- a/src/agents/mcp/server.py +++ b/src/agents/mcp/server.py @@ -184,6 +184,17 @@ def _log_transport_warning(message: str, http_error: Exception) -> None: log_tool_action_warning(logger, message, safe_error) +def _get_cleanup_transport_error_message(http_error: Exception) -> str: + """Return the cleanup warning message for an HTTPX transport failure.""" + if isinstance(http_error, httpx.HTTPStatusError): + return "HTTP error during cleanup of MCP server" + if isinstance(http_error, httpx.ConnectError): + return "Connection error during cleanup of MCP server" + if isinstance(http_error, httpx.TimeoutException): + return "Timeout error during cleanup of MCP server" + return "Request error during cleanup of MCP server" + + def _create_default_streamable_http_client( headers: dict[str, str] | None = None, timeout: httpx.Timeout | None = None, @@ -1282,16 +1293,10 @@ async def cleanup(self): del selected_http_error del unsafe_http_error else: - if isinstance(selected_http_error, httpx.HTTPStatusError): - cleanup_message = "HTTP error during cleanup of MCP server" - elif isinstance(selected_http_error, httpx.ConnectError): - cleanup_message = "Connection error during cleanup of MCP server" - elif isinstance(selected_http_error, httpx.TimeoutException): - cleanup_message = "Timeout error during cleanup of MCP server" - else: - cleanup_message = "Request error during cleanup of MCP server" _log_transport_warning( - get_mcp_server_log_message(cleanup_message, self), + get_mcp_server_log_message( + _get_cleanup_transport_error_message(selected_http_error), self + ), selected_http_error, ) else: @@ -1314,6 +1319,15 @@ async def cleanup(self): get_mcp_server_log_message("Error cleaning up MCP server", self), eg, ) + except (httpx.HTTPStatusError, httpx.RequestError) as e: + if is_failed_connection_cleanup: + cleanup_error = self._user_error_for_http_error(e) + cleanup_cause = _safe_transport_cause(e) + else: + _log_transport_warning( + get_mcp_server_log_message(_get_cleanup_transport_error_message(e), self), + e, + ) except Exception as e: # Suppress RuntimeError about cancel scopes - this is a known issue with the MCP # library when background tasks fail during async generator cleanup diff --git a/tests/mcp/test_server_errors.py b/tests/mcp/test_server_errors.py index b5ffc88406..0e4886e6dd 100644 --- a/tests/mcp/test_server_errors.py +++ b/tests/mcp/test_server_errors.py @@ -810,21 +810,28 @@ async def test_call_tool_request_error_only_maps_credentialed_urls( @pytest.mark.asyncio -async def test_failed_connection_cleanup_hides_url_credentials_from_exception_graph(): +@pytest.mark.parametrize("grouped", [False, True]) +async def test_failed_connection_cleanup_hides_url_credentials_from_exception_graph( + grouped: bool, +): server = MCPServerSse(params={"url": _CREDENTIALED_URL}) request = httpx.Request("GET", _CREDENTIALED_URL) http_error = httpx.HTTPStatusError( "boom", request=request, response=httpx.Response(502, request=request) ) - cleanup_group = BaseExceptionGroup("cleanup failed", [http_error]) + cleanup_error: BaseException = http_error + if grouped: + cleanup_error = BaseExceptionGroup("cleanup failed", [http_error]) - with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_group)): + with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)): with pytest.raises(UserError) as exc_info: await server.cleanup() assert "mcp.example.com/sse" in str(exc_info.value) assert "HTTP error 502" in str(exc_info.value) _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, cleanup_error) + _assert_not_retained_in_exception_graph(exc_info.value, http_error) _assert_not_retained_in_traceback_locals(exc_info.value, http_error) @@ -847,7 +854,7 @@ async def test_failed_connection_cleanup_checks_every_nested_transport_error(): @pytest.mark.asyncio @pytest.mark.parametrize("redacted", [True, False]) -@pytest.mark.parametrize("nested", [False, True]) +@pytest.mark.parametrize("exception_shape", ["direct", "grouped", "nested_group"]) @pytest.mark.parametrize( ("url", "safe_to_attach"), [ @@ -859,7 +866,7 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions( monkeypatch, caplog, redacted: bool, - nested: bool, + exception_shape: str, url: str, safe_to_attach: bool, ): @@ -870,13 +877,15 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions( "timed out", request=httpx.Request("GET", url), ) - inner_error: BaseException = timeout_error - if nested: - inner_error = BaseExceptionGroup("nested cleanup failed", [inner_error]) - cleanup_group = BaseExceptionGroup("cleanup failed", [inner_error]) + cleanup_error: BaseException = timeout_error + if exception_shape == "grouped": + cleanup_error = BaseExceptionGroup("cleanup failed", [timeout_error]) + elif exception_shape == "nested_group": + inner_group = BaseExceptionGroup("nested cleanup failed", [timeout_error]) + cleanup_error = BaseExceptionGroup("cleanup failed", [inner_group]) with ( - patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_group)), + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), caplog.at_level(logging.WARNING, logger="openai.agents"), ): await server.cleanup() @@ -884,16 +893,16 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions( record = caplog.records[-1] if not redacted and safe_to_attach: assert record.exc_info is not None - if nested: + if exception_shape == "nested_group": assert record.levelno == logging.ERROR - assert record.exc_info[1] is cleanup_group + assert record.exc_info[1] is cleanup_error else: assert record.levelno == logging.WARNING assert record.exc_info[1] is timeout_error else: assert record.exc_info is None assert record.exc_text is None - _assert_not_retained_in_log_record(record, cleanup_group) + _assert_not_retained_in_log_record(record, cleanup_error) _assert_not_retained_in_log_record(record, timeout_error) if not safe_to_attach: From f5ae695721e372bc58157c38e4d89fce33100fb3 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 1 Aug 2026 07:39:11 +0900 Subject: [PATCH 2/5] fix --- src/agents/mcp/server.py | 115 ++++++++++++++++++++------------ tests/mcp/test_server_errors.py | 89 ++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 43 deletions(-) diff --git a/src/agents/mcp/server.py b/src/agents/mcp/server.py index ee97cfba1d..fff13a31d2 100644 --- a/src/agents/mcp/server.py +++ b/src/agents/mcp/server.py @@ -105,11 +105,10 @@ class RequireApprovalObject(TypedDict, total=False): _SAFE_EXCEPTION_MESSAGE = "An additional error occurred during the MCP request." -def _safe_transport_cause(http_error: Exception) -> Exception | None: - """Keep a transport exception only when its HTTPX URLs need no sanitization.""" - if not isinstance(http_error, httpx.HTTPStatusError | httpx.RequestError): - return http_error - +def _transport_error_urls_are_safe( + http_error: httpx.HTTPStatusError | httpx.RequestError, +) -> bool: + """Return whether one HTTPX exception contains only credential-safe URLs.""" request_urls: list[str] = [] try: request_urls.append(str(http_error.request.url)) @@ -121,7 +120,7 @@ def _safe_transport_cause(http_error: Exception) -> Exception | None: try: response_url = response.request.url except RuntimeError: - return None + return False request_urls.append(str(response_url)) redirect_location = response.headers.get("location") @@ -129,9 +128,38 @@ def _safe_transport_cause(http_error: Exception) -> Exception | None: try: request_urls.append(str(response_url.join(redirect_location))) except (httpx.InvalidURL, ValueError): - return None + return False + + return all(get_mcp_server_log_name(url) == url for url in request_urls) + + +def _safe_transport_cause(http_error: Exception) -> Exception | None: + """Keep a transport exception only when its full graph has credential-safe HTTPX URLs.""" + if not isinstance(http_error, httpx.HTTPStatusError | httpx.RequestError): + return http_error + + pending: list[BaseException] = [http_error] + seen: set[int] = set() + while pending: + error = pending.pop() + if id(error) in seen: + continue + seen.add(id(error)) + + if isinstance(error, httpx.HTTPStatusError | httpx.RequestError): + if not _transport_error_urls_are_safe(error): + return None - return http_error if all(get_mcp_server_log_name(url) == url for url in request_urls) else None + cause = BaseException.__getattribute__(error, "__cause__") + if cause is not None: + pending.append(cause) + context = BaseException.__getattribute__(error, "__context__") + if context is not None: + pending.append(context) + if isinstance(error, BaseExceptionGroup): + pending.extend(error.exceptions) + + return http_error def _first_unsafe_transport_error(http_errors: list[Exception]) -> Exception | None: @@ -847,6 +875,33 @@ def _extract_http_errors_from_exception(self, e: BaseException) -> list[Exceptio return [] + def _select_cleanup_transport_error(self, error: BaseException) -> Exception | None: + """Select a cleanup transport error for specialized handling.""" + unsafe_http_error = _first_unsafe_transport_error( + self._extract_http_errors_from_exception(error) + ) + if unsafe_http_error is not None: + return unsafe_http_error + + candidates = error.exceptions if isinstance(error, BaseExceptionGroup) else (error,) + for error_type in ( + httpx.HTTPStatusError, + httpx.ConnectError, + httpx.TimeoutException, + ): + selected_http_error = next( + ( + candidate + for candidate in reversed(candidates) + if isinstance(candidate, Exception) and isinstance(candidate, error_type) + ), + None, + ) + if selected_http_error is not None: + return selected_http_error + + return None + def _user_error_for_http_error(self, http_error: Exception) -> UserError: """Build a UserError from safe HTTP diagnostics.""" error_message = f"Failed to connect to MCP server '{self._error_name}': " @@ -1261,37 +1316,14 @@ async def cleanup(self): e, ) raise - except BaseExceptionGroup as eg: - http_errors = self._extract_http_errors_from_exception(eg) - unsafe_http_error = _first_unsafe_transport_error(http_errors) - selected_http_error = unsafe_http_error - - if selected_http_error is None: - # Preserve legacy group diagnostics when HTTP errors are nested but safe. - for error_type in ( - httpx.HTTPStatusError, - httpx.ConnectError, - httpx.TimeoutException, - ): - selected_http_error = next( - ( - error - for error in reversed(eg.exceptions) - if isinstance(error, Exception) and isinstance(error, error_type) - ), - None, - ) - if selected_http_error is not None: - break - + except (BaseExceptionGroup, httpx.HTTPStatusError, httpx.RequestError) as e: + selected_http_error = self._select_cleanup_transport_error(e) if selected_http_error is not None: if is_failed_connection_cleanup: cleanup_error = self._user_error_for_http_error(selected_http_error) cleanup_cause = _safe_transport_cause(selected_http_error) if cleanup_cause is None: - http_errors.clear() del selected_http_error - del unsafe_http_error else: _log_transport_warning( get_mcp_server_log_message( @@ -1299,11 +1331,11 @@ async def cleanup(self): ), selected_http_error, ) - else: + elif isinstance(e, BaseExceptionGroup): # No HTTP error found, suppress RuntimeError about cancel scopes has_cancel_scope_error = any( isinstance(exc, RuntimeError) and "cancel scope" in str(exc) - for exc in eg.exceptions + for exc in e.exceptions ) if has_cancel_scope_error: log_tool_action_debug( @@ -1311,21 +1343,18 @@ async def cleanup(self): get_mcp_server_log_message( "Ignoring cancel scope error during cleanup of MCP server", self ), - eg, + e, ) else: log_tool_action_error( logger, get_mcp_server_log_message("Error cleaning up MCP server", self), - eg, + e, ) - except (httpx.HTTPStatusError, httpx.RequestError) as e: - if is_failed_connection_cleanup: - cleanup_error = self._user_error_for_http_error(e) - cleanup_cause = _safe_transport_cause(e) else: - _log_transport_warning( - get_mcp_server_log_message(_get_cleanup_transport_error_message(e), self), + log_tool_action_error( + logger, + get_mcp_server_log_message("Error cleaning up MCP server", self), e, ) except Exception as e: diff --git a/tests/mcp/test_server_errors.py b/tests/mcp/test_server_errors.py index 0e4886e6dd..ab9c2089cd 100644 --- a/tests/mcp/test_server_errors.py +++ b/tests/mcp/test_server_errors.py @@ -487,6 +487,19 @@ def _mixed_request_error_group( return BaseExceptionGroup("mixed failures", [safe_error, nested_group]), safe_error, later_error +def _chained_request_errors() -> tuple[httpx.ReadTimeout, httpx.ReadError]: + unsafe_error = httpx.ReadError( + "inner read failed", + request=httpx.Request("GET", _CREDENTIALED_URL), + ) + safe_outer_error = httpx.ReadTimeout( + "outer timeout", + request=httpx.Request("GET", _SAFE_URL), + ) + safe_outer_error.__context__ = unsafe_error + return safe_outer_error, unsafe_error + + @pytest.mark.asyncio async def test_connect_checks_every_request_error_before_preserving_exception_group(): server = MCPServerSse(params={"url": _SAFE_URL}) @@ -835,6 +848,54 @@ async def test_failed_connection_cleanup_hides_url_credentials_from_exception_gr _assert_not_retained_in_traceback_locals(exc_info.value, http_error) +@pytest.mark.asyncio +async def test_connect_preserves_original_error_when_cleanup_has_safe_generic_request_error( + monkeypatch, + caplog, +): + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) + server = MCPServerSse(params={"url": _SAFE_URL}) + connection_error = ValueError("original connection failure") + cleanup_error = httpx.ReadError( + "cleanup read failed", + request=httpx.Request("GET", _SAFE_URL), + ) + + with ( + patch.object(server, "create_streams", side_effect=connection_error), + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), + caplog.at_level(logging.ERROR, logger="openai.agents"), + ): + with pytest.raises(ValueError) as exc_info: + await server.connect() + + assert exc_info.value is connection_error + record = caplog.records[-1] + assert record.exc_info is not None + assert record.exc_info[1] is cleanup_error + assert server.session is None + assert server._get_session_id is None + + +@pytest.mark.asyncio +async def test_failed_connection_cleanup_hides_chained_url_credentials(): + server = MCPServerSse(params={"url": _SAFE_URL}) + cleanup_error, unsafe_error = _chained_request_errors() + + with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)): + with pytest.raises(UserError) as exc_info: + await server.cleanup() + + assert "Connection timeout" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, cleanup_error) + _assert_not_retained_in_exception_graph(exc_info.value, unsafe_error) + _assert_not_retained_in_traceback_locals(exc_info.value, cleanup_error) + _assert_not_retained_in_traceback_locals(exc_info.value, unsafe_error) + assert server.session is None + assert server._get_session_id is None + + @pytest.mark.asyncio async def test_failed_connection_cleanup_checks_every_nested_transport_error(): server = MCPServerSse(params={"url": _SAFE_URL}) @@ -912,6 +973,34 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions( assert server._get_session_id is None +@pytest.mark.asyncio +@pytest.mark.parametrize("redacted", [True, False]) +async def test_normal_cleanup_hides_chained_url_credentials_from_log_record( + monkeypatch, + caplog, + redacted: bool, +): + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", redacted) + server = MCPServerSse(params={"url": _SAFE_URL}) + server.session = MagicMock() + cleanup_error, unsafe_error = _chained_request_errors() + + with ( + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), + caplog.at_level(logging.WARNING, logger="openai.agents"), + ): + await server.cleanup() + + record = caplog.records[-1] + assert record.exc_info is None + assert record.exc_text is None + _assert_not_retained_in_log_record(record, cleanup_error) + _assert_not_retained_in_log_record(record, unsafe_error) + _assert_url_credentials_hidden_from_log_record(record) + assert server.session is None + assert server._get_session_id is None + + @pytest.mark.asyncio async def test_normal_cleanup_preserves_safe_nested_group_diagnostics(monkeypatch, caplog): monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) From 947bfbeba02fef60a0d8b1efdff11af5cff44a34 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 1 Aug 2026 07:52:45 +0900 Subject: [PATCH 3/5] fix --- src/agents/mcp/server.py | 62 +++++++++++++++----------- tests/mcp/test_server_errors.py | 78 +++++++++++++++++++++++++++------ 2 files changed, 100 insertions(+), 40 deletions(-) diff --git a/src/agents/mcp/server.py b/src/agents/mcp/server.py index fff13a31d2..1db63f7496 100644 --- a/src/agents/mcp/server.py +++ b/src/agents/mcp/server.py @@ -134,37 +134,33 @@ def _transport_error_urls_are_safe( def _safe_transport_cause(http_error: Exception) -> Exception | None: - """Keep a transport exception only when its full graph has credential-safe HTTPX URLs.""" + """Keep an unchained transport exception only when its HTTPX URLs are credential-safe.""" if not isinstance(http_error, httpx.HTTPStatusError | httpx.RequestError): return http_error - pending: list[BaseException] = [http_error] - seen: set[int] = set() - while pending: - error = pending.pop() - if id(error) in seen: - continue - seen.add(id(error)) - - if isinstance(error, httpx.HTTPStatusError | httpx.RequestError): - if not _transport_error_urls_are_safe(error): - return None - - cause = BaseException.__getattribute__(error, "__cause__") - if cause is not None: - pending.append(cause) - context = BaseException.__getattribute__(error, "__context__") - if context is not None: - pending.append(context) - if isinstance(error, BaseExceptionGroup): - pending.extend(error.exceptions) + if not _transport_error_urls_are_safe(http_error): + return None + if BaseException.__getattribute__(http_error, "__cause__") is not None: + return None + if BaseException.__getattribute__(http_error, "__context__") is not None: + return None + if BaseException.__getattribute__(http_error, "__dict__").get("__notes__"): + return None return http_error def _first_unsafe_transport_error(http_errors: list[Exception]) -> Exception | None: """Return the first transport error whose HTTPX URLs require sanitization.""" - return next((error for error in http_errors if _safe_transport_cause(error) is None), None) + return next( + ( + error + for error in http_errors + if isinstance(error, httpx.HTTPStatusError | httpx.RequestError) + and not _transport_error_urls_are_safe(error) + ), + None, + ) def _is_http_transport_error(error: BaseException) -> bool: @@ -902,11 +898,18 @@ def _select_cleanup_transport_error(self, error: BaseException) -> Exception | N return None - def _user_error_for_http_error(self, http_error: Exception) -> UserError: + def _user_error_for_http_error( + self, + http_error: Exception, + *, + include_http_reason_phrase: bool = True, + ) -> UserError: """Build a UserError from safe HTTP diagnostics.""" error_message = f"Failed to connect to MCP server '{self._error_name}': " if isinstance(http_error, httpx.HTTPStatusError): - error_message += f"HTTP error {http_error.response.status_code} ({http_error.response.reason_phrase})" # noqa: E501 + error_message += f"HTTP error {http_error.response.status_code}" + if include_http_reason_phrase: + error_message += f" ({http_error.response.reason_phrase})" elif isinstance(http_error, httpx.ConnectError): error_message += "Could not reach the server." @@ -1320,8 +1323,15 @@ async def cleanup(self): selected_http_error = self._select_cleanup_transport_error(e) if selected_http_error is not None: if is_failed_connection_cleanup: - cleanup_error = self._user_error_for_http_error(selected_http_error) - cleanup_cause = _safe_transport_cause(selected_http_error) + cleanup_error = self._user_error_for_http_error( + selected_http_error, + include_http_reason_phrase=False, + ) + cleanup_cause = ( + None + if isinstance(selected_http_error, httpx.HTTPStatusError) + else _safe_transport_cause(selected_http_error) + ) if cleanup_cause is None: del selected_http_error else: diff --git a/tests/mcp/test_server_errors.py b/tests/mcp/test_server_errors.py index ab9c2089cd..044f41757f 100644 --- a/tests/mcp/test_server_errors.py +++ b/tests/mcp/test_server_errors.py @@ -487,17 +487,30 @@ def _mixed_request_error_group( return BaseExceptionGroup("mixed failures", [safe_error, nested_group]), safe_error, later_error -def _chained_request_errors() -> tuple[httpx.ReadTimeout, httpx.ReadError]: - unsafe_error = httpx.ReadError( - "inner read failed", - request=httpx.Request("GET", _CREDENTIALED_URL), - ) +def _transport_error_with_sensitive_attachment( + attachment: str, +) -> tuple[httpx.ReadTimeout, object]: safe_outer_error = httpx.ReadTimeout( "outer timeout", request=httpx.Request("GET", _SAFE_URL), ) - safe_outer_error.__context__ = unsafe_error - return safe_outer_error, unsafe_error + if attachment == "http_context": + http_context = httpx.ReadError( + "inner read failed", + request=httpx.Request("GET", _CREDENTIALED_URL), + ) + sensitive_value: object = http_context + safe_outer_error.__context__ = http_context + elif attachment == "non_http_context": + non_http_context = ValueError(_CREDENTIALED_URL) + sensitive_value = non_http_context + safe_outer_error.__context__ = non_http_context + elif attachment == "note": + sensitive_value = _CREDENTIALED_URL + safe_outer_error.add_note(sensitive_value) + else: + raise AssertionError(f"Unexpected attachment type: {attachment}") + return safe_outer_error, sensitive_value @pytest.mark.asyncio @@ -878,9 +891,12 @@ async def test_connect_preserves_original_error_when_cleanup_has_safe_generic_re @pytest.mark.asyncio -async def test_failed_connection_cleanup_hides_chained_url_credentials(): +@pytest.mark.parametrize("attachment", ["http_context", "non_http_context", "note"]) +async def test_failed_connection_cleanup_hides_sensitive_exception_attachments( + attachment: str, +): server = MCPServerSse(params={"url": _SAFE_URL}) - cleanup_error, unsafe_error = _chained_request_errors() + cleanup_error, sensitive_value = _transport_error_with_sensitive_attachment(attachment) with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)): with pytest.raises(UserError) as exc_info: @@ -889,9 +905,41 @@ async def test_failed_connection_cleanup_hides_chained_url_credentials(): assert "Connection timeout" in str(exc_info.value) _assert_url_credentials_hidden(exc_info.value) _assert_not_retained_in_exception_graph(exc_info.value, cleanup_error) - _assert_not_retained_in_exception_graph(exc_info.value, unsafe_error) + _assert_not_retained_in_exception_graph(exc_info.value, sensitive_value) _assert_not_retained_in_traceback_locals(exc_info.value, cleanup_error) - _assert_not_retained_in_traceback_locals(exc_info.value, unsafe_error) + _assert_not_retained_in_traceback_locals(exc_info.value, sensitive_value) + assert server.session is None + assert server._get_session_id is None + + +@pytest.mark.asyncio +@pytest.mark.parametrize("grouped", [False, True]) +async def test_failed_connection_cleanup_omits_untrusted_http_reason_phrase(grouped: bool): + server = MCPServerSse(params={"url": _SAFE_URL}) + request = httpx.Request("GET", _SAFE_URL) + http_error = httpx.HTTPStatusError( + "boom", + request=request, + response=httpx.Response( + 502, + request=request, + extensions={"reason_phrase": _CREDENTIALED_URL.encode()}, + ), + ) + cleanup_error: BaseException = http_error + if grouped: + cleanup_error = BaseExceptionGroup("cleanup failed", [http_error]) + + with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)): + with pytest.raises(UserError) as exc_info: + await server.cleanup() + + assert "HTTP error 502" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, cleanup_error) + _assert_not_retained_in_exception_graph(exc_info.value, http_error) + _assert_not_retained_in_traceback_locals(exc_info.value, cleanup_error) + _assert_not_retained_in_traceback_locals(exc_info.value, http_error) assert server.session is None assert server._get_session_id is None @@ -975,15 +1023,17 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions( @pytest.mark.asyncio @pytest.mark.parametrize("redacted", [True, False]) -async def test_normal_cleanup_hides_chained_url_credentials_from_log_record( +@pytest.mark.parametrize("attachment", ["http_context", "non_http_context", "note"]) +async def test_normal_cleanup_hides_sensitive_exception_attachments_from_log_record( monkeypatch, caplog, redacted: bool, + attachment: str, ): monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", redacted) server = MCPServerSse(params={"url": _SAFE_URL}) server.session = MagicMock() - cleanup_error, unsafe_error = _chained_request_errors() + cleanup_error, sensitive_value = _transport_error_with_sensitive_attachment(attachment) with ( patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), @@ -995,7 +1045,7 @@ async def test_normal_cleanup_hides_chained_url_credentials_from_log_record( assert record.exc_info is None assert record.exc_text is None _assert_not_retained_in_log_record(record, cleanup_error) - _assert_not_retained_in_log_record(record, unsafe_error) + _assert_not_retained_in_log_record(record, sensitive_value) _assert_url_credentials_hidden_from_log_record(record) assert server.session is None assert server._get_session_id is None From eaa746999a7b1e277bf3db7eef31494efbdce359 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 1 Aug 2026 08:10:50 +0900 Subject: [PATCH 4/5] fix --- src/agents/mcp/server.py | 63 +++++++++++------- tests/mcp/test_server_errors.py | 110 ++++++++++++++++++++++++++++++-- 2 files changed, 145 insertions(+), 28 deletions(-) diff --git a/src/agents/mcp/server.py b/src/agents/mcp/server.py index 1db63f7496..3435213f4c 100644 --- a/src/agents/mcp/server.py +++ b/src/agents/mcp/server.py @@ -1034,7 +1034,7 @@ async def connect(self): unsafe_http_error = _first_unsafe_transport_error(http_errors) http_error = unsafe_http_error or http_errors[0] - connection_cause = None if unsafe_http_error is not None else http_error + connection_cause = _safe_transport_cause(http_error) maps_safe_error = isinstance( http_error, httpx.HTTPStatusError | httpx.ConnectError | httpx.TimeoutException, @@ -1308,7 +1308,6 @@ async def cleanup(self): # masking the original exception. is_failed_connection_cleanup = self.session is None cleanup_error: UserError | None = None - cleanup_cause: Exception | None = None try: await self.exit_stack.aclose() @@ -1327,13 +1326,7 @@ async def cleanup(self): selected_http_error, include_http_reason_phrase=False, ) - cleanup_cause = ( - None - if isinstance(selected_http_error, httpx.HTTPStatusError) - else _safe_transport_cause(selected_http_error) - ) - if cleanup_cause is None: - del selected_http_error + del selected_http_error else: _log_transport_warning( get_mcp_server_log_message( @@ -1341,26 +1334,48 @@ async def cleanup(self): ), selected_http_error, ) + elif isinstance(e, httpx.RequestError): + _log_transport_warning( + get_mcp_server_log_message(_get_cleanup_transport_error_message(e), self), + e, + ) elif isinstance(e, BaseExceptionGroup): - # No HTTP error found, suppress RuntimeError about cancel scopes - has_cancel_scope_error = any( - isinstance(exc, RuntimeError) and "cancel scope" in str(exc) - for exc in e.exceptions + http_errors = self._extract_http_errors_from_exception(e) + unsafe_http_error = next( + ( + http_error + for http_error in http_errors + if _safe_transport_cause(http_error) is None + ), + None, ) - if has_cancel_scope_error: - log_tool_action_debug( - logger, + if unsafe_http_error is not None: + _log_transport_warning( get_mcp_server_log_message( - "Ignoring cancel scope error during cleanup of MCP server", self + _get_cleanup_transport_error_message(unsafe_http_error), self ), - e, + unsafe_http_error, ) else: - log_tool_action_error( - logger, - get_mcp_server_log_message("Error cleaning up MCP server", self), - e, + # No HTTP error found, suppress RuntimeError about cancel scopes. + has_cancel_scope_error = any( + isinstance(exc, RuntimeError) and "cancel scope" in str(exc) + for exc in e.exceptions ) + if has_cancel_scope_error: + log_tool_action_debug( + logger, + get_mcp_server_log_message( + "Ignoring cancel scope error during cleanup of MCP server", self + ), + e, + ) + else: + log_tool_action_error( + logger, + get_mcp_server_log_message("Error cleaning up MCP server", self), + e, + ) else: log_tool_action_error( logger, @@ -1389,7 +1404,7 @@ async def cleanup(self): self._get_session_id = None if cleanup_error is not None: - self._raise_mapped_transport_error(cleanup_error, cleanup_cause) + self._raise_mapped_transport_error(cleanup_error, None) class MCPServerStdioParams(TypedDict): @@ -1986,7 +2001,7 @@ async def call_tool( unsafe_http_error = _first_unsafe_transport_error(http_errors) http_error = unsafe_http_error or http_errors[0] - transport_cause = None if unsafe_http_error is not None else http_error + transport_cause = _safe_transport_cause(http_error) if isinstance(http_error, httpx.HTTPStatusError): status_code = http_error.response.status_code transport_error = UserError( diff --git a/tests/mcp/test_server_errors.py b/tests/mcp/test_server_errors.py index 044f41757f..a87f08bc73 100644 --- a/tests/mcp/test_server_errors.py +++ b/tests/mcp/test_server_errors.py @@ -507,7 +507,7 @@ def _transport_error_with_sensitive_attachment( safe_outer_error.__context__ = non_http_context elif attachment == "note": sensitive_value = _CREDENTIALED_URL - safe_outer_error.add_note(sensitive_value) + safe_outer_error.__dict__["__notes__"] = [sensitive_value] else: raise AssertionError(f"Unexpected attachment type: {attachment}") return safe_outer_error, sensitive_value @@ -545,6 +545,50 @@ async def test_call_tool_checks_every_request_error_before_preserving_exception_ _assert_not_retained_in_traceback_locals(exc_info.value, unsafe_error) +@pytest.mark.asyncio +async def test_connect_group_hides_sensitive_transport_error_context(): + server = MCPServerSse(params={"url": _SAFE_URL}) + transport_error, sensitive_value = _transport_error_with_sensitive_attachment( + "non_http_context" + ) + error_group = BaseExceptionGroup("connection failed", [transport_error]) + + with patch.object(server, "create_streams", side_effect=error_group): + with pytest.raises(UserError) as exc_info: + await server.connect() + + assert "Connection timeout" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, transport_error) + _assert_not_retained_in_exception_graph(exc_info.value, sensitive_value) + _assert_not_retained_in_traceback_locals(exc_info.value, error_group) + _assert_not_retained_in_traceback_locals(exc_info.value, transport_error) + _assert_not_retained_in_traceback_locals(exc_info.value, sensitive_value) + + +@pytest.mark.asyncio +async def test_call_tool_group_hides_sensitive_transport_error_context(): + server = MCPServerStreamableHttp(params={"url": _SAFE_URL}) + server.session = MagicMock() + server.max_retry_attempts = 0 + transport_error, sensitive_value = _transport_error_with_sensitive_attachment( + "non_http_context" + ) + error_group = BaseExceptionGroup("tool call failed", [transport_error]) + + with patch.object(server, "_call_tool_with_isolated_retry", side_effect=error_group): + with pytest.raises(UserError) as exc_info: + await server.call_tool("test_tool", {}) + + assert "Connection timeout" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, transport_error) + _assert_not_retained_in_exception_graph(exc_info.value, sensitive_value) + _assert_not_retained_in_traceback_locals(exc_info.value, error_group) + _assert_not_retained_in_traceback_locals(exc_info.value, transport_error) + _assert_not_retained_in_traceback_locals(exc_info.value, sensitive_value) + + @pytest.mark.asyncio async def test_connect_preserves_exception_group_when_every_request_error_is_safe(): server = MCPServerSse(params={"url": _SAFE_URL}) @@ -877,15 +921,53 @@ async def test_connect_preserves_original_error_when_cleanup_has_safe_generic_re with ( patch.object(server, "create_streams", side_effect=connection_error), patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), - caplog.at_level(logging.ERROR, logger="openai.agents"), + caplog.at_level(logging.WARNING, logger="openai.agents"), ): with pytest.raises(ValueError) as exc_info: await server.connect() assert exc_info.value is connection_error record = caplog.records[-1] - assert record.exc_info is not None - assert record.exc_info[1] is cleanup_error + assert record.exc_info is None + assert record.exc_text is None + _assert_not_retained_in_log_record(record, cleanup_error) + assert server.session is None + assert server._get_session_id is None + + +@pytest.mark.asyncio +@pytest.mark.parametrize("grouped", [False, True]) +async def test_normal_cleanup_hides_generic_request_error_context_from_log_record( + monkeypatch, + caplog, + grouped: bool, +): + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) + server = MCPServerSse(params={"url": _SAFE_URL}) + server.session = MagicMock() + request_error = httpx.ReadError( + "cleanup read failed", + request=httpx.Request("GET", _SAFE_URL), + ) + sensitive_value = ValueError(_CREDENTIALED_URL) + request_error.__context__ = sensitive_value + cleanup_error: BaseException = request_error + if grouped: + cleanup_error = BaseExceptionGroup("cleanup failed", [request_error]) + + with ( + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), + caplog.at_level(logging.WARNING, logger="openai.agents"), + ): + await server.cleanup() + + record = caplog.records[-1] + assert record.exc_info is None + assert record.exc_text is None + _assert_not_retained_in_log_record(record, cleanup_error) + _assert_not_retained_in_log_record(record, request_error) + _assert_not_retained_in_log_record(record, sensitive_value) + _assert_url_credentials_hidden_from_log_record(record) assert server.session is None assert server._get_session_id is None @@ -912,6 +994,26 @@ async def test_failed_connection_cleanup_hides_sensitive_exception_attachments( assert server._get_session_id is None +@pytest.mark.asyncio +async def test_failed_connection_cleanup_hides_sensitive_transport_error_message(): + server = MCPServerSse(params={"url": _SAFE_URL}) + cleanup_error = httpx.ReadTimeout( + _CREDENTIALED_URL, + request=httpx.Request("GET", _SAFE_URL), + ) + + with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)): + with pytest.raises(UserError) as exc_info: + await server.cleanup() + + assert "Connection timeout" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, cleanup_error) + _assert_not_retained_in_traceback_locals(exc_info.value, cleanup_error) + assert server.session is None + assert server._get_session_id is None + + @pytest.mark.asyncio @pytest.mark.parametrize("grouped", [False, True]) async def test_failed_connection_cleanup_omits_untrusted_http_reason_phrase(grouped: bool): From 93ea5a6891be2f927ce126d8e0de33b009a88c68 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 1 Aug 2026 08:56:25 +0900 Subject: [PATCH 5/5] fix --- src/agents/mcp/server.py | 153 +++++++++++++++++-------------- tests/mcp/test_server_errors.py | 158 ++++++++++++++++++++++++++++---- 2 files changed, 226 insertions(+), 85 deletions(-) diff --git a/src/agents/mcp/server.py b/src/agents/mcp/server.py index 3435213f4c..168a476e12 100644 --- a/src/agents/mcp/server.py +++ b/src/agents/mcp/server.py @@ -163,6 +163,11 @@ def _first_unsafe_transport_error(http_errors: list[Exception]) -> Exception | N ) +def _first_unretainable_transport_error(http_errors: list[Exception]) -> Exception | None: + """Return the first transport error that cannot be retained as an exception cause.""" + return next((error for error in http_errors if _safe_transport_cause(error) is None), None) + + def _is_http_transport_error(error: BaseException) -> bool: """Return whether an exception is an HTTPX transport error.""" return isinstance(error, httpx.HTTPStatusError | httpx.RequestError) @@ -219,6 +224,11 @@ def _get_cleanup_transport_error_message(http_error: Exception) -> str: return "Request error during cleanup of MCP server" +def _log_cleanup_transport_warning(message: str) -> None: + """Log a fixed cleanup warning without retaining the transport exception.""" + logger.warning("%s", message, stacklevel=3) + + def _create_default_streamable_http_client( headers: dict[str, str] | None = None, timeout: httpx.Timeout | None = None, @@ -1004,6 +1014,8 @@ async def connect(self): connection_succeeded = False connection_error: UserError | None = None connection_cause: Exception | None = None + connection_exception: BaseException | None = None + cleanup_failure: BaseException | None = None try: transport = await self.exit_stack.enter_async_context(self.create_streams()) # streamablehttp_client returns (read, write, get_session_id) @@ -1027,56 +1039,69 @@ async def connect(self): self.server_initialize_result = server_result self.session = session connection_succeeded = True - except Exception as e: - http_errors = self._extract_http_errors_from_exception(e) - if not http_errors: - raise + except BaseException as e: + if not isinstance(e, Exception): + connection_exception = e + else: + http_errors = self._extract_http_errors_from_exception(e) + if not http_errors: + connection_exception = e + else: + unsafe_http_error = _first_unretainable_transport_error(http_errors) + http_error = unsafe_http_error or http_errors[0] + connection_cause = _safe_transport_cause(http_error) + maps_safe_error = isinstance( + http_error, + httpx.HTTPStatusError | httpx.ConnectError | httpx.TimeoutException, + ) + if connection_cause is not None and not maps_safe_error: + connection_exception = e + connection_cause = None + else: + connection_error = self._user_error_for_http_error(http_error) + http_errors.clear() + del http_error + del unsafe_http_error + + # Run cleanup after leaving the connection exception handler so a cleanup UserError does + # not retain the pending connection failure as its implicit context. + if not connection_succeeded: + try: + await self.cleanup() + except UserError as e: + cleanup_failure = e + except Exception as cleanup_error: + # Suppress RuntimeError about cancel scopes during cleanup - this is a known + # issue with the MCP library's async generator cleanup and shouldn't mask the + # original error. + if isinstance(cleanup_error, RuntimeError) and "cancel scope" in str(cleanup_error): + logger.debug( + "%s", + get_mcp_server_log_message( + "Ignoring cancel scope error during cleanup of MCP server", self + ), + stacklevel=2, + ) + else: + # Log other cleanup errors but don't raise - original error is more important. + logger.warning( + "%s", + get_mcp_server_log_message("Error during cleanup of MCP server", self), + stacklevel=2, + ) + except BaseException as e: + cleanup_failure = e - unsafe_http_error = _first_unsafe_transport_error(http_errors) - http_error = unsafe_http_error or http_errors[0] - connection_cause = _safe_transport_cause(http_error) - maps_safe_error = isinstance( - http_error, - httpx.HTTPStatusError | httpx.ConnectError | httpx.TimeoutException, - ) - if connection_cause is not None and not maps_safe_error: - raise + if cleanup_failure is not None: + connection_exception = None + connection_error = None + connection_cause = None + if isinstance(cleanup_failure, UserError): + self._raise_mapped_transport_error(cleanup_failure, None) + raise cleanup_failure - connection_error = self._user_error_for_http_error(http_error) - if connection_cause is None: - http_errors.clear() - del http_error - del unsafe_http_error - finally: - # Always attempt cleanup on error, but suppress cleanup errors that mask the original - if not connection_succeeded: - try: - await self.cleanup() - except UserError: - # Re-raise UserError from cleanup (contains the real HTTP error) - raise - except Exception as cleanup_error: - # Suppress RuntimeError about cancel scopes during cleanup - this is a known - # issue with the MCP library's async generator cleanup and shouldn't mask the - # original error - if isinstance(cleanup_error, RuntimeError) and "cancel scope" in str( - cleanup_error - ): - log_tool_action_debug( - logger, - get_mcp_server_log_message( - "Ignoring cancel scope error during cleanup of MCP server", self - ), - cleanup_error, - ) - else: - # Log other cleanup errors but don't raise - original error is more - # important - log_tool_action_warning( - logger, - get_mcp_server_log_message("Error during cleanup of MCP server", self), - cleanup_error, - ) + if connection_exception is not None: + raise connection_exception if connection_error is not None: self._raise_mapped_transport_error(connection_error, connection_cause) @@ -1328,33 +1353,23 @@ async def cleanup(self): ) del selected_http_error else: - _log_transport_warning( + _log_cleanup_transport_warning( get_mcp_server_log_message( _get_cleanup_transport_error_message(selected_http_error), self - ), - selected_http_error, + ) ) elif isinstance(e, httpx.RequestError): - _log_transport_warning( - get_mcp_server_log_message(_get_cleanup_transport_error_message(e), self), - e, + _log_cleanup_transport_warning( + get_mcp_server_log_message(_get_cleanup_transport_error_message(e), self) ) elif isinstance(e, BaseExceptionGroup): http_errors = self._extract_http_errors_from_exception(e) - unsafe_http_error = next( - ( - http_error - for http_error in http_errors - if _safe_transport_cause(http_error) is None - ), - None, - ) - if unsafe_http_error is not None: - _log_transport_warning( - get_mcp_server_log_message( - _get_cleanup_transport_error_message(unsafe_http_error), self - ), - unsafe_http_error, + if http_errors: + safe_error_group = _credential_safe_exception_group(e) + log_tool_action_error( + logger, + get_mcp_server_log_message("Error cleaning up MCP server", self), + safe_error_group, ) else: # No HTTP error found, suppress RuntimeError about cancel scopes. @@ -1999,7 +2014,7 @@ async def call_tool( if not http_errors: raise - unsafe_http_error = _first_unsafe_transport_error(http_errors) + unsafe_http_error = _first_unretainable_transport_error(http_errors) http_error = unsafe_http_error or http_errors[0] transport_cause = _safe_transport_cause(http_error) if isinstance(http_error, httpx.HTTPStatusError): diff --git a/tests/mcp/test_server_errors.py b/tests/mcp/test_server_errors.py index a87f08bc73..fc4d3f2a56 100644 --- a/tests/mcp/test_server_errors.py +++ b/tests/mcp/test_server_errors.py @@ -126,7 +126,17 @@ def _assert_not_retained_in_log_record( continue seen.add(id(value)) - if isinstance(value, dict): + if isinstance(value, BaseException): + pending.extend(value.args) + if value.__cause__ is not None: + pending.append(value.__cause__) + if value.__context__ is not None: + pending.append(value.__context__) + pending.extend(getattr(value, "__notes__", ())) + pending.append(value.__dict__) + if isinstance(value, BaseExceptionGroup): + pending.extend(value.exceptions) + elif isinstance(value, dict): pending.extend(value.keys()) pending.extend(value.values()) elif isinstance(value, list | tuple | set | frozenset): @@ -589,6 +599,50 @@ async def test_call_tool_group_hides_sensitive_transport_error_context(): _assert_not_retained_in_traceback_locals(exc_info.value, sensitive_value) +@pytest.mark.asyncio +async def test_connect_group_checks_every_transport_error_attachment(): + server = MCPServerSse(params={"url": _SAFE_URL}) + error_group, _, later_error = _mixed_request_error_group(_SAFE_URL) + sensitive_value = ValueError(_CREDENTIALED_URL) + later_error.__context__ = sensitive_value + + with patch.object(server, "create_streams", side_effect=error_group): + with pytest.raises(UserError) as exc_info: + await server.connect() + + assert "Could not reach the server" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, error_group) + _assert_not_retained_in_exception_graph(exc_info.value, later_error) + _assert_not_retained_in_exception_graph(exc_info.value, sensitive_value) + _assert_not_retained_in_traceback_locals(exc_info.value, error_group) + _assert_not_retained_in_traceback_locals(exc_info.value, later_error) + _assert_not_retained_in_traceback_locals(exc_info.value, sensitive_value) + + +@pytest.mark.asyncio +async def test_call_tool_group_checks_every_transport_error_attachment(): + server = MCPServerStreamableHttp(params={"url": _SAFE_URL}) + server.session = MagicMock() + server.max_retry_attempts = 0 + error_group, _, later_error = _mixed_request_error_group(_SAFE_URL) + sensitive_value = ValueError(_CREDENTIALED_URL) + later_error.__context__ = sensitive_value + + with patch.object(server, "_call_tool_with_isolated_retry", side_effect=error_group): + with pytest.raises(UserError) as exc_info: + await server.call_tool("test_tool", {}) + + assert "Connection lost" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, error_group) + _assert_not_retained_in_exception_graph(exc_info.value, later_error) + _assert_not_retained_in_exception_graph(exc_info.value, sensitive_value) + _assert_not_retained_in_traceback_locals(exc_info.value, error_group) + _assert_not_retained_in_traceback_locals(exc_info.value, later_error) + _assert_not_retained_in_traceback_locals(exc_info.value, sensitive_value) + + @pytest.mark.asyncio async def test_connect_preserves_exception_group_when_every_request_error_is_safe(): server = MCPServerSse(params={"url": _SAFE_URL}) @@ -935,6 +989,32 @@ async def test_connect_preserves_original_error_when_cleanup_has_safe_generic_re assert server._get_session_id is None +@pytest.mark.asyncio +async def test_connect_cleanup_mapped_error_omits_pending_connection_failure(): + server = MCPServerSse(params={"url": _SAFE_URL}) + connection_error = ValueError(_CREDENTIALED_URL) + cleanup_error = httpx.ReadTimeout( + "cleanup timed out", + request=httpx.Request("GET", _SAFE_URL), + ) + + with ( + patch.object(server, "create_streams", side_effect=connection_error), + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), + ): + with pytest.raises(UserError) as exc_info: + await server.connect() + + assert "Connection timeout" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, connection_error) + _assert_not_retained_in_exception_graph(exc_info.value, cleanup_error) + _assert_not_retained_in_traceback_locals(exc_info.value, connection_error) + _assert_not_retained_in_traceback_locals(exc_info.value, cleanup_error) + assert server.session is None + assert server._get_session_id is None + + @pytest.mark.asyncio @pytest.mark.parametrize("grouped", [False, True]) async def test_normal_cleanup_hides_generic_request_error_context_from_log_record( @@ -962,8 +1042,12 @@ async def test_normal_cleanup_hides_generic_request_error_context_from_log_recor await server.cleanup() record = caplog.records[-1] - assert record.exc_info is None - assert record.exc_text is None + if grouped: + assert record.exc_info is not None + assert record.exc_info[1] is not cleanup_error + else: + assert record.exc_info is None + assert record.exc_text is None _assert_not_retained_in_log_record(record, cleanup_error) _assert_not_retained_in_log_record(record, request_error) _assert_not_retained_in_log_record(record, sensitive_value) @@ -972,6 +1056,45 @@ async def test_normal_cleanup_hides_generic_request_error_context_from_log_recor assert server._get_session_id is None +@pytest.mark.asyncio +@pytest.mark.parametrize("payload", ["message", "reason_phrase"]) +async def test_normal_cleanup_hides_transport_exception_payload_from_log_record( + monkeypatch, + caplog, + payload: str, +): + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) + server = MCPServerSse(params={"url": _SAFE_URL}) + server.session = MagicMock() + request = httpx.Request("GET", _SAFE_URL) + if payload == "message": + cleanup_error: Exception = httpx.ReadTimeout(_CREDENTIALED_URL, request=request) + else: + cleanup_error = httpx.HTTPStatusError( + "cleanup failed", + request=request, + response=httpx.Response( + 502, + request=request, + extensions={"reason_phrase": _CREDENTIALED_URL.encode()}, + ), + ) + + with ( + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), + caplog.at_level(logging.WARNING, logger="openai.agents"), + ): + await server.cleanup() + + record = caplog.records[-1] + assert record.exc_info is None + assert record.exc_text is None + _assert_not_retained_in_log_record(record, cleanup_error) + _assert_url_credentials_hidden_from_log_record(record) + assert server.session is None + assert server._get_session_id is None + + @pytest.mark.asyncio @pytest.mark.parametrize("attachment", ["http_context", "non_http_context", "note"]) async def test_failed_connection_cleanup_hides_sensitive_exception_attachments( @@ -1102,19 +1225,16 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions( await server.cleanup() record = caplog.records[-1] - if not redacted and safe_to_attach: + if not redacted and safe_to_attach and exception_shape == "nested_group": assert record.exc_info is not None - if exception_shape == "nested_group": - assert record.levelno == logging.ERROR - assert record.exc_info[1] is cleanup_error - else: - assert record.levelno == logging.WARNING - assert record.exc_info[1] is timeout_error + assert record.levelno == logging.ERROR + assert record.exc_info[1] is not cleanup_error else: assert record.exc_info is None assert record.exc_text is None - _assert_not_retained_in_log_record(record, cleanup_error) - _assert_not_retained_in_log_record(record, timeout_error) + + _assert_not_retained_in_log_record(record, cleanup_error) + _assert_not_retained_in_log_record(record, timeout_error) if not safe_to_attach: _assert_url_credentials_hidden_from_log_record(record) @@ -1154,7 +1274,7 @@ async def test_normal_cleanup_hides_sensitive_exception_attachments_from_log_rec @pytest.mark.asyncio -async def test_normal_cleanup_preserves_safe_nested_group_diagnostics(monkeypatch, caplog): +async def test_normal_cleanup_sanitizes_safe_nested_group_diagnostics(monkeypatch, caplog): monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) server = MCPServerSse(params={"url": _SAFE_URL}) server.session = MagicMock() @@ -1162,10 +1282,11 @@ async def test_normal_cleanup_preserves_safe_nested_group_diagnostics(monkeypatc "timed out", request=httpx.Request("GET", _SAFE_URL), ) + ordinary_error = ValueError("ordinary sibling failure") cleanup_group = BaseExceptionGroup( "cleanup failed", [ - ValueError("ordinary sibling failure"), + ordinary_error, BaseExceptionGroup("nested cleanup failed", [timeout_error]), ], ) @@ -1179,8 +1300,13 @@ async def test_normal_cleanup_preserves_safe_nested_group_diagnostics(monkeypatc record = caplog.records[-1] assert record.levelno == logging.ERROR assert record.exc_info is not None - assert record.exc_info[1] is cleanup_group - assert "ordinary sibling failure" in logging.Formatter().format(record) + assert record.exc_info[1] is not cleanup_group + rendered = logging.Formatter().format(record) + assert "An additional error occurred during the MCP request." in rendered + assert "ordinary sibling failure" not in rendered + _assert_not_retained_in_log_record(record, cleanup_group) + _assert_not_retained_in_log_record(record, ordinary_error) + _assert_not_retained_in_log_record(record, timeout_error) assert server.session is None assert server._get_session_id is None