fix(types)!: enforce closed creative format kinds

[bokelley]

Creative assets, listed creatives, and input manifests accepted arbitrary format_kind strings despite referencing the closed canonical enum. Public CreativeAsset, Creative, and public/generated CreativeManifest now validate CanonicalFormatKind; canonical stubs expose the same closed types.

Under Brian's input/readback policy, manifests returned by another agent retain unknown future kinds through private, independent readback views. Known kinds still normalize to enum members. The response audit covers:

Response	Tolerant manifest path
Canonical and legacy creative delivery	creatives[].variants[].manifest
LegacyPreviewCreativeResponse3	manifest
LegacyBuildCreativeResponse1	creative_manifest
LegacyBuildCreativeResponse3	creative_manifests[]
LegacyBuildCreativeResponse4	creatives[].variants[].creative_manifest
ContextMatchResponse / router-to-publisher response	offers[].creative_manifest
Provider-to-router context-match response	offers[].creative_manifest
AdcpAsyncResponseData / completed MCP task results	Nested build and preview response paths above

The generated response classes compose the private views, so existing public aliases and indirect wrappers agree. Async union and webhook validators are refreshed after composition. Input classes are not widened or inherited by tolerant views, preventing tolerant instances from bypassing strict input validation. Constraints, version envelopes, defaults, extension fields, and typed source-model composition are retained.

Direct Creative.format_kind and CreativeAsset.format_kind response fields remain strict in this PR. In particular, ListCreativesResponse.creatives[].format_kind rejects unknown kinds. DeliveryCreative.format_kind retains its existing open readback type. Private manifest/variant/offer views are not added to top-level exports, and versioned wire-schema validation remains unchanged.

BREAKING CHANGE: unknown kinds in public creative/input models now raise Pydantic ValidationError. Known strings still normalize to enum members; required creative kinds and optional manifest defaults are unchanged. The changelog and migration guide explain the input/readback boundary and upgrading from releases through 8.0.0-rc.2.

Validation:

• make lint, make typecheck-all, make validate-generated, and normal commit hooks passed.
• 468 focused creative, compatibility, import, legacy-surface, delivery, manifest, and webhook tests passed (15 existing A2A-version skips).
• Regressions cover Python/JSON round-trips on every path, MCP adapter parsing, the async union and public completed-task parser, strict rejection of tolerant instances/dumps, exact public creative types, source-model composition, and nested constraints.
• Full make test passed through the reporting harness: 12,345 passed, 2,395 skipped, 9 deselected, 1 expected failure; 81.67% coverage.
• Independent exact-head review cleared b10d5308 with no implementation blockers.
• Full CI attempt 2 passed, including PostgreSQL conformance, all eight receipt compatibility shards and their aggregate, installed VCS/sdist packages, and Python 3.10–3.13. This reran the full workflow after an artifact-download action timeout in attempt 1; the source stayed unchanged.

Closes #1241

[bokelley]

Review — strict CanonicalFormatKind, exact head 32cfc5b8

No blockers. Reviewed in an isolated worktree; one non-blocking observation at the end.

The asymmetry is the right call, and I traced it rather than assumed it

The change closes the authoring types and leaves delivery readback open. My first concern was that closing a type which parses wire data would reject valid payloads from a newer protocol revision — the exact hazard the deleted comment described. So I followed every model_validate on the now-closed types:

• The only call site is build_creative_response (server/responses.py:769/774), which is the adopter's own outbound authoring helper — the manifest dict comes from the adopter's code, not the wire. Strict is correct there: it catches "vidoe_hosted" before it ships instead of emitting a malformed response.
• The mirror risk — a client parsing a seller's reply into the closed types — doesn't exist either. client.py handles format_kind as dict keys throughout, and response models go through LegacyBuildCreativeResponse*.
• DeliveryCreative keeps its open patch, and that's the surface that actually reads back unknown kinds.

So no inbound surface was closed by this change.

Schema consistency checks out, including across versions

core/canonical-format-kind.json is a closed 16-member enum, and it is byte-identical from 3.2.0-rc.0 through 3.2.0-rc.7 — rc.7 being the currently supported revision:

3.1 … 3.2.0-beta.0   13 members  sha=79500f7caf02
3.2.0-beta.6/.9      15 members  sha=489daed61ab3
3.2.0-rc.0 … rc.7    16 members  sha=628a3885ee2a   <- all identical

So the closed runtime enum matches the pinned schema exactly for every supported 3.2 version — no stale-cache gap. Worth noting the set did grow 13→15→16 across the betas, which is precisely why the open type existed; it has been stable across the entire rc series, which is what makes closing it reasonable now.

Stub parity holds: the .pyi drops | str in lockstep, and in the isolated checkout the live annotation is <enum 'CanonicalFormatKind'>.

Migration guidance is honest about the cost

It states the breaking boundary ("through 8.0.0-rc.1"), and it includes the sentence it would have been easy to leave out: "Applications receiving a kind introduced by a newer protocol version need an SDK version that supports that kind." It also explicitly warns that custom is not a fallback for unknown values — which is the misuse this change would otherwise invite. The CHANGELOG carries a proper ⚠ BREAKING CHANGES entry linking the guide.

Breaking-change hygiene is right too: the commit carries a well-formed BREAKING CHANGE: footer. That now matters operationally — #1201's merged policy rejects a ! subject without one, so a missing footer here would have turned main red on push.

Tests

The coverage is where it needs to be: rejection via constructor, model_validate and model_validate_json; nested manifests; request-level nesting; required-and-non-nullable; the manifest's retained optional default; positive coercion of known values; the custom-format shape/schema rules; and test_delivery_creative_alias_is_open_and_keeps_its_identity, which pins the open readback path so a later refactor can't quietly close it.

What I ran at 32cfc5b8 (isolated worktree, import path verified)

• tests/test_forward_compat_format_kind.py — 101 passed
• tests/test_import_layering.py — 2 passed
• -k "creative and (format or kind or canonical)" — 222 passed, 17 skipped
• mypy --strict tests/type_checks/creative_asset_binding.py — clean
• ruff check src/ — clean

---

Observation (non-blocking) — the forward-compat side is guarded by the weaker check

The contract file upgrades the closed types to assert_type, which is a real strengthening over the previous assignability test. But DeliveryCreative was left on the old pattern:

delivery_kind: CanonicalFormatKind | str | None = delivery.format_kind

I checked whether that would catch a regression, and it wouldn't. Assigning a narrow CanonicalFormatKind to a CanonicalFormatKind | str | None annotation type-checks cleanly under mypy --strict — narrower is assignable to wider. So if someone later narrowed DeliveryCreative.format_kind to the closed enum, this contract would still pass.

That open annotation is the forward-compat guarantee this whole change leans on, so it's the one most worth locking down:

assert_type(delivery.format_kind, CanonicalFormatKind | str | None)

Cheap, and it makes both halves of the asymmetry equally enforced.

CI at time of writing: 7 SUCCESS / 1 skipped / 41 pending, no failures.

[bokelley]

Addressed the non-blocking DeliveryCreative type-contract observation in 4c702286. The adopter fixture now uses assert_type(delivery.format_kind, CanonicalFormatKind | str | None), so accidentally narrowing the readback type fails the check.

make lint, make typecheck-all, all ordinary commit hooks, and the focused creative/compatibility/import tests (200 passed) pass. Production code and the breaking migration guidance are unchanged. Full make test and CI are still running.

Requesting db56's recheck of this test-only follow-up.

[bokelley]

Exact-head verdict — 4c702286

No blockers. Delta is exactly the one line, and I verified the guard now does what the old form couldn't.

-delivery_kind: CanonicalFormatKind | str | None = delivery.format_kind
+assert_type(delivery.format_kind, CanonicalFormatKind | str | None)

Test-the-test, simulating a future narrowing of DeliveryCreative.format_kind to the closed enum and running both forms side by side under mypy --strict:

error: Expression is of type "CanonicalFormatKind", not "CanonicalFormatKind | str | None"  [assert-type]
Found 1 error in 1 file

One error, on the assert_type line. The old assignment line in the same file produced none — narrower is assignable to wider, so it was blind to exactly the regression it was meant to catch. The new form discriminates.

The contract file itself is clean at this head (mypy --strict: Success), and tests/test_forward_compat_format_kind.py is 101 passed.

Both halves of the asymmetry introduced by this PR are now equally enforced: closed for CreativeAsset/Creative/CreativeManifest, open for DeliveryCreative. Everything from my 32cfc5b8 review stands unchanged — that head's findings were all clean apart from this one observation, which is now closed.

CI on this head: 1 success / 1 skipped / 7 pending, no failures.

[bokelley]

Full make test found one localization fixture using third_party_tag, which the closed schema enum correctly rejects. Commit 1d467863 changes that value to canonical display_tag; the localization clear assertion is unchanged. This is the only change since the reviewed 4c702286 head.

All 228 focused tests, make lint, make typecheck-all, and ordinary commit hooks pass. The full coverage suite is rerunning. Production code and the breaking migration guide remain unchanged.

Requesting db56's recheck of this test-only fixture correction.

[bokelley]

Exact-head verdict — 1d467863

No blockers. The delta is exactly the one line described, test-only, and I checked for siblings of the same problem — there are none.

The change is necessary and correct

-                    "format_kind": "third_party_tag",
+                    "format_kind": "display_tag",

third_party_tag is not a CanonicalFormatKind member; display_tag is. So this fixture was carrying a value the newly-closed enum rejects — it had to move, and display_tag is the right canonical kind for a third-party tag creative.

The assertion under test is untouched:

assert wire["creatives"][0]["localization"] is None

format_kind is incidental to what test_a_clear_survives_on_sync_creatives_localization verifies — that an explicit null survives serialization — so swapping to a valid canonical kind doesn't weaken it. No production or migration file is touched.

I checked whether more fixtures are lurking

Since a full-suite run is what surfaced this one, I scanned every format_kind literal under tests/ against the enum rather than waiting for the rerun. Two non-canonical values remain, and both are intentional:

site	value	why it's correct
tests/type_checks/creative_asset_binding.py:42	future_canonical_format	the deliberate DeliveryCreative fixture proving the open readback path
tests/test_codegen_contract_compatibility.py:95,107	unknown	both inside pytest.raises(ValidationError) — negative assertions requiring rejection

So the closed enum shouldn't surface any further fixture breakage in the full run.

What I ran at 1d467863

• test_rc3_media_buy_runtime.py + test_forward_compat_format_kind.py + test_codegen_contract_compatibility.py — 134 passed
• Enum membership check for both literals, and the tests-wide format_kind scan above

Everything from my 32cfc5b8 and 4c702286 reviews stands: the closed/open asymmetry is correct, schema-consistent through rc.7, both halves now equally enforced by assert_type, and the BREAKING CHANGE footer is in place for the merged main-push policy.

CI on this head: 2 SUCCESS / 1 skipped / 7 pending, no failures.

[bokelley]

CI triage on 1d46786: Validate conventional commit format rejects the breaking subject because the actual commit message lacks a BREAKING CHANGE footer. Add the footer to the breaking commit (not only the PR body), then push and recheck CI. The production/test review remains clear on this head.

[bokelley]

Correction to my CI triage: the current PR title/body and the breaking commit both pass the policy validator, including the required footer. The failed run used an earlier pull_request event body (it fails at validate_message(pr title + body), before commit checks). No commit-message fix is needed. A fresh synchronize after the rc2 rebase will use the current body and rerun this check; please keep the present footer.

[bokelley]

The conventional-commit job rejected the PR title/body captured before the description had a literal BREAKING CHANGE: footer. The current description and all three commit messages now pass scripts.check_main_policies validation locally.

Pushed 1be9b590 with only a commit-message update to start CI using the corrected PR metadata. git diff 1d467863 1be9b590 is empty; the reviewed source, tests, and migration guide are unchanged. The full local test rerun continues.

Requesting db56 to confirm the unchanged source on this refreshed head.

[bokelley]

Exact-head confirmation — 1be9b590

Confirmed metadata-only; no blockers. My 1d467863 verdict carries over unchanged.

Verified by tree identity rather than by the diff alone:

tree @ 1d467863: c715c26952a1b957c1542e45b40285d4ee72bec7
tree @ 1be9b590: c715c26952a1b957c1542e45b40285d4ee72bec7   identical
git diff 1d467863..1be9b590 -> 0 lines

The commit object differs in exactly two respects — the committer timestamp, and an added body paragraph explaining why display_tag is the right kind. Same author, same subject, same tree. Source, tests and migration are untouched.

The amend achieves its purpose

Since the stated goal was refreshing CI against the current PR body, I ran #1201's actual validator logic over title + "\n\n" + body, which is what the PR-path policy check evaluates:

subject conventional    : True
description safe        : True
breaking marker         : True
BREAKING CHANGE in body : True  -> policy ACCEPTS

So fix(types)!: enforce closed creative format kinds plus the body footer satisfies the merged main-push policy on the PR path.

One small nit, since a squash would make it permanent

The migration-guide link in the PR body is an absolute branch URL:

blob/fix/strict-canonical-format-kind-1241/docs/canonical-format-kinds-migration.md

That 404s once the branch is deleted. The CHANGELOG already uses the durable relative form (docs/canonical-format-kinds-migration.md). Normally a stale link in a PR body is harmless history — but if Wave 2 squash-merges, the body becomes the commit message and the dead link lands in permanent git history. Swapping it to a blob/main/... or relative path costs nothing.

Nothing else changed, so the 32cfc5b8 / 4c702286 / 1d467863 findings all stand: closed/open asymmetry correct and schema-consistent through rc.7, both halves enforced by assert_type, no remaining non-canonical fixtures beyond the two intentional ones.

[aao-secretariat]

Ladon verdict: Request changes

Request changes — 1 blocking finding.

Blocking findings

• src/adcp/types/canonical_creative.py — Removing the CreativeManifest widening also closes format_kind on the manifest nested inside the deliberately-open DeliveryCreative readback path (variants[].manifest). A delivery response carrying a future canonical kind now raises ValidationError and zeros the whole response, contradicting the stated tolerant-readback contract. This is forward-compat regression on a load-bearing discriminated-union/readback path (high).

The intentional breaking change on the producer models (CreativeAsset/Creative) is well-signaled — fix!: prefix, BREAKING CHANGE: footer, migration guide, and CHANGELOG prose all present, satisfying the semver-signal requirement. The issue is scope leakage: the closure must not extend to the tolerant DeliveryCreative readback manifest. Preserve the open/forward-compatible arm on the delivery readback path (e.g. retain the widening or add an UnknownFormatAsset-style escape hatch for the nested manifest) so future canonical kinds deserialize without raising.

Decision table row 1 applies (one high finding) → request-changes.

Blocking findings

• src/adcp/types/canonical_creative.py — Closing format_kind on the DeliveryCreative tolerant-readback manifest (variants[].manifest) causes ValidationError on future canonical kinds, breaking forward-compat readback contract (high)

[bokelley]

Aao's exact-head review identified a blocking readback regression in DeliveryCreative.variants[].manifest (review thread on canonical_creative.py). Please keep the public CreativeAsset/Creative/CreativeManifest producer types strict per Brian's #1241 decision, but give the delivery readback path its own tolerant manifest arm so a future format_kind deserializes without zeroing get_creative_delivery. Add a focused nested-variant regression that fails on 1be9b590 and passes with the fix, then rerun lint/type/focused tests, full make test, CI, and exact-head review. The fix(types)! title and migration footer are already correct; rc2 is unaffected.

[bokelley]

Brian's #1241 product-boundary decision (2026-09-29): keep CreativeAsset, Creative, and the public/generated CreativeManifest strict for seller input. Preserve tolerant buyer delivery readback with a delivery-only manifest/variant type used only inside DeliveryCreative.variants[].manifest. Keep that tolerant type out of top-level adcp.types exports; it may be reachable through the delivery model but must not bypass input validation.

Please add a regression proving an unknown format_kind round-trips through delivery readback while CreativeManifest still rejects it, note the boundary in the #1241 changelog entry, and then resolve the aao thread and request exact-head review. This is the requested correction to #1248; no change to the breaking marker/title or the rc2 release.

[bokelley]

Brian boundary implemented on 530b2c5: public CreativeAsset, Creative, CreativeManifest, and CreativeVariant remain strict; DeliveryCreative now nests private tolerant manifest and variant models. A complete GetCreativeDeliveryResponse with an unknown nested format_kind round-trips, while public CreativeManifest rejects it. Changelog and typing updated. Focused tests: 101 passed; lint, source and adopter typing, and commit hooks passed. I pushed directly to the PR branch; please sync before any further edits. Re-requesting exact-head review once CI confirms this head.

[aao-secretariat]

Ladon cannot review this PR until merge conflicts are resolved.

[bokelley]

Follow-up on current head 6e76ceb: the generated/legacy GetCreativeDeliveryResponse had the same nested-manifest rejection. I reproduced it red, added a private generated delivery-only manifest/variant view, and now both canonical and generated complete responses round-trip unknown nested format_kind while both public/generated CreativeManifest remain strict. 102 focused tests, lint, typecheck-all, and commit hooks pass. The prior Ladon run was superseded; please review this exact head. Full CI is running.

[bokelley]

Exact-head verdict — 72b2fcf907

No blockers. The design change is correct and closes a real hole that the previous shape had — one I missed in my earlier reviews.

The hole was genuine, and I'd missed it

At 1be9b590, DeliveryCreative.variants was list[CreativeVariant], and the public CreativeVariant nests the closed CreativeManifest. So a delivery readback carrying an unknown future kind inside a variant's manifest would have been rejected, even though the top-level format_kind was open. My earlier reviews verified the top-level open path (there was a test pinning it) but never exercised the nested one. Good catch.

The fix is the right shape: private _DeliveryCreativeManifest / _DeliveryCreativeVariant tolerant clones, used only by the readback model, with the public input models left strict.

The private types are genuinely private

They appear only in canonical_creative.py and its .pyi, are referenced solely by DeliveryCreative.variants, and are absent from every __all__ and public export map. No path lets the tolerant manifest substitute for the strict one in authoring validation.

Behaviour verified through the path that actually parses the wire

GetCreativeDeliveryResponse.creatives -> Sequence[canonical_creative.DeliveryCreative]
  element.variants                    -> list[canonical_creative._DeliveryCreativeVariant]

and strictness is intact on the input side — CreativeManifest with an unknown kind rejects, and the public CreativeVariant rejects a nested unknown manifest (dedicated test at line 162). Their round-trip test additionally asserts the tolerant types are the ones instantiated (type(manifest) is not CreativeManifest) and that a JSON round-trip is byte-stable, which is stronger than just checking the value survives. 101 passed.

Observation — pre-existing, not this PR's doing

I want to flag how I got here, because I first read it as a blocker. Probing directly:

from adcp.types import DeliveryCreative
DeliveryCreative.model_validate({... "variants":[{"variant_id":"v1",
    "manifest":{"format_kind":"future_canonical_format","assets":{}}}]})
# -> rejected at ('variants', 0, 'manifest', 'format_kind')

That looked like the change not working. It isn't — there are two classes named DeliveryCreative:

symbol	variants element
adcp.types.canonical_creative.DeliveryCreative (used by GetCreativeDeliveryResponse)	_DeliveryCreativeVariant — tolerant
adcp.types.DeliveryCreative (the public export)	generated_poc.core.creative_variant.CreativeVariant — closed

The public export resolves to generated_poc.creative.get_creative_delivery_response.DeliveryCreative. I checked the merge base and this split already exists there, unchanged — so it is not introduced here and is not a blocker for this PR.

It is, however, more consequential after this change than before it. Previously both classes rejected nested unknown kinds, so the split was invisible; now they differ behaviourally, and an adopter writing from adcp.types import DeliveryCreative and validating a payload will reject readback the SDK's own response parsing accepts. The migration guide's line — "DeliveryCreative keeps its separate tolerant response parsing contract" — reads as though it applies to the exported symbol, which it doesn't.

Worth either aligning the export to the canonical clone or naming the distinction in the migration guide. Either is a follow-up, not a condition on this PR.

What I ran at 72b2fcf907 (isolated worktree)

• tests/test_forward_compat_format_kind.py — 101 passed
• Private-type export scan, response-vs-export model introspection, direct nested-kind probes on both classes, and the same introspection at the merge base for attribution

[aao-secretariat]

Ladon verdict: Approve

Approve — clean pass, no blocking or medium findings.

This is a subsequent review following a prior request-changes. The prior run recorded no critical/high/medium findings, and this run is also clean: the developer's changes preserve the intended behavior. The PR correctly closes CreativeAsset/Creative/CreativeManifest to the canonical enum on seller-input surfaces while preserving tolerant delivery readback via private _DeliveryCreativeManifest/_DeliveryCreativeVariant subclasses on both the canonical (GetCreativeDeliveryResponse) and generated (DeliveryCreative alias, GeneratedGetCreativeDeliveryResponse) paths, each covered by a parametrized round-trip test.

Semver signal is correct: fix(types)!: + BREAKING CHANGE footer + migration guide + CHANGELOG entry. No import-layering violations, no generated-code hand-edits, no credential issues.

Decision-table walk:

• Row 1 (critical/high): no findings → skip.
• Row 2 (gated_paths + not APPROVED): gated_paths: false → skip.
• Row 3 (high-risk deleted): _forward_compat.py is (modified), not deleted → skip.
• Row 4 (medium data-loss/schema/infra): no medium findings → skip.
• Row 5 (high-risk modified + medium finding): _forward_compat.py is modified but there are zero medium findings → does not fire.
• Row 6 (sticky escalate): prior outcome was request-changes, not escalate → skip.
• Row 7 (no-auto-approve team): no team match → skip.
• Row 8 (≥3 medium): zero medium → skip.
• Row 9: approve.

Note: review_decision is CHANGES_REQUESTED, but the deprecated protected-branch/review-status gate does not apply here because gated_paths is false and there are no blocking findings. The high_risk flag is a look-closer heuristic on a modified (not deleted) load-bearing file with no medium-or-higher concern, so it is presumed safe per the reasons guidance.

[bokelley]

Follow-up on 6e76ceb4: the private generated readback subclasses are still accepted unchanged by the strict generated CreativeManifest.model_validate() and CreativeVariant.model_validate(), including an unknown kind. I reproduced both instance bypasses. New tests also catch rejected known typed manifests/variants and lost standalone asset normalization on canonical readback.

I have preserved the rebase and both published fixes locally. The correction uses independent readback clones, retains strict input validation, and restores typed composition without widening input models. Focused tests, static checks, and the full suite are next; please hold merge for that follow-up.

[bokelley]

Please recheck d77a96ea. The generated readback models now copy fields independently instead of subclassing strict inputs; neither manifest nor variant instances can bypass input enum validation. Canonical readback also preserves known typed input composition and standalone assets. The breaking guidance remains intact, with the input/readback boundary spelled out.

Validation: 266 focused tests, lint, source/adopter typing, generated checks, and normal commit hooks pass. Tests cover both response surfaces, Python/JSON and adapter parsing, direct-instance/dump validation, constraints/defaults, and exact canonical readback types. Full local make test and fresh CI are running.

[aao-secretariat]

Ladon verdict: Request changes

Request changes — 1 blocking finding.

Blocking findings

• src/adcp/types/canonical_creative.py — Dropping the open-enum patch on the generated CreativeManifest narrows the preview_creative and build_creative readback responses to strict deserialization. A newer seller's unknown canonical format kind will break buyer-side deserialization on those paths, while only GetCreativeDeliveryResponse received a tolerant replacement. This is a forward-compat / wire-shape regression on load-bearing response models (high per repo forward-compat and response-shape rules).

Row 1 of the decision table fires: a high severity finding is present, so the outcome is request-changes. The remaining aspects the reviewer checked (semver signal, migration docs, .pyi fidelity, import layering, delivery-readback tolerance) are correct — the fix is to extend the tolerant/open-union treatment consistently across the preview_creative and build_creative readback paths.

Blocking findings

• src/adcp/types/canonical_creative.py — Open-enum patch dropped on CreativeManifest narrows preview_creative and build_creative readback responses to strict; unknown canonical format kinds from newer sellers break buyer-side deserialization (high, forward-compat/response-shape regression)

[bokelley]

Exact-head verdict — d77a96ea

No blockers. The bypass is closed, and the readback path survived the change — which was the specific risk in swapping subclasses for independent clones.

The bypass is genuinely closed

I reproduced it on 6e76ceb4 first so I'd be checking a real regression rather than a described one:

6e76ceb4:  GeneratedCreativeManifest.model_validate(tolerant_instance)
           -> ACCEPTED, format_kind='future_canonical_format'   (revalidate_instances defaults to 'never')

At d77a96ea the tolerant models are no longer subclasses (isinstance(tol, GeneratedCreativeManifest) → False) and every strict entry point refuses them:

GeneratedCreativeManifest.model_validate(tolerant)  -> rejected (model_type)
GeneratedCreativeVariant.model_validate(tolerant)   -> rejected (model_type)
public CreativeManifest.model_validate(tolerant)    -> rejected (model_type)

I also checked the obvious workaround, since an adopter reaching for one is the realistic next step — dumping the tolerant instance and revalidating:

GStrict.model_validate(tol.model_dump())              -> rejected (enum)
GStrict.model_validate(tol.model_dump(mode="json"))   -> rejected (enum)

No residual path. The unknown kind is caught by the enum on the dict route and by model_type on the instance route.

The predicted failure mode didn't happen

When subclassing was doing the work, model_rebuild(force=True) was enough to keep readback tolerant; with independent clones the response fields have to be re-pointed or readback breaks in the other direction. It holds:

GetCreativeDeliveryResponse nested unknown kind   -> future_canonical_format
JSON round-trip stable                            -> future_canonical_format

Strict input is still strict (raw dict unknown kind → enum), known typed input still works in both directions, and the _normalize_readback before-validator is what lets the tolerant model accept a strict instance by dumping it — tolerance in the right direction only, without inheritance.

The clone mechanism, and the risk it carries

_delivery_readback_clone copies each field as (annotation, deepcopy(field)) onto an independent base. That's the correct way to get constraint parity without inheritance, but it means any non-kind constraint could silently be lost. test_delivery_variants_keep_non_kind_constraints guards exactly that, and test_delivery_only_types_are_not_top_level_exports pins the privacy property I checked by hand last round. Good coverage of the failure modes this approach introduces.

extra="allow" on _DeliveryReadbackModel is right for readback — unknown future fields are preserved rather than dropped — and it can't leak, since strict models reject both the instance and its dump.

What I ran at d77a96ea (isolated worktree)

• test_delivery_manifest_readback.py + test_forward_compat_format_kind.py + test_rc3_media_buy_runtime.py — 167 passed
• mypy --strict tests/type_checks/creative_asset_binding.py — clean; ruff check src/ — clean
• Independent bypass/strictness/readback probes above, plus the 6e76ceb4 reproduction for red/green

Note on my earlier reviews

This class of leak is one I missed. When I checked that the tolerant type didn't reach strict paths, I verified static type references and export maps — never whether an instance could cross the boundary through Pydantic's default revalidate_instances='never'. The subclass relationship made that reachable and my checks wouldn't have caught it. Worth recording, since the same pattern applies anywhere a tolerant variant is derived from a strict model.

[aao-secretariat]

Ladon verdict: Approve

Approve.

Subsequent pass on PR #1248. This is a net-clean iteration: the prior approve run found nothing, and this run also surfaces no critical/high/medium findings.

What was checked:

• Delivery-readback tolerance extended into nested rendered manifests (DeliveryCreative.variants[].manifest retains unknown future format_kind) while input CreativeAsset/Creative/CreativeManifest tighten to strict CanonicalFormatKind — forward-compat fallback arms preserved, not removed.
• Forward-compat clones copy fields without subclassing strict inputs (bypass-tested); sole generated validator re-attached; both generated and canonical delivery paths round-trip.
• semver BREAKING signal + migration doc present.
• No generated-source hand-edits, no import-layering breaches; delivery-only types stay unexported.

Decision-table walkthrough:

• Row 1 (critical/high): none.
• Row 2 (gated_paths + not APPROVED): gated_paths is false — does not fire.
• Row 3 (high-risk deleted): _forward_compat.py is (modified), no deletions.
• Rows 4–5 (medium findings): none.
• Row 6 (sticky escalate): prior decision was approve, not escalate.
• Row 7 (no-auto-approve team): no match.
• Row 8 (3+ medium): none.
• Row 9: approve.

Note: review_decision is CHANGES_REQUESTED, but that only forces a gate when gated_paths is true (row 2), which it is not here. No blocking findings exist, so the diff falls through to row 9.

Superseded by Ladon approval of d77a96e.

[bokelley]

Merge remains held despite the d77a96e Ladon APPROVED: discussion_r4136922407 is still unresolved. It identifies generated preview_creative and build_creative response manifests that use strict CreativeManifest, so a future format_kind can still fail buyer readback. Please reproduce and extend private tolerant response views (keeping public/generated CreativeManifest strict), or document a justified exemption on that thread. Resolve it before final handoff.

[bokelley]

Blocker — buyer build_creative / preview_creative readback (exact head d77a96ea)

Confirming discussion_r4136922407. This is a regression introduced by this PR, not a pre-existing gap, and I have the full chain.

Attribution: tolerant before, strict now

At this PR's merge base (ad7e8229, the rc.2 release):

BASE: LegacyBuildCreativeResponse1.model_validate({"creative_manifest":
        {"format_kind": "future_canonical_format", "assets": {}}, ...})  -> ACCEPTED
HEAD d77a96ea: same payload                                              -> REJECTED (enum)

The cause is the patch this PR removed from _forward_compat:

# removed by this PR
_patch_model_field(CreativeManifest, "format_kind", _OpenCanonicalFormatKind | None)
CreativeManifest.model_rebuild(force=True)

That patch targeted the generated core.creative_manifest.CreativeManifest, which is what the buyer-facing response models embed. Removing it closed the enum on a readback path, not just on the input models.

The impact chain, each link verified

1. LegacyBuildCreativeResponse1.creative_manifest → generated_poc.core.creative_manifest.CreativeManifest; LegacyBuildCreativeResponse3.creative_manifests → list[...] of the same. Both strict at this head.
2. Unknown kind rejects at loc=('creative_manifest','format_kind') type=enum, while a known kind is accepted — so the rejection is the enum, not payload shape.
3. The client parses through exactly these models:
   • client.py:2548 — self.adapter._parse_response(raw_result, LegacyPreviewCreativeResponse)
   • client.py:5719-5720, 5790-5791 — task map with "build_creative": LegacyBuildCreativeResponse, "preview_creative": LegacyPreviewCreativeResponse
4. protocols/base.py:_parse_response catches ValueError (Pydantic's ValidationError subclasses it) and returns TaskResult(status=FAILED, success=False, data=None, error="Failed to parse response: …").

So a buyer on this SDK talking to a seller on a newer protocol revision sees an otherwise-successful build_creative / preview_creative reported as FAILED with the data dropped. That is the same forward-compat break the delivery work in this PR exists to prevent, on a different readback path.

Why the seller side must stay strict

Worth separating, because the fix shouldn't over-correct: server/responses.py:build_creative_response validates through the canonical CreativeManifest on the seller's outbound authoring path, where the manifest dict comes from the adopter's own code. Strict is right there — it catches a seller typo before it ships. The tolerance is needed only where a buyer parses a seller's reply.

That's the identical seller/buyer asymmetry this PR already established for delivery, so the machinery exists: point the build/preview response manifests at a tolerant readback clone (_delivery_readback_clone or a sibling) and leave the input models and the authoring helper alone.

Suggested regression shape

Mirroring test_delivery_manifest_readback.py:

• unknown format_kind round-trips through LegacyBuildCreativeResponse1, LegacyBuildCreativeResponse3 and LegacyPreviewCreativeResponse;
• the tolerant response manifest still cannot bypass strict input (GeneratedCreativeManifest.model_validate(instance) → model_type, and .model_dump() → enum);
• build_creative_response still rejects an unknown kind on the authoring path.

Everything else at this head still verifies

The delivery work reviewed in my previous comment is unaffected and re-confirmed: the instance bypass is closed on both manifest and variant, the dump-and-revalidate workaround is closed, nested unknown kinds still round-trip through GetCreativeDeliveryResponse, and 167 focused tests plus mypy --strict and ruff are clean.

Verdict: blocked on the above. Merge should stay held until the build/preview readback path is either made tolerant or an exemption is justified on the record — an exemption would mean accepting that buyers fail these two tasks against any seller that adopts a future format kind.

[bokelley]

Brian's latest product decision supersedes the delivery-only scope noted in the resolved thread: strict CreativeManifest applies to input; a buyer's manifest readback from another agent is tolerant. This includes delivery, preview_creative, and all build_creative response shapes that embed a manifest, including nested variants. The public/generated CreativeManifest remains strict, and the private tolerant view stays out of top-level exports. Direct Creative/CreativeAsset response fields remain strict for this PR and will be listed in the PR description.

The owner is implementing and testing this response boundary on the same branch. Please hold merge of d77a96e until the new head has green CI and renewed exact-head review.

[bokelley]

Exact-head review — b10d5308

My build_creative/preview_creative blocker is closed and Option B is soundly built. But the explicitly scoped-out part leaves one more regression of the same class: list_creatives buyer readback. Not clear to merge yet.

What's fixed and verified

LegacyBuildCreativeResponse1 unknown kind      -> ACCEPTED
LegacyBuildCreativeResponse3 unknown kind      -> ACCEPTED
GetCreativeDeliveryResponse nested unknown     -> ACCEPTED   (no delivery regression)
generated CreativeManifest raw dict unknown    -> rejected (enum)
public CreativeManifest raw dict unknown       -> rejected (enum)
known kind                                     -> ACCEPTED

The expanded clone surface is the obvious place for the instance bypass to creep back, so I swept it. Seven readback models are now defined in _forward_compat — _ManifestReadbackModel, _VersionedManifestReadbackModel, _ReadbackCreativeManifest, _DeliveryVariant, _BuildReadbackVariant, _BuildReadbackCreative, _ReadbackOffer — and none is a subclass of a strict model. The one carrying format_kind rejects on all six bypass probes (instance and .model_dump(), against generated manifest, generated variant and public manifest). The isolation property holds across the wider surface.

225 focused tests pass; ruff clean.

Remaining: list_creatives is the same regression

ListCreativesResponse.creatives is a direct Creative field, which this PR closed and Option B deliberately leaves strict. Differential against the merge base with an identical payload — the control (known kind) errors the same way on both sides, so the only behavioural difference is the enum:

	known kind	unknown kind
BASE ad7e8229 (rc.2)	missing ('creatives',0,'status')	missing ('creatives',0,'status') — kind accepted
HEAD b10d5308	missing ('creatives',0,'status')	enum ('creatives',0,'format_kind')

(The missing status is my fixture being incomplete; it's the constant across both sides that isolates the comparison.)

So at rc.2 a buyer could read a list_creatives response containing a future kind, and at this head it fails. The impact chain is identical to the one I reported for build/preview: list_creatives is in the client's task→response map, and protocols/base.py:_parse_response converts the ValidationError into TaskResult(status=FAILED, success=False, data=None) — an otherwise-successful listing reported as failed with the data dropped.

This is a regression against rc.2, not a pre-existing gap, which is the distinction that matters for whether "strict for now" is a deferral or a break. list_creatives is also the highest-traffic of these readbacks: it's how a buyer enumerates a seller's existing creatives, so it fails as soon as a seller has any creative using a future kind, not just when one is newly built.

On the scope decision

Keeping Creative/CreativeAsset strict as input models is clearly right — that's the whole point of the PR. The question is only the handful of response fields typed as Creative, and the same seller/buyer asymmetry applies: authoring strict, readback tolerant. _BuildReadbackCreative already exists, so the pattern is in place.

If the intent is to defer, it'd be worth saying so in the migration guide alongside the other guidance, because an adopter reading "response manifests are tolerant" will reasonably assume response creatives are too.

Verdict

Not clear. The build/preview blocker is resolved and the construction is good — but list_creatives readback regresses against rc.2 in the same way, and I'd want it either covered by Option B or explicitly justified on the record before merge.

[bokelley]

The list_creatives change is intentional under Brian's explicit policy for this PR: “Direct Creative/CreativeAsset response fields remain strict for this PR and will be listed in the PR description.” This is the requested breaking correction to the exported Creative.format_kind contract in #1241.

The current PR description explicitly lists ListCreativesResponse.creatives[].format_kind as strict. The migration guide on b10d5308 also states that exact path remains strict and distinguishes it from tolerant response manifests. The breaking changelog states the same exclusion. test_direct_listed_creative_kind_stays_strict uses a complete valid listing (including status, query summary, and pagination), verifies the known-kind control, and checks the unknown-kind enum failure.

So the regression is acknowledged as an intentional, documented breaking change within the approved policy, not deferred as a pre-existing issue. Please assess the remaining clearance against that explicit boundary. The broader creative-response tolerance would require Brian to supersede this specific part of the decision; I am preserving his current instruction.

[bokelley]

Conditional review cleared — exact head b10d5308

Clear. No blockers, no separate implementation defect.

The list_creatives boundary is a product-policy call and Brian has made it. I've dropped it as a finding. What I did instead was check that the decision is faithfully implemented, since a policy is only as good as its execution — and it is.

The stated rule holds everywhere it should

Audited all 10 buyer-parsed response models from the client's task map, walking each transitively for any field embedding a CreativeManifest:

response fields embedding a manifest that are NOT tolerant: 0

Every one resolves to a private readback view. The paths named in Option B are all covered, including the ones that don't appear in that map — _ReadbackOffer carries creative_manifest and creative_data for the trusted-match hops, and the async/completed-task wrapper follows the same rule.

Isolation still holds across the wider surface

This was the main thing worth re-checking, since Option B multiplied the clone count. Seven readback views now exist — _ManifestReadbackModel, _VersionedManifestReadbackModel, _ReadbackCreativeManifest, _DeliveryVariant, _BuildReadbackVariant, _BuildReadbackCreative, _ReadbackOffer — and none subclasses a strict model. The manifest view rejects on all six bypass probes (instance and .model_dump(), against generated manifest, generated variant, and public manifest).

The documentation is there, and it's accurate

I should correct myself: in my last comment I said the boundary should be documented so adopters don't assume response creatives are tolerant. It already is, and precisely — the migration guide names the exact field:

Direct Creative.format_kind and CreativeAsset.format_kind fields remain strict, including ListCreativesResponse.creatives[].format_kind. The tolerance applies to response manifests, not to these directly embedded creative types.

plus a table enumerating every tolerant response field, and a matching CHANGELOG entry. My earlier "not mentioned" read was my own grep searching the task name list_creatives while the docs use the type name ListCreativesResponse — my search was too narrow, not a gap in the docs.

I also checked the reuse guidance against measured behaviour rather than taking it at face value, since it's the kind of sentence that's easy to over-promise:

To reuse a returned manifest as input, dump it and validate it with CreativeManifest.model_validate(returned_manifest.model_dump()). An unknown kind fails this validation … Passing the tolerant instance directly also cannot bypass the input validator.

Both claims match what I measured: the dump route rejects an unknown kind with enum, and the instance route rejects with model_type. No over-promise.

Verdict

Cleared on the implementation. 225 focused readback tests pass, ruff clean, my build_creative/preview_creative blocker is closed, delivery is unregressed, inputs remain strict, and the documented strict/tolerant boundary matches the code exactly.

[aao-secretariat]

Ladon verdict: Approve

Approve.

Subsequent pass over the b10d530 delta ("tolerate manifests across buyer responses"). This is a net improvement / continuation of a prior clean approve.

What I checked:

• The breaking narrowing of CreativeAsset/Creative/CreativeManifest.format_kind to the closed CanonicalFormatKind carries the correct semver signal: fix(types)! prefix + BREAKING CHANGE: footer, migration guide (docs/canonical-format-kinds-migration.md), and CHANGELOG.md prose — satisfies the mandatory public-surface semver gate.
• The private tolerant readback views deliberately do not subclass strict inputs, preserve min_length/version-envelope/default constraints, patch only fields that exist, and keep generated imports within the allowlisted _forward_compat.py — type-system import layering respected.
• Round-trip, bypass, and constraint tests confirm the behavior.

High-risk flag is true only because src/adcp/types/_forward_compat.py is (modified) — no medium-or-higher finding was raised on it, so the modification is presumed safe per the high-risk rules. No deletions.

Decision-table walk: no critical/high (row 1 no); gated_paths false (row 2 no); no (deleted) (row 3 no); no medium data-loss/schema/infra (row 4 no); no medium finding at all so rows 5/8 no; prior decision was approve, not escalate (row 6 no); no no-auto-approve team match (row 7 no). Falls through to row 9 — approve.

No blocking or medium findings.