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..ba5f011c9 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,8 +200,53 @@ def maybe_emit_sync_completion( logged-and-swallowed. """ try: - if not enabled or sender is None: + if not enabled: return + + # 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 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 " + "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_for_log if url_for_log else "", + method_name, + ) + return + extracted = _extract_push_notification_url_and_token(params) if extracted is None: return diff --git a/src/adcp/server/mcp_tools.py b/src/adcp/server/mcp_tools.py index 8f4ef348b..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,15 +1567,27 @@ 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( - "typed params annotation failed to resolve for %r: %s; " - "falling back to dict dispatch", + # 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 " + "(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_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..ebdee8932 --- /dev/null +++ b/tests/test_decisioning_wire_dispatch.py @@ -0,0 +1,203 @@ +"""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 +from typing import Any + +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) + + +# 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", _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 + 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", _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. 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, ( + 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 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 ---- + + +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" + # 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 + + +@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") + 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" + )