feat: add creative format compatibility helpers#887
Conversation
5243789 to
309040f
Compare
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Clean split — the new helpers are pure, the schema rewording is consistent across cache + generated arms + README, and media_buy_response()'s default path is unchanged so existing callers are untouched.
Things I checked
- PR title carries
feat:prefix — release-please will pick up the three new public exports. - Public-API surface:
format_is_supported,formats_are_equivalent,upgrade_legacy_format_idall land insrc/adcp/__init__.pyandtests/fixtures/public_api_snapshot.jsonis updated to match. No drift. - Type-system layering still clean — the refactor extracts
canonicalize_agent_urlintosrc/adcp/canonical_formats/identity.pyand updates both call sites informat_options.py; nothing new reaches intogenerated_poc/. confirmed_atnullability flip on 3.1.0-beta.4 is consistent acrossschemas/cache/3.1.0-beta.4/{bundled/,}{core/tasks-get-response,media-buy/create-media-buy-response,media-buy/update-media-buy-response,media-buy/get-media-buys-response}.jsonand the matching generated Pydantic models. 3.0 stays"type": "string"— explicitnullcorrectly remains invalid there.media_buy_response(confirmed_at=_UNSET)preserves the old auto-_rfc3339_now()default; only an explicitNonechanges behavior. New ValueError onadcp_version="3.0"+confirmed_at=Noneis the right fail-closed choice.ad-tech-protocol-expert: sound-with-caveats — canonical agent URLhttps://creative.adcontextprotocol.orgmatches_fixtures/v1-reference-formats.json; the "bare string upgrades / seller-namespaced FormatId stays put" asymmetry is the correct call (seller namespaces own their own ID semantics; SDK must not silently rebrandseller.example/display_300x250as the AAO canonical template).tests/test_canonical_formats_compatibility.py:101pins that contract.code-reviewer: sound-with-caveats — no blockers, four minor findings rolled into follow-ups below.
Follow-ups (non-blocking — file as issues)
media_buy_responseguard is narrower than its docstring.src/adcp/server/responses.pyraises only whenadcp_version is not None and not _is_adcp_31_or_newer(...). Theadcp_version=None(dispatcher-projected) path letsconfirmed_at=Nonethrough into the response dict. If a downstream 3.0 projection then emits that asnull, we've handed the buyer a 3.0-invalid payload. Tighten the gate or document that the dispatcher is the only safeNoneconsumer.- Legacy upgrade regex covers display sizes only.
_DISPLAY_SIZE_REinsrc/adcp/canonical_formats/compat_helpers.pymatchesdisplay_NxN(_image)?. The shippedsrc/adcp/canonical_formats/_fixtures/v1-reference-formats.jsonalso definesdisplay_NxN_html,display_NxN_generative, andvideo_{1920x1080,1280x720,1080x1920,1080x1080}. Adopters with legacy v1 catalogs will reasonably expect those to upgrade too. Follow-up issue, not a block. upgrade_legacy_format_idpropagatesduration_msinto the upgraded display ID.src/adcp/canonical_formats/compat_helpers.py:73copiesfid.duration_msthrough unconditionally — a caller passing a legacy display ID with a strayduration_msends up with a nonsensicaldisplay_image+ duration. Either dropduration_mswhen the display regex matched, or document the caller contract.- Hand-maintained fields inside
generated_poc/.CreateMediaBuyResponse1.revision/confirmed_atandUpdateMediaBuyResponse1.revisionare added directly insrc/adcp/types/generated_poc/media_buy/{create,update}_media_buy_response.py. The file header already notes these are SDK-maintained backward-compat arms, but a regeneration could silently clobber them. Worth a per-field# manual: not in codegen sourcemarker so future-you doesn't have to git-archaeology the comment.
Minor nits (non-blocking)
- Dead
try/exceptin_coerce_format_id.src/adcp/canonical_formats/compat_helpers.py:43catchesValidationErroronly to bare-raiseit — the wrapper adds nothing. Drop the except. _coerce_format_id("__default__", ...)round-trip.src/adcp/canonical_formats/compat_helpers.py:67performs a full Pydanticmodel_validatepurely to read back the agent URL the caller passed in.canonicalize_agent_url(default_agent_url)is equivalent and avoids coupling the helper to the FormatIdidregex accepting__default__.
Approving on the strength of the layering hygiene plus the 3.0/3.1 wire compat being handled symmetrically across schema cache, generated models, server-DX helper, and tests.
309040f to
3b56114
Compare
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Track A is purely additive public surface; Track B narrows wire shape inside 3.1.0-beta.4 where confirmed_at was never in required[] — feat: with no ! is the right semver signal.
Things I checked
- Public surface diff matches snapshot.
format_is_supported/formats_are_equivalent/upgrade_legacy_format_idland intests/fixtures/public_api_snapshot.jsonexactly once;CANONICAL_CREATIVE_AGENT_URLis exported only fromadcp.canonical_formats, not top-level (intentional — matches snapshot). No public-API removal anywhere. - 3.0 fail-closed on
confirmed_at=None.media_buy_responseatsrc/adcp/server/responses.py:430raisesValueErrorwhenadcp_version="3.0"is paired with explicitNone— correct, since 3.0's schema istype: "string"(no null arm).tests/test_server_dx.pycovers both the 3.1+ allow path and the 3.0 reject path. - Sentinel default for
confirmed_at. Grep acrosssrc/,tests/,examples/confirms no in-repo caller passedconfirmed_at=Noneexplicitly tomedia_buy_response— the soft behavior change (None used to auto-populate, now passes through) breaks nobody in the tree. - Schema/generated alignment.
confirmed_atflipped to["string","null"]increate-media-buy-response,get-media-buys-response,tasks-get-responsebundled+modular; not inupdate-media-buy-response(correct — updates don't re-confirm). Newrevision/confirmed_atfields on hand-curatedCreateMediaBuyResponse1/UpdateMediaBuyResponse1are concrete optional scalars, soaliases.py/_ergonomic.py/_forward_compat.pyneed no updates. canonicalize_agent_urlextraction. Moved out offormat_options.pyintoidentity.py; call sites infind_declaration_by_v1_format_idrewired; not re-exported fromcanonical_formats/__init__.py, so it stays effectively internal.- Seller-owned namespace guard.
upgrade_legacy_format_idcorrectly refuses to rewritedisplay_300x250when the input is a structuredFormatIdwhoseagent_urlis not the canonical AAO host. Covered bytest_upgrade_legacy_display_size_does_not_rewrite_seller_owned_namespace. feat:notfeat!:.confirmed_atwas never inrequired[]on 3.1, so widening to nullable is a beta-internal refinement, not a wire-break. Net-additive helpers, no public removal.release-pleasewill cut a minor.
Follow-ups (non-blocking — file as issues)
upgrade_legacy_format_idname overpromises coverage. Regex is^display_(\d+)x(\d+)(?:_image)?$— display-image-by-size only. Misses load-bearing v1 ids:display_300x250_html,display_300x250_generative,video_1920x1080,video_vast_30s. Either rename toupgrade_legacy_display_size_id, or extend the regex with test coverage.ad-tech-protocol-expertflagged this against_fixtures/v1-reference-formats.json:3338.- Hardcoded v1↔v2 mapping diverges from the registry.
registries/v1-canonical-mapping.jsondefines resolution-order via the catalog'scanonical:annotation, not by enumerated literals. A future AAO catalog change todisplay_300x250_image'scanonical:annotation silently won't reach this helper. Consider deriving the upgrade from the loaded v1 reference catalog and falling back to the regex only as a heuristic — plus surfacingFORMAT_PROJECTION_FAILEDerrors via the SDK advisory channel when the regex path fires. display_1x1is a 1x1 tracker, not adisplay_image. The regex matches it but projecting todisplay_image1x1 is semantically wrong (it's a pixel/tag). Either exclude sub-threshold sizes or whitelist IAB standard sizes.canonicalize_agent_urldoesn't normalize path dot-segments. Spec rule (percore/format-id.json:11) calls for lowercase scheme/host, default-port strip, and path dot-segment normalization. PR does the first two;https://creative.adcontextprotocol.org/./mis-matches today. Low impact in practice.CreateMediaBuyResponse1.revisionisint | Nonebut the README treats it as always-present. README's update example readsrevision = media_buy_result.data.revisionand passes it unconditionally intoUpdateMediaBuyPackagesRequest(revision=revision, ...). In pending/manual-approval flows the seller will legitimately omit revision (consistent with omittingconfirmed_at), making the buyer sendrevision=Noneto the seller. Guard the README example withif revision is not None: ..., or land an SDK helper that handles the unconfirmed-yet path.- Snapshot test for ergonomic alias vs. generated drift. The hand-curated
CreateMediaBuyResponse1/UpdateMediaBuyResponse1now carryconfirmed_at/revisionfields with their own descriptions. Nextmake validate-generatedagainst a re-pulled schema could drift. Worth a test that asserts the ergonomic alias optional-ness agrees with the correspondingResult557-style auto-generated arm.
Minor nits (non-blocking)
_coerce_format_id(\"__default__\", ...)for the default-URL probe.src/adcp/canonical_formats/compat_helpers.py:75constructs a throwawayFormatIdjust to read backdefault_fid.agent_url. Works today becauseFormatId.id's regex allows underscores, but it couples this helper to that regex forever. One-line refactor: cache_CANONICAL_DEFAULT_NORMALIZED = canonicalize_agent_url(CANONICAL_CREATIVE_AGENT_URL)at module load and compare against it directly — no dummy validation per call.- Dead
try/except ValidationError: raise.src/adcp/canonical_formats/compat_helpers.py:42-46catches and re-raises identically. Drop the block. confirmed_at: str | None | object = _UNSET.src/adcp/server/responses.py:398collapses toobjectfor mypy — downstream callers lose static checking. ALiteral[_UNSET]or a dedicated sentinel singleton type would preserve the hint.- Generated-model description drift.
src/adcp/types/generated_poc/media_buy/create_media_buy_response.pysays "should remain stable" while the upstream schema says "remains stable". Cosmetic, but a strict regenerate would overwrite.
Approving.
3b56114 to
3bd8fe9
Compare
There was a problem hiding this comment.
LGTM. Follow-ups noted below. The new helpers operate on the typed FormatId model (not string-blob compares), pull canonicalization into a dedicated identity.py module, and the PROPOSAL_NOT_FOUND recovery flip brings the proposal path into alignment with how MEDIA_BUY_NOT_FOUND already classifies — the right shape on all three.
Things I checked
- New public exports (
format_is_supported,formats_are_equivalent,upgrade_legacy_format_id,CANONICAL_CREATIVE_AGENT_URL) are additive —tests/fixtures/public_api_snapshot.jsonupdated to match. confirmed_atcache widening from required string to[\"string\",\"null\"]is non-breaking: the field was never in anyrequired[]array in 3.1.0-beta.4 — adopters parsing existing responses still see strings on the happy path.- Generated types (
generated_poc/bundled/**) carry description-only diffs consistent with a regeneration pass. - Seller-owned namespace guard at
compat_helpers.py:65-71holds: structured input carrying a non-canonicalagent_urlround-trips untouched (test_upgrade_legacy_display_size_does_not_rewrite_seller_owned_namespace), only bare-string legacy IDs auto-tag canonical. canonicalize_agent_urldoes RFC 3986 §6 host-casefolding + default-port stripping —test_canonical_format_helpers_canonicalize_agent_url_case_and_default_portpins this.- Security: `security-reviewer` confirmed the terminal→correctable flip on PROPOSAL_NOT_FOUND introduces no new oracle — the cross-tenant branch and the missing-record branch raise byte-identical errors (same code, message template, field, now same recovery). The store-layer `get()` collapses cross-tenant to `None` at `proposal_store.py:489` before the recovery field is ever set.
Follow-ups (non-blocking — file as issues)
- `media_buy_response` behavior change on `confirmed_at=None`. Old path was `confirmed_at or _rfc3339_now()` — every `None` got silently auto-filled. New path raises `ValueError` when `adcp_version="3.0"` and preserves `None` on 3.1+. Existing callers that explicitly passed `None` hit new behavior. The `feat:` prefix without `!` is borderline-defensible because the omitted-kwarg path is unchanged, but worth calling out in the next release notes.
- 3.0 helper sharpness (per `ad-tech-protocol-expert`). In 3.0, `confirmed_at` is optional but non-nullable — the schema-valid projection for "unknown commitment time" is to omit the key, not auto-fill `now()`. Auto-fill is pre-existing behavior, not new here, but it is "a real lie about seller commitment" for pending/manual-approval buys. Consider an `omit_when_null` path so adopters running 3.0 and 3.1 in one code path don't conditional at every call site.
- `revision` description should name the error code. Per `ad-tech-protocol-expert`: AdCP 3.1.0-beta.4 uses `CONFLICT` for stale-revision rejection (the SDK helper catalog already registers it: "Revision conflict - refetch and retry"). The README addition and the regenerated `Field(description=...)` blobs say "if a seller rejects an update because the supplied revision is stale" without naming `CONFLICT`. Adopters reading the docstring won't know which `error.code` to switch on.
- Docstring drift at `src/adcp/decisioning/proposal_lifecycle.py:71`. The class docstring still asserts `(recovery=terminal)` for the cross-tenant probe case. The raise underneath now emits `correctable`. Stale wording on the surface that documents the anti-enumeration property.
- Catalog inconsistency on `PROPOSAL_EXPIRED`. `src/adcp/decisioning/proposal_lifecycle.py:148` raises with `recovery="terminal"`; `src/adcp/server/helpers.py:37` lists `PROPOSAL_EXPIRED` as `correctable`. One should give.
- Hand-maintained shim in `generated_poc/`. `CreateMediaBuyResponse1` and `UpdateMediaBuyResponse1` gained `confirmed_at` and `revision` fields. The in-file comment explicitly calls these out as hand-maintained back-compat aliases, but the file header still names `datamodel-codegen` — confirm the regen script preserves the shim class additions, or move the shims to a non-`generated_poc/` module so the boundary is unambiguous.
- Docs drift on new public exports. `format_is_supported` / `formats_are_equivalent` / `upgrade_legacy_format_id` aren't mentioned in `README.md`, `AGENTS.md`, or `llms.txt`. The README's media-buy section gained a revision/confirmed_at paragraph but the format-compat helpers are the headline feature in the PR title and don't appear in the user-facing docs.
- Rate-limit guidance for `correctable` `*_NOT_FOUND` codes (per `security-reviewer` Low). No new oracle, but `correctable` signals retry middleware to keep going — adopters should cap automatic retries on `*_NOT_FOUND` and rate-limit emissions per principal at the HTTP front. Worth a sentence in the `_CORRECTABLE_ERRORS` documentation.
Minor nits (non-blocking)
- Throwaway `FormatId` to read a URL we already have. `compat_helpers.py:65` builds `default_fid = _coerce_format_id("default", default_agent_url=default_agent_url)` just to compare `canonicalize_agent_url(fid.agent_url)` against `canonicalize_agent_url(default_fid.agent_url)`. `default_agent_url` is already a plain string in scope — `canonicalize_agent_url(default_agent_url)` is the same answer without minting a model instance whose only purpose is to be discarded. A notable choice.
- `str | None | object = _UNSET` collapses type narrowing. `src/adcp/server/responses.py:400`. Every `str` and `None` is an `object`, so mypy/pyright will accept anything. Standard pattern: `class _UnsetType: ...; _UNSET: Final = _UnsetType()` and annotate `str | None | _UnsetType`.
- `duration_ms` propagated through a `display_image` upgrade. `compat_helpers.py:75`. A legacy `display_300x250` ID carrying a `duration_ms` is incoherent — either drop it for display upgrades or assert `duration_ms is None` before forwarding.
- `try/except ValidationError: raise` is a no-op. `compat_helpers.py:33-39`. Drop the wrapper or comment what it's defending.
- `TypeError` message at `compat_helpers.py:43` should name the offending type to aid debugging.
LGTM. Follow-ups noted below.
There was a problem hiding this comment.
LGTM on the helper surface and the description-tightening. Holding for one missing artifact: the schemas/cache/ edits need a schemas/patches/ entry per the documented policy.
Things I checked
src/adcp/canonical_formats/compat_helpers.py:73upgradesdisplay_300x250→id="display_image". Verifieddisplay_imageis a realFormatId.id(not a fabricated kind) — present atsrc/adcp/canonical_formats/_fixtures/v1-reference-formats.jsonunderagent_url: https://creative.adcontextprotocol.org/. Helper isn't inventing an ID. Theformat_kindenum atcore/canonical-format-kind.jsonis a separate axis (12-member closed enum) — not what this helper traffics in.formats_are_equivalentvsformat_is_supportedsemantics: equivalence is family-match with non-conflicting params (omitted = unspecified), gating requires the request to state every fixed param the supported side declares. Tests attests/test_canonical_formats_compatibility.py:62and:84exercise both directions. Right shape.identity.py:12extraction ofcanonicalize_agent_url. Host-casefold, default-port strip, malformed pass-through preserved. Thefind_declaration_by_v1_format_idcallers informat_options.py:184/188migrated cleanly.- New public exports
format_is_supported,formats_are_equivalent,upgrade_legacy_format_id— additive,feat:semver is right. Snapshot fixturetests/fixtures/public_api_snapshot.jsonupdated. src/adcp/types/generated_poc/media_buy/{create,update}_media_buy_response.pyhand edits addingconfirmed_at/revisiontoCreateMediaBuyResponse1/UpdateMediaBuyResponse1. The file-level docstring at lines 10-12 explicitly carves these out as SDK-maintained arms ("Backward-compatible SDK response arms... not regenerated"). Hand edit is legitimate here.- Schema widening
confirmed_at: string→[string, null]is non-breaking (the field is not in therequiredlist atschemas/cache/3.1.0-beta.4/media-buy/create-media-buy-response.json:104). Wire-shape relaxation, not a contract break. - Four
recovery=\"terminal\"→recovery=\"correctable\"flips onPROPOSAL_NOT_FOUNDacrossproposal_store.py,pg/proposal_store.py,proposal_dispatch.py,proposal_lifecycle.py, plus the matching test updates and the new e2etest_refine_unknown_proposal_is_correctable_not_found. Cross-tenant probe still returns the same code with the same recovery, so the principal-enumeration defense (proposal_lifecycle.py:68-72 docstring) is intact.
Major (resolve before merge or in a fast follow-up)
schemas/cache/3.1.0-beta.4/**hand-edited without aschemas/patches/entry. Six cache JSONs touched (media-buy/create-media-buy-response.json,media-buy/update-media-buy-response.json,media-buy/get-media-buys-response.json, and theirbundled/siblings, plusbundled/core/tasks-get-response.json).generatedAttimestamp unchanged (2026-05-26T03:04:14.740Z) and the narrow 6-file scope rules out a fresh sync. Project policy atschemas/patches/README.md:123-125is explicit: "Don't editschemas/cache/without a corresponding.patchfile. Next regen overwrites the edit." The README cites PR #791 as the exact failure mode — silent revert on regen while the Pydantic-model layer keeps the new shape. CI'sschema-checkjob skips drift validation on beta versions (.github/workflows/ci.ymlgates onis_prerelease != 'true'), so this slips through automated gates today and surfaces on the next stable-version pin. Fix: addschemas/patches/0N-confirmed-at-nullable.patchwith the documentedPatch / Reason / Filed / Upstream status / Drop whenheader, and either (a) confirm upstream 3.1.0-beta.4 actually permitsnullhere (then the patch documents "upstream landed, will go away on next regen") or (b) flag this as a forward-looking patch pending the upstream adcp PR.
Follow-ups (non-blocking — file as issues)
examples/seller_agent.py:660-666returnsPACKAGE_NOT_FOUNDwhen the media buy itself is missing. Branch fires whenever any package inparams[\"packages\"]has apackage_idAND the buy doesn't exist. Misleading: buyers retry against a differentpackage_idand never recover. Checkmb_id in media_buysfirst; reservePACKAGE_NOT_FOUNDfor the existing in-loop site where the buy is found but the package isn't. Lower stakes because this is example/test-controller code, but it's the storyboard buyers learn from.src/adcp/server/responses.pymedia_buy_response(confirmed_at: str | None | object = _UNSET). Type annotation widens toobject, which mypy will not narrow at call sites — a caller passingconfirmed_at=42type-checks. Prefer a typed sentinel (class _UnsetType: ...; _UNSET: Final = _UnsetType()) and annotatestr | None | _UnsetType = _UNSET.
Minor nits (non-blocking)
src/adcp/canonical_formats/compat_helpers.py_coerce_format_id. Thetry: return FormatId.model_validate(body) except ValidationError: raiseblock is a no-op — bareraiseadds nothing over letting the exception propagate. Drop the try/except.src/adcp/canonical_formats/compat_helpers.py:74upgrade_legacy_format_id.default_fid = _coerce_format_id(\"__default__\", default_agent_url=default_agent_url)is a throwaway pydantic round-trip to read backdefault_agent_url. Inline ascanonicalize_agent_url(default_agent_url).src/adcp/canonical_formats/__init__.py__all__ordering. New entries break the file's local alphabetical-within-case grouping (CANONICAL_CREATIVE_AGENT_URLafterRegistryLoadError,format_is_supportedbeforefind_declaration_by_kind,upgrade_legacy_format_idafterupgrade_v1_trackers). Snapshot fixture is sorted so this doesn't fail tests — just file-local consistency.src/adcp/server/responses.pymedia_buy_responsedocstring. Repeats the "confirmed_at=Noneis only schema-valid for AdCP 3.1+" guidance twice.
Add the patches file and I'll approve.
Summary
upgrade_legacy_format_id,formats_are_equivalent, andformat_is_supportedCloses #885.
Closes #886.
Validation
uv run --extra dev python -m pytest tests/ -quv run --extra dev python -m pytest tests/test_canonical_formats_compatibility.py tests/test_canonical_formats_options.py tests/test_server_dx.py tests/test_public_api.py::test_public_api_surface_matches_snapshot -quv run --extra dev mypy src/adcp/uv run --extra dev ruff check src/adcp/canonical_formats/compat_helpers.py src/adcp/canonical_formats/format_options.py src/adcp/canonical_formats/identity.py src/adcp/canonical_formats/__init__.py src/adcp/__init__.py tests/test_canonical_formats_compatibility.py tests/test_canonical_formats_options.py tests/test_server_dx.pyuv run --extra dev make validate-generatedgit diff --check