From 858b4fdb0dd311786196589f9869fa06bc3ae61f Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 1 May 2026 09:55:02 -0400 Subject: [PATCH 1/2] fix(decisioning): wire-path dispatch + F12 silent no-op (Emma sales-direct P0s) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #337 merged 31 handler shims that closed the AudioStack 404 issue but Emma's sales-direct backend test (verdict 2/10) surfaced three P0s downstream of the shim layer: every MCP ``tools/call`` 500'd with ``'dict' object has no attribute 'account'``, and buyer-registered push_notification_config.urls were silently dropped when the adopter didn't wire ``webhook_sender``. Unit tests passed because they bypass ``create_tool_caller`` and call shim methods directly. handler.py — moved every ``adcp.types`` Request/Response import out of ``if TYPE_CHECKING:`` so ``typing.get_type_hints()`` resolves the forward refs at runtime. Without this, the dispatcher's typed-params resolver raises ``NameError``, falls back to dict dispatch, and the shim's ``params.account`` access crashes. webhook_emit.py — warn when ``sender=None`` but the buyer registered ``push_notification_config.url``. Adopters who skip ``webhook_sender`` in ``serve()`` now see a WARNING citing the buyer's URL on first call instead of having buyers complain that their notifications never arrive. mcp_tools.py — bumped ``_resolve_params_pydantic_model`` fallback log from DEBUG to WARNING with explicit remediation guidance (import the model at module scope vs. declare ``params: dict``). Silent dispatch fallback is what hid the wire-dispatch break for two PRs. tests/test_decisioning_wire_dispatch.py (new, 29 tests) — pins: - ``typing.get_type_hints`` resolves on every shim - ``_resolve_params_pydantic_model`` returns the typed Pydantic class - ``create_tool_caller`` round-trips a wire-shape dict through the full dispatch path without the params.account crash Co-Authored-By: Claude Opus 4.7 (1M context) --- src/adcp/decisioning/handler.py | 182 +++++++++++----------- src/adcp/decisioning/webhook_emit.py | 36 ++++- src/adcp/server/mcp_tools.py | 22 ++- tests/test_decisioning_webhook_emit.py | 39 ++++- tests/test_decisioning_wire_dispatch.py | 192 ++++++++++++++++++++++++ 5 files changed, 371 insertions(+), 100 deletions(-) create mode 100644 tests/test_decisioning_wire_dispatch.py diff --git a/src/adcp/decisioning/handler.py b/src/adcp/decisioning/handler.py index d0c7051ee..bc082318a 100644 --- a/src/adcp/decisioning/handler.py +++ b/src/adcp/decisioning/handler.py @@ -42,6 +42,103 @@ from adcp.decisioning.webhook_emit import maybe_emit_sync_completion from adcp.server.base import ADCPHandler, ToolContext +# Pydantic Request/Response types are imported at module scope (NOT +# under TYPE_CHECKING) so that ``typing.get_type_hints(method)`` can +# resolve every shim's ``params`` annotation at runtime. The dispatcher +# at ``adcp.server.mcp_tools._resolve_params_pydantic_model`` walks +# these hints to deserialise wire-shape dicts into the typed Pydantic +# models the shims expect; without runtime visibility, ``get_type_hints`` +# raises ``NameError`` on the forward refs (the file uses +# ``from __future__ import annotations``), the resolver swallows the +# exception, and the dispatcher falls back to the dict path — which +# crashes inside the shim with ``'dict' object has no attribute +# 'account'`` (Emma sales-direct backend test, verdict 2/10). +from adcp.types import ( + AccountReference, + AcquireRightsRequest, + AcquireRightsResponse, + ActivateSignalRequest, + ActivateSignalSuccessResponse, + BuildCreativeRequest, + BuildCreativeResponse, + CalibrateContentRequest, + CalibrateContentResponse, + CheckGovernanceRequest, + CheckGovernanceResponse, + CreateCollectionListRequest, + CreateCollectionListResponse, + CreateContentStandardsRequest, + CreateContentStandardsResponse, + CreateMediaBuyRequest, + CreateMediaBuySuccessResponse, + CreatePropertyListRequest, + CreatePropertyListResponse, + DeleteCollectionListRequest, + DeleteCollectionListResponse, + DeletePropertyListRequest, + DeletePropertyListResponse, + GetBrandIdentityRequest, + GetBrandIdentitySuccessResponse, + GetCollectionListRequest, + GetCollectionListResponse, + GetContentStandardsRequest, + GetContentStandardsResponse, + GetCreativeDeliveryRequest, + GetCreativeDeliveryResponse, + GetCreativeFeaturesRequest, + GetCreativeFeaturesResponse, + GetMediaBuyArtifactsRequest, + GetMediaBuyArtifactsResponse, + GetMediaBuyDeliveryRequest, + GetMediaBuyDeliveryResponse, + GetMediaBuysRequest, + GetMediaBuysResponse, + GetPlanAuditLogsRequest, + GetPlanAuditLogsResponse, + GetProductsRequest, + GetProductsResponse, + GetPropertyListRequest, + GetPropertyListResponse, + GetRightsRequest, + GetRightsSuccessResponse, + GetSignalsRequest, + GetSignalsResponse, + ListCollectionListsRequest, + ListCollectionListsResponse, + ListContentStandardsRequest, + ListContentStandardsResponse, + ListCreativeFormatsRequest, + ListCreativeFormatsResponse, + ListCreativesRequest, + ListCreativesResponse, + ListPropertyListsRequest, + ListPropertyListsResponse, + PreviewCreativeRequest, + PreviewCreativeResponse, + ProvidePerformanceFeedbackRequest, + ProvidePerformanceFeedbackResponse, + ReportPlanOutcomeRequest, + ReportPlanOutcomeResponse, + SyncAudiencesRequest, + SyncAudiencesSuccessResponse, + SyncCreativesRequest, + SyncCreativesSuccessResponse, + SyncPlansRequest, + SyncPlansResponse, + UpdateCollectionListRequest, + UpdateCollectionListResponse, + UpdateContentStandardsRequest, + UpdateContentStandardsResponse, + UpdateMediaBuyRequest, + UpdateMediaBuySuccessResponse, + UpdatePropertyListRequest, + UpdatePropertyListResponse, + UpdateRightsRequest, + UpdateRightsResponse, + ValidateContentDeliveryRequest, + ValidateContentDeliveryResponse, +) + if TYPE_CHECKING: from concurrent.futures import ThreadPoolExecutor @@ -50,91 +147,6 @@ from adcp.decisioning.state import StateReader from adcp.decisioning.task_registry import TaskRegistry from adcp.decisioning.types import Account - from adcp.types import ( - AccountReference, - AcquireRightsRequest, - AcquireRightsResponse, - ActivateSignalRequest, - ActivateSignalSuccessResponse, - BuildCreativeRequest, - BuildCreativeResponse, - CalibrateContentRequest, - CalibrateContentResponse, - CheckGovernanceRequest, - CheckGovernanceResponse, - CreateCollectionListRequest, - CreateCollectionListResponse, - CreateContentStandardsRequest, - CreateContentStandardsResponse, - CreateMediaBuyRequest, - CreateMediaBuySuccessResponse, - CreatePropertyListRequest, - CreatePropertyListResponse, - DeleteCollectionListRequest, - DeleteCollectionListResponse, - DeletePropertyListRequest, - DeletePropertyListResponse, - GetBrandIdentityRequest, - GetBrandIdentitySuccessResponse, - GetCollectionListRequest, - GetCollectionListResponse, - GetContentStandardsRequest, - GetContentStandardsResponse, - GetCreativeDeliveryRequest, - GetCreativeDeliveryResponse, - GetCreativeFeaturesRequest, - GetCreativeFeaturesResponse, - GetMediaBuyArtifactsRequest, - GetMediaBuyArtifactsResponse, - GetMediaBuyDeliveryRequest, - GetMediaBuyDeliveryResponse, - GetMediaBuysRequest, - GetMediaBuysResponse, - GetPlanAuditLogsRequest, - GetPlanAuditLogsResponse, - GetProductsRequest, - GetProductsResponse, - GetPropertyListRequest, - GetPropertyListResponse, - GetRightsRequest, - GetRightsSuccessResponse, - GetSignalsRequest, - GetSignalsResponse, - ListCollectionListsRequest, - ListCollectionListsResponse, - ListContentStandardsRequest, - ListContentStandardsResponse, - ListCreativeFormatsRequest, - ListCreativeFormatsResponse, - ListCreativesRequest, - ListCreativesResponse, - ListPropertyListsRequest, - ListPropertyListsResponse, - PreviewCreativeRequest, - PreviewCreativeResponse, - ProvidePerformanceFeedbackRequest, - ProvidePerformanceFeedbackResponse, - ReportPlanOutcomeRequest, - ReportPlanOutcomeResponse, - SyncAudiencesRequest, - SyncAudiencesSuccessResponse, - SyncCreativesRequest, - SyncCreativesSuccessResponse, - SyncPlansRequest, - SyncPlansResponse, - UpdateCollectionListRequest, - UpdateCollectionListResponse, - UpdateContentStandardsRequest, - UpdateContentStandardsResponse, - UpdateMediaBuyRequest, - UpdateMediaBuySuccessResponse, - UpdatePropertyListRequest, - UpdatePropertyListResponse, - UpdateRightsRequest, - UpdateRightsResponse, - ValidateContentDeliveryRequest, - ValidateContentDeliveryResponse, - ) from adcp.webhook_sender import WebhookSender diff --git a/src/adcp/decisioning/webhook_emit.py b/src/adcp/decisioning/webhook_emit.py index 3cedda76f..8efd54b2b 100644 --- a/src/adcp/decisioning/webhook_emit.py +++ b/src/adcp/decisioning/webhook_emit.py @@ -176,11 +176,18 @@ def maybe_emit_sync_completion( Skips silently when: * ``enabled`` is False (operator opted out). - * ``sender`` is None (no emitter wired). * The request didn't carry ``push_notification_config.url``. - * ``method_name`` isn't in :data:`SPEC_WEBHOOK_TASK_TYPES` (logged - as a warning so adopters notice they extended the tool surface - beyond the spec enum). + + Logs a WARNING when: + + * ``sender`` is None but the buyer DID register + ``push_notification_config.url`` — the buyer's notification + registration is being silently dropped, which the adopter + almost certainly didn't intend. Wire ``webhook_sender`` into + :func:`adcp.decisioning.serve` or pass + ``auto_emit_completion_webhooks=False`` to silence this. + * ``method_name`` isn't in :data:`SPEC_WEBHOOK_TASK_TYPES` (the + adopter extended the tool surface beyond the spec enum). Schedules the actual delivery via the running event loop's ``create_task`` so the sync response path is non-blocking. @@ -193,12 +200,31 @@ def maybe_emit_sync_completion( logged-and-swallowed. """ try: - if not enabled or sender is None: + if not enabled: return extracted = _extract_push_notification_url_and_token(params) if extracted is None: return url, token = extracted + if sender is None: + # Buyer registered a webhook URL but the adopter didn't + # wire a sender. Without this branch, the buyer's + # notification quietly disappears — they think they + # registered for completion webhooks and just never + # receive any. Surfacing a warning on first call gives + # the adopter a fast path to the misconfig. + logger.warning( + "[adcp.decisioning] buyer registered " + "push_notification_config.url=%s for %s but auto-emit " + "webhook_sender is None — webhook silently dropped. " + "Pass webhook_sender to " + "adcp.decisioning.serve.create_adcp_server_from_platform, " + "or set auto_emit_completion_webhooks=False to silence " + "this warning.", + url, + method_name, + ) + return if method_name not in SPEC_WEBHOOK_TASK_TYPES: logger.warning( "[adcp.decisioning] sync completion webhook for %s skipped — " diff --git a/src/adcp/server/mcp_tools.py b/src/adcp/server/mcp_tools.py index 8f4ef348b..ae7900179 100644 --- a/src/adcp/server/mcp_tools.py +++ b/src/adcp/server/mcp_tools.py @@ -1563,13 +1563,23 @@ class when the annotation is: try: hints = typing.get_type_hints(method) except Exception as exc: # forward-ref failure, missing import, etc. - # Log at debug so an author whose typed annotation silently - # failed to resolve (typo in the class name, import not at - # module top-level, PEP 563 name bound in a local scope) can - # find out why their handler is dispatching via the dict path. - logger.debug( + # WARNING (not debug): silent dict-path fallback was the root + # cause of the wire-dispatch regression Emma's sales-direct + # backend test surfaced (handler.py kept Request types under + # ``if TYPE_CHECKING:`` so PEP 563 forward refs couldn't resolve + # at runtime; resolver returned None; shims crashed on + # ``params.account``). Author's choice: declare ``params: dict`` + # for the dict path, or ensure the typed annotation's class is + # importable at the method's module scope. A debug log is too + # quiet for a footgun this expensive — bumping to WARNING ensures + # adopters see it on first server boot. + logger.warning( "typed params annotation failed to resolve for %r: %s; " - "falling back to dict dispatch", + "falling back to dict dispatch. If this method declares " + "``params: ``, import that model at the " + "method's module scope (not under ``TYPE_CHECKING``); " + "otherwise declare ``params: dict[str, Any]`` to silence " + "this warning.", method, exc, ) diff --git a/tests/test_decisioning_webhook_emit.py b/tests/test_decisioning_webhook_emit.py index 8f5a04e17..aab0bd7d5 100644 --- a/tests/test_decisioning_webhook_emit.py +++ b/tests/test_decisioning_webhook_emit.py @@ -148,8 +148,16 @@ class _Params: @pytest.mark.asyncio -async def test_maybe_emit_skips_when_sender_none() -> None: - """``sender=None`` → silent skip (no emitter wired).""" +async def test_maybe_emit_warns_when_sender_none_but_buyer_registered_url( + caplog: pytest.LogCaptureFixture, +) -> None: + """``sender=None`` while buyer DID register + ``push_notification_config.url`` → log a WARNING. Adopters often + ship without wiring ``webhook_sender`` and only discover the + misconfig when buyers complain about missing notifications. The + warning surfaces this on first call. Regression for Emma + sales-direct backend test (verdict 2/10) — the prior silent-skip + branch hid the gap entirely.""" class _Config: url = "https://buyer.example.com/wh" @@ -158,12 +166,35 @@ class _Config: class _Params: push_notification_config = _Config() - # Smoke — must not raise. + with caplog.at_level("WARNING", logger="adcp.decisioning.webhook_emit"): + maybe_emit_sync_completion( + sender=None, + enabled=True, + method_name="create_media_buy", + params=_Params(), + result={"media_buy_id": "mb_1"}, + ) + messages = [r.message for r in caplog.records] + assert any( + "webhook_sender is None" in m and "buyer.example.com/wh" in m for m in messages + ), f"expected sender-None warning citing the buyer URL; got {messages}" + + +@pytest.mark.asyncio +async def test_maybe_emit_skips_silently_when_sender_none_and_no_url() -> None: + """``sender=None`` AND no ``push_notification_config.url`` → silent + skip. Buyers who don't register webhooks aren't a misconfig — no + point warning.""" + + class _Bare: + pass + + # Smoke — must not raise, must not warn (no caplog capture). maybe_emit_sync_completion( sender=None, enabled=True, method_name="create_media_buy", - params=_Params(), + params=_Bare(), result={"media_buy_id": "mb_1"}, ) diff --git a/tests/test_decisioning_wire_dispatch.py b/tests/test_decisioning_wire_dispatch.py new file mode 100644 index 000000000..05b1dc208 --- /dev/null +++ b/tests/test_decisioning_wire_dispatch.py @@ -0,0 +1,192 @@ +"""Wire-path dispatch tests for the PlatformHandler shims. + +Regression for the Emma sales-direct backend test (verdict 2/10): every +wire ``tools/call`` failed with ``'dict' object has no attribute +'account'``. Root cause was a layering bug — ``handler.py`` imported the +Pydantic Request types only inside ``if TYPE_CHECKING:`` while the +dispatcher's :func:`_resolve_params_pydantic_model` calls +``typing.get_type_hints(method)`` at runtime. The forward-ref names +weren't in the module's globals, ``get_type_hints`` raised +``NameError``, the resolver swallowed it (debug log only), and the +dispatcher fell back to the dict path. Handler shims then did +``params.account`` on a dict and 500'd. + +Unit tests in ``test_decisioning_handler_shims.py`` passed because they +call shim methods directly, bypassing ``create_tool_caller`` and the +resolver. This file pins the wire path so a future regression can't +slip through. +""" + +from __future__ import annotations + +import typing +from concurrent.futures import ThreadPoolExecutor + +import pytest + +from adcp.decisioning import ( + DecisioningCapabilities, + DecisioningPlatform, + InMemoryTaskRegistry, + SingletonAccounts, +) +from adcp.decisioning.handler import PlatformHandler +from adcp.server.mcp_tools import _resolve_params_pydantic_model, create_tool_caller + + +@pytest.fixture +def executor(): + pool = ThreadPoolExecutor(max_workers=4, thread_name_prefix="wire-dispatch-") + yield pool + pool.shutdown(wait=True) + + +# ---- Direct unit-level repro ---- + + +@pytest.mark.parametrize( + "tool_name", + [ + "get_products", + "create_media_buy", + "update_media_buy", + "sync_creatives", + "get_media_buy_delivery", + "build_creative", + "get_signals", + "activate_signal", + "sync_audiences", + "check_governance", + "get_brand_identity", + "list_content_standards", + "create_property_list", + "create_collection_list", + ], +) +def test_get_type_hints_resolves_for_every_shim(tool_name: str) -> None: + """``typing.get_type_hints`` must succeed on every shim. Without + this, the dispatcher's typed-params resolver silently falls back to + the dict path and the shim's ``params.account`` access blows up at + runtime with ``'dict' object has no attribute 'account'``.""" + method = getattr(PlatformHandler, tool_name) + hints = typing.get_type_hints(method) # MUST NOT raise + assert "params" in hints, f"{tool_name} missing 'params' annotation" + + +@pytest.mark.parametrize( + "tool_name,expected_request", + [ + ("get_products", "GetProductsRequest"), + ("create_media_buy", "CreateMediaBuyRequest"), + ("update_media_buy", "UpdateMediaBuyRequest"), + ("sync_creatives", "SyncCreativesRequest"), + ("get_media_buy_delivery", "GetMediaBuyDeliveryRequest"), + ("build_creative", "BuildCreativeRequest"), + ("get_signals", "GetSignalsRequest"), + ("sync_audiences", "SyncAudiencesRequest"), + ("check_governance", "CheckGovernanceRequest"), + ("get_brand_identity", "GetBrandIdentityRequest"), + ("list_content_standards", "ListContentStandardsRequest"), + ("create_property_list", "CreatePropertyListRequest"), + ("create_collection_list", "CreateCollectionListRequest"), + ], +) +def test_resolver_returns_typed_request_class_not_none( + tool_name: str, expected_request: str +) -> None: + """The dispatcher's resolver must return the Pydantic request class, + NOT ``None``. ``None`` triggers the dict-fallback dispatch path that + causes the wire-side 500.""" + method = getattr(PlatformHandler, tool_name) + resolved = _resolve_params_pydantic_model(method) + assert resolved is not None, ( + f"_resolve_params_pydantic_model returned None for {tool_name} — " + "dispatcher will fall back to dict path and shim will crash on " + "params.account" + ) + assert ( + resolved.__name__ == expected_request + ), f"{tool_name}: expected {expected_request}, got {resolved.__name__}" + + +# ---- End-to-end wire path ---- + + +class _SalesDirectStub(DecisioningPlatform): + """Minimal sales-direct platform — covers every required SalesAgent + method with believable in-memory fixtures.""" + + capabilities = DecisioningCapabilities(specialisms=["sales-direct"]) + accounts = SingletonAccounts(account_id="emma-test") + + def __init__(self) -> None: + super().__init__() + self.calls: list[tuple[str, object]] = [] + + def get_products(self, req, ctx): + self.calls.append(("get_products", req)) + return {"products": [{"product_id": "p1", "name": "stub"}]} + + def create_media_buy(self, req, ctx): + self.calls.append(("create_media_buy", req)) + return {"media_buy_id": "mb_1", "status": "active"} + + def update_media_buy(self, media_buy_id, patch, ctx): + self.calls.append(("update_media_buy", (media_buy_id, patch))) + return {"media_buy_id": media_buy_id, "status": "active"} + + def sync_creatives(self, req, ctx): + self.calls.append(("sync_creatives", req)) + return {"creatives": []} + + def get_media_buy_delivery(self, req, ctx): + self.calls.append(("get_media_buy_delivery", req)) + return {"media_buy_deliveries": []} + + +@pytest.mark.asyncio +async def test_wire_dispatch_get_products_does_not_crash(executor) -> None: + """End-to-end: ``create_tool_caller`` wrapping ``PlatformHandler`` + must dispatch a wire-shape dict payload to the platform method + without crashing on ``params.account``. This is the exact failure + mode Emma's sales-direct backend test surfaced.""" + platform = _SalesDirectStub() + handler = PlatformHandler( + platform, + executor=executor, + registry=InMemoryTaskRegistry(), + ) + caller = create_tool_caller(handler, "get_products") + wire_payload = { + "brief": "test campaign", + "promoted_offering": "shoes", + "buying_mode": "brief", + } + result = await caller(wire_payload) + assert platform.calls and platform.calls[0][0] == "get_products" + assert "products" in result + + +@pytest.mark.asyncio +async def test_wire_dispatch_non_sales_tool_does_not_crash(executor) -> None: + """Non-sales tool (PR #337's surface): same wire path, same bug. + Covers the breadth-sprint Protocol families that are most exposed + by this regression.""" + + class _CreativeBuilder(DecisioningPlatform): + capabilities = DecisioningCapabilities(specialisms=["creative-generative"]) + accounts = SingletonAccounts(account_id="emma-test") + + def build_creative(self, req, ctx): + return {"creative_manifest": {"creative_id": "cr_1"}} + + handler = PlatformHandler( + _CreativeBuilder(), + executor=executor, + registry=InMemoryTaskRegistry(), + ) + caller = create_tool_caller(handler, "build_creative") + result = await caller( + {"brief": "synthesize a 30s spot", "idempotency_key": "emma-test-build-creative-001"} + ) + assert "creative_manifest" in result From 11fcc2c44837c2b8c2b9b3072fbf49c9ba3f683e Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 1 May 2026 10:11:15 -0400 Subject: [PATCH 2/2] fix(decisioning): expert-review P1s on PR #338 wire-dispatch fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three reviewers (code-reviewer, python-expert, adtech-product-expert) converged on five P1/P2 fixes before merge. Wire-dispatch test: - Parametrize over ``PlatformHandler.advertised_tools`` so any new shim auto-extends regression coverage. Was 14 hardcoded tools (28 cases); now 40 (80 cases). - Pin the typed-dispatch contract end-to-end: assert the platform method actually receives a Pydantic ``GetProductsRequest`` / ``BuildCreativeRequest``, not a raw dict. - Drop hardcoded class-name assertions in favor of ``issubclass(resolved, BaseModel)``. Resolver warning (mcp_tools.py): - Surface the failing class name from ``NameError.name`` directly so adopters don't have to parse the bound-method repr. - Trim historical reference per CLAUDE.md. - Forward-compat note for PEP 749 (3.14 lazy annotations). F12 webhook silent-drop (webhook_emit.py): - Move the ``sender=None`` warning ABOVE the full URL/token extraction. A weird ``params`` shape that makes ``_extract_push_notification_url_and_token`` raise mid-traversal would otherwise re-introduce the silent-drop the previous fix was supposed to eliminate. Cheap pre-check on ``getattr(params, "push_notification_config", None)`` decides whether to warn; URL extraction is best-effort for log context. Test count: 2832 passed (was 2777 — +55 from wider parametrize coverage). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/adcp/decisioning/webhook_emit.py | 40 ++++++++-- src/adcp/server/mcp_tools.py | 44 ++++++----- tests/test_decisioning_wire_dispatch.py | 101 +++++++++++++----------- 3 files changed, 114 insertions(+), 71 deletions(-) diff --git a/src/adcp/decisioning/webhook_emit.py b/src/adcp/decisioning/webhook_emit.py index 8efd54b2b..ba5f011c9 100644 --- a/src/adcp/decisioning/webhook_emit.py +++ b/src/adcp/decisioning/webhook_emit.py @@ -202,29 +202,55 @@ def maybe_emit_sync_completion( try: if not enabled: return - extracted = _extract_push_notification_url_and_token(params) - if extracted is None: - return - url, token = extracted + + # Cheap pre-check: did the buyer register ANY + # ``push_notification_config``? Done BEFORE the full + # extraction so the sender=None warning fires even on weird + # ``params`` shapes that would have made + # ``_extract_push_notification_url_and_token`` raise. The + # outer ``try/except Exception`` would otherwise swallow such + # extraction errors and we'd reproduce the very silent-drop + # behavior this gate is supposed to eliminate. + config = getattr(params, "push_notification_config", None) + if config is None and isinstance(params, dict): + config = params.get("push_notification_config") + if config is None: + return # buyer didn't register — nothing to do + if sender is None: - # Buyer registered a webhook URL but the adopter didn't + # Buyer registered a webhook config but the adopter didn't # wire a sender. Without this branch, the buyer's # notification quietly disappears — they think they # registered for completion webhooks and just never # receive any. Surfacing a warning on first call gives # the adopter a fast path to the misconfig. + # + # Try to surface the URL for actionable error context; + # fall back to the config repr when extraction raises + # mid-traversal (still better than silent skip). + try: + url_for_log = getattr(config, "url", None) + if url_for_log is None and isinstance(config, dict): + url_for_log = config.get("url") + except Exception: + url_for_log = None logger.warning( "[adcp.decisioning] buyer registered " - "push_notification_config.url=%s for %s but auto-emit " + "push_notification_config (url=%s) for %s but auto-emit " "webhook_sender is None — webhook silently dropped. " "Pass webhook_sender to " "adcp.decisioning.serve.create_adcp_server_from_platform, " "or set auto_emit_completion_webhooks=False to silence " "this warning.", - url, + url_for_log if url_for_log else "", method_name, ) return + + extracted = _extract_push_notification_url_and_token(params) + if extracted is None: + return + url, token = extracted if method_name not in SPEC_WEBHOOK_TASK_TYPES: logger.warning( "[adcp.decisioning] sync completion webhook for %s skipped — " diff --git a/src/adcp/server/mcp_tools.py b/src/adcp/server/mcp_tools.py index ae7900179..203c91e36 100644 --- a/src/adcp/server/mcp_tools.py +++ b/src/adcp/server/mcp_tools.py @@ -1552,8 +1552,12 @@ class when the annotation is: references that fail to resolve — the dispatcher then falls back to the legacy dict path. - Cached per method object via the returned value being computed once - at ``create_tool_caller`` setup time. + The result is computed once at ``create_tool_caller`` setup time + (not per request) and captured in the closure returned to the + transport layer; warnings fire at server boot, not per call. + Forward-compat with PEP 749 (3.14 lazy annotations): ``get_type_hints`` + is the supported migration target for runtime annotation + resolution, so this code keeps working as the language evolves. """ import typing from types import UnionType @@ -1563,25 +1567,27 @@ class when the annotation is: try: hints = typing.get_type_hints(method) except Exception as exc: # forward-ref failure, missing import, etc. - # WARNING (not debug): silent dict-path fallback was the root - # cause of the wire-dispatch regression Emma's sales-direct - # backend test surfaced (handler.py kept Request types under - # ``if TYPE_CHECKING:`` so PEP 563 forward refs couldn't resolve - # at runtime; resolver returned None; shims crashed on - # ``params.account``). Author's choice: declare ``params: dict`` - # for the dict path, or ensure the typed annotation's class is - # importable at the method's module scope. A debug log is too - # quiet for a footgun this expensive — bumping to WARNING ensures - # adopters see it on first server boot. + # WARNING (not debug): silent dict-path fallback hides shim + # crashes on ``params.`` access when the typed annotation + # didn't resolve. Author's choice: declare ``params: dict`` for + # the dict path, or ensure the typed annotation's class is + # importable at the method's module scope (not under + # ``TYPE_CHECKING``). + # + # Surface the failing name explicitly so adopters don't have to + # parse the method repr — ``NameError`` exposes it on + # ``.name`` on 3.10+. Falls back to ``str(exc)`` for other + # exception classes (rare). + failing_name = getattr(exc, "name", None) or str(exc) logger.warning( - "typed params annotation failed to resolve for %r: %s; " - "falling back to dict dispatch. If this method declares " - "``params: ``, import that model at the " - "method's module scope (not under ``TYPE_CHECKING``); " - "otherwise declare ``params: dict[str, Any]`` to silence " - "this warning.", + "typed params annotation failed to resolve for %r " + "(unresolved name: %s); falling back to dict dispatch. " + "If this method declares ``params: ``, " + "import that model at the method's module scope (not " + "under ``TYPE_CHECKING``); otherwise declare " + "``params: dict[str, Any]`` to silence this warning.", method, - exc, + failing_name, ) return None annotation = hints.get("params") diff --git a/tests/test_decisioning_wire_dispatch.py b/tests/test_decisioning_wire_dispatch.py index 05b1dc208..ebdee8932 100644 --- a/tests/test_decisioning_wire_dispatch.py +++ b/tests/test_decisioning_wire_dispatch.py @@ -21,6 +21,7 @@ import typing from concurrent.futures import ThreadPoolExecutor +from typing import Any import pytest @@ -41,28 +42,17 @@ def executor(): pool.shutdown(wait=True) +# Compute the shim list dynamically from the class's ``advertised_tools`` +# so any new shim added in a future PR auto-extends regression coverage — +# a contributor can't add a new tool with its Request type stuck under +# ``TYPE_CHECKING`` and have CI miss it. +_ALL_SHIMS = sorted(PlatformHandler.advertised_tools) + + # ---- Direct unit-level repro ---- -@pytest.mark.parametrize( - "tool_name", - [ - "get_products", - "create_media_buy", - "update_media_buy", - "sync_creatives", - "get_media_buy_delivery", - "build_creative", - "get_signals", - "activate_signal", - "sync_audiences", - "check_governance", - "get_brand_identity", - "list_content_standards", - "create_property_list", - "create_collection_list", - ], -) +@pytest.mark.parametrize("tool_name", _ALL_SHIMS) def test_get_type_hints_resolves_for_every_shim(tool_name: str) -> None: """``typing.get_type_hints`` must succeed on every shim. Without this, the dispatcher's typed-params resolver silently falls back to @@ -73,30 +63,14 @@ def test_get_type_hints_resolves_for_every_shim(tool_name: str) -> None: assert "params" in hints, f"{tool_name} missing 'params' annotation" -@pytest.mark.parametrize( - "tool_name,expected_request", - [ - ("get_products", "GetProductsRequest"), - ("create_media_buy", "CreateMediaBuyRequest"), - ("update_media_buy", "UpdateMediaBuyRequest"), - ("sync_creatives", "SyncCreativesRequest"), - ("get_media_buy_delivery", "GetMediaBuyDeliveryRequest"), - ("build_creative", "BuildCreativeRequest"), - ("get_signals", "GetSignalsRequest"), - ("sync_audiences", "SyncAudiencesRequest"), - ("check_governance", "CheckGovernanceRequest"), - ("get_brand_identity", "GetBrandIdentityRequest"), - ("list_content_standards", "ListContentStandardsRequest"), - ("create_property_list", "CreatePropertyListRequest"), - ("create_collection_list", "CreateCollectionListRequest"), - ], -) -def test_resolver_returns_typed_request_class_not_none( - tool_name: str, expected_request: str -) -> None: - """The dispatcher's resolver must return the Pydantic request class, +@pytest.mark.parametrize("tool_name", _ALL_SHIMS) +def test_resolver_returns_typed_request_class_not_none(tool_name: str) -> None: + """The dispatcher's resolver must return a Pydantic request class, NOT ``None``. ``None`` triggers the dict-fallback dispatch path that - causes the wire-side 500.""" + causes the wire-side 500. Asserts a Pydantic ``BaseModel`` subclass + rather than a specific class name so this auto-covers new shims.""" + from pydantic import BaseModel + method = getattr(PlatformHandler, tool_name) resolved = _resolve_params_pydantic_model(method) assert resolved is not None, ( @@ -104,9 +78,13 @@ def test_resolver_returns_typed_request_class_not_none( "dispatcher will fall back to dict path and shim will crash on " "params.account" ) - assert ( - resolved.__name__ == expected_request - ), f"{tool_name}: expected {expected_request}, got {resolved.__name__}" + assert issubclass(resolved, BaseModel), ( + f"{tool_name}: resolver returned {resolved!r} which is not a " "Pydantic BaseModel subclass" + ) + assert resolved.__name__.endswith("Request"), ( + f"{tool_name}: resolved class {resolved.__name__!r} doesn't look " + "like a wire Request type" + ) # ---- End-to-end wire path ---- @@ -164,6 +142,19 @@ async def test_wire_dispatch_get_products_does_not_crash(executor) -> None: } result = await caller(wire_payload) assert platform.calls and platform.calls[0][0] == "get_products" + # Pin the typed-dispatch contract: the platform method MUST receive + # a Pydantic ``GetProductsRequest`` instance, NOT a raw dict. The + # wire-dispatch regression this PR fixes was fundamentally about the + # dispatcher silently handing a dict through the typed annotation + # path; without this assertion, a future re-break of the resolver + # that still routes the dict through would pass. + from adcp.types import GetProductsRequest + + received = platform.calls[0][1] + assert isinstance(received, GetProductsRequest), ( + f"platform got {type(received).__name__}, expected GetProductsRequest " + "— resolver regressed to dict-fallback path" + ) assert "products" in result @@ -186,7 +177,27 @@ def build_creative(self, req, ctx): registry=InMemoryTaskRegistry(), ) caller = create_tool_caller(handler, "build_creative") + received_req: list[Any] = [] + # Wrap the platform method via __dict__ so we capture what the + # dispatcher actually delivered post-resolution, before + # ``_invoke_platform_method`` consumes it. + orig_build = handler._platform.build_creative + + def _capture(req: Any, ctx: Any) -> Any: + received_req.append(req) + return orig_build(req, ctx) + + handler._platform.build_creative = _capture # type: ignore[method-assign] + result = await caller( {"brief": "synthesize a 30s spot", "idempotency_key": "emma-test-build-creative-001"} ) assert "creative_manifest" in result + # Same regression guard as get_products — the platform must see a + # typed ``BuildCreativeRequest``, not the raw wire dict. + from adcp.types import BuildCreativeRequest + + assert received_req and isinstance(received_req[0], BuildCreativeRequest), ( + f"platform got {type(received_req[0] if received_req else None).__name__}, " + "expected BuildCreativeRequest" + )