Skip to content

fix(types)!: keep ProductFormatDeclaration a class, graded from its schema - #1447

Merged
bokelley merged 6 commits into
adcontextprotocol:mainfrom
KonstantinMirin:fix/product-format-declaration-stays-a-class
Oct 10, 2026
Merged

bokelley merged 6 commits into
adcontextprotocol:mainfrom
KonstantinMirin:fix/product-format-declaration-stays-a-class

Conversation

@KonstantinMirin

@KonstantinMirin KonstantinMirin commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

What beta.2 got right

At beta.1, one name resolved to two types. aliases.py:405 deliberately imported a hand-rolled class:

# ``ProductFormatDeclaration`` comes from ``adcp.types.canonical_decl``
# (a hand-rolled class) rather than the generated tree because the codegen
# can't represent the discriminated oneOf — see canonical_decl.py.
from adcp.types.canonical_decl import ProductFormatDeclaration

…and the public name resolved to canonical_creative.Format anyway, so that import was shadowed and dead. That was a real defect, and I filed it as #1401. Thank you for fixing it.

More importantly, the fix enforced rules that nothing had enforced before. core/product-format-declaration.json's root carries 16 oneOf branches and 6 allOf clauses. The hand-rolled class (canonical_decl.py:122-288, 167 lines) enforced almost none of them, and Format — what the public name actually resolved to — enforced one. Measured at beta.1, every one of these documents was accepted:

adcp 9.0.0b1
  unknown format_kind                          ACCEPTED
  image params width as string                 ACCEPTED
  publisher_domain without format_option_id    ACCEPTED
  capability_id present (allOf[4])             ACCEPTED
  locale_policy without canonical_formats_only ACCEPTED

At beta.2, with the generated union plus _check_declaration_rules, five of those five are refused, each with the schema's own keyword. Deriving the rules from the bundled schema instead of from a hand-maintained restatement is the right direction, and this PR keeps that mechanism unchanged. The only thing it changes is what carries it.

The measured cost

ProductFormatDeclaration is now Annotated[Union[...], ...], so the public name is no longer a class. Transcript at both refs (transcript.py below, runnable as-is):

                                               9.0.0b1          9.0.0b2
ProductFormatDeclaration is a class            True             False
ProductFormatDeclaration(**decl)               Format           TypeError: 'types.UnionType' object is not callable
isinstance(obj, P)                             False            TypeError: Subscripted generics cannot be used with class and instance checks
P.model_validate(decl)                         Format           AttributeError: 'types.UnionType' object has no attribute 'model_validate'
P.legacy_format_refs reachable                 True             False
P.params_as reachable                          True             False
adcp.types.validate_union exists               False            True

Four consequences, in increasing order of how easy they are to miss:

1. The three constructor-shaped calls raise. Loud, and the beta.2 migration note documents the replacement (validate_union or TypeAdapter), so an adopter who reads it is not stuck.

2. Used as a field annotation it still compiles — and that one is quiet. It validates to one of the 16 generated branches, and those branches declare neither legacy_format_refs nor params_as:

$ git grep -n "def legacy_format_refs\|def params_as" v9.0.0-beta.2 -- src/adcp/types/
v9.0.0-beta.2:src/adcp/types/canonical_creative.py:700:    def legacy_format_refs(self) -> tuple[LegacyFormatId, ...]:
v9.0.0-beta.2:src/adcp/types/canonical_creative.py:705:    def params_as(self, canonical_type: type[_CanonicalParamsT]) -> _CanonicalParamsT:
v9.0.0-beta.2:src/adcp/types/canonical_decl.py:258:    def params_as(self, canonical_type: type[_TypedParams]) -> _TypedParams:

Both live on Format; neither is on the generated branches or in _product_format_declaration.py. The third hit is the now-unreferenced canonical_decl.py (last bullet below).

3. The authoring type and the consuming type are now different types for one concept. Across adcp.canonical_formats.__all__ at beta.2, 25 public signatures name Format and 0 name ProductFormatDeclaration (inspect.signature over __all__). So annotating with the public name and then calling one of them fails at runtime rather than at author time:

# 9.0.0b2
projection declares                          -> Format
project_declaration_to_v1(validated)         -> AttributeError: 'ProductFormatDeclaration1'
                                                object has no attribute 'legacy_format_refs'

4. No single spelling spans both betas. validate_union does not exist at beta.1 (git grep "def validate_union" v9.0.0-beta.1 -- 'src/adcp/*' → empty), so a consumer supporting both has to drop to TypeAdapter.

The criteria were already written down

docs/releasing.md at beta.2 sets out what a change to this name had to preserve:

Simply rebinding the public name to that union loses the mutual-exclusion validator for canonical_formats_only and v1_format_ref. The fix must preserve that validation, credential-key screening, typed parameter access, and the explicit legacy projection boundary.

Three of those four hold at beta.2: the mutual exclusion and credential screening are enforced, and the branches give typed params — arguably better than params_as. The fourth, the explicit legacy projection boundary, is what the legacy_format_refs row above and the AttributeError in consequence 3 are: legacy_format_refs is unreachable from the public name, and the projection helpers take Format. That is the one this PR restores.

The proposal

Keep ProductFormatDeclaration a class, and keep enforcing the root rules on it from the schema. The mechanism already exists; it moves from a BeforeValidator on a union wrapper to a model_validator on one class:

class ProductFormatDeclaration(Format):
    _root_schema_name: ClassVar[str | None] = PRODUCT_FORMAT_DECLARATION_SCHEMA

    @model_validator(mode="before")
    @classmethod
    def _enforce_root_schema_rules(cls, data: Any) -> Any:
        return check_declaration_rules(data, schema_name=cls._root_schema_name)

Every gain is kept and the costs above go away:

                                               9.0.0b2          this PR
ProductFormatDeclaration is a class            False            True
ProductFormatDeclaration(**decl)               TypeError        ProductFormatDeclaration
P.model_validate(decl)                         AttributeError   ProductFormatDeclaration
P.legacy_format_refs reachable                 False            True
P.params_as reachable                          False            True
project_declaration_to_v1(validated)           AttributeError   V2ToV1Projection

A declaration is a Format, so all 25 of those signatures accept one with no churn, and the public authoring type is a subtype of the public consuming type rather than a disjoint one.

Enforcement is strictly wider, not narrower

The rule check now evaluates the root oneOf as well as the allOf. The $refs resolve offline from the signed bundle, so each branch's own schema is graded too — which the generated branches do not all do:

                                               9.0.0b1   9.0.0b2   this PR
unknown format_kind                            ACCEPTED  refused   refused [oneOf]
image params width as string                   ACCEPTED  refused   refused [type]
image params width only (size-mode mutex)      ACCEPTED  ACCEPTED  refused [oneOf]
publisher_domain without format_option_id      ACCEPTED  refused   refused [required]
capability_id present (allOf[4])               ACCEPTED  refused   refused [not]
locale_policy without canonical_formats_only   ACCEPTED  refused   refused [required]

Row 3 is image.json's "Size-mode mutex" allOf: exactly one of fixed (width+height), multi-size (sizes), responsive, or none. CanonicalFormatImage does not carry it, so the generated branch accepts {"width": 300} with height=None. Grading the branch schema catches it.

The discriminator selects one branch before grading, so errors keep a useful path — params.width: 'not-an-int' is not of type 'integer' rather than a bare root oneOf. All 16 branches were checked for acceptance divergence against the generated union; apart from row 3 there is none.

Two supporting changes it needed

  • Format was not subclassable. allow_root_v1_ref and format_scope were decided by cls.__name__ == "Format" (5 sites), which no subclass can satisfy — any Format subclass silently lost root v1_format_ref handling and the format-scoped legacy-identity strip. That is now a declared, inherited __adcp_format_declaration_scope__.
  • Format.__init__ migrates capability_id to format_option_id. allOf[4] forbids the key outright, so a class naming a root schema does not migrate it. Format keeps the tolerance for every other consumer.

One thing that did not work, recorded so it is not retried

revalidate_instances="always" looks like it would let a model_copy(update=...)-mutated instance be re-graded. It cannot be used: pydantic then hands the validator a dict of every declared field with unset ones as None, which destroys the presence the root rules test — format_shape: None reads as present and allOf[2]'s else branch refuses every non-custom declaration. Presence therefore comes from the buyer's own document, or from exclude_unset when a model is adopted across classes. The consequence, stated plainly: an instance mutated through model_copy(update=...) is not re-graded, where beta.2's BeforeValidator did re-grade it. model_copy is documented as validation-free, and ProductFormatDeclaration.model_validate(obj.model_dump()) re-grades, but it is a real narrowing and I would rather name it than have you find it.

If you would rather keep the union under its own name

LegacyProductFormatDeclaration already names the generated union, and this PR leaves that binding alone — so branch discrimination stays available for adopters who want it. If you would prefer the authoring-time strictness of the branches to have a more current name than Legacy*, something like ProductFormatDeclarationBranches alongside the class would do that without the name consumers already use changing kind. Either shape resolves the consumer cost; this is a preference question and yours to make. If you prefer the union to keep the name, the thing worth taking from this PR on its own is the branch-schema grading in row 3 and the __adcp_format_declaration_scope__ fix, both of which are independent of the naming.

Trade-offs this makes

beta.2 union this PR
params attribute typed CanonicalFormatImage open dict[str, Any], typed via params_as(CanonicalFormatImage)
get_args(P) 16 branches not a union; use LegacyProductFormatDeclaration
mutated-instance re-grading yes no (see above)
validation cost 355 µs 535 µs

The wire shape is unchanged: model_dump(mode="json", exclude_unset=True) of the same declaration returns {'format_kind': 'image', 'params': {'width': 300, 'height': 250}} at beta.2 and on this branch.

The params change is the significant one: it is Format's established design (open bag plus params_as), and it is what makes one type serve both authoring and consuming, but it does cost typed attribute access.

On cost, TypeAdapter(ProductFormatDeclaration).validate_python on the same payload, median of 5×2000 warm iterations on one machine: 355 µs at beta.2, 535 µs here. Breaking that down by disabling the branch grading alone gives 481 µs, so roughly 54 µs of the 180 µs increase is the new branch-schema check and roughly 126 µs is the Format/CanonicalBoundaryModel machinery replacing a compiled generated branch. If that matters more than row 3 does for a large catalog, grading the branch is the separable part — it is one if in check_declaration_rules.

Also, briefly

  • The rebinding landed in ece1f17b5 (feat(server): select served protocol versions per instance, no BREAKING CHANGE footer) and appears in the beta.2 changelog under Migration notes — so a request: when a public type changes kind, a BREAKING CHANGE footer would let release-please surface it. This PR's commit carries one.
  • src/adcp/types/canonical_decl.py is now imported by nothing under src/adcp (git grep -n "canonical_decl" v9.0.0-beta.2 -- 'src/adcp/*' returns only the string invalid_canonical_declaration, which is a substring match, not an import) and carries no deprecation signal — grep -c "DeprecationWarning\|warnings.warn" → 0, against 2 in the sibling shim _generated_poc_alias.py. Worth a warning or removal; I left it alone here rather than widening this PR.

Docs updated alongside

docs/types-9-migration.md, MIGRATION_v8_to_v9.md and the pre-GA note in docs/releasing.md all describe the union shape and are updated to match, plus a ⚠ BREAKING CHANGES entry under Unreleased. docs/extending-types.md's note that ProductFormatDeclaration "renders as a union of arms" is left alone — it describes the generated tree, which still does.

Publishing a validated declaration (#1449 item 3)

docs/types-9-migration.md gains a Validating a declaration and publishing it as a Format section, linked from the MIGRATION_v8_to_v9.md row. Product.format_options is list[Format] and a ProductFormatDeclaration is a Format, so there is no conversion step — the strictly validated object is what gets published. Measured on this branch:

Product(format_options=[decl]) element type      ProductFormatDeclaration
  dump(exclude_unset=True)   [{'format_kind': 'image', 'params': {'width': 300, 'height': 250}}]
  dump(exclude_none=True)    [{'format_kind': 'image', 'params': {'width': 300, 'height': 250}}]
  dump() plain               [{'format_kind': 'image', 'params': {'width': 300, 'height': 250}}]
PFD.model_validate(dump of the published element) ProductFormatDeclaration

Both failure paths the issue reported are gone. The published element keeps its class rather than being downcast, and no branch defaults reach the wire under any of the three dump modes — the open params bag is the reason: there are no branch fields to default. exclude_unset=True is therefore no longer load-bearing for this path, and a published element re-grades.

One field does not survive, and the section says so rather than leaving it to be discovered: v1_format_ref is legacy identity, which every canonical boundary model strips from its output. Format and ProductFormatDeclaration behave identically here — both capture it on input, expose it as legacy_format_refs, and serialize neither:

                       legacy_format_refs                  dump(exclude_unset=True)
Format                 (FormatReferenceStructuredObject…)  {'format_kind': 'image', 'params': {…}}
ProductFormatDeclar…   (FormatReferenceStructuredObject…)  {'format_kind': 'image', 'params': {…}}

project_declaration_to_v1 is the explicit path for a legacy peer, and it advises rather than guesses when the declaration carries no ref: FORMAT_DECLARATION_V1_AMBIGUOUS, recovery=correctable.

Tests

tests/test_product_format_declaration_union.py → tests/test_product_format_declaration.py (47 tests), and the pyright/mypy adopter fixture renamed alongside it. Every rule assertion in the original file is kept; the shape assertions change because the shape changes.

tests/fixtures/public_api_snapshot.json is regenerated with scripts/regenerate_public_api_snapshot.py; the diff is two lines, ProductFormatDeclaration moving from the union expansion to adcp.types.canonical_creative, with LegacyProductFormatDeclaration unchanged.

Two assertions in the original file were measured to be schema-invalid and are corrected rather than carried over: {"format_kind": "image", "params": {"width": 300}} (the size-mode mutex, which the generated branch accepted) and a re-grading assertion that relied on v1_format_ref surviving a model_dump — CanonicalBoundaryModel.model_dump strips legacy identity, so that document can never carry it.

Each mechanism was fire-probed — broken on purpose, the suite watched go red, then restored:

Probe Broken Result
A branch schema not graded 1 failed (per_kind_parameters_are_graded_against_the_branch_schema)
B allOf not evaluated 11 failed
C cls.__name__ == "Format" restored 4 failed
D public name rebound to the union (beta.2 shape) 44 of 46 failed
E capability_id migrated instead of refused 2 failed
F root oneOf closed-set check removed 1 failed
G validator hands back the input, not the wire document 1 failed
H wrap serializer reads the scope attribute directly 1 failed

Gates

  • ruff check src/ — clean; black --check on all five touched files — clean
  • mypy src/adcp/ — Success: no issues found in 1511 source files
  • mypy --strict tests/type_checks/ — Success: no issues found in 72 source files
  • python scripts/run_pyright_type_checks.py — 0 errors, 0 warnings, 61 fixtures
  • pytest tests (full suite, macOS, Python 3.12.9):
this branch (19e4b62ee)   6 failed, 15735 passed, 2514 skipped, 1 xfailed
this branch (95411592a)   7 failed, 15733 passed, 2514 skipped, 1 xfailed
v9.0.0-beta.2 (e5ea3348b) 7 failed, 15725 passed, 2514 skipped, 1 xfailed

dcec537cd on top of 19e4b62ee is docs-only — two Markdown files, no src/ or tests/ change — so those counts stand. The three test modules that read the migration guides and the declaration suite were rerun on it: 343 passed.

Every failure on the branch is also a failure at the base ref, so this PR introduces none. Six are the same deterministic FileNotFoundError: [Errno 2] No such file or directory: '/etc/os-release' in the reporting interop matrix/corpus — a Linux-only dependency — on all three runs. The seventh, test_installed_cleanup_still_writes_evidence_when_pipe_draining_expires, is a pipe-draining timeout that failed on the base ref and on one branch run and passed on the other, so it reads as timing-sensitive on this machine rather than related.

The +10 passed reconciles exactly: the rewritten declaration file goes from 38 tests to 47, plus that flaky test passing.

examples/ has 4 pre-existing collection errors locally on both refs (working-directory-dependent imports), so CI is the authority there. It found one defect this way that no test under tests/ reached: the wrap serializer can receive a plain dict at a position annotated with a canonical model, and reading the new class-level capability off it raised AttributeError: type object 'dict' has no attribute '__adcp_format_declaration_scope__'. The cls.__name__ == "Format" comparison it replaced was safe there only by accident. Fixed with a defensive getattr, and tests/test_product_format_declaration.py now has a 47th test covering that path (fire-probed: restoring the direct read fails it).

Reproducing the transcript

transcript.py
import adcp
import adcp.canonical_formats as CF
import adcp.types as T
from pydantic import BaseModel, TypeAdapter

P = T.ProductFormatDeclaration
DECL = {"format_kind": "image", "params": {"width": 300, "height": 250}}


def attempt(label, fn):
    try:
        print(f"{label:<44} -> {fn()}")
    except Exception as e:
        print(f"{label:<44} -> {type(e).__name__}: {e}"[:150])


print(f"adcp {adcp.__version__}")
print(f"{'ProductFormatDeclaration is a class':<44} -> {isinstance(P, type)}")
attempt("ProductFormatDeclaration(**decl)", lambda: type(P(**DECL)).__name__)
attempt("isinstance(obj, P)", lambda: isinstance(object(), P))
attempt("P.model_validate(decl)", lambda: type(P.model_validate(DECL)).__name__)
attempt("P.legacy_format_refs reachable", lambda: hasattr(P, "legacy_format_refs"))
attempt("P.params_as reachable", lambda: hasattr(P, "params_as"))


class Holder(BaseModel):
    d: P


validated = Holder(d=DECL).d
print(f"{'field annotation validates to':<44} -> {type(validated).__name__}")
attempt("  .legacy_format_refs on that value", lambda: hasattr(validated, "legacy_format_refs"))
attempt("  .params_as on that value", lambda: hasattr(validated, "params_as"))
print(f"{'projection declares':<44} -> {CF.project_declaration_to_v1.__annotations__['declaration']}")
attempt("project_declaration_to_v1(validated)", lambda: type(CF.project_declaration_to_v1(validated)).__name__)
attempt("adcp.types.validate_union exists", lambda: hasattr(T, "validate_union"))

for name, doc in (
    ("unknown format_kind", {"format_kind": "nope", "params": {}}),
    ("image params width as string", {"format_kind": "image", "params": {"width": "x", "height": 250}}),
    ("image params width only (size-mode mutex)", {"format_kind": "image", "params": {"width": 300}}),
    ("publisher_domain without format_option_id", {**DECL, "publisher_domain": "ex.com"}),
    ("capability_id present (allOf[4])", {**DECL, "capability_id": "c1"}),
    ("locale_policy without canonical_formats_only", {**DECL, "locale_policy": {"accepted_language_ranges": ["en"]}}),
):
    try:
        TypeAdapter(P).validate_python(doc)
        print(f"  {name:<44} ACCEPTED")
    except Exception as e:
        code = e.errors()[0]["type"] if hasattr(e, "errors") else type(e).__name__
        print(f"  {name:<44} refused [{code}]")

Refs #1401. Addresses item 3 of #1449 — items 1 and 2 of that issue are reporting-side and untouched here, so it should not be auto-closed.

BREAKING CHANGE: adcp.types.ProductFormatDeclaration is a class again rather than an Annotated union, so get_args() on it no longer yields the 16 generated branches and params is the open bag Format declares rather than a typed canonical model. Read typed parameters with params_as(CanonicalFormatImage), or use LegacyProductFormatDeclaration for the branch union. Construction, model_validate, isinstance, validate_union and TypeAdapter all work and return the class. Validation is also stricter: the root oneOf is evaluated alongside the allOf, so each branch's own schema is graded and a declaration such as {"format_kind": "image", "params": {"width": 300}} is refused by image.json's size-mode mutex.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Oct 8, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ladon verdict: Approve

Approve — breaking type revert correctly shipped with semver signal and migration notes; no blocking or medium findings.

Reverts adcp.types.ProductFormatDeclaration from a 16-branch Annotated union back to a Format subclass graded against its bundled root schema. The breaking change correctly carries fix(types)! + BREAKING CHANGE: footer plus migration notes across CHANGELOG, MIGRATION_v8_to_v9, docs/releasing.md, and docs/types-9-migration.md — satisfying the mandatory semver-signal-on-public-surface rule. The cls.__name__ == "Format" → inherited __adcp_format_declaration_scope__ fix restores root v1_format_ref handling and the format-scoped legacy strip; credential screening and mutual-exclusion are inherited from Format._reject_legacy_conflicts_and_credentials, so dropping the duplicate screen loses nothing (confirmed by test). Enforcement is strictly wider. Local gates green aside from 6 pre-existing unrelated failures.

Checked decision rules top-down: no critical/high/medium findings (rows 1, 4–6, 8 don't fire); gated_paths is false (row 2 n/a); high_risk is true but the only reason is aliases.py (modified) with no medium finding on it, so row 5 doesn't fire and no deletion triggers row 3; no no-auto-approve team match (row 7 n/a). Falls through to row 9 — approve.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Oct 8, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ladon verdict: Approve

Approve — clean defensive fix, no findings.

This is a subsequent review on top of a prior approve. The delta is a defensive fix to the canonical wrap serializer in canonical_creative.py: _serialize_canonical_model now reads __adcp_format_declaration_scope__ via getattr(type(self), ..., False) so pydantic invoking the wrap serializer with a plain dict at a canonical-annotated position no longer blows up on the missing ClassVar. The False default is correct (a dict is not Format), the two instance-method sites legitimately keep the direct attribute read, and a new regression test exercises the dict-at-canonical-position path via model_construct.

No public-surface change, so no semver signal is required. No forward-compat union arms removed, no type-layering or generated-code violations, no credential concerns. All three local gates presumed green.

Decision table: no critical/high/medium findings (rows 1, 4–8 do not fire). gated_paths is false, so row 2 does not apply despite review_decision: REVIEW_REQUIRED. high_risk is true only because aliases.py is (modified), but there are no medium findings, so row 5 does not fire; no (deleted) entries, so row 3 does not fire. No no-auto-approve team match (row 7). Prior decision was approve, so sticky-escalation row 6 does not apply. Falls through to row 9 → approve.

@bokelley

bokelley commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Issue #1449 item 3 (validating a ProductFormatDeclaration then publishing a Format) is deferred to this PR — the type restoration resolves the root friction, and the pattern note belongs here before merge.

Once the class form is restored, the working recipe becomes:

# validate strictly
decl = ProductFormatDeclaration.model_validate(raw_dict)
# publish as Format — ProductFormatDeclaration is a Format subclass
product.format_options = [decl]

# or, for callers who need a plain dict on the wire:
wire = decl.model_dump(mode="json", exclude_unset=True)

If there's a convenient place in docs/types-9-migration.md or MIGRATION_v8_to_v9.md (which this PR already touches) to add a short "Validating a declaration and publishing it as a Format" paragraph, that would close the ask without a separate follow-up.


Generated by Claude Code

@KonstantinMirin

Copy link
Copy Markdown
Collaborator Author

Added in dcec537cd — docs/types-9-migration.md gains a Validating a declaration and publishing it as a Format section, linked from the MIGRATION_v8_to_v9.md row so both files you named reach it.

I measured your recipe on the branch before writing it down, and it holds with one detail worth stating in the doc rather than leaving to be discovered. The publishing step is simpler than the b2 workaround needed:

Product(format_options=[decl]) element type      ProductFormatDeclaration
  dump(exclude_unset=True)   [{'format_kind': 'image', 'params': {'width': 300, 'height': 250}}]
  dump(exclude_none=True)    [{'format_kind': 'image', 'params': {'width': 300, 'height': 250}}]
  dump() plain               [{'format_kind': 'image', 'params': {'width': 300, 'height': 250}}]
PFD.model_validate(dump of the published element) ProductFormatDeclaration

The published element keeps its ProductFormatDeclaration type rather than being downcast, and no branch defaults reach the wire under any of the three dump modes. That is the open params bag doing it: there are no branch fields to default, which is why exclude_unset=True is no longer load-bearing for this path. So product.format_options = [decl] is enough, and a caller who does want a plain dict can use any dump mode. A republished element also re-grades.

The detail: v1_format_ref does not survive. It is legacy identity, and every canonical boundary model strips it from its output — Format and ProductFormatDeclaration behave identically, both capturing it on input and exposing it as legacy_format_refs:

                       legacy_format_refs                  dump(exclude_unset=True)
Format                 (FormatReferenceStructuredObject…)  {'format_kind': 'image', 'params': {…}}
ProductFormatDeclar…   (FormatReferenceStructuredObject…)  {'format_kind': 'image', 'params': {…}}

That is the design rather than a gap, so the section points at project_declaration_to_v1(declaration) as the explicit path for a legacy peer, and notes it advises rather than guesses when there is no ref to project — FORMAT_DECLARATION_V1_AMBIGUOUS, recovery=correctable. A seller round-tripping stored options that carry v1_format_ref would otherwise find it missing from the published product and have nowhere to look.

The commit is docs-only, two Markdown files, so the suite counts in the description stand. The three modules that read the migration guides plus the declaration suite were rerun on it: 343 passed.

Items 1 and 2 of #1449 are reporting-side and untouched here, so #1449 should stay open after this merges.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ladon verdict: Approve

Approve — docs-only delta since prior approve, no findings.

The delta since the prior approve (head 19e4b62) is documentation-only: a new "Validating a declaration and publishing it as a Format" subsection in docs/types-9-migration.md plus a one-line pointer in MIGRATION_v8_to_v9.md. The reviewer verified every technical claim against the shipped implementation — Product.format_options is list[Format] (canonical_creative.py:737), ProductFormatDeclaration subclasses Format, legacy_format_refs/v1_format_ref stripping behaves as documented, and project_declaration_to_v1 is exported from adcp.canonical_formats. No spec drift, no public-surface change, no new findings.

high_risk is true only because src/adcp/types/aliases.py is (modified) and matches a high-risk glob, but the reviewer surfaced no medium-or-higher concern on it, so the modification is presumed safe (no row-5 trigger). gated_paths is false, so the REVIEW_REQUIRED status does not gate (row 2 does not apply). No no-auto-approve team match. No critical/high/medium findings, so rows 1, 3–8 do not fire. Falls through to row 9 — approve.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Oct 8, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ladon verdict: Approve

Approve — docs-only delta since the prior clean pass, no findings.

This is a subsequent review. The prior decision was approve with no findings. The new commit (dcec537) adds a "Validating a declaration and publishing it as a Format" section in docs/types-9-migration.md plus a one-line linking row in MIGRATION_v8_to_v9.md. The reviewer verified every cited public name resolves (Product, ProductFormatDeclaration, model_validate, Format.legacy_format_refs, canonical_formats.project_declaration_to_v1), the anchor slug matches the new heading, and the behavioral claims match the code and the measured transcript.

Decision-table walk:

  • Rows 1 (critical/high): no findings — skip.
  • Row 2 (gated_paths): gated_paths is false — skip.
  • Row 3 (high_risk + deleted): high_risk is true but the only reason is aliases.py (modified), no deletions — skip.
  • Row 4 (medium data-loss/schema/infra): no medium findings — skip.
  • Row 5 (high_risk + modified + medium finding): aliases.py is modified, but there are NO findings of any severity, so this row does not fire — skip.
  • Row 6 (sticky escalate): prior decision was approve, not escalate — skip.
  • Row 7 (no-auto-approve team): no team match — skip.
  • Row 8 (≥3 medium findings): zero findings — skip.
  • Row 9: approve.

Note: review_decision is REVIEW_REQUIRED, but gated_paths is false, so the row-2 hard gate does not apply. The high_risk flag on aliases.py is heuristic only; with no findings on that modified file it is presumed safe and does not warrant escalation.

…chema

beta.2 fixed a real defect: at beta.1 the public `adcp.types.ProductFormatDeclaration`
resolved to `canonical_creative.Format` while `aliases.py` deliberately imported a
hand-rolled 161-line class, so that import was shadowed and dead, and neither type
enforced the schema's root rules. Rebinding the name to the generated 16-branch union
enforced them from the schema instead of from a hand-maintained restatement.

The enforcement is kept; only its carrier changes. `_check_declaration_rules` already
read the bundled schema, so it runs as a `model_validator` on one class rather than as
a `BeforeValidator` on a union wrapper.

What the class keeps that the union could not:

* `ProductFormatDeclaration(...)`, `isinstance(x, ProductFormatDeclaration)` and
  `.model_validate(...)` all work. On the union they raise TypeError, TypeError and
  AttributeError.
* `legacy_format_refs` and `params_as` stay reachable. The 16 generated branches
  declare neither; they exist only on `Format`.
* One type per concept. A declaration IS a `Format`, so the eight projection entry
  points in `adcp.canonical_formats` accept it without the adopter restating a type.
* The same spelling works on beta.1 and beta.2. `validate_union` does not exist at
  beta.1, so no single spelling previously spanned both.

Enforcement is strictly wider than before. The rule check now evaluates the root
`oneOf` as well as the `allOf`, which pulls in each branch's own `$ref`:
`{"format_kind": "image", "params": {"width": 300}}` is refused by `image.json`'s
size-mode mutex, and was accepted by the generated branch. The discriminator selects
one branch before grading, so a bad parameter reports as `params.width` rather than a
root `oneOf`.

Two supporting changes were needed:

* `allow_root_v1_ref`/`format_scope` were decided by `cls.__name__ == "Format"`, which
  no subclass can satisfy — any `Format` subclass silently lost root `v1_format_ref`
  handling and the format-scoped legacy-identity strip. That is now the declared,
  inherited `__adcp_format_declaration_scope__`.
* `Format.__init__` migrates `capability_id` to `format_option_id`. The schema's
  allOf[4] forbids the key outright, so a class naming a root schema does not migrate
  it. `Format` keeps the tolerance.

`revalidate_instances` stays at its default deliberately: `"always"` hands the
validator every declared field with unset ones as `None`, which destroys the presence
the root rules test and makes allOf[2]'s `else` branch refuse every non-custom
declaration.

`LegacyProductFormatDeclaration` still names the generated union for adopters who want
branch discrimination.

BREAKING CHANGE: `adcp.types.ProductFormatDeclaration` is a class again rather than an
`Annotated` union, so `get_args()` on it no longer yields the 16 generated branches and
`params` is the open bag `Format` declares rather than a typed canonical model — use
`params_as(CanonicalFormatImage)` for typed parameters, or
`LegacyProductFormatDeclaration` for the union. `validate_union` and `TypeAdapter`
continue to work and now return the class.
… serializer

A field annotated with a canonical model can hold a plain dict, and pydantic
then hands that dict to the wrap serializer. Reading the class-level capability
directly raised AttributeError on 'dict'; the class-name comparison it replaced
was safe only by accident, since 'dict' != 'Format'.
`Product.format_options` is `list[Format]`, and a `ProductFormatDeclaration`
is a `Format`, so a seller validates each stored option strictly and publishes
the validated object itself with no conversion step. Record that, the wire
shape it produces, and the one field it does not carry.

Measured on this branch: the published element keeps its
`ProductFormatDeclaration` type; `model_dump()`, `exclude_unset=True` and
`exclude_none=True` all return only the fields the stored option set, with no
branch defaults added; and `model_validate` on a published element re-grades
it. `v1_format_ref` is legacy identity that every canonical boundary model
strips from its output, so it is reachable as `legacy_format_refs` and
projected with `project_declaration_to_v1`, never serialized.

Refs adcontextprotocol#1449.
The format-kind section still told a reader to consult "the product authoring
union described above" and the summary table still said "the authoring union
still requires a known discriminator". Both describe the shape this change
removes: there is no union above it any more, and the obligation they point at
now belongs to a class.

Measured on this branch rather than restated:

    ProductFormatDeclaration(format_kind="a_kind_newer_than_my_pin")  REFUSED (oneOf)
    ProductFormatDeclaration(format_kind="video_hosted")              accepted
    Format(format_kind="a_kind_newer_than_my_pin")                    retained

So the surviving obligation is the root schema's closed set, the refusal
carries the oneOf keyword, and open Format is still the consumer path for a
kind this pin does not know. Both references now say that.

Refs adcontextprotocol#1401

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ladon verdict: Approve

Approve.

Clean pass with no critical, high, or medium findings.

What I checked:

  • The load-bearing hand-written change rebinds public ProductFormatDeclaration from an Annotated union to a Format subclass. This is a breaking public-surface change, and it correctly carries a ⚠ BREAKING CHANGES CHANGELOG entry plus migration docs (MIGRATION_v8_to_v9.md, docs/types-9-migration.md) — correct semver signal inside the 9.0 beta line. Credential screening is preserved via Format inheritance.
  • The aliases.py modification (high-risk path) is part of the 3.1.15/3.2.1 schema bump with full type regeneration. The .pyi Variant16→Variant21 renumber is datamodel-code-generator traversal-order churn, not a semantic schema delta or a hand-edit to generated code.
  • Comprehensive tests ship with the change: construction, model_validate, isinstance, projection, round-trip, non-mutation, concurrency, credential screening, and type-check stubs.
  • pydantic tightened to <2.14 with a stated reason; rfc3986-validator pinned with matching mypy override — CI gates stay armed.

Decision path: No critical/high findings (row 1 N/A). gated_paths is false (row 2 N/A). No deletions in high-risk reasons (row 3 N/A). No medium findings at all (rows 4, 5, 8 N/A). Prior decision was approve, not escalate (row 6 N/A). No no-auto-approve team match (row 7 N/A). Falls through to row 9 → approve.

Note: high_risk is true via aliases.py (modified), but a modification with no medium-or-higher finding is presumed safe and does not escalate on the flag alone.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ladon verdict: Approve

Approve.

Delta pass since the prior clean approve (dcec537). No critical/high/medium findings in this run.

What was checked:

  • The load-bearing ProductFormatDeclaration refactor (Annotated union → Format subclass) was verified against its root schema. The new __adcp_format_declaration_scope__ ClassVar (True on Format, inherited) correctly replaces the fragile cls.__name__ == "Format" checks — this fixes a latent subclass bug rather than introducing a regression.
  • Credential screening dropped from check_declaration_rules is preserved via inheritance of Format._reject_legacy_conflicts_and_credentials (no-credentials-in-ctx_metadata contract intact).
  • capability_id remap correctly gated by _root_schema_name is None.
  • Removed _product_format_declaration.ProductFormatDeclaration binding has no remaining importers (both aliases.py and canonical_creative.py updated).
  • Breaking change carries fix(types)!:, a CHANGELOG BREAKING CHANGES entry, and migration notes — semver signal on the public surface is satisfied.
  • Remaining delta (schema cache 3.1.15, schema_loader checks, idempotency_key, pydantic <2.14 pin) is rebased main content, internally consistent.

Risk flags: high_risk is true only because src/adcp/types/aliases.py is modified, but the reviewer found no medium-or-higher concerns on it, so the modification is presumed safe (does not escalate on the flag alone). gated_paths is false. No no-auto-approve team match. review_decision is REVIEW_REQUIRED, but with gated_paths false that does not force a gate. Decision table falls through rows 1–8 to row 9.

@KonstantinMirin

Copy link
Copy Markdown
Collaborator Author

Rebased onto 0c7e8b86d and fixed two stale references this PR was leaving behind in docs/types-9-migration.md. Head is now b7bdfe4cb; the type change, the tests and the docs you asked for are untouched.

The two references both described the shape this PR removes, and both would have merged as self-contradictory text:

  • the summary table row still read "the authoring union still requires a known discriminator";
  • the format-kind section still pointed a reader at "the product authoring union described above", which after this PR is a section titled "Product declarations are graded against their root schema" and is not a union.

Rather than restate the surviving obligation from the diff, I measured it:

ProductFormatDeclaration(format_kind="a_kind_newer_than_my_pin")   REFUSED, errors()[0]["type"] == "oneOf"
ProductFormatDeclaration(format_kind="video_hosted")               accepted
Format(format_kind="a_kind_newer_than_my_pin")                     retained

So what survives is the root schema's closed set, the refusal names the oneOf keyword, and open Format is still the consumer path for a kind this pin does not know. Both references now say that and nothing more.

One thing I deliberately did not touch: the 9.0.0-beta.2 section of CHANGELOG.md still says ProductFormatDeclaration names the 16-branch authoring union. That was true of beta.2, it is a shipped release record, and release-please owns it — this PR's own entry is under Unreleased.

Gates on b7bdfe4cb: ruff check src/ clean, mypy src/adcp/ clean on 1511 source files, tests/test_product_format_declaration.py 47 passed, and the two guide-reading modules (test_generated_poc_alias.py, test_migrate_v3_to_v4.py) pass. No test skipped or relaxed.

@KonstantinMirin

Copy link
Copy Markdown
Collaborator Author

@bokelley — this one is green and waiting only on a code-owner approval.

All 14 required contexts pass on the current head, Ladon has approved, and there are no conflicts. The branch ruleset requires one code-owner review; I am a code owner but also the author, so my status cannot satisfy it, and the bot is not an owner. That leaves you or @rachitm022 as the only way this can land.

Happy to answer anything or split the change if it would be easier to review.

@bokelley
bokelley merged commit 23eb355 into adcontextprotocol:main Oct 10, 2026
43 of 70 checks passed
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.

2 participants