From d9cd560f220c9b9e94caa4450227e758f5733d12 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 4 May 2026 07:31:14 -0400 Subject: [PATCH 1/2] test(server): regression test for context-echo on dispatcher-wrapped non-AdcpError (closes #562) PR #560 added context-echo on the AdcpError raise path. This test verifies the implicitly-wrapped path: a raw ValueError from a DecisioningPlatform method goes through _invoke_platform_method's except-Exception clause which wraps to AdcpError(INTERNAL_ERROR), which then projects through serve.py's catch + build_mcp_error_result with params=kwargs and echoes the request context. If a future refactor lets a non-AdcpError exception escape the dispatch wrap, this test fails first. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/test_mcp_structured_error.py | 80 ++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/tests/test_mcp_structured_error.py b/tests/test_mcp_structured_error.py index 782b4625..12ea1c1f 100644 --- a/tests/test_mcp_structured_error.py +++ b/tests/test_mcp_structured_error.py @@ -343,6 +343,86 @@ async def caller(_kwargs: dict[str, Any], *, context: Any = None) -> Any: assert result.structuredContent.get("context") == request_context +@pytest.mark.asyncio +async def test_non_adcp_error_via_dispatch_wrap_still_echoes_context(): + """Issue #562: verify the dispatcher's wrap path preserves + context echo on the error envelope. + + ``_invoke_platform_method`` (decisioning/dispatch.py) catches every + non-:class:`AdcpError` exception from a platform method and wraps + it to ``AdcpError("INTERNAL_ERROR")``. Once wrapped, the AdcpError + travels up to ``serve.py``'s ``except Exception``/decisioning + branch and projects to the structured envelope via + :func:`build_mcp_error_result`, which echoes ``context`` from the + request kwargs. + + Pre-PR-#560 the error envelope dropped ``context``; #560 fixed + the AdcpError raise path; this test pins that the + *implicitly-wrapped* path (raw ``ValueError`` → wrap → + AdcpError → envelope) still carries context. + + If a future refactor lets a non-AdcpError exception escape + ``_invoke_platform_method``'s wrap (e.g., an early-return path + that skips the catch), this test fails first — adopters lose + correlation IDs across the boundary and we should know. + """ + from mcp.server.fastmcp import FastMCP + + from adcp.server.serve import _register_tool + + async def caller(_kwargs: dict[str, Any], *, context: Any = None) -> Any: + # Exercise the real dispatch wrap path: ``_invoke_platform_method`` + # catches every non-AdcpError and wraps to AdcpError("INTERNAL_ERROR"). + # The wrapped error then propagates to ``serve.py``'s decisioning + # branch which builds the error envelope via ``build_mcp_error_result``. + from concurrent.futures import ThreadPoolExecutor + + from pydantic import BaseModel + + from adcp.decisioning.dispatch import _build_request_context, _invoke_platform_method + from adcp.decisioning.task_registry import InMemoryTaskRegistry + from adcp.decisioning.types import Account + from adcp.server.base import ToolContext + + class _CrashingPlatform: + async def get_products(self, params, ctx): + raise ValueError("oops, internal-state bug") + + class _Req(BaseModel): + pass + + executor = ThreadPoolExecutor(max_workers=1) + try: + ctx_obj = _build_request_context( + ToolContext(), + Account(id="acct-1"), + None, + ) + return await _invoke_platform_method( + _CrashingPlatform(), + "get_products", + _Req(), + ctx_obj, + executor=executor, + registry=InMemoryTaskRegistry(), + ) + finally: + executor.shutdown(wait=False) + + mcp = FastMCP("test-562-dispatch-wrap") + _register_tool(mcp, "get_products", "test", {"type": "object"}, caller) + + request_context = {"correlation_id": "buyer-req-562"} + result = await mcp.call_tool("get_products", {"context": request_context}) + + assert isinstance(result, CallToolResult) + assert result.isError is True + # The dispatcher wrapped the ValueError → AdcpError(INTERNAL_ERROR). + assert result.structuredContent["adcp_error"]["code"] == "INTERNAL_ERROR" + # And the request context survived all the way to the wire envelope. + assert result.structuredContent.get("context") == request_context + + @pytest.mark.asyncio async def test_success_path_unchanged(): """Regression: success-path responses still validate against the From 90012f1833e965dd880264b19259b2e3e505d3b4 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 4 May 2026 07:47:09 -0400 Subject: [PATCH 2/2] test: strengthen #562 regression assertion to pin the wrap step Code-reviewer caught that the original assertion (INTERNAL_ERROR + context echoed) would pass even if the dispatcher wrap step were skipped. Adds details.caused_by.type == 'ValueError' assertion which is set only by _internal_error_details inside the wrap path. Other review nits: executor.shutdown(wait=True) for cleanup; trim PR-#560 history from docstring per CLAUDE.md; rename for symmetry. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/test_mcp_structured_error.py | 50 +++++++++++++----------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/tests/test_mcp_structured_error.py b/tests/test_mcp_structured_error.py index 12ea1c1f..3f45752a 100644 --- a/tests/test_mcp_structured_error.py +++ b/tests/test_mcp_structured_error.py @@ -344,37 +344,30 @@ async def caller(_kwargs: dict[str, Any], *, context: Any = None) -> Any: @pytest.mark.asyncio -async def test_non_adcp_error_via_dispatch_wrap_still_echoes_context(): - """Issue #562: verify the dispatcher's wrap path preserves - context echo on the error envelope. - - ``_invoke_platform_method`` (decisioning/dispatch.py) catches every - non-:class:`AdcpError` exception from a platform method and wraps - it to ``AdcpError("INTERNAL_ERROR")``. Once wrapped, the AdcpError - travels up to ``serve.py``'s ``except Exception``/decisioning - branch and projects to the structured envelope via - :func:`build_mcp_error_result`, which echoes ``context`` from the - request kwargs. - - Pre-PR-#560 the error envelope dropped ``context``; #560 fixed - the AdcpError raise path; this test pins that the - *implicitly-wrapped* path (raw ``ValueError`` → wrap → - AdcpError → envelope) still carries context. - - If a future refactor lets a non-AdcpError exception escape - ``_invoke_platform_method``'s wrap (e.g., an early-return path - that skips the catch), this test fails first — adopters lose - correlation IDs across the boundary and we should know. +async def test_dispatcher_wrap_to_internal_error_preserves_context_echo(): + """Pin the chain: a non-AdcpError raised from a DecisioningPlatform + method is wrapped to ``AdcpError("INTERNAL_ERROR")`` by + ``_invoke_platform_method``, then projected through ``serve.py``'s + decisioning branch via ``build_mcp_error_result``, with the + request's ``context`` field echoed onto the wire envelope. + + The test asserts both halves: + 1. The wrap actually ran — ``details.caused_by`` carries the + original :class:`ValueError` class name (set by + ``_internal_error_details``). + 2. The request context survived the wrap and lands as a sibling + of ``adcp_error`` in ``structuredContent``. + + Without (1) the test would pass even if the wrap step were + skipped — we'd be re-asserting #560's coverage of an explicit + AdcpError raise. The ``caused_by`` check pins the wrap path + specifically. """ from mcp.server.fastmcp import FastMCP from adcp.server.serve import _register_tool async def caller(_kwargs: dict[str, Any], *, context: Any = None) -> Any: - # Exercise the real dispatch wrap path: ``_invoke_platform_method`` - # catches every non-AdcpError and wraps to AdcpError("INTERNAL_ERROR"). - # The wrapped error then propagates to ``serve.py``'s decisioning - # branch which builds the error envelope via ``build_mcp_error_result``. from concurrent.futures import ThreadPoolExecutor from pydantic import BaseModel @@ -407,7 +400,7 @@ class _Req(BaseModel): registry=InMemoryTaskRegistry(), ) finally: - executor.shutdown(wait=False) + executor.shutdown(wait=True) mcp = FastMCP("test-562-dispatch-wrap") _register_tool(mcp, "get_products", "test", {"type": "object"}, caller) @@ -417,9 +410,10 @@ class _Req(BaseModel): assert isinstance(result, CallToolResult) assert result.isError is True - # The dispatcher wrapped the ValueError → AdcpError(INTERNAL_ERROR). + # (1) The wrap ran — INTERNAL_ERROR with caused_by = ValueError. assert result.structuredContent["adcp_error"]["code"] == "INTERNAL_ERROR" - # And the request context survived all the way to the wire envelope. + assert result.structuredContent["adcp_error"]["details"]["caused_by"]["type"] == "ValueError" + # (2) Context echoed end-to-end. assert result.structuredContent.get("context") == request_context