feat(types): support AdCP 3.1 beta 7#895
Conversation
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Clean schema-pin bump that brings the SDK back into wire conformance — the post-generate fix restoring confirmed_at/revision as required matches schemas/cache/3.1.0-beta.7/media-buy/create-media-buy-response.json required: ["media_buy_id", "confirmed_at", "revision", "packages"] and update-media-buy-response.json required: ["media_buy_id", "revision"]. The 805-file diff is mostly codegen variant-numbering churn (Disclosure522 → Disclosure754, VerifyAgent1043 → VerifyAgent1047) — the documented instability from CLAUDE.md, absorbed by the alias layer.
Things I checked
update_media_buy_response()raise atsrc/adcp/server/responses.py:489— schema-faithful.UpdateMediaBuySuccess.requiredincludesrevision, so fail-closed on missingrevisionfor 3.1+ is correct.revisionplacement moved into the early dict construction atsrc/adcp/server/responses.py:496-498, which serializes consistently.CreateMediaBuyResponse1/UpdateMediaBuyResponse1field additions atscripts/post_generate_fixes.py:2089-2093and:2167—confirmed_at: AwareDatetime | None(required-but-nullable) is the correct Pydantic translation fortype: ["string", "null"]+ listed inrequired; respects the schema'sallOfinvariant banningmedia_buy_status="active"+confirmed_at: null.- Manifest patch retarget
schemas/patches/03-manifest-signal-owned-discovery-only.patch— comment honestly updated beta.5→beta.7, line offsets shifted 1394→1414 consistent with surrounding manifest growth. Upstream bug it corrects (signal_ownedlisted underactivate_signal.specialisms) still present in beta.7. - The Signals
Scope"swap" (-pricing/-signals/-wholesale_feed, +countries/+date_range/+kind/...) is NOT a wire regression — it's a brand-newScopeclass minted by codegen for the additivesignal_coverage_forecast.scopeobject (schemas/cache/3.1.0-beta.7/core/signal-coverage-forecast.json). The original top-levelmissing_data.scopeenum atsignals/get-signals-response.json:148-156is unchanged. SCHEMA_DELTAS keyed on class name; reads worse than the wire actually is. - Conventional-commit semver. Precedent on main:
1bae6e89 feat: support AdCP 3.1 Beta 4,bdf79efb chore: bump AdCP schemas to beta.5. Schema-pin bumps within a 6.x-beta line usefeat:without!— title matches. No semver block. - Test coverage of the new contract —
test_typed_success_requires_revision_and_confirmed_at(tests/test_server_dx.py:241),test_typed_success_requires_revision(tests/test_server_dx.py:332), andtest_update_response_requires_revision_for_current_shape(tests/test_server_helpers.py:413) all pin the new raise behavior.
Follow-ups (non-blocking — file as issues)
fix_signal_coverage_forecast_point_typesis silent-failure-prone. The new post-generate fix atscripts/post_generate_fixes.py:2634-2687does fivestr.replace(..., count=1)calls with no marker-drift guard. The first replace — a multi-line import-block rewrite — is brittle: if upstream codegen ever alphabetizes the import block or drops theForecastPointimport (because none of the rewritten classes reference it directly), the first replace silently no-ops, the subsequent class-line replaces still succeed, and the file ships referencing the unimportedforecast_pointmodule — runtimeNameErroron import. Addassert new is not old, "marker drift in <field>"after each replace and fail the post-generate run loudly. Same shape as the codegen-renumbering pitfall called out in CLAUDE.md.AwareDatetimeis stricter thanformat: date-timestrictly requires. RFC 3339 implies tz-aware so this is defensible, but a 3.1-beta seller emitting a naive ISO timestamp will now reject. Worth one line in SCHEMA_DELTAS.md / migration notes.- PR body could call out that the 805-file diff is ~99% codegen variant-numbering churn. Helps future maintainers (and Argus) triage similar beta bumps without reading every Watermark/Disclosure/VerifyAgent rename.
Minor nits (non-blocking)
- API asymmetry on
adcp_version=None.update_media_buy_responseatsrc/adcp/server/responses.py:489treatsadcp_version is Noneas "implicitly 3.1+, revision required."create_media_buy_responseat L428 only raises on explicit sub-3.1. Docstring documents the new strict default, but the asymmetry will trip adopters who learn one helper and assume the other matches. Either align them or call the divergence out in both docstrings. confirmed_atUX for provisional buys. Direct constructors ofCreateMediaBuySuccessResponse(...)must now explicitly passconfirmed_at=Nonewhen the buy is provisional — they can't just omit it. Themedia_buy_responsebuilder handles this transparently via the_UNSETsentinel; the public alias does not. Schema's UX, not a PR bug, but a one-line note on the model docstring would save a support round-trip.
LGTM. Follow-ups noted below.
There was a problem hiding this comment.
Holding for the breaking-change marker. Diff is correct — the block is the missing semver signal, not the code.
Schema bump is faithful to upstream 3.1.0-beta.7 (ad-tech-protocol-expert: sound — update-media-buy-response.json success arm declares required: ["media_buy_id", "revision"]; create-media-buy-response.json requires ["media_buy_id", "confirmed_at", "revision", "packages"] with confirmed_at typed ["string","null"]). Helper tightening at src/adcp/server/responses.py:491 is the right shape — fail-closed at build time beats shipping a non-conformant payload. Examples migrated. Alias layer untouched by the codegen renumber.
Blocking
- Required-field flip on public Pydantic models, no
!.scripts/post_generate_fixes.py:2089-2090,2167addsrevision: int(no default) andconfirmed_at: AwareDatetime | None(no default) toCreateMediaBuyResponse1, andrevision: inttoUpdateMediaBuyResponse1. Both re-export throughadcp.typesasCreateMediaBuySuccessResponse/UpdateMediaBuySuccessResponse. Adopter code that constructed these typed models withoutrevisionworked under beta.6 and now raisesValidationError. PR title isfeat(types): support AdCP 3.1 beta 7— no!, noBREAKING CHANGE:footer. release-please will minor-bump from6.3.0-beta.4instead of cutting the major. Fix: retitle tofeat(types)!: support AdCP 3.1 beta 7 (revision/confirmed_at now required), or add aBREAKING CHANGE:footer to the merge commit body namingCreateMediaBuySuccessResponseandUpdateMediaBuySuccessResponse. Either signal is fine; one of them is required before merge.
Things I checked
- Upstream schema actually requires the fields. Verified
successarms inschemas/cache/3.1.0-beta.7/bundled/media-buy/update-media-buy-response.jsonandcreate-media-buy-response.json. The runtime tightening is faithful, not over-reach. - Version-axis gating is correct.
src/adcp/server/responses.py:491fires only whenadcp_version is None(treated as 3.1+ default) or 3.1+. 3.0 branch still allows omission. Matches the schema split. - Alias layer survives the codegen renumber.
Metric4 → Metric5,VerifyAgent867 → VerifyAgent871,SignalRef4-6 → SignalRef7-9,Payload1-8 → Payload13-21— no references inaliases.py,_ergonomic.py,_forward_compat.py, orcapabilities.pyto the changed numbers._generated.pyabsorbs the shift in lockstep, which is the contract per CLAUDE.md. - Example migrations are the adopter evidence.
examples/v3_reference_seller/src/platform.pyinitializes_buy_revisionson create and increments on update;examples/sales_proposal_mode_seller/src/platform.pydoes the same with an instance field. Both now threadrevision=to the typed builders. That is the migration path adopters need to follow. - Manifest patch retargets cleanly from
cache/3.1.0-beta.5/tocache/3.1.0-beta.7/. Same body, new paths. ADCP_VERSIONreads3.1.0-beta.7at the head, matches the cache directory.signal_coverage_forecast.Point.metricspost-gen narrowing is sound — upstreamsignal-coverage-forecast.jsonrequirescoverage_rateon every point'smetrics, transitively forcingmetricspresent per point.RegistryEvent1..16family is coherent, not a partial regeneration — 16event_typeenum values map 1:1 to the codegen variants and the named payload classes all resolve to$defsin the upstream schema.
Follow-ups (non-blocking — file as issues)
- Migration note for
revisionandconfirmed_at. AMIGRATION_v6.3_to_v7.md(or whatever the major lands as) documenting the required-field flip on the two success response models would land the adopter context. The examples are the only current migration evidence; a dedicated doc beats grepping examples. confirmed_at: AwareDatetime | None(no default) reads as "optional" but is required-with-nullable-value. Pydantic treats it correctly, but the type annotation alone reads as kwarg-default-optional to a casual reader. A one-line docstring on the patchedCreateMediaBuyResponse1("confirmed_at=Nonemust be passed explicitly for provisional buys") would prevent the misread.SCHEMA_DELTAS.mddoesn't cross-reference the post-fix re-add. The-confirmed_at, -revisionlines onmedia_buy/create_media_buy_response.pywill puzzle the next reader who doesn't knowpost_generate_fixes.pyadds them back. One line cross-referencing the fix would close the loop.
Minor nits
update_media_buy_response()ValueError message is terse.src/adcp/server/responses.py:492reads"revision is required for AdCP 3.1+ update_media_buy_response". Tell the caller what to do:"update_media_buy_response requires revision=<int> for AdCP 3.1+ (pass adcp_version='3.0' for the legacy shape)". Mirrors the 3.0 error message at L429.
Safe to merge once the title carries ! or the merge commit body carries BREAKING CHANGE:.
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
There was a problem hiding this comment.
LGTM. Spec-correct beta.7 pin with the required revision / confirmed_at enforcement landing fail-closed at both the Pydantic layer and the helper layer.
Things I checked
- Required-ness aligns with upstream.
schemas/cache/3.1.0-beta.7/media-buy/create-media-buy-response.jsonlists["media_buy_id", "confirmed_at", "revision", "packages"]in the success-branchrequiredarray;update-media-buy-response.jsonlists["media_buy_id", "revision"]. The post-fix atscripts/post_generate_fixes.py:2089-2093(CreateMediaBuyResponse1) and:2164-2167(UpdateMediaBuyResponse1) is the right type-level enforcement, andupdate_media_buy_response()atsrc/adcp/server/responses.py:491-492is the matching helper gate._is_adcp_31_or_newershort-circuits toTrueon parse failure and the gate reads(adcp_version is None or _is_adcp_31_or_newer(adcp_version)) and revision is None, so the only path that omitsrevisionwithout raising is an explicit pre-3.1adcp_version—test_explicit_30_status_shapeattests/test_server_dx.py:264-267exercises that. Pre-3.1 callers are preserved. - Manifest patch retargeted correctly.
schemas/patches/03-manifest-signal-owned-discovery-only.patchnow points atschemas/cache/3.1.0-beta.7/manifest.json. Cached schema's post-patch lines 18-25 (activate_signal.specialisms = ["signal_marketplace"]) and 1409-1419 (signal_owned.exercised_toolswithoutactivate_signal) reflect the patched state —sync_schemas.pywould have failed loud otherwise. Same conformance concern as beta.5. - Alias layer is renumber-resilient by design.
aliases.py:1407-1424uses_format_asset_class()/_repeatable_asset_group_class()to iteratevars(_format_module)and select by field-default introspection, not numeric class name. Greppedaliases.pyfor every renumbered class in SCHEMA_DELTAS (Disclosure[0-9]+,VerifyAgent[0-9]+,Watermark[0-9]+,Provenance[0-9]+,Role[0-9]+, etc.) — zero structural hits. TheAssets94references at lines 1393, 1620, 1674, 1709, 1758, 1760, 1965 are docstring/comment drift only. - Forward-compat patches still resolve.
src/adcp/types/_forward_compat.py:31-32importsFormat(present atgenerated_poc/core/format.py:488) andRepeatableAssetGroupvia the dynamic alias (now lands onAssets39atgenerated_poc/core/format.py:455). - Examples consistent.
_buy_revisionsis initialized in__init__atexamples/v3_reference_seller/src/platform.py:741;_MediaBuyinsales_proposal_mode_sellergainsrevision: int = 1;seller_agent.pybumpsmb[\"revision\"]on every mutating path. - Test coverage. New
test_typed_success_requires_revision_and_confirmed_at/test_typed_success_requires_revisionlock the Pydantic side;test_update_response_requires_revision_for_current_shapelocks the helper-sideValueError.
Follow-ups (non-blocking — file as issues)
SCHEMA_DELTAS.mdsays fields were removed when they were actually strengthened. The entriesmedia_buy/create_media_buy_response.py: CreateMediaBuyResponse1: -confirmed_at, -revisionandmedia_buy/update_media_buy_response.py: UpdateMediaBuyResponse1: -revisionread as deletions in release notes. The on-disk types atgenerated_poc/media_buy/create_media_buy_response.py:26-35andupdate_media_buy_response.py:28-37carry both fields — just without defaults instead ofAnnotated[..., Field(...)] = None.scripts/diff_generated_types.py:64is keying offAnnAssign.target.idpresence, which doesn't distinguish required-vs-optional. Either tighten the diff tool to surface required-ness flips, or annotate this entry explicitly so buyers reading the changelog don't think the spec dropped fields it actually mandated.feat(types):understates the helper contract change.update_media_buy_response()now raisesValueErrorwhen called withoutrevisionagainst 3.1+ or an unversioned adopter — previously it silently produced a non-compliant response. Pre-3.1 callers are preserved by the gate, so blast radius is narrow, but aBREAKING CHANGE:footer orfeat!would have given release-please the right signal. Worth a brief migration note in the release PR body.- Stale
Assets94references inaliases.pycomments (lines 1393, 1620, 1674, 1709, 1758, 1760, 1965). Beta.7's repeatable group isAssets39; the dynamic resolver finds it, but the comments are doc rot.
Minor nits (non-blocking)
fix_signal_coverage_forecast_point_types()is silently no-op-safe.scripts/post_generate_fixes.py:2654-2682runs foursource.replace(old, new, 1)calls and never checks any matched. If a futuredatamodel-code-generatorrelease reflows the imports or metrics-field whitespace, every replace silently no-ops, the file is written unchanged, and the success log still prints "aligned Point field overrides."mypy --strictwould eventually catch it via thePointoverride failure, but the script itself should match the loud-fail posture of the other fixes —assert \"class Point(forecast_point.ForecastPoint):\" in sourceafter, or comparesource != originalbefore writing.- Manifest patch hunk-2 header is off-by-one.
schemas/patches/03-manifest-signal-owned-discovery-only.patch:26says@@ -1414,7 +1414,6 @@but the first context line\"get_signals\"actually sits at line 1413 in the post-patch manifest. GNUpatchabsorbs this via default fuzz (offset -1 lines) sosync_schemas.pykeeps working, but the header should read@@ -1413,7 +1413,6 @@. First hunk is correct. - No regression test for the new helper
ValueError.tests/test_server_dx.pycovers the Pydantic-sideValidationError, andtest_update_response_requires_revision_for_current_shapeintests/test_server_helpers.py:413-417covers the helper, but only with defaultadcp_version. Add a sibling that pinsadcp_version=Noneand another at the explicit 3.1+ version string to nail the gate's full surface. if params.get(\"revision\") and ...atexamples/seller_agent.py:674is falsy forrevision=0. Not a real bug — revisions start at 1 — but useis not Noneto match the convention elsewhere in the file.
Approving on the strength of the spec-alignment plus the fail-closed gate.
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Tightens Python-layer enforcement of confirmed_at / revision to match the actual required arrays in schemas/cache/3.1.0-beta.7/media-buy/{create,update}-media-buy-response.json — _generated.py and responses.py end up agreeing with the wire contract instead of papering over it.
Things I checked
SCHEMA_DELTAS.mdshowingCreateMediaBuyResponse1: -confirmed_at, -revisionis a misleading codegen-diff signal — it tracks loss of inlineField(description=…)annotations, not the underlying fields. The beta.7 JSON Schemas still require both.ad-tech-protocol-expert: sound._generated.py:2089-2095:confirmed_at: AwareDatetime | None(no default, nullable) mirrors the wiretype: ["string","null"]in therequiredarray. Required-but-nullable is the right shape, not a Pydantic footgun.src/adcp/server/responses.py:488:ValueErrorguard fires whenadcp_version is None or _is_adcp_31_or_newer(...). Fail-closed beats fail-open here — pre-3.1 callers can still passadcp_version="3.0"explicitly; the only callers who now break are ones that were silently emitting non-conformant 3.1 responses.- New tests
test_typed_success_requires_revision_and_confirmed_atandtest_typed_success_requires_revisionintests/test_server_dx.pygenuinely assertValidationErroron omission — validating new behavior, not just keeping CI green. - Examples thread
confirmed_at/revisionthrough the full lifecycle:seller_agent.py:619-720,v3_reference_seller/src/platform.pyadds_order_confirmed_athelper and bumps revisions viaself._buy_revisions.setdefault,sales_proposal_mode_seller/src/platform.pyincrementsbuy.revisionon update. schemas/patches/03-manifest-signal-owned-discovery-only.patchcorrectly re-targets the beta.5→beta.7 refs; the@@ -1394→@@ -1414hunk shift matches the regenerated manifest offsets.- Type-system layering: only
_generated.pyimportspydantic.AwareDatetime; no breach oftests/test_import_layering.pyallowlist. - New
fix_signal_coverage_forecast_point_typesinscripts/post_generate_fixes.py:2634aligns Point/Metrics/CoverageRate field overrides for strict mypy via subclass inheritance — the schema intent (sibling-type narrowing) is preserved. - 818 files / +711k / -20k — the bulk is
schemas/cache/3.1.0-beta.7/**andsrc/adcp/types/generated_poc/**regeneration churn, expected.
Follow-ups (non-blocking — file as issues)
revision: intloses the schema'sminimum: 1bound at_generated.py:2092and:2167. WorthAnnotated[int, Field(ge=1)]so the Python layer matches the wire constraint, not just the type. Flagged byad-tech-protocol-expert.adcp_version=None → requires revisionis a behavior change worth a docstring line atsrc/adcp/server/responses.py:478-487. The existing docstring says "When omitted, the dispatcher projects by the buyer's requested version" — add a sentence that "omitted version" now also implies "must supply revision" so adopters reading the helper don't get surprised by the newValueError.- Required-nullable on a public model is observable to adopters constructing
CreateMediaBuySuccessResponse(...)withoutconfirmed_at=. Conventional commits isfeat:notfeat!:, consistent with the1bae6e89 feat: support AdCP 3.1 Beta 4precedent and the6.x.x-beta.xpre-release lane. Not a block — but a one-line migration note in the release-please changelog body would soften the transition for adopters on6.3.0-beta.5. - String-replacement patches in
fix_signal_coverage_forecast_point_typessilently no-op on whitespace drift. Consistent with the other fixers in this file (house style), so not blocking — butassert source.count(old) == 1before each replace would catch upstream codegen changes early. Worth a sweep across allpost_generate_fixes.pyfixers as one follow-up.
Minor nits (non-blocking)
- PR description framing. "SDK-invented tokens" slightly mis-frames the fix — it's wire-conformance, not token provenance. Worth a one-line edit before merging the release-please-generated changelog notes.
Approving on the strength of expert-confirmed wire fidelity plus the two new ValidationError-asserting tests.
Pin the SDK's bundled AdCP protocol schema to 3.1.0-beta.7, add the beta.7 schema cache, and regenerate the generated Pydantic surface plus
SCHEMA_DELTAS.md.This also retargets the signal-owned manifest patch and tightens media-buy success response handling so 3.1+ update responses require caller-supplied revisions rather than SDK-invented tokens.
Validation:
uv run --extra dev --group dev pytest tests/test_server_dx.py tests/test_type_guards.py tests/test_discriminated_unions.py tests/test_type_coercion.py tests/test_public_api.py -q;uv run --extra dev --group dev pytest tests/test_server_dx.py tests/test_server_helpers.py tests/test_schema_validation_server.py tests/test_schemas_version_pin.py tests/test_validation_version.py tests/test_spec_coverage.py tests/test_code_generation.py -q; pre-commit hooks.