Skip to content

feat(types): support AdCP 3.1 beta 7#895

Merged
bokelley merged 4 commits into
mainfrom
v3.1.0-beta.7-release
May 28, 2026
Merged

feat(types): support AdCP 3.1 beta 7#895
bokelley merged 4 commits into
mainfrom
v3.1.0-beta.7-release

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

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.

@bokelley bokelley marked this pull request as ready for review May 28, 2026 11:17
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 28, 2026
Copy link
Copy Markdown
Contributor

@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. 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 at src/adcp/server/responses.py:489 — schema-faithful. UpdateMediaBuySuccess.required includes revision, so fail-closed on missing revision for 3.1+ is correct. revision placement moved into the early dict construction at src/adcp/server/responses.py:496-498, which serializes consistently.
  • CreateMediaBuyResponse1 / UpdateMediaBuyResponse1 field additions at scripts/post_generate_fixes.py:2089-2093 and :2167confirmed_at: AwareDatetime | None (required-but-nullable) is the correct Pydantic translation for type: ["string", "null"] + listed in required; respects the schema's allOf invariant banning media_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_owned listed under activate_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-new Scope class minted by codegen for the additive signal_coverage_forecast.scope object (schemas/cache/3.1.0-beta.7/core/signal-coverage-forecast.json). The original top-level missing_data.scope enum at signals/get-signals-response.json:148-156 is 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 use feat: 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), and test_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_types is silent-failure-prone. The new post-generate fix at scripts/post_generate_fixes.py:2634-2687 does five str.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 the ForecastPoint import (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 unimported forecast_point module — runtime NameError on import. Add assert 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.
  • AwareDatetime is stricter than format: date-time strictly 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)

  1. API asymmetry on adcp_version=None. update_media_buy_response at src/adcp/server/responses.py:489 treats adcp_version is None as "implicitly 3.1+, revision required." create_media_buy_response at 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.
  2. confirmed_at UX for provisional buys. Direct constructors of CreateMediaBuySuccessResponse(...) must now explicitly pass confirmed_at=None when the buy is provisional — they can't just omit it. The media_buy_response builder handles this transparently via the _UNSET sentinel; 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.

Copy link
Copy Markdown
Contributor

@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.

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

  1. Required-field flip on public Pydantic models, no !. scripts/post_generate_fixes.py:2089-2090,2167 adds revision: int (no default) and confirmed_at: AwareDatetime | None (no default) to CreateMediaBuyResponse1, and revision: int to UpdateMediaBuyResponse1. Both re-export through adcp.types as CreateMediaBuySuccessResponse / UpdateMediaBuySuccessResponse. Adopter code that constructed these typed models without revision worked under beta.6 and now raises ValidationError. PR title is feat(types): support AdCP 3.1 beta 7 — no !, no BREAKING CHANGE: footer. release-please will minor-bump from 6.3.0-beta.4 instead of cutting the major. Fix: retitle to feat(types)!: support AdCP 3.1 beta 7 (revision/confirmed_at now required), or add a BREAKING CHANGE: footer to the merge commit body naming CreateMediaBuySuccessResponse and UpdateMediaBuySuccessResponse. Either signal is fine; one of them is required before merge.

Things I checked

  • Upstream schema actually requires the fields. Verified success arms in schemas/cache/3.1.0-beta.7/bundled/media-buy/update-media-buy-response.json and create-media-buy-response.json. The runtime tightening is faithful, not over-reach.
  • Version-axis gating is correct. src/adcp/server/responses.py:491 fires only when adcp_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 in aliases.py, _ergonomic.py, _forward_compat.py, or capabilities.py to the changed numbers. _generated.py absorbs the shift in lockstep, which is the contract per CLAUDE.md.
  • Example migrations are the adopter evidence. examples/v3_reference_seller/src/platform.py initializes _buy_revisions on create and increments on update; examples/sales_proposal_mode_seller/src/platform.py does the same with an instance field. Both now thread revision= to the typed builders. That is the migration path adopters need to follow.
  • Manifest patch retargets cleanly from cache/3.1.0-beta.5/ to cache/3.1.0-beta.7/. Same body, new paths.
  • ADCP_VERSION reads 3.1.0-beta.7 at the head, matches the cache directory.
  • signal_coverage_forecast.Point.metrics post-gen narrowing is sound — upstream signal-coverage-forecast.json requires coverage_rate on every point's metrics, transitively forcing metrics present per point.
  • RegistryEvent1..16 family is coherent, not a partial regeneration — 16 event_type enum values map 1:1 to the codegen variants and the named payload classes all resolve to $defs in the upstream schema.

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

  • Migration note for revision and confirmed_at. A MIGRATION_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 patched CreateMediaBuyResponse1 ("confirmed_at=None must be passed explicitly for provisional buys") would prevent the misread.
  • SCHEMA_DELTAS.md doesn't cross-reference the post-fix re-add. The -confirmed_at, -revision lines on media_buy/create_media_buy_response.py will puzzle the next reader who doesn't know post_generate_fixes.py adds them back. One line cross-referencing the fix would close the loop.

Minor nits

  1. update_media_buy_response() ValueError message is terse. src/adcp/server/responses.py:492 reads "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:.

@aao-ipr-bot
Copy link
Copy Markdown
Contributor

aao-ipr-bot Bot commented May 28, 2026

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

This is an automated message from the Argus AI review workflow.

Copy link
Copy Markdown
Contributor

@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. 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.json lists ["media_buy_id", "confirmed_at", "revision", "packages"] in the success-branch required array; update-media-buy-response.json lists ["media_buy_id", "revision"]. The post-fix at scripts/post_generate_fixes.py:2089-2093 (CreateMediaBuyResponse1) and :2164-2167 (UpdateMediaBuyResponse1) is the right type-level enforcement, and update_media_buy_response() at src/adcp/server/responses.py:491-492 is the matching helper gate. _is_adcp_31_or_newer short-circuits to True on 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 omits revision without raising is an explicit pre-3.1 adcp_versiontest_explicit_30_status_shape at tests/test_server_dx.py:264-267 exercises that. Pre-3.1 callers are preserved.
  • Manifest patch retargeted correctly. schemas/patches/03-manifest-signal-owned-discovery-only.patch now points at schemas/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_tools without activate_signal) reflect the patched state — sync_schemas.py would have failed loud otherwise. Same conformance concern as beta.5.
  • Alias layer is renumber-resilient by design. aliases.py:1407-1424 uses _format_asset_class() / _repeatable_asset_group_class() to iterate vars(_format_module) and select by field-default introspection, not numeric class name. Grepped aliases.py for 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. The Assets94 references 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-32 imports Format (present at generated_poc/core/format.py:488) and RepeatableAssetGroup via the dynamic alias (now lands on Assets39 at generated_poc/core/format.py:455).
  • Examples consistent. _buy_revisions is initialized in __init__ at examples/v3_reference_seller/src/platform.py:741; _MediaBuy in sales_proposal_mode_seller gains revision: int = 1; seller_agent.py bumps mb[\"revision\"] on every mutating path.
  • Test coverage. New test_typed_success_requires_revision_and_confirmed_at / test_typed_success_requires_revision lock the Pydantic side; test_update_response_requires_revision_for_current_shape locks the helper-side ValueError.

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

  • SCHEMA_DELTAS.md says fields were removed when they were actually strengthened. The entries media_buy/create_media_buy_response.py: CreateMediaBuyResponse1: -confirmed_at, -revision and media_buy/update_media_buy_response.py: UpdateMediaBuyResponse1: -revision read as deletions in release notes. The on-disk types at generated_poc/media_buy/create_media_buy_response.py:26-35 and update_media_buy_response.py:28-37 carry both fields — just without defaults instead of Annotated[..., Field(...)] = None. scripts/diff_generated_types.py:64 is keying off AnnAssign.target.id presence, 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 raises ValueError when called without revision against 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 a BREAKING CHANGE: footer or feat! would have given release-please the right signal. Worth a brief migration note in the release PR body.
  • Stale Assets94 references in aliases.py comments (lines 1393, 1620, 1674, 1709, 1758, 1760, 1965). Beta.7's repeatable group is Assets39; the dynamic resolver finds it, but the comments are doc rot.

Minor nits (non-blocking)

  1. fix_signal_coverage_forecast_point_types() is silently no-op-safe. scripts/post_generate_fixes.py:2654-2682 runs four source.replace(old, new, 1) calls and never checks any matched. If a future datamodel-code-generator release 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 --strict would eventually catch it via the Point override failure, but the script itself should match the loud-fail posture of the other fixes — assert \"class Point(forecast_point.ForecastPoint):\" in source after, or compare source != original before writing.
  2. Manifest patch hunk-2 header is off-by-one. schemas/patches/03-manifest-signal-owned-discovery-only.patch:26 says @@ -1414,7 +1414,6 @@ but the first context line \"get_signals\" actually sits at line 1413 in the post-patch manifest. GNU patch absorbs this via default fuzz (offset -1 lines) so sync_schemas.py keeps working, but the header should read @@ -1413,7 +1413,6 @@. First hunk is correct.
  3. No regression test for the new helper ValueError. tests/test_server_dx.py covers the Pydantic-side ValidationError, and test_update_response_requires_revision_for_current_shape in tests/test_server_helpers.py:413-417 covers the helper, but only with default adcp_version. Add a sibling that pins adcp_version=None and another at the explicit 3.1+ version string to nail the gate's full surface.
  4. if params.get(\"revision\") and ... at examples/seller_agent.py:674 is falsy for revision=0. Not a real bug — revisions start at 1 — but use is not None to match the convention elsewhere in the file.

Approving on the strength of the spec-alignment plus the fail-closed gate.

@bokelley bokelley merged commit 3eee8d5 into main May 28, 2026
26 checks passed
@bokelley bokelley deleted the v3.1.0-beta.7-release branch May 28, 2026 11:47
Copy link
Copy Markdown
Contributor

@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. 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.md showing CreateMediaBuyResponse1: -confirmed_at, -revision is a misleading codegen-diff signal — it tracks loss of inline Field(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 wire type: ["string","null"] in the required array. Required-but-nullable is the right shape, not a Pydantic footgun.
  • src/adcp/server/responses.py:488: ValueError guard fires when adcp_version is None or _is_adcp_31_or_newer(...). Fail-closed beats fail-open here — pre-3.1 callers can still pass adcp_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_at and test_typed_success_requires_revision in tests/test_server_dx.py genuinely assert ValidationError on omission — validating new behavior, not just keeping CI green.
  • Examples thread confirmed_at / revision through the full lifecycle: seller_agent.py:619-720, v3_reference_seller/src/platform.py adds _order_confirmed_at helper and bumps revisions via self._buy_revisions.setdefault, sales_proposal_mode_seller/src/platform.py increments buy.revision on update.
  • schemas/patches/03-manifest-signal-owned-discovery-only.patch correctly re-targets the beta.5→beta.7 refs; the @@ -1394@@ -1414 hunk shift matches the regenerated manifest offsets.
  • Type-system layering: only _generated.py imports pydantic.AwareDatetime; no breach of tests/test_import_layering.py allowlist.
  • New fix_signal_coverage_forecast_point_types in scripts/post_generate_fixes.py:2634 aligns 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/** and src/adcp/types/generated_poc/** regeneration churn, expected.

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

  • revision: int loses the schema's minimum: 1 bound at _generated.py:2092 and :2167. Worth Annotated[int, Field(ge=1)] so the Python layer matches the wire constraint, not just the type. Flagged by ad-tech-protocol-expert.
  • adcp_version=None → requires revision is a behavior change worth a docstring line at src/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 new ValueError.
  • Required-nullable on a public model is observable to adopters constructing CreateMediaBuySuccessResponse(...) without confirmed_at=. Conventional commits is feat: not feat!:, consistent with the 1bae6e89 feat: support AdCP 3.1 Beta 4 precedent and the 6.x.x-beta.x pre-release lane. Not a block — but a one-line migration note in the release-please changelog body would soften the transition for adopters on 6.3.0-beta.5.
  • String-replacement patches in fix_signal_coverage_forecast_point_types silently no-op on whitespace drift. Consistent with the other fixers in this file (house style), so not blocking — but assert source.count(old) == 1 before each replace would catch upstream codegen changes early. Worth a sweep across all post_generate_fixes.py fixers as one follow-up.

Minor nits (non-blocking)

  1. 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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant