Skip to content

fix(idempotency): support @wrap on arg-projected methods (update_media_buy)#564

Closed
bokelley wants to merge 2 commits intomainfrom
claude/issue-559-idem-argproj
Closed

fix(idempotency): support @wrap on arg-projected methods (update_media_buy)#564
bokelley wants to merge 2 commits intomainfrom
claude/issue-559-idem-argproj

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

@bokelley bokelley commented May 4, 2026

Closes #559

Summary

@IdempotencyStore.wrap raised TypeError: missing required positional argument: 'params' when applied to methods the framework dispatches via the arg-projector path (e.g. update_media_buy). The arg-projector calls method(media_buy_id=..., patch=..., ctx=ctx) with no positional params — so the wrapper's params: Any parameter was never filled.

Root cause: the wrapper had no way to obtain the canonical wire-shape Pydantic model needed for RFC 8785 hashing and idempotency_key extraction, since the projected kwargs are a field-subset and cannot be reconstituted into a valid model.

Fix approach: dispatch now threads the original params object as __adcp_params__=params when is_wrapped(method) is True, giving the wrapper the full model without touching the normal call site. The wrapper pops the sentinel, detects the arg-projector convention (params is None and len(field_kwargs) > 0), and uses the canonical model for hashing while forwarding the original projected kwargs to the handler.

Edge case fixed by pre-PR review: direct adopter unit-test calls (e.g. seller.update_media_buy(media_buy_id=..., ctx=ctx) without dispatch or sentinel) previously would have fallen into _prepare(None, ...) and raised a confusing TypeError inside _to_dict. Now the wrapper falls through to the handler when arg_projected=True and the sentinel is absent, so wrapped methods are unit-testable without going through dispatch.

Changes

  • src/adcp/server/idempotency/store.py_MISSING sentinel, updated _wrapped signature (params: Any = None), arg-projector detection, sentinel pop, sentinel-absent fall-through, kwargs.clear() for clarity
  • src/adcp/decisioning/dispatch.pyis_wrapped as _is_idem_wrapped import; conditional __adcp_params__=params injection on both async and sync executor paths; inline comment on the sync path (forward-compat only, since wrap always returns an async def)
  • tests/test_server_idempotency.py — 5 new tests in TestArgProjectedCallingConvention: no-TypeError, cache-hit dedup, payload conflict, no-key fall-through, and direct-call-without-sentinel fall-through

Tested

  • pytest tests/test_server_idempotency.py — 54 passed (5 new regression tests)
  • pytest tests/ (full suite) — 3,940 passed, 1 pre-existing failure (TLS network test unrelated to this change)
  • mypy src/adcp/ — 0 errors
  • ruff check src/ — 0 errors

Pre-PR review

  • code-reviewer: approved after blocker fix — blocker (direct-call without sentinel → confusing TypeError in _prepare) resolved by sentinel-absent fall-through; kwargs = {}kwargs.clear() for clarity; sync executor dead path documented with inline comment. 1 nit surfaced below.
  • ad-tech-protocol-expert: approved — non-breaking per AdCP #2315; RFC 8785 hash equivalence preserved (full wire-shape model hashed, not projected subset); per-principal scope isolation and context-echo contract unaffected; idempotency_key extraction from canonical model is sound.

Nit (not blocking): __adcp_params__ is not a reserved kwarg at the Pydantic level — a request model with a field named __adcp_params__ would have its field value shadowed at the call site by the sentinel. The double-underscore prefix is a reasonable deterrent; the sentinel name is documented in store.py's module docstring as reserved in the arg-projector calling convention.


Triage-managed PR. This bot does not currently iterate on
review comments or PR conversation threads (only on the source
issue). To unblock:

  • Push fixup commits directly: gh pr checkout 560
    fix → push.
  • Or re-trigger: comment /triage execute on the source
    issue.

See adcp#3121
for context.

Session: https://claude.ai/code/session_01HopGAcSiNk5SWbLpE9fd6b


Generated by Claude Code

claude added 2 commits May 4, 2026 10:17
…a_buy)

IdempotencyStore.wrap now handles the arg-projector calling convention
used by dispatch for update_media_buy and similar methods.  Dispatch
threads the original wire-shape Pydantic model as __adcp_params__ so
the wrapper can hash the canonical RFC 8785 flat request (with
idempotency_key at the top level) rather than the projected subset.
The sentinel-based ctx extraction avoids the falsy-object bypass
identified in security review.

Fixes #559

https://claude.ai/code/session_01HopGAcSiNk5SWbLpE9fd6b
…a_buy)

Closes #559. Dispatch threads the original wire-shape Pydantic model as
__adcp_params__ when calling idempotency-wrapped methods via the
arg-projector path, so the wrapper can hash the canonical flat request
(RFC 8785 / AdCP #2315) rather than the projected field subset which
neither carries idempotency_key nor serializes cleanly.

Direct calls without the sentinel (adopter unit tests) now fall through
to the handler instead of raising TypeError inside _prepare.

https://claude.ai/code/session_01HopGAcSiNk5SWbLpE9fd6b
@bokelley bokelley marked this pull request as ready for review May 4, 2026 15:01
@bokelley
Copy link
Copy Markdown
Contributor Author

bokelley commented May 4, 2026

Superseded by #567 (merged today as 1abdae6), which closed #559 with the wrap-side resolution architecture rather than the dispatch-side sentinel approach this draft proposed. Per CLAUDE.md's parallel-race guidance for stale claude-triaged draft PRs: closing rather than rebasing, since the two architectures aren't compatible (#567 explicitly chose against threading dispatch→idempotency coupling). Issue #559 already in 4.4.0 changelog via #567.

@bokelley bokelley closed this May 4, 2026
@bokelley
Copy link
Copy Markdown
Contributor Author

bokelley commented May 4, 2026

Noted — supersedence by #567 (1abdae6) confirmed. Issue #559 is closed via the wrap-side resolution. No further action on this draft.


Triaged by Claude Code. Session: https://claude.ai/code/session_01Rvm9f1JFJLjBfyvVrXVe1A


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@IdempotencyStore.wrap incompatible with arg-projected methods (update_media_buy)

2 participants