From a2dc0c56fd121b12082e102ba1aab9608d891ce4 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Sat, 9 May 2026 14:26:14 -0400 Subject: [PATCH] fix(decisioning): wire sync_accounts/list_accounts dispatch to AccountStore Protocols MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes adcp-client#1631. ``PlatformHandler.sync_accounts`` and ``PlatformHandler.list_accounts`` were inherited as ``_not_supported`` stubs from ``ADCPHandler`` — every adopter that implements :class:`AccountStoreUpsert` / :class:`AccountStoreList` on their platform's ``accounts`` store was invisible on the wire, returning ``OPERATION_NOT_SUPPORTED`` for both tools regardless of what they declared. The dispatch path now mirrors the documented design: * ``PlatformHandler.sync_accounts(params, ctx)`` → ``platform.accounts.upsert(params, ctx=ResolveContext(...))`` * ``PlatformHandler.list_accounts(params, ctx)`` → ``platform.accounts.list(params, ctx=ResolveContext(...))`` The ``ResolveContext`` carries the caller's :class:`AuthInfo` and resolved :class:`BuyerAgent` (when a registry is wired) — same threading :meth:`AccountStore.resolve` already uses, so adopter impls that implement principal-keyed gates (e.g. spec ``BILLING_NOT_PERMITTED_FOR_AGENT``) read the principal off the same context. Account methods aren't part of any per-specialism Protocol — they live on the ``AccountStore`` Protocol surface per ``LazyPlatformRouter._ACCOUNT_STORE_METHODS``. Both ``upsert`` and ``list`` are OPTIONAL on the store; the new shims surface ``OPERATION_NOT_SUPPORTED`` (via ``_not_supported``) when the store doesn't expose them, rather than ``AttributeError``. Wire advertisement adds a new ``_ACCOUNT_ADVERTISED_TOOLS`` set, unioned into every ``sales-*`` entry of ``SPECIALISM_TO_ADVERTISED_TOOLS`` (every sales-* claim already implies an account roster — buyers must reach it). The override- detection filter still drops the tools when the platform's store doesn't implement the corresponding Protocol method. Tests: * ``test_advertised_tools_covers_every_specialism_wire_tool`` extended with the two new tools. * New positive tests: ``sync_accounts`` and ``list_accounts`` route through to ``platform.accounts.upsert`` / ``.list`` with the ``ResolveContext.tool_name`` set correctly. * New negative tests: stores that don't implement the optional Protocols surface ``OPERATION_NOT_SUPPORTED`` via the framework's ``NotImplementedResponse`` — distinct from ``AttributeError``. Storyboard impact for downstream adopters: scenarios in ``media_buy_seller/refine_products/setup`` (which require either ``sync_accounts`` or ``list_accounts``) graduate from skipped to graded the moment the adopter wires their store's optional Protocol methods. --- src/adcp/decisioning/handler.py | 127 ++++++++++++++++- tests/test_decisioning_handler_shims.py | 174 ++++++++++++++++++++++++ 2 files changed, 295 insertions(+), 6 deletions(-) diff --git a/src/adcp/decisioning/handler.py b/src/adcp/decisioning/handler.py index 54e205a74..1a75495a4 100644 --- a/src/adcp/decisioning/handler.py +++ b/src/adcp/decisioning/handler.py @@ -81,6 +81,7 @@ # 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.decisioning.accounts import ResolveContext, _call_with_optional_ctx from adcp.types import ( AccountReference, AcquireRightsRequest, @@ -131,6 +132,8 @@ GetRightsSuccessResponse, GetSignalsRequest, GetSignalsResponse, + ListAccountsRequest, + ListAccountsResponse, ListCollectionListsRequest, ListCollectionListsResponse, ListContentStandardsRequest, @@ -147,6 +150,8 @@ ProvidePerformanceFeedbackResponse, ReportPlanOutcomeRequest, ReportPlanOutcomeResponse, + SyncAccountsRequest, + SyncAccountsResponse, SyncAudiencesRequest, SyncAudiencesSuccessResponse, SyncCreativesRequest, @@ -205,6 +210,20 @@ "list_creatives", } ) +#: Account-management tools — buyers reach the seller's account +#: roster via these. Per spec, every seller MUST expose at least one +#: of ``sync_accounts`` (declarative push) or ``list_accounts`` (read- +#: only enumerate). Wired through the platform's :class:`AccountStore` +#: surface (:class:`AccountStoreUpsert` / :class:`AccountStoreList`), +#: not via the per-specialism Protocol method dispatch — see +#: :data:`adcp.decisioning.platform_router._ACCOUNT_STORE_METHODS` +#: for the corresponding router-side carve-out. +_ACCOUNT_ADVERTISED_TOOLS: frozenset[str] = frozenset( + { + "list_accounts", + "sync_accounts", + } +) _CREATIVE_ADVERTISED_TOOLS: frozenset[str] = frozenset( { "build_creative", @@ -309,12 +328,18 @@ #: another specialism that does. SPECIALISM_TO_ADVERTISED_TOOLS: dict[str, frozenset[str]] = { # Sales-* archetypes — all use the unified SalesPlatform surface. - "sales-non-guaranteed": _SALES_ADVERTISED_TOOLS, - "sales-guaranteed": _SALES_ADVERTISED_TOOLS, - "sales-broadcast-tv": _SALES_ADVERTISED_TOOLS, - "sales-social": _SALES_ADVERTISED_TOOLS, - "sales-catalog-driven": _SALES_ADVERTISED_TOOLS, - "sales-proposal-mode": _SALES_ADVERTISED_TOOLS, + # Sales adopters expose account discovery via ``sync_accounts`` / + # ``list_accounts`` — adding the union here so the per-instance + # advertisement filter doesn't strip them. The override filter still + # drops them when the platform's :class:`AccountStore` doesn't + # implement the optional :class:`AccountStoreUpsert` / + # :class:`AccountStoreList` Protocols. + "sales-non-guaranteed": _SALES_ADVERTISED_TOOLS | _ACCOUNT_ADVERTISED_TOOLS, + "sales-guaranteed": _SALES_ADVERTISED_TOOLS | _ACCOUNT_ADVERTISED_TOOLS, + "sales-broadcast-tv": _SALES_ADVERTISED_TOOLS | _ACCOUNT_ADVERTISED_TOOLS, + "sales-social": _SALES_ADVERTISED_TOOLS | _ACCOUNT_ADVERTISED_TOOLS, + "sales-catalog-driven": _SALES_ADVERTISED_TOOLS | _ACCOUNT_ADVERTISED_TOOLS, + "sales-proposal-mode": _SALES_ADVERTISED_TOOLS | _ACCOUNT_ADVERTISED_TOOLS, # Creative — Builder + AdServer. Builder claims expose # build_creative + optional preview_creative; AdServer adds # get_creative_delivery (per CreativeAdServerPlatform Protocol). @@ -677,6 +702,7 @@ class PlatformHandler(ADCPHandler[ToolContext]): #: would advertise all 40+ shims (Emma cross-cutting P1). advertised_tools: ClassVar[set[str]] = ( set(_SALES_ADVERTISED_TOOLS) + | set(_ACCOUNT_ADVERTISED_TOOLS) | set(_CREATIVE_ADVERTISED_TOOLS) | set(_SIGNALS_ADVERTISED_TOOLS) | set(_AUDIENCE_ADVERTISED_TOOLS) @@ -877,6 +903,29 @@ async def _resolve_account( record_resolved_account_mode(resolved) return resolved + def _make_resolve_context( + self, + tool_ctx: ToolContext, + tool_name: str, + ) -> ResolveContext: + """Build a :class:`ResolveContext` for AccountStore-routed tools. + + Mirrors :meth:`_resolve_account`'s auth/agent-registry wiring + without going through ``AccountStore.resolve`` itself — the + ``upsert`` / ``list`` / ``sync_governance`` Protocols receive + their own ``ResolveContext`` rather than a + :class:`RequestContext`, since the resolved account isn't yet + available (sync_accounts) or operates on multiple accounts at + once (list_accounts). + """ + auth_info = self._extract_auth_info(tool_ctx) + buyer_agent = tool_ctx.metadata.get("adcp.buyer_agent") if tool_ctx.metadata else None + return ResolveContext( + auth_info=auth_info, + tool_name=tool_name, + agent=buyer_agent, + ) + @staticmethod def _extract_auth_info(ctx: ToolContext) -> AuthInfo | None: """Pull AuthInfo from ToolContext.metadata when present. @@ -1582,6 +1631,72 @@ async def list_creatives( # type: ignore[override] ), ) + # ----- AccountStore-routed dispatchers ---------------------------- + # + # ``sync_accounts`` and ``list_accounts`` flow through the platform's + # :class:`AccountStore` Protocol surface (specifically + # :class:`AccountStoreUpsert` / :class:`AccountStoreList`), not the + # per-specialism platform-method dispatch — see + # :data:`adcp.decisioning.platform_router._ACCOUNT_STORE_METHODS` + # for the corresponding router-side carve-out. Both Protocols are + # OPTIONAL on the AccountStore: surface ``UNSUPPORTED_FEATURE`` + # rather than ``AttributeError`` when the adopter's store doesn't + # implement them. + + async def sync_accounts( # type: ignore[override] + self, + params: SyncAccountsRequest, + context: ToolContext | None = None, + ) -> SyncAccountsResponse: + """Forward ``sync_accounts`` to ``platform.accounts.upsert``. + + Wire request has no ``account`` field — the ``accounts`` array + IS the discovery payload. Resolve via auth only; the + :class:`ResolveContext` carries the caller's + :class:`AuthInfo` / agent so adopter impls can apply + principal-keyed gates (e.g. spec + ``BILLING_NOT_PERMITTED_FOR_AGENT``). + """ + upsert = getattr(self._platform.accounts, "upsert", None) + if upsert is None: + return cast("SyncAccountsResponse", self._not_supported("sync_accounts")) + tool_ctx = context or ToolContext() + account = await self._resolve_account(None, tool_ctx) + ctx = self._build_ctx(tool_ctx, account) + resolve_ctx = self._make_resolve_context(tool_ctx, "sync_accounts") + result = _call_with_optional_ctx(upsert, params, ctx=resolve_ctx) + if inspect.isawaitable(result): + result = await result + # Adopters MAY return either the wire-shaped ``SyncAccountsResponse`` + # (typical when forwarding to a buyer-shaped impl) or a + # ``list[SyncAccountsResultRow]`` per the Protocol's narrower + # contract. Pass through unchanged — the typed dispatcher serializer + # downstream handles both shapes. + del ctx, account + return cast("SyncAccountsResponse", result) + + async def list_accounts( # type: ignore[override] + self, + params: ListAccountsRequest, + context: ToolContext | None = None, + ) -> ListAccountsResponse: + """Forward ``list_accounts`` to ``platform.accounts.list``. + + Wire request has no ``account`` field. Resolve via auth only. + """ + list_fn = getattr(self._platform.accounts, "list", None) + if list_fn is None: + return cast("ListAccountsResponse", self._not_supported("list_accounts")) + tool_ctx = context or ToolContext() + account = await self._resolve_account(None, tool_ctx) + ctx = self._build_ctx(tool_ctx, account) + resolve_ctx = self._make_resolve_context(tool_ctx, "list_accounts") + result = _call_with_optional_ctx(list_fn, params, ctx=resolve_ctx) + if inspect.isawaitable(result): + result = await result + del ctx, account + return cast("ListAccountsResponse", result) + # ----- Optional-method gate ----- def _require_platform_method(self, method_name: str) -> None: diff --git a/tests/test_decisioning_handler_shims.py b/tests/test_decisioning_handler_shims.py index 7e7d89c9a..d726c5531 100644 --- a/tests/test_decisioning_handler_shims.py +++ b/tests/test_decisioning_handler_shims.py @@ -25,6 +25,7 @@ import asyncio from concurrent.futures import ThreadPoolExecutor +from typing import Any from unittest.mock import AsyncMock import pytest @@ -71,6 +72,9 @@ def test_advertised_tools_covers_every_specialism_wire_tool() -> None: "provide_performance_feedback", "list_creative_formats", "list_creatives", + # Account management — routed through AccountStore Protocols + "sync_accounts", + "list_accounts", # Creative (Builder + AdServer) "build_creative", "preview_creative", @@ -376,6 +380,176 @@ def delete_property_list(self, req, ctx): assert result == {"list_id": "pl_1", "fetch_token": "tok_x"} +# ---- AccountStore-routed dispatchers (sync_accounts / list_accounts) ---- + + +@pytest.mark.asyncio +async def test_sync_accounts_shim_routes_to_account_store_upsert(executor) -> None: + """``sync_accounts`` flows through ``platform.accounts.upsert`` (the + framework's AccountStoreUpsert Protocol), NOT ``platform.sync_accounts`` + — accounts surface lives on the AccountStore per the + ``LazyPlatformRouter._ACCOUNT_STORE_METHODS`` carve-out.""" + + upsert_calls: list[tuple[Any, Any]] = [] + + class _StoreWithUpsert(SingletonAccounts): + def upsert(self, params, ctx=None): + upsert_calls.append((params, ctx)) + return {"accounts": [{"account_id": "acc_42", "action": "created"}]} + + class _SalesAgentWithUpsert(DecisioningPlatform): + capabilities = DecisioningCapabilities(specialisms=["sales-non-guaranteed"]) + accounts = _StoreWithUpsert(account_id="hello") + + def get_products(self, req, ctx): + return {"products": []} + + def create_media_buy(self, req, ctx): + raise NotImplementedError + + def update_media_buy(self, *, media_buy_id, patch, ctx): + raise NotImplementedError + + def sync_creatives(self, req, ctx): + raise NotImplementedError + + handler = PlatformHandler( + _SalesAgentWithUpsert(), + executor=executor, + registry=InMemoryTaskRegistry(), + ) + from adcp.types import SyncAccountsRequest + + request = SyncAccountsRequest.model_construct( + accounts=[], + idempotency_key="idem-acc-1234567890abcdef", + ) + result = await handler.sync_accounts(request, ToolContext()) + + assert len(upsert_calls) == 1 + assert upsert_calls[0][0] is request + # ResolveContext was threaded through with the tool name set + resolve_ctx = upsert_calls[0][1] + assert resolve_ctx.tool_name == "sync_accounts" + assert result == {"accounts": [{"account_id": "acc_42", "action": "created"}]} + + +@pytest.mark.asyncio +async def test_list_accounts_shim_routes_to_account_store_list(executor) -> None: + """``list_accounts`` flows through ``platform.accounts.list``.""" + + list_calls: list[tuple[Any, Any]] = [] + + class _StoreWithList(SingletonAccounts): + def list(self, params, ctx=None): + list_calls.append((params, ctx)) + return {"accounts": [{"account_id": "acc_42"}]} + + class _SalesAgentWithList(DecisioningPlatform): + capabilities = DecisioningCapabilities(specialisms=["sales-non-guaranteed"]) + accounts = _StoreWithList(account_id="hello") + + def get_products(self, req, ctx): + return {"products": []} + + def create_media_buy(self, req, ctx): + raise NotImplementedError + + def update_media_buy(self, *, media_buy_id, patch, ctx): + raise NotImplementedError + + def sync_creatives(self, req, ctx): + raise NotImplementedError + + handler = PlatformHandler( + _SalesAgentWithList(), + executor=executor, + registry=InMemoryTaskRegistry(), + ) + from adcp.types import ListAccountsRequest + + request = ListAccountsRequest.model_construct() + result = await handler.list_accounts(request, ToolContext()) + + assert len(list_calls) == 1 + assert list_calls[0][0] is request + resolve_ctx = list_calls[0][1] + assert resolve_ctx.tool_name == "list_accounts" + assert result == {"accounts": [{"account_id": "acc_42"}]} + + +@pytest.mark.asyncio +async def test_sync_accounts_unsupported_when_store_lacks_upsert(executor) -> None: + """When the platform's AccountStore doesn't implement + :class:`AccountStoreUpsert`, ``sync_accounts`` surfaces + ``OPERATION_NOT_SUPPORTED`` rather than ``AttributeError``.""" + + class _SalesAgentNoUpsert(DecisioningPlatform): + capabilities = DecisioningCapabilities(specialisms=["sales-non-guaranteed"]) + accounts = SingletonAccounts(account_id="hello") # no upsert method + + def get_products(self, req, ctx): + return {"products": []} + + def create_media_buy(self, req, ctx): + raise NotImplementedError + + def update_media_buy(self, *, media_buy_id, patch, ctx): + raise NotImplementedError + + def sync_creatives(self, req, ctx): + raise NotImplementedError + + handler = PlatformHandler( + _SalesAgentNoUpsert(), + executor=executor, + registry=InMemoryTaskRegistry(), + ) + from adcp.types import SyncAccountsRequest + + result = await handler.sync_accounts( + SyncAccountsRequest.model_construct( + accounts=[], + idempotency_key="idem-acc-1234567890abcdef", + ), + ToolContext(), + ) + # ``_not_supported`` returns a NotImplementedResponse object whose + # ``status`` carries the canonical "not supported" envelope. + assert getattr(result, "supported", None) is False + assert getattr(getattr(result, "error", None), "code", None) == "NOT_SUPPORTED" + + +@pytest.mark.asyncio +async def test_list_accounts_unsupported_when_store_lacks_list(executor) -> None: + class _SalesAgentNoList(DecisioningPlatform): + capabilities = DecisioningCapabilities(specialisms=["sales-non-guaranteed"]) + accounts = SingletonAccounts(account_id="hello") # no list method + + def get_products(self, req, ctx): + return {"products": []} + + def create_media_buy(self, req, ctx): + raise NotImplementedError + + def update_media_buy(self, *, media_buy_id, patch, ctx): + raise NotImplementedError + + def sync_creatives(self, req, ctx): + raise NotImplementedError + + handler = PlatformHandler( + _SalesAgentNoList(), + executor=executor, + registry=InMemoryTaskRegistry(), + ) + from adcp.types import ListAccountsRequest + + result = await handler.list_accounts(ListAccountsRequest.model_construct(), ToolContext()) + assert getattr(result, "supported", None) is False + assert getattr(getattr(result, "error", None), "code", None) == "NOT_SUPPORTED" + + # ---- Optional-method UNSUPPORTED_FEATURE gate ----