fix(decisioning): wire-path dispatch + F12 silent no-op (Emma sales-direct P0s) (#338)

bokelley · claude · web-flow · commit 50c985eda951 · 2026-05-01T10:16:11.000-04:00
* fix(decisioning): wire-path dispatch + F12 silent no-op (Emma sales-direct P0s) 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) <noreply@anthropic.com> * fix(decisioning): expert-review P1s on PR #338 wire-dispatch fix 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
diff --git 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
@@ -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 "<unextractable>",
+                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
@@ -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.<field>`` 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: <PydanticModel>``, "
+            "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
@@ -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
