Skip to content

Commit 1abdae6

Browse files
bokelleyclaude
andauthored
fix(idempotency): @IdempotencyStore.wrap supports arg-projected methods (#567)
* fix(idempotency): @IdempotencyStore.wrap supports arg-projected methods (closes #559) 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 4ead607 commit 1abdae6

2 files changed

Lines changed: 441 additions & 19 deletions

File tree

src/adcp/server/idempotency/store.py

Lines changed: 117 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -131,32 +131,49 @@ def capability(self) -> dict[str, Any]:
131131
def wrap(self, handler: HandlerFn) -> HandlerFn:
132132
"""Decorator that adds idempotency semantics to an AdCP handler method.
133133
134-
The wrapped handler is called as ``handler(self, params, context)``.
135-
``params`` may be a dict or a Pydantic model — both are normalized to
136-
a dict before hashing. The return value is coerced to a dict for
137-
caching (via ``model_dump`` if Pydantic).
138-
139-
The decorator always returns the handler's original object on a cache
140-
miss and a best-effort Pydantic re-validation on a hit (when the
141-
handler's declared return type exposes ``model_validate``). Callers
142-
that return raw dicts get dicts back.
134+
Supports three calling conventions the framework dispatches with:
135+
136+
1. **Positional** ``handler(self, params, context)`` — the
137+
default for non-projected tools (``get_products``,
138+
``create_media_buy``, etc.).
139+
2. **Keyword** ``handler(self, params=..., context=...)`` —
140+
same shape, just kwargs.
141+
3. **Arg-projected** ``handler(self, **arg_projector_kwargs, ctx=...)``
142+
where ``params`` is split into per-field kwargs by the
143+
framework dispatcher (e.g. ``update_media_buy`` is called
144+
as ``handler(self, media_buy_id=..., patch=..., ctx=...)``).
145+
In this mode the wrap searches the kwargs for a Pydantic
146+
model (``patch`` for update_media_buy) to extract the
147+
idempotency key and hash payload from. Adopters whose
148+
projection contains no Pydantic model (e.g. a method
149+
projecting only a list of ids) get fall-through behavior:
150+
no key found → handler runs without dedup.
151+
152+
``params`` is normalized to a dict before hashing; the return
153+
value is coerced to a dict for caching (via ``model_dump`` if
154+
Pydantic). The decorator always returns the handler's original
155+
object on a cache miss and a best-effort Pydantic
156+
re-validation on a hit (when the handler's declared return
157+
type exposes ``model_validate``). Callers that return raw
158+
dicts get dicts back.
143159
"""
144160

145161
@wraps(handler)
146-
async def _wrapped(
147-
handler_self: Any,
148-
params: Any,
149-
context: Any = None,
150-
*args: Any,
151-
**kwargs: Any,
152-
) -> Any:
153-
scope_key, idempotency_key, params_dict = self._prepare(params, context)
162+
async def _wrapped(*args: Any, **kwargs: Any) -> Any:
163+
handler_self, hash_source, context = _resolve_call_args(args, kwargs)
164+
165+
scope_key, idempotency_key, params_dict = self._prepare(hash_source, context)
154166
if scope_key is None or idempotency_key is None:
155167
# No key → spec says the server MUST reject with INVALID_REQUEST.
156168
# We let the handler run so validation layers above us (Pydantic,
157169
# FastAPI, etc.) can reject with a typed error; the middleware's
158170
# job is only to dedup when a key IS present.
159-
return await handler(handler_self, params, context, *args, **kwargs)
171+
#
172+
# Forward the call exactly as received so all three calling
173+
# conventions (positional / keyword / arg-projected) reach
174+
# the inner handler unchanged. The wrap is signature-
175+
# transparent on the no-key path.
176+
return await handler(*args, **kwargs)
160177

161178
payload_hash = self._hash_fn(params_dict)
162179

@@ -183,7 +200,7 @@ async def _wrapped(
183200
],
184201
)
185202

186-
response = await handler(handler_self, params, context, *args, **kwargs)
203+
response = await handler(*args, **kwargs)
187204
# Deep-copy when caching so post-return mutation of the caller's
188205
# copy can't poison future replays. `_clone_response` also deep-
189206
# copies on the hit path, giving independent objects per replay.
@@ -287,6 +304,87 @@ def _warn_missing_principal_once(self) -> None:
287304
)
288305

289306

307+
def _resolve_call_args(args: tuple[Any, ...], kwargs: dict[str, Any]) -> tuple[Any, Any, Any]:
308+
"""Resolve ``(handler_self, hash_source, context)`` across the three
309+
calling conventions the framework dispatches with.
310+
311+
Returns ``hash_source`` — what the wrap should hand to
312+
:meth:`IdempotencyStore._prepare` for ``idempotency_key`` extraction
313+
and payload hashing. The original ``args`` / ``kwargs`` are
314+
untouched and forwarded verbatim to the inner handler.
315+
316+
Calling conventions::
317+
318+
# 1. Positional (default for non-projected tools)
319+
_wrapped(self, params, ctx)
320+
# → handler_self=self, hash_source=params, context=ctx
321+
322+
# 2. Keyword (same shape, kwargs form)
323+
_wrapped(self, params=params, context=ctx)
324+
# → handler_self=self, hash_source=params, context=ctx
325+
326+
# 3. Arg-projected (update_media_buy: params split into kwargs)
327+
_wrapped(self, media_buy_id=..., patch=<UpdateMediaBuyRequest>, ctx=...)
328+
# → handler_self=self,
329+
# hash_source=<UpdateMediaBuyRequest> (first kwarg with model_dump),
330+
# context=<ctx>
331+
332+
For arg-projected calls without a Pydantic-shaped kwarg
333+
(e.g. ``arg_projector={"audiences": [...]}``), ``hash_source``
334+
falls back to the kwargs dict itself — :meth:`_prepare` will look
335+
for ``idempotency_key`` at the top level and skip dedup if absent.
336+
Same fall-through as a missing key, no regression.
337+
"""
338+
handler_self = args[0] if args else None
339+
rest_args = args[1:]
340+
341+
# Convention 1: positional ``params, ctx`` after self.
342+
if rest_args:
343+
params = rest_args[0]
344+
context = rest_args[1] if len(rest_args) > 1 else kwargs.get("context")
345+
return handler_self, params, context
346+
347+
# Convention 2: keyword ``params=, context=``. Use ``in`` rather
348+
# than ``or`` so an explicitly-passed falsy ``context=`` (None,
349+
# an object whose ``__bool__`` returns False) doesn't silently
350+
# fall through to ``ctx``.
351+
if "params" in kwargs:
352+
if "context" in kwargs:
353+
context = kwargs["context"]
354+
else:
355+
context = kwargs.get("ctx")
356+
return handler_self, kwargs["params"], context
357+
358+
# Convention 3: arg-projected. ``ctx`` (not ``context``) is what
359+
# dispatch.py:1081 passes; tolerate both for hand-rolled adopters.
360+
context = kwargs["ctx"] if "ctx" in kwargs else kwargs.get("context")
361+
# Prefer kwargs literally named ``params`` / ``request`` / ``patch``
362+
# before falling back to "first kwarg with ``model_dump``". The
363+
# named lookup is dict-order-independent and matches the framework's
364+
# explicit projection contract: ``update_media_buy`` projects via
365+
# ``patch=``; future tools may use ``params=`` or ``request=``.
366+
# Without this preference, a tool with two Pydantic kwargs would
367+
# hash the wrong one when iteration order ever shifts (Python 3.7+
368+
# guarantees dict insertion order, but the call-site insertion
369+
# order is the framework's choice, not the handler signature).
370+
for preferred_name in ("params", "request", "patch"):
371+
candidate = kwargs.get(preferred_name)
372+
if candidate is not None and isinstance(candidate, BaseModel):
373+
return handler_self, candidate, context
374+
# Fall back to first kwarg whose value is a Pydantic ``BaseModel``.
375+
# ``isinstance`` is stricter than ``hasattr(model_dump)`` — a
376+
# non-Pydantic duck type with a ``model_dump`` method would no
377+
# longer accidentally match.
378+
for key, value in kwargs.items():
379+
if key in ("ctx", "context"):
380+
continue
381+
if isinstance(value, BaseModel):
382+
return handler_self, value, context
383+
384+
fallback = {k: v for k, v in kwargs.items() if k not in ("ctx", "context")}
385+
return handler_self, fallback, context
386+
387+
290388
def _scope_log_id(scope_key: str) -> str:
291389
"""Return a non-reversible short identifier for ``scope_key`` log lines.
292390

0 commit comments

Comments
 (0)