From e33e07c5afda44de38c133e6b9317ed9946aceb0 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 4 May 2026 07:10:41 -0400 Subject: [PATCH 1/2] fix(idempotency): @IdempotencyStore.wrap supports arg-projected methods (closes #559) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wrap was hard-coded for the (self, params, ctx) calling convention, so framework-dispatched arg-projected tools like update_media_buy — called as method(self, media_buy_id=..., patch=..., ctx=ctx) by dispatch.py's arg_projector path — raised TypeError before the wrap body ran. Salesagent shipped a workaround dropping @wrap from update_media_buy entirely, losing idempotency on retries. Wrap now resolves the calling convention via _resolve_call_args() and supports three shapes: 1. Positional (self, params, ctx) — original. 2. Keyword (self, params=..., context=...). 3. Arg-projected (self, **arg_projector_kwargs, ctx=...) — searches kwargs for a Pydantic model (the framework's contract for the request model, e.g. patch on update_media_buy) to pull idempotency_key from. Falls back to the kwargs dict (excluding ctx/context) when no Pydantic model is present, so projections exposing idempotency_key at the top level still work. Tests: 6 new in TestWrapArgProjectionCalling, 154 across the broader idempotency surface green. Lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/adcp/server/idempotency/store.py | 117 ++++++++++++--- tests/test_server_idempotency.py | 217 +++++++++++++++++++++++++++ 2 files changed, 315 insertions(+), 19 deletions(-) diff --git a/src/adcp/server/idempotency/store.py b/src/adcp/server/idempotency/store.py index 29a47034..7d2c16f7 100644 --- a/src/adcp/server/idempotency/store.py +++ b/src/adcp/server/idempotency/store.py @@ -131,32 +131,49 @@ def capability(self) -> dict[str, Any]: def wrap(self, handler: HandlerFn) -> HandlerFn: """Decorator that adds idempotency semantics to an AdCP handler method. - The wrapped handler is called as ``handler(self, params, context)``. - ``params`` may be a dict or a Pydantic model — both are normalized to - a dict before hashing. The return value is coerced to a dict for - caching (via ``model_dump`` if Pydantic). - - The decorator always returns the handler's original object on a cache - miss and a best-effort Pydantic re-validation on a hit (when the - handler's declared return type exposes ``model_validate``). Callers - that return raw dicts get dicts back. + Supports three calling conventions the framework dispatches with: + + 1. **Positional** ``handler(self, params, context)`` — the + default for non-projected tools (``get_products``, + ``create_media_buy``, etc.). + 2. **Keyword** ``handler(self, params=..., context=...)`` — + same shape, just kwargs. + 3. **Arg-projected** ``handler(self, **arg_projector_kwargs, ctx=...)`` + where ``params`` is split into per-field kwargs by the + framework dispatcher (e.g. ``update_media_buy`` is called + as ``handler(self, media_buy_id=..., patch=..., ctx=...)``). + In this mode the wrap searches the kwargs for a Pydantic + model (``patch`` for update_media_buy) to extract the + idempotency key and hash payload from. Adopters whose + projection contains no Pydantic model (e.g. a method + projecting only a list of ids) get fall-through behavior: + no key found → handler runs without dedup. + + ``params`` is normalized to a dict before hashing; the return + value is coerced to a dict for caching (via ``model_dump`` if + Pydantic). The decorator always returns the handler's original + object on a cache miss and a best-effort Pydantic + re-validation on a hit (when the handler's declared return + type exposes ``model_validate``). Callers that return raw + dicts get dicts back. """ @wraps(handler) - async def _wrapped( - handler_self: Any, - params: Any, - context: Any = None, - *args: Any, - **kwargs: Any, - ) -> Any: - scope_key, idempotency_key, params_dict = self._prepare(params, context) + async def _wrapped(*args: Any, **kwargs: Any) -> Any: + handler_self, hash_source, context = _resolve_call_args(args, kwargs) + + scope_key, idempotency_key, params_dict = self._prepare(hash_source, context) if scope_key is None or idempotency_key is None: # No key → spec says the server MUST reject with INVALID_REQUEST. # We let the handler run so validation layers above us (Pydantic, # FastAPI, etc.) can reject with a typed error; the middleware's # job is only to dedup when a key IS present. - return await handler(handler_self, params, context, *args, **kwargs) + # + # Forward the call exactly as received so all three calling + # conventions (positional / keyword / arg-projected) reach + # the inner handler unchanged. The wrap is signature- + # transparent on the no-key path. + return await handler(*args, **kwargs) payload_hash = self._hash_fn(params_dict) @@ -183,7 +200,7 @@ async def _wrapped( ], ) - response = await handler(handler_self, params, context, *args, **kwargs) + response = await handler(*args, **kwargs) # Deep-copy when caching so post-return mutation of the caller's # copy can't poison future replays. `_clone_response` also deep- # copies on the hit path, giving independent objects per replay. @@ -287,6 +304,68 @@ def _warn_missing_principal_once(self) -> None: ) +def _resolve_call_args(args: tuple[Any, ...], kwargs: dict[str, Any]) -> tuple[Any, Any, Any]: + """Resolve ``(handler_self, hash_source, context)`` across the three + calling conventions the framework dispatches with. + + Returns ``hash_source`` — what the wrap should hand to + :meth:`IdempotencyStore._prepare` for ``idempotency_key`` extraction + and payload hashing. The original ``args`` / ``kwargs`` are + untouched and forwarded verbatim to the inner handler. + + Calling conventions:: + + # 1. Positional (default for non-projected tools) + _wrapped(self, params, ctx) + # → handler_self=self, hash_source=params, context=ctx + + # 2. Keyword (same shape, kwargs form) + _wrapped(self, params=params, context=ctx) + # → handler_self=self, hash_source=params, context=ctx + + # 3. Arg-projected (update_media_buy: params split into kwargs) + _wrapped(self, media_buy_id=..., patch=, ctx=...) + # → handler_self=self, + # hash_source= (first kwarg with model_dump), + # context= + + For arg-projected calls without a Pydantic-shaped kwarg + (e.g. ``arg_projector={"audiences": [...]}``), ``hash_source`` + falls back to the kwargs dict itself — :meth:`_prepare` will look + for ``idempotency_key`` at the top level and skip dedup if absent. + Same fall-through as a missing key, no regression. + """ + handler_self = args[0] if args else None + rest_args = args[1:] + + # Convention 1: positional ``params, ctx`` after self. + if rest_args: + params = rest_args[0] + context = rest_args[1] if len(rest_args) > 1 else kwargs.get("context") + return handler_self, params, context + + # Convention 2: keyword ``params=, context=``. + if "params" in kwargs: + return handler_self, kwargs["params"], kwargs.get("context") or kwargs.get("ctx") + + # Convention 3: arg-projected. ``ctx`` (not ``context``) is what + # dispatch.py:1081 passes; tolerate both for hand-rolled adopters. + context = kwargs.get("ctx", kwargs.get("context")) + # Find the first kwarg value that exposes ``model_dump``, the + # framework's contract for "this is a Pydantic request model". + # ``update_media_buy``'s ``patch`` lands here; falls back to the + # full kwargs dict (excluding ``ctx`` / ``context``) when no model + # is present, matching the pre-projection wire shape. + for key, value in kwargs.items(): + if key in ("ctx", "context"): + continue + if hasattr(value, "model_dump") and callable(value.model_dump): + return handler_self, value, context + + fallback = {k: v for k, v in kwargs.items() if k not in ("ctx", "context")} + return handler_self, fallback, context + + def _scope_log_id(scope_key: str) -> str: """Return a non-reversible short identifier for ``scope_key`` log lines. diff --git a/tests/test_server_idempotency.py b/tests/test_server_idempotency.py index fd441185..c43c031c 100644 --- a/tests/test_server_idempotency.py +++ b/tests/test_server_idempotency.py @@ -482,6 +482,223 @@ async def create_media_buy( assert seller.calls == 1 +class TestWrapArgProjectionCalling: + """Issue #559: ``@IdempotencyStore.wrap`` was called with the + framework's arg-projector convention (``method(**kwargs, ctx=ctx)``) + on tools like ``update_media_buy`` and raised ``TypeError: missing + 1 required positional argument: 'params'``. The salesagent kill- + nginx spike shipped a workaround that disabled idempotency on + ``update_media_buy`` entirely. + + These tests pin the wrap's three-convention behavior: + 1. Positional ``(self, params, ctx)`` — original behavior. + 2. Keyword ``(self, params=..., context=...)``. + 3. Arg-projected ``(self, **arg_projector_kwargs, ctx=...)`` — + the bug case. + """ + + @pytest.mark.asyncio + async def test_arg_projected_with_pydantic_kwarg_succeeds(self) -> None: + """``update_media_buy`` style: the framework calls + ``method(media_buy_id=..., patch=, ctx=...)``. + The wrap should find ``patch`` (the only Pydantic kwarg), + extract ``idempotency_key`` from it, and forward the original + kwargs unchanged to the inner handler.""" + from pydantic import BaseModel + + class Patch(BaseModel): + idempotency_key: str + new_total_budget: float + + store = IdempotencyStore(backend=MemoryBackend(), ttl_seconds=86400) + + class SellerHandler: + def __init__(self) -> None: + self.calls = 0 + self.last_kwargs: dict[str, Any] = {} + + @store.wrap + async def update_media_buy( + self, media_buy_id: str, patch: Patch, ctx: ToolContext + ) -> dict[str, Any]: + self.calls += 1 + self.last_kwargs = { + "media_buy_id": media_buy_id, + "patch": patch, + } + return {"media_buy_id": media_buy_id, "status": "updated"} + + seller = SellerHandler() + key = str(uuid.uuid4()) + ctx = ToolContext(caller_identity="principal-a") + patch = Patch(idempotency_key=key, new_total_budget=500.0) + + # Two retries with same key + payload → handler runs once. + r1 = await seller.update_media_buy(media_buy_id="mb-1", patch=patch, ctx=ctx) + r2 = await seller.update_media_buy(media_buy_id="mb-1", patch=patch, ctx=ctx) + assert seller.calls == 1 + assert r1 == r2 + # Confirm the inner handler received the original arg-projected + # kwargs verbatim — wrap is signature-transparent. + assert seller.last_kwargs["media_buy_id"] == "mb-1" + assert seller.last_kwargs["patch"] is patch + + @pytest.mark.asyncio + async def test_arg_projected_conflict_raises_idempotency_conflict(self) -> None: + """Same key + different patch payload → ``IdempotencyConflictError``, + same as the positional path.""" + from pydantic import BaseModel + + class Patch(BaseModel): + idempotency_key: str + new_total_budget: float + + store = IdempotencyStore(backend=MemoryBackend(), ttl_seconds=86400) + + class SellerHandler: + def __init__(self) -> None: + self.calls = 0 + + @store.wrap + async def update_media_buy( + self, media_buy_id: str, patch: Patch, ctx: ToolContext + ) -> dict[str, Any]: + self.calls += 1 + return {"media_buy_id": media_buy_id, "status": "updated"} + + seller = SellerHandler() + key = str(uuid.uuid4()) + ctx = ToolContext(caller_identity="principal-a") + await seller.update_media_buy( + media_buy_id="mb-1", + patch=Patch(idempotency_key=key, new_total_budget=500.0), + ctx=ctx, + ) + with pytest.raises(IdempotencyConflictError): + await seller.update_media_buy( + media_buy_id="mb-1", + patch=Patch(idempotency_key=key, new_total_budget=999.0), + ctx=ctx, + ) + assert seller.calls == 1 + + @pytest.mark.asyncio + async def test_arg_projected_no_pydantic_kwarg_falls_through(self) -> None: + """``sync_audiences`` style: ``arg_projector={"audiences": [...]}``. + No Pydantic model in kwargs and no top-level ``idempotency_key`` + — wrap finds no key, runs handler without dedup. Same fall- + through as a missing key. Not a regression — adopters who want + idempotency on this shape need to project the params model + directly.""" + store = IdempotencyStore(backend=MemoryBackend(), ttl_seconds=86400) + + class SellerHandler: + def __init__(self) -> None: + self.calls = 0 + + @store.wrap + async def sync_audiences( + self, audiences: list[dict], ctx: ToolContext + ) -> dict[str, Any]: + self.calls += 1 + return {"synced": len(audiences)} + + seller = SellerHandler() + ctx = ToolContext(caller_identity="principal-a") + + # Two calls with identical args — no key → both run. + await seller.sync_audiences(audiences=[{"id": "a1"}], ctx=ctx) + await seller.sync_audiences(audiences=[{"id": "a1"}], ctx=ctx) + assert seller.calls == 2 + + @pytest.mark.asyncio + async def test_arg_projected_top_level_idempotency_key_works(self) -> None: + """When ``arg_projector`` happens to expose ``idempotency_key`` + at the top level (the wrap's fallback path), dedup still + works. This covers tools whose projection strips out the + Pydantic wrapper but keeps the key as a kwarg.""" + store = IdempotencyStore(backend=MemoryBackend(), ttl_seconds=86400) + + class SellerHandler: + def __init__(self) -> None: + self.calls = 0 + + @store.wrap + async def some_tool( + self, idempotency_key: str, payload: list, ctx: ToolContext + ) -> dict[str, Any]: + self.calls += 1 + return {"ran": True} + + seller = SellerHandler() + key = str(uuid.uuid4()) + ctx = ToolContext(caller_identity="principal-a") + await seller.some_tool(idempotency_key=key, payload=[1, 2], ctx=ctx) + await seller.some_tool(idempotency_key=key, payload=[1, 2], ctx=ctx) + assert seller.calls == 1 + + @pytest.mark.asyncio + async def test_arg_projected_uses_ctx_kwarg_for_scope(self) -> None: + """The framework uses ``ctx=`` (not ``context=``) for projected + calls. Verify the scope key is extracted from the ``ctx`` + kwarg correctly — without this, principals collapse and + cross-buyer replay becomes possible.""" + from pydantic import BaseModel + + class Patch(BaseModel): + idempotency_key: str + value: int + + store = IdempotencyStore(backend=MemoryBackend(), ttl_seconds=86400) + + class SellerHandler: + def __init__(self) -> None: + self.calls = 0 + + @store.wrap + async def update_media_buy( + self, media_buy_id: str, patch: Patch, ctx: ToolContext + ) -> dict[str, Any]: + self.calls += 1 + return {"media_buy_id": media_buy_id} + + seller = SellerHandler() + key = str(uuid.uuid4()) + ctx_a = ToolContext(caller_identity="principal-a") + ctx_b = ToolContext(caller_identity="principal-b") + patch = Patch(idempotency_key=key, value=1) + + # Same key, different principals → DIFFERENT cache scope. + # Both calls run — no cross-principal replay. + await seller.update_media_buy(media_buy_id="mb-1", patch=patch, ctx=ctx_a) + await seller.update_media_buy(media_buy_id="mb-1", patch=patch, ctx=ctx_b) + assert seller.calls == 2 + + @pytest.mark.asyncio + async def test_keyword_calling_convention_works(self) -> None: + """``method(self, params=..., context=...)`` — the third + convention, less common but valid.""" + store = IdempotencyStore(backend=MemoryBackend(), ttl_seconds=86400) + + class SellerHandler: + def __init__(self) -> None: + self.calls = 0 + + @store.wrap + async def create_media_buy( + self, params: dict, context: ToolContext | None = None + ) -> dict[str, Any]: + self.calls += 1 + return {"id": "mb-1"} + + seller = SellerHandler() + key = str(uuid.uuid4()) + ctx = ToolContext(caller_identity="principal-a") + await seller.create_media_buy(params={"idempotency_key": key, "b": 1}, context=ctx) + await seller.create_media_buy(params={"idempotency_key": key, "b": 1}, context=ctx) + assert seller.calls == 1 + + class TestCachedResponseImmutability: @pytest.mark.asyncio async def test_mutating_replay_does_not_poison_cache(self) -> None: From eec4f00c9dcc34425cb81e78bdf26e3601d49afe Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 4 May 2026 07:23:07 -0400 Subject: [PATCH 2/2] fix(idempotency): expert-review fixes for #567 (None-context, multi-Pydantic, BaseModel strictness) - _resolve_call_args: 'kwargs.get(context) or kwargs.get(ctx)' silently fell through to ctx when context was explicitly None. Switched to 'in'-based check so explicit None wins. - Multi-Pydantic-kwarg: prefer 'params' / 'request' / 'patch' by name before falling back to first-by-iteration. Eliminates dict-order fragility when a tool has two model kwargs. - isinstance(BaseModel) instead of hasattr(model_dump). A duck-typed object with model_dump no longer accidentally matches. 4 new tests cover: explicit None-context wins, multi-Pydantic preference order, no-preferred-name first-by-iteration fallback, duck-typed non-Pydantic exclusion. 59 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/adcp/server/idempotency/store.py | 37 ++++++--- tests/test_server_idempotency.py | 107 +++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 9 deletions(-) diff --git a/src/adcp/server/idempotency/store.py b/src/adcp/server/idempotency/store.py index 7d2c16f7..8f583343 100644 --- a/src/adcp/server/idempotency/store.py +++ b/src/adcp/server/idempotency/store.py @@ -344,22 +344,41 @@ def _resolve_call_args(args: tuple[Any, ...], kwargs: dict[str, Any]) -> tuple[A context = rest_args[1] if len(rest_args) > 1 else kwargs.get("context") return handler_self, params, context - # Convention 2: keyword ``params=, context=``. + # Convention 2: keyword ``params=, context=``. Use ``in`` rather + # than ``or`` so an explicitly-passed falsy ``context=`` (None, + # an object whose ``__bool__`` returns False) doesn't silently + # fall through to ``ctx``. if "params" in kwargs: - return handler_self, kwargs["params"], kwargs.get("context") or kwargs.get("ctx") + if "context" in kwargs: + context = kwargs["context"] + else: + context = kwargs.get("ctx") + return handler_self, kwargs["params"], context # Convention 3: arg-projected. ``ctx`` (not ``context``) is what # dispatch.py:1081 passes; tolerate both for hand-rolled adopters. - context = kwargs.get("ctx", kwargs.get("context")) - # Find the first kwarg value that exposes ``model_dump``, the - # framework's contract for "this is a Pydantic request model". - # ``update_media_buy``'s ``patch`` lands here; falls back to the - # full kwargs dict (excluding ``ctx`` / ``context``) when no model - # is present, matching the pre-projection wire shape. + context = kwargs["ctx"] if "ctx" in kwargs else kwargs.get("context") + # Prefer kwargs literally named ``params`` / ``request`` / ``patch`` + # before falling back to "first kwarg with ``model_dump``". The + # named lookup is dict-order-independent and matches the framework's + # explicit projection contract: ``update_media_buy`` projects via + # ``patch=``; future tools may use ``params=`` or ``request=``. + # Without this preference, a tool with two Pydantic kwargs would + # hash the wrong one when iteration order ever shifts (Python 3.7+ + # guarantees dict insertion order, but the call-site insertion + # order is the framework's choice, not the handler signature). + for preferred_name in ("params", "request", "patch"): + candidate = kwargs.get(preferred_name) + if candidate is not None and isinstance(candidate, BaseModel): + return handler_self, candidate, context + # Fall back to first kwarg whose value is a Pydantic ``BaseModel``. + # ``isinstance`` is stricter than ``hasattr(model_dump)`` — a + # non-Pydantic duck type with a ``model_dump`` method would no + # longer accidentally match. for key, value in kwargs.items(): if key in ("ctx", "context"): continue - if hasattr(value, "model_dump") and callable(value.model_dump): + if isinstance(value, BaseModel): return handler_self, value, context fallback = {k: v for k, v in kwargs.items() if k not in ("ctx", "context")} diff --git a/tests/test_server_idempotency.py b/tests/test_server_idempotency.py index c43c031c..bd074543 100644 --- a/tests/test_server_idempotency.py +++ b/tests/test_server_idempotency.py @@ -699,6 +699,113 @@ async def create_media_buy( assert seller.calls == 1 +class TestWrapResolveCallArgsEdgeCases: + """Edge cases code-reviewer flagged on PR #567.""" + + @pytest.mark.asyncio + async def test_explicit_none_context_does_not_fall_through_to_ctx(self) -> None: + """``handler(self, params=p, context=None, ctx=real_ctx)`` — the + explicit ``context=None`` must win, not silently fall back to + ``ctx``. Falsy-context values (None, empty objects) shouldn't + trigger the alternate-key lookup.""" + from adcp.server.idempotency.store import _resolve_call_args + + real_ctx = ToolContext(caller_identity="real-principal") + handler_self, params, context = _resolve_call_args( + args=(object(),), # self + kwargs={"params": {"k": "v"}, "context": None, "ctx": real_ctx}, + ) + assert context is None # Explicit None wins over ctx fallback. + + @pytest.mark.asyncio + async def test_multi_pydantic_kwarg_prefers_named_params(self) -> None: + """When multiple Pydantic kwargs are present, the resolver + prefers ``params`` / ``request`` / ``patch`` by name (in that + order) before falling back to first-by-iteration. Without + this, a tool with two Pydantic models would hash whichever + the framework happened to insert first into kwargs — order- + dependent and brittle.""" + from pydantic import BaseModel + + from adcp.server.idempotency.store import _resolve_call_args + + class Filter(BaseModel): + field: str + + class Patch(BaseModel): + idempotency_key: str + value: int + + # ``filter`` is inserted before ``patch`` — first-by-iteration + # would pick ``filter``. Named-preference picks ``patch``. + handler_self, params, context = _resolve_call_args( + args=(object(),), + kwargs={ + "filter": Filter(field="x"), + "patch": Patch(idempotency_key="k", value=1), + "ctx": ToolContext(caller_identity="p"), + }, + ) + assert isinstance(params, Patch) + + @pytest.mark.asyncio + async def test_first_pydantic_falls_back_when_no_preferred_name(self) -> None: + """No ``params`` / ``request`` / ``patch`` kwarg → fall back to + first-by-iteration. Pin this so future maintainers don't break + the contract by accident.""" + from pydantic import BaseModel + + from adcp.server.idempotency.store import _resolve_call_args + + class Audience(BaseModel): + idempotency_key: str + id: str + + handler_self, params, context = _resolve_call_args( + args=(object(),), + kwargs={ + "audience": Audience(idempotency_key="k", id="a1"), + "ctx": ToolContext(caller_identity="p"), + }, + ) + assert isinstance(params, Audience) + + @pytest.mark.asyncio + async def test_duck_typed_non_pydantic_model_dump_no_longer_matches(self) -> None: + """``isinstance(BaseModel)`` is stricter than ``hasattr(model_dump)``. + A duck-typed object with a ``model_dump`` method (e.g. a custom + SQLAlchemy adapter) should NOT accidentally be treated as the + params source.""" + + class FakeModel: + def model_dump(self) -> dict[str, Any]: + return {"idempotency_key": "k"} + + store = IdempotencyStore(backend=MemoryBackend(), ttl_seconds=86400) + + class SellerHandler: + def __init__(self) -> None: + self.calls = 0 + + @store.wrap + async def some_tool(self, fake: FakeModel, ctx: ToolContext) -> dict[str, Any]: + self.calls += 1 + return {"ok": True} + + seller = SellerHandler() + ctx = ToolContext(caller_identity="p") + # FakeModel is NOT a BaseModel, so it falls through to the + # kwargs-dict fallback. No idempotency_key at the top level + # of kwargs (the key lives inside fake.model_dump() — which + # the resolver doesn't introspect on non-BaseModel objects). + # Both calls run; this is the intended behavior — adopters + # who want idempotency must use real Pydantic models OR + # surface idempotency_key at the top of kwargs. + await seller.some_tool(fake=FakeModel(), ctx=ctx) + await seller.some_tool(fake=FakeModel(), ctx=ctx) + assert seller.calls == 2 + + class TestCachedResponseImmutability: @pytest.mark.asyncio async def test_mutating_replay_does_not_poison_cache(self) -> None: