Skip to content

fix(webhooks): promote McpWebhookPayload.token from extras shim to typed field#835

Merged
bokelley merged 1 commit into
mainfrom
claude/issue-638-drop-token-extras-shim
May 26, 2026
Merged

fix(webhooks): promote McpWebhookPayload.token from extras shim to typed field#835
bokelley merged 1 commit into
mainfrom
claude/issue-638-drop-token-extras-shim

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

@bokelley bokelley commented May 23, 2026

Closes #638

Upstream adcontextprotocol/adcp#4339 promoted token from an undocumented additionalProperties round-trip to a typed field in mcp-webhook-payload.json. The generated type already has token: Annotated[str | None, Field(max_length=4096, min_length=16)] = None (regenerated 2026-05-22). This PR removes the extras shim from create_mcp_webhook_payload and wires token directly as a typed kwarg.

Wire shape is unchanged — adopters reading to_wire_dict() output see no difference. Adopters using payload.token now get typed attribute access instead of model_extra["token"], and McpWebhookPayload.model_fields now includes token.

Note: min_length=16 is now enforced at construction time on the typed field. Under the old extras-bag path no length validation ran, but push-notification-config.json already enforces minLength: 16 on the buyer side — any token shorter than 16 chars was already schema-invalid at registration. This is a correct tightening, not a latent break.

What changed

  • src/adcp/webhooks.py: removed the extras: dict[str, Any] shim block; "token": token is now passed directly in the model_validate call like every other typed field.
  • tests/test_webhooks_to_wire_dict.py: added test_token_is_typed_field_not_model_extra (checks model_fields, attribute access, wire parity) and test_token_none_omitted_from_wire.

What was tested

  • pytest tests/test_webhooks_to_wire_dict.py — 13/13 passed (2 new)
  • pytest tests/test_webhook_handling.py tests/test_webhooks_deliver.py — 140 passed, 15 skipped
  • ruff check src/adcp/webhooks.py tests/test_webhooks_to_wire_dict.py — all checks passed
  • mypy src/adcp/webhooks.py — no issues found

Nits (not fixed, noted for reviewer):

  • create_mcp_webhook_payload docstring for the token kwarg could mention payload.token now provides direct typed attribute access (dx-expert nit).
  • test_webhooks_to_wire_dict.py already imports McpWebhookPayload from the internal generated path (adcp.types.generated_poc…) rather than adcp.types — pre-existing, not introduced here; layering test still passes.
  • Tests don't cover the min_length=16 rejection path for token; follow-on candidate.

Pre-PR review

  • code-reviewer: approved — no blockers; nits on test coverage boundary and parity assertion strength
  • dx-expert: approved — no blockers; mechanical change is correct, wire shape identical, both regression tests substantive

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 835
    fix → push.
  • Or re-trigger: comment /triage execute on the source
    issue.

See adcp#3121
for context.

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

…ped field

Upstream adcontextprotocol/adcp#4339 added token as a typed field in
mcp-webhook-payload.json. The generated type already has the field;
remove the additionalProperties round-trip shim from
create_mcp_webhook_payload and wire token directly like any other
typed kwarg.

Wire shape is unchanged — adopters reading to_wire_dict() output see
no difference. Adopters using payload.token now get typed attribute
access instead of model_extra["token"].

Closes #638
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Follow-ups noted below. The extras→typed promotion is the right shape: wire output is byte-identical for valid inputs, and the construction-time tightening just enforces what push-notification-config.json already required at registration.

Things I checked

  • Wire parity: to_wire_dict uses model_dump(mode="json", exclude_none=True) (src/adcp/webhooks.py:1884), so token=None still drops out of the wire dict and a supplied token serializes at the same JSON path it did under the old extras path. The new test_token_is_typed_field_not_model_extra asserts hand-built vs typed-kwarg parity at tests/test_webhooks_to_wire_dict.py:268-277.
  • Generated field matches upstream: token: Annotated[str | None, Field(max_length=4096, min_length=16)] = None at src/adcp/types/generated_poc/core/mcp_webhook_payload.py:84-91. Bounds mirror the sender-side push_notification_config.token at push_notification_config.py:52-57 — spec text explicitly says "Length bounds mirror the config-side field," so SDK-side enforcement is the publisher obligation the schema describes.
  • extra='allow' is still set on the model (mcp_webhook_payload.py:19), so other unknown adopter-side keys continue to round-trip — this isn't a wholesale lockdown, just promoting one field.
  • No other call sites in src/adcp/ read model_extra["token"] on McpWebhookPayload. src/adcp/webhook_sender.py:590-602 already passes token as a typed kwarg.
  • Receiver side is unaffected: verify_webhook_signature / verify_webhook_hmac operate on raw bytes, not the parsed model, so the typed-vs-extras distinction is invisible to verification.
  • Layering check: tests file imports McpWebhookPayload from adcp.types.generated_poc.core.mcp_webhook_payload — pre-existing and the layering test (tests/test_import_layering.py) scopes SRC_ROOT.rglob only, so test imports are out of scope. Not introduced here.
  • Asymmetry with the legacy _inject_push_token path at webhooks.py:1941-1965 is intentional — that path covers non-MCP generic webhook bodies which remain free-form.

Follow-ups (non-blocking — file as issues)

  • No test asserts a sub-16-char token raises ValidationError at construction. The min_length=16 enforcement is the one new runtime behavior; worth a one-line regression guard. PR body already flagged this.
  • create_mcp_webhook_payload docstring (src/adcp/webhooks.py:170-171) doesn't surface the 16-char minimum — callers would have to dive into the generated model to find it.
  • ad-tech-protocol-expert raised the open question of whether the JS SDK applies the same minLength: 16 at construction post-#4339, or only at the receiver. Asymmetric enforcement across SDKs would create an interop trap. Worth confirming before adopters cross-wire.

LGTM.

@bokelley bokelley merged commit 3832433 into main May 26, 2026
25 checks passed
@bokelley bokelley deleted the claude/issue-638-drop-token-extras-shim branch May 26, 2026 00:42
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.

feat(webhooks): drop extra='allow' token round-trip once adcp#4339 lands

2 participants