fix(idempotency): @IdempotencyStore.wrap supports arg-projected methods

[bokelley]

Closes #559.

The bug

`@IdempotencyStore.wrap` had signature `(handler_self, params, context=None, *args, **kwargs)`. The framework's arg-projector path in `dispatch.py:1081` calls methods as `method(**arg_projector, ctx=ctx)` — for `update_media_buy`, that's `method(self, media_buy_id=..., patch=..., ctx=...)`. Python can't bind `params` (positional, no default) → `TypeError: missing 1 required positional argument: 'params'` before the wrap body runs.

Salesagent's kill-nginx spike shipped a workaround dropping `@_IDEMPOTENCY.wrap` from `update_media_buy` entirely. Buyer retries re-execute side effects.

Fix

Wrap signature is now `async def _wrapped(*args, **kwargs)`. A new `_resolve_call_args` helper detects which of three calling conventions the framework dispatched with:

1. Positional: `(self, params, ctx)` — default for non-projected tools.
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`) and uses it as the hash source. Falls back to the kwargs dict (excluding `ctx` / `context`) when no Pydantic model is present.

The inner handler is invoked via `*args/**kwargs` verbatim. The wrap is signature-transparent — no matter which convention the framework used, the inner handler sees its expected arguments unchanged.

Why a wrap-side fix (not dispatch-side)?

Three reasons:

• Dispatch is generic and shouldn't know about `@wrap` — the abstraction was already correct.
• Adopters who call `@store.wrap` outside the framework dispatch (in custom transports, in tests) get the fix automatically without retrofitting their callers.
• The `_resolve_call_args` heuristic uses a public Pydantic contract (`hasattr(value, "model_dump")`), not framework-internal knowledge, so future arg-projector additions work without further wrap changes.

Edge cases handled

• `update_media_buy` (`arg_projector={"media_buy_id": id, "patch": }`): finds `patch` (the only Pydantic kwarg), extracts `idempotency_key` from it. Cache hit + replay + conflict all work. ✅
• `sync_audiences` style (`arg_projector={"audiences": [...]}`): no Pydantic model, no top-level `idempotency_key` → wrap finds no key, runs handler without dedup. Same fall-through as before for missing-key — not a regression. Adopters who want idempotency on this shape should project the request model directly. ✅
• Top-level `idempotency_key` in arg_projector kwargs: fallback path treats kwargs (minus `ctx`/`context`) as the hash source. Dedup works. ✅
• Cross-principal isolation: arg-projected calls use `ctx=` (not `context=`). The resolver checks both. Same key from different principals → different cache scope (no cross-principal replay). ✅
• Conflict on same key + different patch: `IdempotencyConflictError` raised, parity with positional path. ✅

Tests

6 new in `TestWrapArgProjectionCalling`:

• `update_media_buy` shape: cache hit, replay, conflict
• `sync_audiences` style: no-key fall-through
• Top-level `idempotency_key` fallback
• `ctx=` kwarg scope-key extraction (cross-principal isolation)
• Keyword convention (`params=`, `context=`)

154 tests green across the broader idempotency surface (validate-wiring, PG backend, MCP/A2A integration). Lint clean.

Test plan

• `pytest tests/test_server_idempotency.py` — 55 passed (49 existing + 6 new)
• `pytest tests/test_idempotency.py tests/test_validate_idempotency_wiring.py tests/test_pg_idempotency_backend.py` — 154 total green
• `ruff check` — clean
• Salesagent re-applies `@_IDEMPOTENCY.wrap` to `update_media_buy` and verifies storyboard runner passes

🤖 Generated with Claude Code

[bokelley]

Updated with expert-review fixes (code-reviewer + ad-tech-protocol-expert):

Code-reviewer must-fix:

• Falsy-context fall-through: kwargs.get('context') or kwargs.get('ctx') would silently use ctx if context was explicitly None. Switched to in-based check so explicit None wins.
• Multi-Pydantic-kwarg ordering: prefer kwargs literally named params / request / patch before falling back to first-by-iteration. Eliminates dict-order fragility when a tool has two Pydantic models in kwargs.
• Tightened detection from hasattr(model_dump) to isinstance(BaseModel). Duck-typed objects with a model_dump method no longer accidentally match.

Tests added: 4 new in TestWrapResolveCallArgsEdgeCases covering: explicit None context wins, multi-Pydantic preference order, no-preferred-name fallback, duck-typed exclusion.

Protocol-expert open question (#1) — should target IDs (e.g. media_buy_id for update_media_buy) be part of the cache scope so a buggy buyer reusing a key across two different targets gets a CONFLICT rather than a wrong-target replay? Currently scope is principal+tenant only, body hash includes patch (which doesn't carry media_buy_id). I think this is consistent with AdCP #2315's principal-scope model — the spec treats key reuse across targets as buyer error, not server-detected conflict — but worth verifying. Left as a follow-up; not changing scope semantics in this PR.

CI running on the new commit. 59 tests green locally; lint clean.

[bokelley]

Fixes look solid — the in-based context check and isinstance(BaseModel) tightening are exactly the right calls; the duck-typed exclusion in particular was a latent footgun.

On the target-ID scoping question: your read of AdCP #2315 is correct. The principal-scope model places the key-uniqueness burden on the buyer, and key reuse across different targets is buyer error per spec. That said, media_buy_id is a routing discriminant (more like a URL path parameter than a body field), and HTTP-level idempotency convention generally includes the resource identity in scope — so there's a defensible case for optional target-ID-aware scoping as a follow-up. Worth its own issue to track; agreed this is not a blocker for this PR.

---

Generated by Claude Code

[bokelley]

Thanks for the thorough review, @bokelley. Glad the in-based context check and isinstance(BaseModel) tightening land well.

Agreed on the target-ID scoping follow-up — the spec-alignment reasoning (buyer owns key uniqueness) is sound for this PR, and the HTTP-level resource-identity angle is worth its own issue to track cleanly. Won't create it here; leaving that to the team.

Ready for merge when Salesagent's re-application of @_IDEMPOTENCY.wrap to update_media_buy is confirmed green.

---

Generated by Claude Code