Repository navigation
Conversation
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — large but exceptionally well-documented type-lattice + canonical-subclass refactor with no blocking or medium findings.
Checked against repo-specific gates:
- Semver signal: breaking changes correctly flagged via
feat(types)!+BREAKING CHANGE:footers and migration prose; public_api_snapshot byte-identical. - Generated code: the generated_poc tree verified as pipeline output via
generate_types.py --check— regeneration, not hand-edits. - Import layering holds;
types/__init__.pylazy PEP 562 facade preserved under theif not TYPE_CHECKINGguard. - Load-bearing files reviewed in full:
canonical_creative.py__pydantic_init_subclass__field-strip (L2524-2552) and field-less AdcpRequest/AdcpResponse markers inbase.pyare correct; SDK helper widening holds. - 7 pytest failures disclosed as a strict subset of main's environmental failures.
Decision table: no critical/high/medium findings. gated_paths is false, so row 2 does not apply despite review_decision: REVIEW_REQUIRED. high_risk is true only on (modified) files, but with zero medium findings rows 4/5 do not fire. No deleted high-risk paths, no author team gate, no prior decision. Falls through to row 9 → approve.
…he version envelope Hand-written field-less markers in adcp.types.base carry the accessors and the three MRO-derived field classifiers; a post-generation pass inserts them as the first base of every task message named by the pinned bundle's task registry; and a schema transform in generate_types gives the pointer-ref and wire-equivalent inline version declarations real AdcpVersionEnvelope ancestry.
…asymmetry A field-presence check accepts any model declaring adcp_version and adcp_major_version; both markers refuse it. AdcpRequest is field-less by design, so inheriting it supplies nothing and a forger can inherit it and invent fields, while AdcpVersionEnvelope refuses that and accepts a model that is no task's request. The conjunction refuses both; neither predicate alone does.
…against a wheel scripts/task_message_lattice.py now holds the registry walk, the title-to-class mangling, the provenance header map and the root-class rule. Three consumers share it instead of each carrying a copy: the marker-insertion pass, the ratchet test, and a --package measurement that reads the registry and the generated tree out of an installed adcp package, so the ancestry counts can be re-measured against a wheel rather than trusted from a report.
Relocating the version stubs into a root allOf deletes the field from an arm that is a $ref elsewhere: dmcg merges a root properties into every arm it builds, but a root allOf base reaches only the arm built from the root object. Measured on the two pointer-ref proposals responses, RefineProposalsResponse2 went from 10 fields to 9 and RequestProposalsResponse4 from 16 to 15, both losing adcp_version under extra=forbid -- so a seller echoing the version on the submitted arm would have been rejected where it previously validated. Both schemas are refused and stay in the ratchet's residue by name; the all-arms ancestry is 70/77 either way, because that arm gains no ancestry from the rewrite.
…e copy The canonical creative layer built its 32 public classes with ``create_model``, copying ``model_fields`` off the generated wire model and inheriting nothing from it. That is why an injected generation-pass validator reaches the generated class and never the public one — the blocking finding on PR adcontextprotocol#1368. ARCH-ONE-SURFACE §6 refuses a second injection mechanism for the copy layer, so the copy goes instead: each public class is now ``class X(<generated>, CanonicalBoundaryModel)`` in real source. Four things the copy had to re-attach per class are declared once and inherited: * the envelope ancestry — ``_ENVELOPE_BASES`` and ``_canonical_clone_bases`` existed only because a copy loses it; it is transitive now, verified on all eleven canonical responses; * the wrap serializer that strips legacy creative identity; * the boundary's ``extra="allow"``, which still wins because ``CanonicalBoundaryModel`` is the last base; * the declared-field removal of legacy creative identity. This is the one thing inheritance cannot express, and it applies to 12 of the 32 sites (16 optional field declarations) through ``_field_definitions``' unconditional ``is_legacy_creative_identity_key`` filter — not through ``exclude=``, which no site used. It is now one ``__pydantic_init_subclass__`` rule on the boundary, reading the same strip predicate as the input validator, the schema sanitizer and the serializer. Also deleted: ``canonical_creative.pyi`` (253 lines, which hid 715 inherited fields across 31 classes and required four of them while inventing one), eight intermediate ``_*Base`` classes that existed only because ``create_model`` cannot carry a method body, the dead ``_canonical_enum`` helper, and two ``status`` redeclarations that restated what the generated arm already pins. Deleting the stub exposes the module to mypy for the first time; the errors it had been hiding are fixed rather than suppressed, and three assertions whose subject was the stub are corrected to the property that now carries the contract. BREAKING CHANGE: every public canonical creative model is now a subclass of the generated class it refines rather than a runtime copy of its fields. isinstance(x, GeneratedClass) and issubclass(CanonicalClass, GeneratedClass) are now True where they were False, so code using the negative to tell the two layers apart needs a different test. src/adcp/types/canonical_creative.pyi is deleted and the real source is what mypy reads, so previously hidden required fields are enforced at type-check time — ListCreativesResponse requires query_summary and pagination, and a construction that type-checked against the stub may now fail. Validators the generation pipeline injects into a generated class now reach the public canonical model by inheritance, so a public model refuses documents it previously accepted: CreativeManifest requires one of format_id or format_kind.
`ProtocolEnvelope` is on `adcp.types.protocol`; `AdcpVersionEnvelope` was not, so a consumer assembling the pair had to take one from the curated surface and the other from the flat namespace. `adcp.types` already binds the name. This adds it to `adcp.types.protocol` as well — the `__all__` entry and the `TYPE_CHECKING` mirror, matching how `ProtocolEnvelope` is bound there — and grades what the pair is for. A consumer refusing a hand-written model that merely looks like a spec type needs both descent predicates, not either one: ```python issubclass(model, AdcpRequest) and issubclass(model, AdcpVersionEnvelope) ``` `AdcpRequest` is field-less, so inheriting it grants nothing and a class can declare it as a base and invent whatever fields it likes. `AdcpVersionEnvelope` refuses that, because inheriting it is what supplies the two spec fields — and it accepts a model that composes the envelope without being any task's message. The conjunction refuses all three constructed forgeries; either predicate alone refuses two. `test_the_version_envelope_has_a_public_name` asserts object identity between the public name and the generated class — a copy would make the conjunction refuse every real request — and then evaluates the conjunction through the public spelling only.
ARCH-ONE-SURFACE §3's rule, as a module that answers one question — what is this
arm called — and does nothing else. Step zero classifies: an arm that introduces
no property the base schema already declares is a VALIDATION arm and mints no
class, because a public class for it would name something no buyer sends. Then
the rungs, first one that holds: title, discriminator const, FULL required set,
fail closed.
Two findings the rule produced on its own, neither read off the bundle:
* uniqueness is a property OF each rung, not a check bolted onto rung 3.
``core/audience-selector.json`` has three arms all pinning ``type: "signal"``
(binary, categorical, numeric), so the const rung names them identically and
must fall through. The fail-closed collision refusal found it.
* the classifier's ORDER matters. ``create-media-buy-request.json`` anyOf[2]
declares only ``required`` and ``not`` — no ``type``, no ``properties`` — so a
"nothing structural, therefore a scalar" fallback claims the purest validation
arm in the tree. A scalar is recognized by what it positively declares.
Measured over ``schemas/cache/3.2``, the bundle generation reads
(``resolve_bundle_key("3.2.1")`` -> ``3.2``), excluding ``bundled/`` and
``mcp/``: 557 root arms = 278 named (169 title, 82 const, 27 required-set) + 278
minting no class (96 ref, 10 scalar, 172 validation) + 1 refused. The one refusal
is ``content-standards/get-content-standards-response.json`` oneOf[0], exactly
the single arm the design predicts, and the one upstream title the rule costs.
The worked examples reproduce exactly: ``CreateMediaBuySuccess/Error/Submitted``,
``SignalRefProduct/DataProvider/SignalSource``, and all five distinct
``a2ui/bound-value.json`` names the set-difference would collapse.
Counts are pinned by equality, the way the version-envelope residue is: a schema
bump that moves one fails and is read. Nothing is permitted by appearing in them.
…today The exact residue the surface move has to decide about. Of the 58 arm classes `restore_response_variant_aliases` numbers by position across 22 response modules, the rule names 56. Two it does not, for two different reasons: `get-content-standards-response.json` oneOf[0] is rung 4 and needs the one upstream title, and `get-creative-features-response.json` oneOf[0] is a bare $ref whose arm already names the shared class it selects — the emitter minting a new class for it is the defect, not the missing name.
The one interaction between the two halves of the subclass conversion. `list-creatives-response.json` carries an injected `_validate_format_reference_xor` reading `self.format_id`; inheritance is the point of this layer, so it now runs on the canonical model, and with the field merely deleted it raised AttributeError instead of validating. Binding each removed name to None after class creation is the removal's own truth — pydantic never sees a field candidate, model_fields and the JSON schema and the wire are untouched — and the XOR reduces to exactly the invariant the canonical model should hold: format_kind must be set. Measured scope: one such validator across the 31 canonical models, found by `tests/test_manifest_response_readback.py::test_direct_listed_creative_kind_stays_strict`, which the subclass conversion had broken. Both directions of the reduced invariant are now graded.
The task-message marker pass resolves a union root through its type alias and walks the alias expression for the arm names. A composing root is emitted as `Annotated[A | B, Field(...)]`, so that walk collected `Annotated` and the `Field` call beside it as if they were arms, and the pass failed closed on the first one: RuntimeError: media-buy/list-products-response.json: union arm 'Annotated' of ListProductsResponse is not defined in list_products_response.py 91 generated aliases have that shape. _alias_arms_expr now strips the metadata to the first subscript element, which is what Annotated[T, ...] means, before the walk. A plain class root, a bare union alias and a nested alias are all unchanged, and an alias naming a class the module does not define still fails closed -- the fail-closed behavior is the point of the check and is kept. insert_task_message_markers now marks 203 task-message classes across 154 modules, and is recorded as firing in post_generation_manifest.json.
… base Phase 2 made every canonical model a real subclass of the generated wire model it refines. _DeliveryCreativeManifest's own docstring says it is "deliberately not a subtype of the strict input", and after the conversion it is exactly that subtype -- the code contradicts its stated intent. The consequence is the weakening adcontextprotocol#1241 exists to prevent. Pydantic's default revalidate_instances="never" skips validation for an instance of any subclass, so WireCreativeManifest.model_validate(delivery_manifest) -> ACCEPTED handed back a manifest carrying a format_kind the pinned enum does not know, and the tolerance reached creative input. Measured directly: the instance path was accepted while the dump path was still rejected, which is why only the instance parametrizations failed. The strict base now revalidates SUBCLASS instances only. An exact-class instance still passes through untouched, so composing a model into a field keeps its identity (test_composability_invariant). The base is read off __bases__ rather than named, so it follows the declaration. 450 tests across the canonical, manifest, composability and subclass modules pass with it, including the composability grader. Phase 2 never ran tests/test_delivery_manifest_readback.py -- IMPL-CLONES section 7's three batches do not list it -- so this failed on that branch too, unmeasured. Also corrected to the new truth, each with the reason at the site: - the two 'assert not isinstance(x, input_model)' checks, false by construction now that the canonical models are subclasses; the pytest.raises calls carry the obligation and are what grade the bypass above - three fixtures that omitted a required field group adcontextprotocol#1368 now enforces: a CreativeManifest with neither format_id nor format_kind (core/creative-manifest.json root oneOf), a Placement with neither name nor publisher_domain, and a CreateMediaBuyRequest with no package group The four module-level TypeAdapter constructions adcontextprotocol#1353 and adcontextprotocol#1375 added are now @deferred_adapter, the repo's existing primitive for this. tests/test_freezegun_models.py forbids building a schema at import time and landed on main after both branches forked, so neither ever saw it. base.py and _get_products_helpers.py now reach generated types by their public adcp.types.domains path instead of adcp.types.generated_poc, which is what adcontextprotocol#1367's schema-mirroring tree is for. No allowlist grew: test_import_layering is keyed by file and both files were absent from it, so phase 1, phase 2 and the adcontextprotocol#1371 merge each failed that ratchet before this.
`tests/test_canonical_validator_parity.py` held one obligation — a document the
generated class refuses, the public canonical name refuses too — over a mechanism that
no longer exists. The canonical models were built with `create_model` over a copy of
the source's fields, so the generated class was not in the MRO and every validator had
to be re-decorated onto the copy, with any validator touching a dropped field skipped
and recorded in `__adcp_canonical_validators_left_behind__`. The models are real
subclasses now, so inheritance carries them and there is no carrying step, no skip list
and no bookkeeping ClassVars to read.
Every obligation is re-pointed at the new mechanism rather than dropped, and two are
stronger for it:
- `test_no_generated_validator_is_skipped` admits no exceptions, where the old test
asserted only that a skip was justified by a dropped field. Nothing is skipped: the
removal rule binds each removed name to `None` after class creation, so a rule
written against `format_id` reduces instead of raising `AttributeError`.
- `test_canonical_manifest_enforces_its_required_group_through_format_kind` asserts a
refusal where the clone accepted the document. `core/creative-manifest.json` declares
a `format_id | format_kind` root group; the clone skipped that validator because the
canonical boundary has no `format_id`, so `{"assets": {}}` validated. The public name
now refuses it and accepts it through `format_kind`.
The two separate model-validator and field-validator tests are one test over both
decorator kinds. Field validators are kept in scope although no generated ancestor
declares one today, because an assertion covering only the kind that currently exists
stops grading the moment a post-generation fix emits the other. The population is the
54 refinements in `canonical_creative`, which is broader than the 31 publicly bound
models `tests/test_canonical_models_are_subclasses.py` parametrizes over: a validator
dropped on a private `_Legacy*` refinement is just as silent.
Verified by removing the generated ancestor from `CreativeManifest` on a scratch edit:
the manifest test and the anti-vacuity floor both go red, naming the class.
`tests/test_task_message_lattice.py` builds a `ValidateInputRequest` whose manifest now
carries a `format_kind`, because `enforce_root_required_groups` enforces that group on
the generated class. The accessors under test are indifferent to which arm satisfies it.
`_get_products_helpers.py` imported two field enums out of `adcp.types.generated_poc`. Class names inside that tree renumber when the schema bundle moves, so an import that resolves today can name a different class after the next regeneration, and `tests/test_import_layering.py` forbids the import for exactly that reason. Both now take `adcp.types.domains.media_buy.*`, the mirror of the module that declares them. The mirror is a re-export, so each name is the same class object, and it is keyed on the schema path rather than on generator output order. No allowlist entry was added. `ALLOWED_FILES` in the layering test names facade and re-export layers, which this module is not.
The type layer has no architecture documentation. `docs/design/` holds one file, about a FastMCP registration investigation. `docs/extending-types.md` covers how a consumer subclasses a generated type — which base to pick, which variant alias to import, `Field(exclude=True)`, `@model_serializer` — and says nothing about what the surface guarantees or how it is produced. Nothing describes the generation pipeline at all: outside `CHANGELOG.md` and one unrelated proposal, `generate_types`, `post_generate_fixes` and `consolidate_exports` are named in no document in the repository, and `make regenerate-schemas` appears only in `schemas/patches/README.md`. Two documents, split by audience. `docs/type-surface.md` is a consumer's map: where to import from and when each spelling applies, what is deliberately not public and what the layering guard enforces, what `AdcpRequest` and `AdcpResponse` guarantee and what can be done with one before the tool is known, how envelope fields are told from payload fields, where the envelope ancestry is incomplete and why those residues are upstream's, and how response variations are named and dispatched. It defers to `extending-types.md` for subclassing and variant selection rather than restating either. `docs/design/type-generation.md` is for whoever changes the pipeline: every step in order with what it consumes and produces, which step touches the network, the ordering constraints inside the post-generation pass and why a fix must locate by structure rather than by spelling, the two derived guards that keep the export surface derived, how a class gets its name, the guards, how to add a schema feature, and what to do when the generator renders something wrong. It records the generator pin and the reason for it, which was documented nowhere: 0.72.0 and later cannot generate this tree, and 0.71.0 is rejected on measured non-equivalence, so there is no newer-generator lever for any future defect. Both state what the architecture refuses and why, because a reader who does not know what was rejected will propose it again. Two things the documents are careful about, since both are easy to get wrong when reading the tree rather than the history. The schema-derived union-arm naming rule is described as what it is — present, graded, and not wired — with the positional emitter that actually ships named beside it. And the staleness check is `generate_types.py --check`, which works today but which `make validate-generated` does not run, so the document says to run it yourself rather than implying the gate is automatic. `extending-types.md` said `AdcpVersionEnvelope` is not exported from `adcp.types`. It is, and it is now on the curated `adcp.types.protocol` surface too; the note is corrected and points at the new document for the message markers.
datamodel-code-generator wrote its models to `adcp.types.generated_poc`, a private tree, and `consolidate_exports.py` then emitted 1,059 one-line re-export modules under `adcp.types.domains` pointing at it. Two trees of ~1,086 files each, a copying step between them, and the private one was still the definition site — all of the cost of a public domain layout and none of the benefit. `OUTPUT_DIR` is now `src/adcp/types/domains`. The generator writes the public tree, so the module that DEFINES a class is the module an adopter imports it from. There is nothing to copy and nothing that can drift. What consolidation still writes are the namespaces DERIVED from that tree — `_generated.py`, each `<domain>/__init__.py`, the `DOMAINS` roster and `error_details.py` — never a second copy of a module. `generate_module_mirror` is deleted, and with it `restore_generation_dates` and the deferred-format parameter that existed only to make a thousand-file batch write date-stable. Four domains whose single schema sits at the bundle root generate as `domains/<domain>.py`, so that module already IS the domain root and gets no written root; a new post-generation pass gives each of the four an `__all__`, because a domain root is a public namespace and `export_resolution` reads `__all__` to record what one promises. The staging flow changes shape to match: the package copy is built before generation rather than after it, dmcg generates into `types/domains` inside that copy, consolidation writes the roots into the same directory, and the tree installs as one artifact instead of two. The models are untouched. Against the previous tree, exactly one non-`__init__` file differs — `enums/format_category.py`, the removed-type shim, whose message names the new path — and no module is missing. The domain roots bind the same names they bound before, to the same objects: `tests/fixtures/public_api_snapshot.json` is byte-identical, which it can be because `export_resolution` strips the generated prefix before keying a name, so `creative.list_creatives_response:Creative` is the key either way. `adcp.types.generated_poc` becomes a one-file tombstone that raises `ImportError` naming the prefix rename. Every module stem moved unchanged, so the migration is `adcp.types.generated_poc.` -> `adcp.types.domains.`; issue adcontextprotocol#1360 measured one production seller carrying 110 such imports, and fifteen lines turns a bare `ModuleNotFoundError` into that sentence. A working shim was refused: redirecting `__path__` would put a second module object in `sys.modules` for every module, which is two class objects per class and the exact identity break `test_export_surface_is_derived.py` exists to catch. `tests/test_import_layering.py` shrinks, which is the clearest measure of the change. The forbidden-prefix tuple goes from two entries to one — only `adcp.types._generated` resolves a name by something other than the schema — and the frozen violation baseline goes from eight files to one, because the eight were reaching into a private tree that no longer exists. A new assertion keeps the shrink deliberate: re-adding `adcp.types.domains` to the tuple would forbid the definition site, and now fails by name. `test_collision_guard.py`'s mirror-coverage test counted exports out of the generated tree and compared them to exports out of the same tree — two reads of one directory, so it passed whatever the public layout was. It is replaced by two that ask the runtime: every declared `(module, name)` pair must import from `adcp.types.domains.<module>`, and a real class must be DEFINED there. Adding a module-level re-export of a sibling class fails the second clause by name, which is the shape a mirror tree has. Two entries join `scripts/post_generation_manifest.json`, because adcontextprotocol#1374 landed adcontextprotocol#1374's effect manifest on main after both passes were written and it fails the build on a fix it does not know. `declare_root_schema_module_exports` is the pass this commit adds. `insert_task_message_markers` is this PR's own, from the lattice commit, which predates the manifest. Both fire, so both are declared `true`; `--update-fix-manifest` reclassified nothing else. One line inside the v3 codemod changes for a reason worth stating. `_replacement_is_identical` imports the module a flagged adopter import names, to check the proposed public replacement is the same object. The spelling the codemod DETECTS stays the v3 one; the module it IMPORTS is where those classes live now. The import sits inside `except (AttributeError, ImportError)`, so leaving it would have swallowed `ModuleNotFoundError` and silently degraded every per-symbol auto-rewrite to a flag. BREAKING CHANGE: adcp.types.generated_poc is removed. The generated models are defined at adcp.types.domains.<domain>.<schema>, with every module stem unchanged, so the migration is a prefix rename: adcp.types.generated_poc. -> adcp.types.domains. . Importing the old path raises ImportError carrying that instruction rather than ModuleNotFoundError, and the module is removed in v10. No public name changes what it resolves to: tests/fixtures/public_api_snapshot.json is byte-identical. adcp.types.domains is no longer an internal layer either - it is the definition site and a supported import for a name the flat adcp.types namespace cannot bind.
Every document that named `adcp.types.generated_poc` either described a path that no longer exists or, after the prefix rename, said `adcp.types.domains` is internal and must never be imported. That second reading is now false: the generator defines every public class there. The correction is the same everywhere, and it separates two things these documents had conflated. `adcp.types._generated` is internal because it binds a bare name several schemas declare to one sort-order winner, so a schema addition can repoint a name an adopter already imports. A domain path cannot move that way — it names the declaring schema. What a domain path does not protect you from is a NUMBERED class name: codegen numbers anonymous variant classes by traversal order, so `Assets162` can name a different shape after the next regeneration whatever path you reach it by, and `aliases.py` is what gives those a stable spelling. "Private module" and "unstable name" were one warning; they are two, and only the second applies to a domain path. `docs/type-surface.md` gains the statement the layout now supports: one tree, the definition site IS the public address, and the assertion to go with it (`Product.__module__ == "adcp.types.domains.core.product"`). `docs/design/type-generation.md` records the output path, the reordered staging steps, and why four domains have no written root. `CHANGELOG.md` and the five `MIGRATION_*.md` files are deliberately untouched: they describe releases in which that path existed and was private, which is still true of those releases.
KonstantinMirin
changed the title
19809d2 to
724751c
Compare
|
On the automated notice above: this PR touches
This PR makes the generator write the public domain tree directly, so The update is not a rename. Two things that make the diff easier to read:
|
|
Thanks, this is the right direction. One inheritance mechanism instead of a clone plus a validator re-decorator removes the class of bug behind A few things before merge, roughly in priority order:
Process note: the |
…checkers `CanonicalBoundaryModel.__pydantic_init_subclass__` removes the inherited legacy-identity fields through pydantic internals — deleting from `model_fields`, popping `__annotations__`, binding `None`, forcing a rebuild. That works on 2.13 and is not a supported API, so a minor pydantic release could change it silently. `tests/test_canonical_removed_fields.py` makes that loud. For every canonical model that inherits a legacy-identity field — 11 models, 15 declarations, derived from the generated ancestors and `is_legacy_creative_identity_key` rather than from a hand-written table — it asserts the name is absent from `model_fields`, from the JSON schema at every depth, and from `model_dump` / `model_dump_json` of an instance that really carries it, and that `model_validate` refuses a document holding it, for the boundary's stated reason. The same removal left a static disagreement: the annotation is inherited, so both mypy and pyright accepted `CreativeManifest(format_id=<correctly typed>)` and the runtime then raised. Each removal is now redeclared in a `TYPE_CHECKING` block with `Field(default=None, init=False)`, which both checkers read through pydantic's `dataclass_transform`, so the keyword is unavailable statically too. No suppression: four other spellings were measured and each needed one. The runtime surface of all 31 canonical models is byte-identical before and after. A guard test reads the module's AST and pins that every derived removal has such a redeclaration, with `init=False`, annotated exactly as its generated parent — and a second runs both checkers over a generated snippet, with the wire model as the control, so a refusal for the wrong reason cannot pass. `docs/types-9-migration.md` described a stub this branch deletes and a `_canonical_clone` helper it removes. Both paragraphs now describe the subclass model, with the adopter consequence our trial merge found: the runtime still coerces `pacing="even"`, the checkers now want the enum member or `model_validate`. Not a regression — main's stub did not declare the field at all, so the same call was refused there too.
|
Thanks — this is a useful review and five of the six are addressed. One factual correction first, because it may be why you were holding off. The conflict is already gone. The head was force-pushed to GitHub reports 2 — the field strip now fails loudly. Two choices there were deliberate, to avoid a pass that means nothing. The dump test does not dump an empty instance: it Disabling the strip turns 30 of those red. Restoring one field turns 3 red. 3 — you were right, and hiding works on both checkers with no suppression. Measured before changing anything: Five spellings were tried. if TYPE_CHECKING:
format_id: FormatReferenceStructuredObject | None = Field(default=None, init=False)
The drift risk that introduces is graded: a test reads the source AST and asserts, per derived pair, that the 4 — filed as #1397, with both candidate directions written out: non-subtyping with a shared ancestor, versus keeping the subtype and making 5 — your construction works and the hard Worth recording why that needed its own test: I built the rejected second-load design on purpose expecting 6 — corrected, and there was a second one. Line 126's stub bullet now says the stub is gone rather than regenerated. Line 104 still said On splitting: the unused naming rule is a fair objection and extracting |
… moved to The 9.0 tombstone raised `ImportError` for every `adcp.types.generated_poc.*` import. adcontextprotocol#1360 measured 110 of them in one production seller, so that breaks all of them at once, on upgrade, with no migration window. A finder at the front of `sys.meta_path` now serves each deprecated name from its canonical address and warns. The loader executes nothing: it imports `adcp.types.domains.<same stem>` and hands that object back, so `sys.modules` holds ONE module object under both keys and one class object per class. Redirecting a package's `__path__` instead would make the machinery load each module a second time under the old name, and an `isinstance` against the wrong copy fails for no visible reason. The machinery's only write to a module returned from `create_module` is `__spec__` — `_init_module_attrs` sets `__name__`, `__loader__`, `__package__`, `__path__` and `__file__` only when absent — and `exec_module` puts the canonical spec back, so the shared object carries no trace of the old name. `tests/test_generated_poc_alias.py` grades that over the root, all 26 domains and a derived sample of their schema modules: one object under both keys, every public name identical by `is`, no deprecated string on `__name__` / `__spec__` / `__package__` / `__file__`, the warning once per module naming the new path, and the finder declining every name outside the deprecated root. Reverting the loader to a second load turns 203 of them red. A real import is graded in subprocesses, where `sys.modules` is empty and the warning has not been consumed. `importlib.abc` is imported under `TYPE_CHECKING` only: it costs 9ms, on the import path whose cost `adcp.types`' lazy `__getattr__` exists to keep down. The alias module costs 189us. The deprecation has an expiry rather than becoming a permanent second surface: v10 removes it, stated in the warning, in the migration guide and in `REMOVED_IN_MAJOR`, and a test fails once either the declared or the installed major reaches it. BREAKING CHANGE: `adcp.types.generated_poc.*` is a deprecated alias of `adcp.types.domains.*` rather than the 9.0 tombstone that raised ImportError. Every import under it emits a DeprecationWarning naming its canonical path, and v10 removes the alias. Move to `adcp.types.domains.<same stem>` — the module stems are unchanged, so the migration is a prefix rename.
|
Issue #1402 reports a bug in the tombstone's per-module redirect table that's worth fixing before merge. The tombstone maps The narrowest fix is to add per-name overrides in the tombstone entry for the (Flagged by triage from issue #1402.) Generated by Claude Code |
|
Received the fold request from @bokelley. The PR branch is on a fork outside my write scope, so I can't push directly — here are the exact changes needed across the three files.
|
|
Fix for the Root cause confirmed. Narrowest fix: a Broader fix included: Tested: Apply this diff on top of the current head: diff --git a/src/adcp/types/_generated_poc_alias.py b/src/adcp/types/_generated_poc_alias.py
index f6e0906b..6f25b137 100644
--- a/src/adcp/types/_generated_poc_alias.py
+++ b/src/adcp/types/_generated_poc_alias.py
@@ -76,6 +76,31 @@ CANONICAL_ROOT = "adcp.types.domains"
#: deprecation with no expiry is a permanent second surface.
REMOVED_IN_MAJOR = 10
+#: Per-module name overrides for names that moved to a *different* canonical
+#: module when ``generated_poc`` was reorganised (e.g. a flat file split into
+#: a package + sibling module).
+#:
+#: Format: deprecated_full_name -> {attr_name -> canonical_source_module}
+#:
+#: Only names **absent** from the default canonical target are listed — a name
+#: already present in ``domains.<stem>`` is served by the normal redirect and
+#: never needs an entry here.
+_NAME_OVERRIDES: dict[str, dict[str, str]] = {
+ # ``generated_poc/brand_discovery.py`` was a flat sibling of the
+ # ``brand/`` package. When the tree moved to ``domains/``, Brand,
+ # BrandDiscovery3, and LocalizedName stayed in ``domains/brand_discovery.py``
+ # rather than landing in ``domains/brand/__init__.py``. Adopters whose
+ # code pre-dates that split wrote
+ # ``from adcp.types.generated_poc.brand import Brand``
+ # and the simple prefix rename redirects them to ``domains.brand``, which
+ # does not carry those names. This entry injects them so the import works.
+ "adcp.types.generated_poc.brand": {
+ "Brand": "adcp.types.domains.brand_discovery",
+ "BrandDiscovery3": "adcp.types.domains.brand_discovery",
+ "LocalizedName": "adcp.types.domains.brand_discovery",
+ },
+}
+
def canonical_name(deprecated: str) -> str:
"""``adcp.types.generated_poc.core.x`` -> ``adcp.types.domains.core.x``."""
@@ -86,8 +111,15 @@ def canonical_name(deprecated: str) -> str:
class _AliasLoader:
"""Hands back the canonical module object instead of loading anything."""
- def __init__(self, canonical: str) -> None:
+ def __init__(
+ self,
+ canonical: str,
+ name_overrides: dict[str, str] | None = None,
+ ) -> None:
self._canonical = canonical
+ # {attr_name -> canonical_source_module} for names that moved to a
+ # different module than the simple prefix rename would produce.
+ self._name_overrides = name_overrides
self._canonical_spec: ModuleSpec | None = None
def create_module(self, spec: ModuleSpec) -> ModuleType:
@@ -98,6 +130,21 @@ class _AliasLoader:
def exec_module(self, module: ModuleType) -> None:
module.__spec__ = self._canonical_spec
+ if self._name_overrides:
+ # Inject names that moved to a sibling canonical module when the
+ # deprecated module was split. We only add names that are absent —
+ # canonical names always win — so the canonical module is never
+ # corrupted and the invariant (one object under both keys) holds.
+ for attr_name, source_module_name in self._name_overrides.items():
+ if hasattr(module, attr_name):
+ continue
+ try:
+ src = importlib.import_module(source_module_name)
+ value = getattr(src, attr_name, None)
+ if value is not None:
+ setattr(module, attr_name, value)
+ except ImportError:
+ pass
class _AliasFinder:
@@ -151,7 +198,8 @@ class _AliasFinder:
# both. typeshed's structural alternative, ``LoaderProtocol``, declares
# only ``load_module`` — removed from ``Loader`` in 3.12 — so
# implementing it would be the less honest of the two options.
- return ModuleSpec(fullname, cast("Loader", _AliasLoader(canonical)))
+ overrides = _NAME_OVERRIDES.get(fullname)
+ return ModuleSpec(fullname, cast("Loader", _AliasLoader(canonical, overrides)))
def install() -> None:
diff --git a/tests/test_generated_poc_alias.py b/tests/test_generated_poc_alias.py
index 3d52bfc5..f000693c 100644
--- a/tests/test_generated_poc_alias.py
+++ b/tests/test_generated_poc_alias.py
@@ -44,6 +44,7 @@ import pytest
import adcp.types # noqa: F401 — importing it is what installs the finder
import adcp.types.domains as domains
from adcp.types._generated_poc_alias import (
+ _NAME_OVERRIDES,
CANONICAL_ROOT,
DEPRECATED_ROOT,
REMOVED_IN_MAJOR,
@@ -194,6 +195,40 @@ def test_every_spelling_an_adopter_may_have_written_resolves() -> None:
assert adcp.types.generated_poc.core.format_id is new_format_id
+def test_name_overrides_inject_split_names_into_the_deprecated_module() -> None:
+ """Per-name overrides surface names that moved to a sibling module at a split.
+
+ ``generated_poc.brand`` redirects to ``domains.brand`` (the package with
+ the brand tools), but ``Brand``, ``BrandDiscovery3`` and ``LocalizedName``
+ live in ``domains.brand_discovery`` — they came from the flat
+ ``generated_poc/brand_discovery.py`` file, not from the ``brand/`` package.
+ Without per-name overrides, ``from adcp.types.generated_poc.brand import Brand``
+ resolves the module successfully and then raises
+ ``ImportError: cannot import name 'Brand' from 'adcp.types.domains.brand'``.
+
+ This test covers every entry in ``_NAME_OVERRIDES`` so that a future split
+ that adds an entry is automatically graded. Regression for issue #1402.
+ """
+ for deprecated_module_name, name_map in _NAME_OVERRIDES.items():
+ with warnings.catch_warnings():
+ warnings.simplefilter("ignore", DeprecationWarning)
+ deprecated_mod = importlib.import_module(deprecated_module_name)
+
+ for attr_name, canonical_source_name in name_map.items():
+ canonical_src = importlib.import_module(canonical_source_name)
+
+ assert hasattr(deprecated_mod, attr_name), (
+ f"{deprecated_module_name} is missing {attr_name!r} after tombstone "
+ f"injection — the per-name override for the brand/brand_discovery "
+ f"split is not working"
+ )
+ assert getattr(deprecated_mod, attr_name) is getattr(canonical_src, attr_name), (
+ f"{deprecated_module_name}.{attr_name} is not the same object as "
+ f"{canonical_source_name}.{attr_name} — the injected name is a copy, "
+ f"not the canonical class, so isinstance will fail"
+ )
+
+
# ---------------------------------------------------------------------------
# Property 4 — the shared object carries no trace of the old name
# ---------------------------------------------------------------------------Closes #1402. Generated by Claude Code |
The pre-9.0 alias assumed every module stem moved unchanged, so a deprecated name resolved to one canonical module. One case has two: a root discovery schema, whose basename collides with a task-schema directory. brand.json used to generate into generated_poc/brand/__init__.py -- 140 classes including Brand -- and now generates into domains/brand_discovery.py, leaving domains/brand/__init__.py to the generated domain aggregator. So generated_poc.brand resolved and `from ... import Brand` raised an ImportError naming a module the adopter never wrote, which is worse than no alias at all. The second half is derived from the generator's own ROOT_DISCOVERY_SCHEMAS naming rather than listed here: discovery_half appends the _discovery suffix to a domain-level package and asks find_spec. A schema added to that dict is served without touching this module, and a test fails the build if the generator ever names one some other way. _SplitAliasModule serves one deprecated name over both halves, discovery first because that is the surface the pre-9.0 module had, and the two halves share 12 names. It defines no class, so every class reached through it is still the canonical class -- which is the property the one-module-object rule was protecting, and the only one isinstance reads. It borrows the aggregator's __path__ because _find_and_load reads the parent's before any meta-path finder runs. The alias docstring asserted the prefix rename and the single module object as unconditional facts. Both are now stated with the case that breaks them. The suite could not see this: it asserted over the names the CANONICAL module binds, never over the names the PRE-9.0 module exported. The new split family asserts the second direction over every public name of both halves, and takes its split set from ROOT_DISCOVERY_SCHEMAS so the next split is graded with no edit there. Three existing per-pair tests are parameterized over the unsplit pairs, derived by subtracting that same set. Closes adcontextprotocol#1402
…nse arm Two defects with one cause: the Legacy* set was curated by hand, so what it named drifted from what the generator emits. LegacyFormatId was a SUBCLASS of the generated FormatReferenceStructuredObject, and no public name on adcp.types resolved to the generated class at all. The subclass was written when agent_url was an AnyUrl that normalized the wire bytes; adcontextprotocol#1384 moved it to WireUrl -- StrictStr plus the absolute-URL validator plus the format: uri schema -- which is that subclass's entire purpose, and left every redeclaration a weakening of the parent it was written to narrow. Measured: the generated class accepts width: 300.0 through SchemaInt's JSON-schema-integer coercion and the public name for it refused the same payload, so the flat surface and the type the SDK's own model fields declare disagreed about one wire document. The parent already carries extra=allow, the same id pattern and pixel_ratio; its own _validate_agent_url duplicated _require_absolute_url and its model_dump override was a bare super call. So the fix is a deletion: the Legacy name is the generated class, as every other name in that module already is. The arm names had four holes: LegacyUpdateMediaBuy Success and Error shipped while Submitted and all three LegacyCreateMediaBuy arms did not, so a seller holding an arm read off the wire could only name it through adcp.types.domains.media_buy -- the private tree this release retires, because the bare public name is the canonical SUBCLASS and isinstance against it is False for that instance. Adding four names would have fixed four instances: the generator had already opened a fifth by adding a fourth preview_creative arm nobody named, and the derived test found it. The required set is now computed from the generated unions -- the arms come from typing.get_args of the union each module declares, so a schema that gains an arm changes the obligation with no edit -- and the surface is refused if it does not satisfy it. It is keyed on whether a bare public name reaches the generated arm, which is why sync_creatives needs no Legacy arm names: there, the generated class already IS the public one. The Legacy-name test asserts a round trip, not __module__: the class must be the attribute the module it claims actually binds under its own qualname. A probe proved why. __module__ alone is one line to fake, and it is a weak witness here because module_from_spec stamps the canonical spec onto a duplicate too; a shadows-a-same-named-base rule alone misses the shape this repo shipped, since the clone had a different class name. test_root_surface_is_canonical_and_legacy_is_explicit pinned the defect by name, asserting LegacyFormatId.__name__. Restated to the new obligation rather than deleted. Closes adcontextprotocol#1398 Closes adcontextprotocol#1399
…ads it The migration guide stated the prefix rename unconditionally and promised the same module object for every deprecated name. Both are true of 1,087 of the 1,088 stems and false of brand, which is the one an adopter is most likely to have imported: the old generated_poc.brand WAS brand.json, and the new domains.brand is the generated domain aggregator, with brand.json's classes at domains.brand_discovery. A guide that tells an adopter a rename is mechanical when one case is not is how the ImportError in adcontextprotocol#1402 reaches them a second time, after the alias stops serving it in v10. States the split, the import to write, and the one property that deprecated name cannot keep -- module identity -- alongside the one it does, which is class identity and the only one isinstance reads.
The DeprecationWarning an adopter reads on importing generated_poc.brand said "import adcp.types.domains.brand instead; the module stems are unchanged, so this is a prefix rename". For that one name the advice sends them to a module that does not bind what they imported -- the same wrong answer the ImportError gave, restated as guidance, and guidance outlives the alias: it is what they act on in v10 when the alias stops serving them. A split name's warning now names both halves and says which is which, derived from discovery_half rather than branched on a stem. Every other name keeps the prefix-rename text verbatim. The codemod's own resolution helper restated the same claim. It fails safe today -- an unresolvable symbol takes the AttributeError branch and is flagged rather than rewritten, and neither symbol map holds a brand.json class -- so this corrects the comment and records where the fix belongs if one is ever added, which is the discovery half and not a wider except clause.
…otocol#1409 scripts/union_arm_names.py and its tests renamed nothing in this PR: the rule is not wired into the pipeline, and arms are still named positionally by restore_response_variant_aliases. Per review, it lands later together with the renames that use it. The code is preserved on claude/issue-1409-union-arm-naming (this PR's head before this commit), and adcontextprotocol#1409 records the bundle measurements and the two surface-policy decisions wiring it needs. The design doc keeps the description of the positional emitter that ships and points at adcontextprotocol#1409 for the rule. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@KonstantinMirin, agreed: we'll keep this as one PR. Thanks for the detailed reasoning. Your split argument holds: the subclass change is what lets the validators reach the public models, so splitting it out would land the move without the fix. To save you a round, I pushed the one extraction you offered as
Checked locally on
Fold candidates: #1398, #1399 and #1402 are closed by your last push. On #1401 I agree with you: it stays out of this PR and Brian decides it separately. Generated by Claude Code |
|
Thanks for turning all of that around so fast, and for folding in #1398, #1399 and #1402. Decisions:
We're doing a verification pass on the current head now: public export and identity diff against main, |
…checker (#1419) * fix(types): declare the canonical list overrides precisely for every checker Since #1392 made the canonical models real subclasses, ten overriding list fields in `canonical_creative` were annotated `SchemaVariant[...]`. That marker is for adopter overrides and only `adcp.types.mypy_plugin` understands it: with the plugin the fields were `Any`, so `GetProductsResponse.products` lost its `list[Product] | None`; without it, and under pyright and Pylance, it was not a valid type at all, so `for p in response.products` was an error. CI missed it because the repository enables the plugin and did not run pyright. The fields now carry their precise `list[...]` types with a per-line `type: ignore[assignment]` for the intentional invariant override. Both mypy and pyright honour that comment, so the suppression is one line and the read-side type is the real one under either checker, with or without the plugin. `ListCreativesResponse.creatives` needs no suppression at all: the generated field is a covariant `Sequence`. `UpdateMediaBuyRequest.new_packages` regains the generated `min_length=1` the retyped redeclaration had dropped. A strict adopter fixture iterates each of the ten fields and reads an element attribute; `tests/test_canonical_override_field_types.py` grades it with mypy (plugin on and off) and pyright, derives the override set from the runtime models so a new retyped list cannot go uncovered, and refuses `SchemaVariant` in the module. `scripts/run_pyright_type_checks.py` runs pyright over the adopter fixtures as `make typecheck-all` and the CI type-check job now do; eleven fixtures pyright rejects for unrelated, pre-existing reasons are listed there with the diagnostic each one hits, and the list may only shrink. Closes #1416 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(types): pass str as the parametrize id function directly Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ci): skip pyright fixtures whose optional dependency is not installed CI's type-check lane installs the dev extra only, so the fixtures that import psycopg or psycopg_pool failed the new pyright gate with reportMissingImports. mypy tolerates exactly those modules through ignore_missing_imports; pyright has no per-module equivalent, so the runner now skips a fixture whose top-level imports the interpreter cannot find and names it in the output. With the [pg] extra installed, as locally, every fixture is graded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ci): install the pg extra for the pyright gate instead of skipping fixtures Skipping a fixture whose optional dependency is absent was broader than the mypy tolerance it cited: ignore_missing_imports still grades the rest of the file, while the skip dropped six psycopg fixtures from pyright in the dev-only CI type-check lane, so a pyright-only regression there would ship green. The CI step now installs the pg extra before running pyright, and the runner fails up front — naming the module and the extra — when any selected fixture imports something the interpreter cannot find, so no fixture is ever silently dropped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
The guide explained why the closed enum became an open str and gave the helper, but it described the change from the library's side. The thing a seller has to act on is the transfer: before 9.0 a closed enum refused an unknown format_kind inside the model, so a seller got that refusal without asking; now the SDK accepts any string in both directions and the seller owns the refusal. A seller that adds no check has silently stopped validating something the library used to validate for it, with no error and nothing in a passing suite to say so. Three additions, each a gap rather than a restatement. The responsibility, named, with the check placed where it belongs -- at the point a declaration is built, not where it is serialized. That CanonicalFormatKind is a vocabulary to pass in and not a type to annotate with. Annotating a field with it rebuilds the closed enum one layer down and restores the pin-as-ceiling problem the override exists to remove; measured, the enum annotation refuses a kind newer than the pin while str retains it. That the per-product closed-set rule did not move: a seller must still reject a create_media_buy targeting a format_kind absent from that product's format_options, and validate_format_kind_in_options with FormatKindNotInClosedSetError still enforce it. Those names appeared nowhere in the guide, and the two rules are independent -- video_hosted is canonical and is still refused when a product does not publish it -- so a reader had no way to avoid conflating them with the enum refusal that was removed. The enum is unchanged at sixteen members; the guide already said so and still does. Refs adcontextprotocol#1392
…1415) The guide explained why the closed enum became an open str and gave the helper, but it described the change from the library's side. The thing a seller has to act on is the transfer: before 9.0 a closed enum refused an unknown format_kind inside the model, so a seller got that refusal without asking; now the SDK accepts any string in both directions and the seller owns the refusal. A seller that adds no check has silently stopped validating something the library used to validate for it, with no error and nothing in a passing suite to say so. Three additions, each a gap rather than a restatement. The responsibility, named, with the check placed where it belongs -- at the point a declaration is built, not where it is serialized. That CanonicalFormatKind is a vocabulary to pass in and not a type to annotate with. Annotating a field with it rebuilds the closed enum one layer down and restores the pin-as-ceiling problem the override exists to remove; measured, the enum annotation refuses a kind newer than the pin while str retains it. That the per-product closed-set rule did not move: a seller must still reject a create_media_buy targeting a format_kind absent from that product's format_options, and validate_format_kind_in_options with FormatKindNotInClosedSetError still enforce it. Those names appeared nowhere in the guide, and the two rules are independent -- video_hosted is canonical and is still refused when a product does not publish it -- so a reader had no way to avoid conflating them with the enum refusal that was removed. The enum is unchanged at sixteen members; the guide already said so and still does. Refs #1392
2 participants
datamodel-code-generatorwrote its models toadcp.types.generated_poc, andconsolidate_exports.pythen emitted 1,059 one-line re-export modules underadcp.types.domainspointing at them. Two trees of ~1,086 files each with a copying stepbetween them, and the private one was still the definition site — all of the cost of a
public domain layout and none of the benefit.
OUTPUT_DIRis nowsrc/adcp/types/domains. The generator writes the public tree, so themodule that DEFINES a class is the module an adopter imports it from. One tree, no copying
step, nothing that can drift.
What consolidation still writes are the namespaces DERIVED from that tree —
_generated.py,each
<domain>/__init__.py, theDOMAINSroster,error_details.py— never a second copyof a module.
generate_module_mirroris deleted, and with itrestore_generation_datesandthe deferred-format parameter that existed only to make a thousand-file batch write
date-stable. Four domains whose single schema sits at the bundle root generate as
domains/<domain>.py, so that module already IS the domain root and gets no written root; anew post-generation pass gives each of the four an
__all__, because a domain root is apublic namespace and
export_resolutionreads__all__to record what one promises.The staging flow changes shape to match: the package copy is built before generation rather
than after it, dmcg generates into
types/domainsinside that copy, consolidation writesthe roots into the same directory, and the tree installs as one artifact instead of two.
The models are untouched, and that is measurable. Against the previous
generated_poctree, exactly one non-
__init__file differs —enums/format_category.py, the removed-typeshim, whose message names the new path — and no module is missing:
(Measured against the pre-rebase base. After the rebase the same comparison against
197bcde31addscore/format_id.py, which is #1396'sWireUrlchange arriving — seeWhat landed on main below, where the regenerated file is shown identical to main's.)
The domain roots bind the same names they bound before —
core1436,media_buy375,enums207,protocol174,creative100,error_details49,signals36, none lost,none gained. The line-count diff on those files is black re-wrapping shorter import paths.
The MOVE leaves
tests/fixtures/public_api_snapshot.jsonbyte-identical, across all 29namespaces and 7,833 names: not one public name changed what it resolves to. It can be
byte-identical because
export_resolution.pystrips the generated prefix before keying a name,so
creative.list_creatives_response:Creativeis the key either way — retargeting that oneconstant is what makes the move invisible to every adopter who imports by a public spelling.
The snapshot in this branch is NOT byte-identical, because the surface-hole fixes described
under Three holes the move exposed change seven of its entries: two repoints and five
additions. Those are reviewed as their own commits; the tree move itself moves nothing.
Reading a 2,305-file diff: what is generator output and what is not
2,168 of the 2,305 files are pipeline output and should not be read.
make validate-generatedis the statement that they are exactly what the pipeline produces —since #1374 that target runs
generate_types.py --check, which stages a completeregeneration and compares byte-for-byte. It passes here, with 1088 of 1088 files restored
as timestamp-only.
src/adcp/types/domains/**— the dmcg tree at its new pathsrc/adcp/types/generated_poc/**— deleted_generated.py,_ergonomic.py,error_details.py,SCHEMA_DELTAS.mdscripts/**— the pipelinesrc/**excluding the treetests/**pyproject.toml,.pre-commit-config.yaml,ci.ymltests/fixtures/carries one row for the move, andpublic_api_snapshot.jsonisbyte-identical to main's for it: the seven entries that do change belong to the surface-hole
fixes below.
Split by what the change is, rather than by directory:
So the 1,088-file path move contributes almost the entire line count and 127 small
hand-written files; the canonical-layer and lattice work is 29 files of substance. Of the
hand-written lines across the whole PR, 671 are the mechanical prefix rename — a line
identical after substituting
generated_poc↔domains, counted by normalising both sidesof every hunk, not estimated. The ~6,519 real lines concentrate in 75 files, and these are
the ones worth a reviewer's time:
The files that inflate the line count and cost nothing to review are
aliases.py(142changed lines, 10 real),
client.py(82 / 0) andlegacy.py(45 / 0) — pure prefix rename.What landed on main while this PR waited, and how it interacts
Rebased onto
197bcde31. Four merges touch this work and each was checked rather thanassumed:
a5fe08184) wiresgenerate_types.py --checkintomake validate-generatedand adds
scripts/post_generation_manifest.json, which fails the build on apost-generation fix it does not know. Two passes were undeclared: this PR's own
insert_task_message_markers, written before the manifest existed, and the inversion'snew
declare_root_schema_module_exports. Both fire, both are declaredtrue, and--update-fix-manifestreclassified nothing else — the manifest diff is exactly those twolines. fix(codegen): make a dead post-generation fix and a stale generated tree fail the build #1374's determinism wrapper
scripts/run_datamodel_codegen.pyis retained and used.d033c36ab) makes_format_all_blockemit each name once. Retained verbatim(
names = list(dict.fromkeys(names))); the inversion touches the callers of that emitter,not the dedup.
197bcde31) rewrites the format-referenceagent_urltoWireUrlin apost-generation pass. The pass is retained, and the regenerated
src/adcp/types/domains/core/format_id.pyis identical to main'sgenerated_poc/core/format_id.pymodulo the generation timestamp — the rebase'sconflict resolution had taken the pre-fix(codegen): keep the format-reference agent_url as the wire string #1396 content, and the regeneration corrected it,
which is the reason to regenerate after a rebase rather than trust the resolution.
called out rather than buried.
tests/canonical_stub_field_gap.json(786 lines),tests/canonical_stub_gap.pyandscripts/update_canonical_stub_ledger.pyexist to stopcanonical_creative.pyifalling further behind the 715 runtime fields it omits. This PRdeletes the stub, so the ledger's subject is gone: there is no second declaration of a
field to fall behind a first. An allowlist is normally shrink-only and never deleted; the
exception here is that the property it ratcheted became unconditional rather than
unenforced, and
git grep canonical_stubreturns nothing afterwards.adcp.types.generated_pocbecomes a one-file tombstoneA module, not a package, so no directory by that name survives. It raises
ImportErrornaming the prefix rename, because every module stem moved unchanged:
Nothing in-tree imports it after the move, so deletion would work. #1360 measured one
production seller carrying 110 such deep imports, and fifteen lines turns a bare
ModuleNotFoundErrorinto that sentence. Removal major is stated: v10.A working shim was refused. Redirecting
__path__would put a SECOND module object insys.modulesfor every module, which is two class objects per class — the exact identitybreak
tests/test_export_surface_is_derived.pyexists to catch.The guards shrink, and one stops being vacuous
tests/test_import_layering.pyis the clearest measure. The forbidden-prefix tuple goesfrom two entries to one, because only
adcp.types._generatedstill resolves a name bysomething other than the schema — it binds a bare name several schemas declare to one
sort-order winner. The frozen violation baseline goes from eight files to one: the eight
were reaching into a private tree that no longer exists.
test_generated_tree_is_not_forbiddenkeeps the shrink deliberate — re-adding
adcp.types.domainsto buy back the old allowlistforbids the definition site, and fails by name.
test_collision_guard.py's mirror-coverage test counted exports out of the generated treeand compared them to exports out of the same tree. Two reads of one directory: it passed
whatever the public layout was. Two tests replace it, both asking the runtime — every
declared
(module, name)pair must import fromadcp.types.domains.<module>, and a realclass must be DEFINED there. The second clause is the one a re-export tree cannot satisfy,
and it was fired on purpose before being trusted: appending
FireProbeSentinel = AccountStatusto a leaf module turns it red by name.Why
adcp.types.domainsand notadcp.types.<domain>This is a deliberate deviation from the design this PR implements, so it is stated here
rather than left for a reviewer to notice. The design puts the domains one segment shorter,
at
src/adcp/types/<domain>/, and asserts that the curated topic modules can retire intothe generated domain roots because "the 245 names they bind today resolve to the same
objects — snapshot proves it". The snapshot proves the opposite, which is blocker 3
below: four of those modules bind 189 names their domain roots do not.
Three things stand in the way, all of them public-API changes rather than output-path work,
and all three measured rather than assumed:
src/adcp/types/__init__.pyis 2,211 hand-written lines and dmcg writes its own package__init__.py._install_generated_artifactsinstalls a whole directory per artifact, so makingtypes/the artifact directory deletes every hand-written module in it. The fix is 25 per-domain artifacts plus 4 root modules and a rewritten install/compare path.adcp/types/core.pyholds 19 hand-written SDK runtime classes and collides with a generatedcore/package. A directory wins over a same-named module inFileFinder, socore.pywould be silently shadowed.creative.py/media_buy.py/signals.py/protocol.pyin favour of the generated<domain>/__init__.pydrops 189 names that resolve today —set(adcp.types.<m>.__all__) - set(adcp.types.domains.<m>.__all__)is 67, 47, 32 and 43.Blocker 3 is the one that decides it: keeping those names means folding hand-written content
into a generated file, which is the drift channel this change exists to close. Dropping the
segment is a pure rename once 2 and 3 have their own change.
One line inside the v3 codemod, for a reason worth stating
v3_to_v4._replacement_is_identicalimports the module a flagged adopter import names, tocheck the proposed public replacement is the same object. The spelling the codemod DETECTS
stays the v3 one; the module it IMPORTS is now where those classes live.
That import sits inside
except (AttributeError, ImportError). Leaving it would haveswallowed
ModuleNotFoundErrorand silently degraded every per-symbol auto-rewrite to aflag — a regression with no failing test and no error message.
CHANGELOG.mdand the fiveMIGRATION_*.mdfiles keep the old spelling throughout: theydescribe releases in which that path existed and was private, which is still true of those
releases.
Canonical models become real subclasses
Verified on
197bcde31before writing this:CreativeManifest.model_validate({"assets": {}})is still accepted on main, with the same three validators, so the defect below is live.
enforce_root_required_groupslanded in #1368 and emits amodel_validatorcarrying theroot-level
requiredgroups a schema declares, soCreateMediaBuyRequestno longer acceptsa media buy with no packages, no budget and no proposal.
Whether that rule reaches the class an adopter validates against depends on a second
mechanism, because
canonical_creative.pystill builds the public canonical models withcreate_modelover a copy of a generated class's fields — 33 call sites — so the generatedclass is not in the public model's MRO, and
canonical_creative.pyihides the result frommypy.
_own_validatorsis that second mechanism: it re-decorates a source class's ownvalidators onto the clone, deciding which fields each validator touches by inspecting its
code object —
co_namesplus the string constants, walked into nested code objects — andskipping any validator whose touched set meets the fields the clone drops, recording the
skip in
__adcp_canonical_validators_left_behind__.It carries the validators it can. A skip, though, is a recorded non-enforcement, and one of
them matters: on
mainthe publicCreativeManifestvalidates{"assets": {}},because
core/creative-manifest.jsondeclares aformat_id | format_kindroot group andformat_idis a field the canonical boundary drops, so the rule is skipped rather thanreduced.
This removes the copy instead. Every canonical model becomes a real subclass of the
generated class it refines, so inheritance carries every validator the pipeline injects —
the ones #1368 emits, the reporting-selector uniqueness checks, the publisher-property
coercion on
Product— with nothing to re-decorate and no skip list._own_validators,_validator_referencesand both bookkeeping ClassVars are deleted with the clone.The public
CreativeManifestnow refuses{"assets": {}}and accepts{"assets": {}, "format_kind": "image"}.Doubling the mechanism is what makes a gap like this recur: with two carries, every new
validator needs the second one, and the day someone forgets, the public surface silently
stops enforcing a rule the generated class enforces. With one, there is nothing to forget.
tests/test_canonical_models_are_subclasses.py::test_every_generated_validator_reaches_the_public_modelpins the property for every validator rather than for one — each generated ancestor's
model_validatorsmust be a subset of the public model's — and reverting a single model toa copy fails it by name.
What else is in it
A type that means "an AdCP request". There is none today: the name that should mean it,
adcp.types.Request, resolves toadcp.types.domains.creative.preview_creative_request.Request,a body fragment of one tool, flat-exported by the consolidation step and listed in the
curated
adcp.types.protocol.__all__. A consumer writing a transport boundary thereforereaches every cross-cutting field by
getattr(req, "...", None)againstAny, and onewanting to refuse a hand-written look-alike spells
issubclass(model, adcp.types.domains.core.version_envelope.AdcpVersionEnvelope)— adeep import whose coverage over the task registry was 75/80, then 0/77, then 78/92
across three majors.
AdcpRequestandAdcpResponseare hand-written plain classes inadcp.types.base. Theydeclare no fields and are not
BaseModelsubclasses.AdcpRequestcarriesget_account,get_idempotency_key,get_context,get_push_notification_config,get_adcp_versionand
get_adcp_major_version;AdcpResponsecarriesget_status,get_task_id,get_context,get_adcp_error,get_messageandget_replayed. Each returns the field'svalue, or
Nonewhere that tool's schema declares no such field — of 87 request schemas, 80declare
context, 49account, 43idempotency_keyand 19push_notification_config, soan accessor that answers
Noneis answering correctly. A shared private base carries threefield classifiers derived from the MRO —
version_fields,protocol_fields,payload_fields— so which stratum a field belongs to is answered by which ancestordeclared it rather than by a hand list per transport edge. Fields stay flat, exactly as on
the wire, because both envelopes say so: the version envelope "is part of the payload
itself" and
payloadis "a documentary construct — NOT a required wire field".The markers declare no fields deliberately. A field on the marker would put
adcp_versionon
ValidateInputRequest, whose schema declares no version fields.The version envelope, composed where the schema declares it without composing it.
normalize_version_envelope_compositionruns in the existing temp-tree normalization slotand rewrites 12 schemas on the generation copy only — the cached bundle and any
direct-from-schema validation are untouched. It handles two authored forms: a property that
is exactly a JSON-pointer
$refintoversion-envelope.json#/properties/<field>, and aninline declaration equal to the envelope's own modulo
$comment,deprecated,description,examplesandtitle. It refuses four shapes — a bundled schema, a schemathat already composes, a version field listed in
required, and a rootanyOf/oneOfcontaining a bare
$refarm — and a ceiling of 12 makes it raise rather than silentlychange shape when a bump moves the problem.
The fourth refusal exists because the rewrite deleted a field without it. The generator
merges a root
propertiesinto every arm it builds, but a rootallOf$refbecomes abase only of the arm built from the root object; an arm that is a bare
$refto anotherschema is generated from that schema and the root's bases never reach it.
Applied to the two pointer-ref responses, that took
adcp_versionoffRefineProposalsResponse2andRequestProposalsResponse4— their submitted arm is a bare$reftocore/compact-task-submitted.json— and underextra: forbida seller echoingthat field on the submitted arm would then have been rejected where it previously validated.
Those two schemas keep their authored form, and
test_the_two_pointer_ref_responses_keep_the_field_on_every_armholds the field present onevery arm so the deletion cannot come back. The refusal costs no ancestry: the all-arms
count is 70/77 either way, because the submitted arm never gained it.
One post-generation pass inserts the markers, marking 203 classes across 154 modules.
Membership is the task registry's decision —
index.json'sschemas.<domain>.tasks.<task>.{request,response}.$ref— with the schema-to-module mappingread from the generator's own
filename:header and the root class name derived from theschema
title, which reproduces the generated name for 154 of 154 task messages. The markergoes on the class or, for a union root, on each member arm, so
isinstance(req, AdcpRequest)holds for every value a task request can validate into. Thepass re-parses each module it writes and asserts the marker is the first base, and fails
closed on an unresolvable schema-to-module mapping, a schema without a title, an
unresolvable root class, or a union arm not defined in the module. Ten request files that no
task names stay unmarked.
Ancestry at task scope, counting a message as having an ancestor only when every root class
it can validate into descends from it:
AdcpRequestAdcpResponseAdcpVersionEnvelopeAdcpVersionEnvelopeProtocolEnvelopeThe request side gains nine; the response side is unchanged, because the two schemas whose
rewrite would have touched it are the ones the bare-
$refrefusal declines and the restalready compose. Both before-columns were measured by running
scripts/task_message_lattice.pyagainstmain's own tree, where it reports the markers asabsent rather than as
0/77so that "not present" cannot be misread as "present andfailing".
The request residue is the one expected schema,
creative/validate-input-request.json,which declares no version fields at all. The response residue is seven, identical before and
after: the five compact media-buy tools compose the envelope-free
media-buy/media-buy-commitment-response.json, and the two proposals responses carry asubmitted arm that is
core/compact-task-submitted.json. Ten task responses do not composethe protocol envelope. Each residue is pinned by equality, so a schema that gains the
ancestry upstream fails the guard until its pin is removed.
scripts/task_message_lattice.pyprints the table and names every residue, against acheckout or an installed wheel.
A union arm's name, derived from its schema node.
create-media-buy-response.jsontitles its three armsCreateMediaBuySuccess,CreateMediaBuyErrorandCreateMediaBuySubmitted; the generated classes areCreateMediaBuyResponse1/2/3, andaliases.pyhand-transcribes most of the titles back. Apositional name is also one whose meaning can change without the thing it names changing,
and the cost of that is already paid once:
_generatedrebinds the wholeAuthorizedAgents*window at runtime to preserve historical numbering while mypy keeps thepre-rebind declarations, so each semantic alias over the shifted names held the runtime
class it named and the static type of its neighbour — an off-by-one that made every
documented constructor call fail
mypy --strict.The union-arm naming rule was taken out at review and moved to #1409.
scripts/union_arm_names.pyand its tests renamed nothing and weren't wired into the pipeline. They're kept on theclaude/issue-1409-union-arm-namingbranch. Their measurements are recorded in that issue: one arm in the bundle can't be named until adcontextprotocol/adcp#7913 lands. The issue also lists the two decisions about the public surface that wiring the rule needs.AdcpVersionEnvelopejoins the curatedadcp.types.protocolsurface, whereProtocolEnvelopealready is.adcp.typesbinds the name since #1367, so this adds thecurated spelling and grades what the pair is for. Consumers replacing a deep-import
envelope check should use the conjunction of both predicates rather than either alone:
AdcpRequestis field-less, so a class can inherit it and invent its fields, whileAdcpVersionEnveloperefuses that and accepts a model that composes the envelope withoutbeing any task's message. The conjunction refuses all three constructed forgeries; either
alone refuses two. Both results are pinned as tests so the asymmetry is recorded rather
than rediscovered.
The lattice does not move the public API snapshot. It adds no name to
adcp.typesandremoves none; every type it introduces is reached through a module that already exists —
adcp.types.basefor the two markers,adcp.types.protocolfor the envelope pair. Thesnapshot does change in this branch, by seven entries, and all seven come from the
surface-hole fixes below rather than from the lattice or the move.
Two architecture documents, because there are none for this layer today:
docs/design/holds one file about a FastMCP registration investigation,
docs/extending-types.mdcoverssubclassing and variant selection rather than what the surface guarantees or how it is
produced, and outside
CHANGELOG.mdand one proposal,generate_types,post_generate_fixesandconsolidate_exportsare named in no document in therepository.
docs/type-surface.md— where to import from and when each spelling applies, what isdeliberately not public, what the markers guarantee, how envelope fields are told from
payload fields, where the ancestry is incomplete and why those residues are upstream's,
and how response variations are named and dispatched. It defers to
extending-types.mdfor subclassing and to the generated
docs/shared-type-names.mdfor the collision tablerather than restating either.
docs/design/type-generation.md— every pipeline step with what it consumes and produces,which step touches the network, the ordering constraints inside the post-generation pass,
how a class gets its name, the guards, how to add a schema feature, and what to do when the
generator renders something wrong. It records the generator pin and its reason, which was
documented nowhere: 0.72.0 and later cannot generate this tree at all, and 0.71.0 is
rejected on measured non-equivalence.
Both state what the architecture refuses and why. Arm naming is described as
the positional emitter that ships, with the schema-derived rule pointed at #1409, and the
staleness check is described as
generate_types.py --check, which works today butwhich
make validate-generateddoes not run.Deleting the stub put the module under mypy for the first time
canonical_creative.pyiis 255 lines and a.pyishadows its.pyentirely, so everytype error in
canonical_creative.pywas invisible and the stub was the only thing mypyread. It hid 715 inherited fields across 31 classes, required four of them, and invented
one.
Every error it had been hiding is fixed and none suppressed; the module's
type: ignorecount goes from 2 to 0. Two were a dead helper and two redundant
statusredeclarationsthat restated what the generated arm already pins. One was a real type mismatch the stub had
concealed:
_project_product_fieldswas annotated for a 53-member enum that the requestnever holds — one of the names #1360 measured as typed a variant off its referent.
Five assertions are corrected to a new truth with the reason at each site, and three of the
five add a check rather than only adjusting one. Two
assert not isinstance(manifest, input_model)lines are removed as false by construction; thepytest.raisesobligationsbeside them were always the contract and are what now grade the tolerant-model bypass below.
One test read
Package.__annotations__, which holds a class's own annotations and answeredthe question only while
Packagewas a copy re-declaring 59 inherited fields; it readsmodel_fieldsnow. One type-check fixture constructedListCreativesResponse(creatives=[]),which type-checked against the stub and fails at runtime because
query_summaryandpaginationare required.tests/test_canonical_validator_parity.pyis re-pointed at inheritance, and two of itsobligations get stronger for it: one admits no exceptions where it previously asserted only
that a skip was justified, and one asserts a refusal where the clone accepted the document.
One consequence that needed its own fix
_DeliveryCreativeManifest's docstring reads "tolerant served output, deliberately not asubtype of the strict input", and as a real subclass it is exactly that subtype. Pydantic's
default
revalidate_instances="never"skips validation for an instance of any subclass, soa tolerant instance carrying a
format_kindthe pinned enum does not know passed where thestrict wire model was expected — the weakening #1241 exists to prevent.
revalidate_instances="subclass-instances"on the strict base closes it, read off__bases__rather than named so it follows the declaration. An exact-class instance stillpasses through with its identity, so composing a model into a field is unaffected, which
tests/test_composability_invariant.pygrades. The durable fix is the non-subtyping thedocstring describes, and that is a separate design question.
Gate
Measured on this branch, base
197bcde31(upstream/main), Python 3.12.9,datamodel-code-generator==0.64.0, working tree clean before and after.Since #1374,
make validate-generatedrunsgenerate_types.py --checkitself, so the1,088-file output-path move is graded by main's own gate rather than by an assertion in
this description. It stages a complete regeneration, compares every artifact byte-for-byte,
and grades each post-generation fix against the effect manifest.
ruff check tests/goes from 88 findings to 17 and the 17 are a subset of main's,compared entry by entry with
comm: the import-prefix rename happened to fix 71pre-existing
I001sort violations, becauseadcp.types.generated_pocsorted differentlyfrom
adcp.types.<domain>.black --checknames the same 17 files it names on main — ablack-version drift this PR does not touch; only the two files this PR reformats were
formatted.
"Up to date" is the statement that the committed tree is exactly what this branch's
pipeline produces — now at the new output path, with the domain roots written into the same
directory. 1088 of 1088 files restored as timestamp-only is the idempotence half of the
same property.
The suite is judged set-wise, and the failure set is a strict subset of
upstream/main's. All seven were re-run in a worktree at pure197bcde31with its ownvenv and fail there identically, by name:
upstream/maintest_reporting_interop_corpus.py::test_ts_version_and_abort_helpers_fail_closed_without_sdk_executionFileNotFoundError: '/etc/os-release'test_reporting_interop_matrix.py::test_acceptance_requires_dependency_locks_and_exact_runtimestest_reporting_interop_matrix.py::test_legacy_validate_results_command_is_explicitly_nonacceptingtest_reporting_interop_matrix.py::test_result_gate_checks_all_semantics_and_cross_cell_isolationtest_reporting_interop_matrix.py::test_result_gate_rejects_one_missing_semantic_assertiontest_reporting_interop_matrix.py::test_result_gate_rejects_runtime_driftconformance/reporting/test_reporting_production_harness.py::test_installed_cleanup_still_writes_evidence_when_pipe_draining_expiresPermissionError: Operation not permitted/etc/os-releasedoes not exist on macOS and the harness writes where a local run cannot:both are properties of the development machine. Nothing outside that set fails.
The first run after the move returned thirteen, and the six extra were all real — tests
written against the mirror layer. Each is fixed rather than adjusted away: two read
mirror.__all__on a schema module, two patched_copy_package_for_introspectionby name,one imported
GENERATED_POC_DIR, one importedadcp.types.generated_poc.<m>to compareobjects and hit the tombstone. Giving all 1,059 generated modules an
__all__would alsohave fixed the first two and was refused: the snapshot's namespaces are the domain roots,
all 26 of which declare one, nothing star-imports a schema module, and a declared
__all__on a thousand generated modules asserts what an AST pass already derives.
The rebase onto
197bcde31added two more of the same kind, both from files main createdthat name a path the move removes: #1396's own
tests/test_format_reference_wire_url.pyimported
adcp.types.generated_poc.core.format_id, andci.ymlgained a comment namingsrc/adcp/types/generated_poc/**. Both retargeted.Nothing was weakened, counted rather than asserted:
The mypy override that followed the tree now also covers the generated domain roots, which
were previously checked. All three codes it disables —
valid-type,assignment,unused-ignore— are inapplicable to a module that isfrom X import Ylines and an__all__: no annotation, no assignment, notype: ignore.Two tests were replaced and one deleted, all three for the same reason.
test_export_surface_is_derived.py::test_a_mirror_exports_its_schema_verbatimcompared amodule's exports to the same module's exports once the mirror and the source were one
module — a tautology, and its unique content was a
checked > 4000volume floor, whichmoves onto the reachability test.
test_collision_guard.py::test_the_mirror_covers_every_declared_pair_exactly_oncewas already a tautology before this PR: it read one directory twice.
test_code_generation.pycarried two functions importingadcp.types._generated, oneasserting
len(dir()) > 100plus fourhasattrs and one assertingis not None,distinguishable only by the stale
generated_pocin the weaker one's name; renaming itmade the shadowing a ruff
F811and the survivor asserts strictly more.test_no_generated_type_is_reachable_under_zero_namesis the one that gets stronger: itnow requires each declared class to be DEFINED at its own address, not merely bound there.
Collected item count moves by exactly that: 17444 to 17442, three removed and one added.
format_kindbecomes an open string the SDK never refusescore/canonical-format-kind.jsonstates the rule in two halves, and they belong to different directions:A single closed
enumcannot carry that, so the schema encodes one half and the other half has to be solved by every consumer privately. In this SDK it was solved by a second widened type, a tolerant model that subclassed the strict one, andrevalidate_instances="subclass-instances"on the strict base to stop the tolerant instance passing as strict-validated input.This types the field as
strinstead, for both directions, and the SDK refuses nothing. Its sibling fields already work this way:format_shapeandasset_group_idare plain strings governed by versioned registries, andcore/asset-group-vocabulary.json's governance block says so normatively — "non-canonical values remain valid ... validators may emit soft warnings to encourage adoption of canonical values."The closedness was never a property of the value. It is a property of the deployment: a seller supports some set of format kinds, that set is the seller's rather than the pinned bundle's, and it may be larger (the seller handles a kind from a newer release) or smaller (the seller handles four of the sixteen). A pinned library enforcing its own vocabulary makes its pin a ceiling on what the protocol permits — measured before this change,
CreativeManifestrefused aformat_kinda 3.3 server could legitimately send, while the read-back model accepted it. There is no principled basis for the pin being authoritative in one direction and advisory in the other.So the vocabulary is exported rather than enforced:
CanonicalFormatKindkeeps its 16 members and stays exported — the override relocates the vocabulary, it does not discard it. What changes is that the field no longer refuses the seventeenth value. "I accept your request and then tell you I cannot process this creative" is a seller's answer, not a type error, and the SDK deliberately does not make it on the seller's behalf.What this deletes.
_StrictFormatKind(68 lines), and with the field open in both directionsrevalidate_instancesbecame inert — deleted, and the readback, composability and forward-compat suites are identical with and without it, which is the whole reason the flag existed. Net 104 lines removed against 37 added.The generated model no longer agrees with its schema for this one field, deliberately.
tests/conformance/_schema_parity.pycarries a single named entry forcanonical-format-kind.jsonwith this reasoning at the site; the suite's predicate and scope are untouched, and removing the entry turns it red. Upstream adcontextprotocol/adcp#7929 asks for the schema to modelformat_kindthe way its siblings already are, which would make the override unnecessary rather than permanent.What survives and why.
_DeliveryCreativeManifestand_DeliveryCreativeVariantno longer redeclare anything about the vocabulary, and deleting them was measured rather than argued: six cases go red, of which one is behaviour. Because the canonical models are real subclasses of the generated wire models, a delivery row carrying a manifest instance of the generated class presents a parent where the field is typed as the subclass, and pydantic refuses it (model_type)._normalize_readbackdumps it first, which is what lets a response be composed out of models. That seam is a consequence of the subclass refactor, not offormat_kind.Three holes the move exposed
The thesis above — one tree, nothing to copy, nothing to drift — held for the 1,088 generated
modules and left three places where the PUBLIC surface in front of them was still curated by
hand. Each was filed separately against this branch, and each is fixed here by deriving the
obligation rather than by extending the list that drifted.
A root discovery schema is the one case a prefix rename gets wrong (#1402)
scripts/generate_types.py'sROOT_DISCOVERY_SCHEMASnames every schema whose basenamecollides with a task-schema DIRECTORY:
brand.jsonagainstbrand/*.json. Pre-9.0 thegenerator wrote such a schema's models into the colliding package's
__init__—generated_poc/brand/__init__.pyWASbrand.json, 140 classes includingBrand— and 9.0writes them to
domains/brand_discovery.py, leavingdomains/brand/__init__.pyto thegenerated domain aggregator. So one deprecated name has two canonical halves, and the alias
sent it to one:
A compatibility alias that resolves the module and not its contents is worse than none: the
error names a module the adopter never wrote. 93 of
brand's classes relocated this way,measured by AST against this PR's merge base.
_SplitAliasModuleserves one deprecated name over both halves, discovery half first becausethat is the surface the pre-9.0 module HAD, and the two halves share 12 names. It defines no
class, so every class reached through it is still the canonical class — the property the
one-module-object rule was protecting, and the only one
isinstancereads. It borrows theaggregator's
__path__, because_find_and_loadreads the parent's before any meta-pathfinder runs and a shim without one answers "is not a package" for all 20 leaf modules.
The second half is DERIVED from the generator's own naming (
discovery_halfappends_discoveryto a domain-level package and asksfind_spec), not listed again in the runtimemodule. A schema added to
ROOT_DISCOVERY_SCHEMASis served without touching the alias, andtest_a_root_discovery_module_is_named_the_way_the_alias_derives_itfails the build if thegenerator ever names one some other way.
The alias module asserted the prefix rename and the single module object as unconditional
facts. Both now state the case that breaks them.
Why the suite could not see it. It asserted over the names the CANONICAL module binds,
never over the names the PRE-9.0 module exported. The new split family asserts the second
direction, over every public name of both halves, with the split set read from
ROOT_DISCOVERY_SCHEMAS.Reconciling the patch posted in review
A patch for this issue was posted in review — a
_NAME_OVERRIDEStable, an injection step in_AliasLoader.exec_module, and a test iterating the table. It identified the right root causeand the right injection point, and its central invariant is the one kept here: write only
names ABSENT from the canonical module, so a canonical name always wins. What follows is why the
mechanism was reconciled rather than applied verbatim, with the numbers, so the call is cheap to
overrule.
1. The posted test cannot fail. Both of its assertions are nested inside the table loop
(lines 129, 134, 137 and 142 of the posted diff):
Empty table, zero iterations, zero assertions. Measured by applying the patch to a clean
checkout and then removing the
brandentry — which IS this defect:So it grades a table entry that is present but WRONG, and never one that is MISSING — and a
missing entry is how this defect shipped. Its docstring's claim that "a future split that adds
an entry is automatically graded" is true and is not the gap; the gap is a split where nobody
adds an entry.
2. The posted diff no longer applies.
git apply --checkfails on both files: commit607adf441rewrote that region of_generated_poc_alias.py. A reconciliation was requiredeither way — this is a reconciled patch, not a declined one.
3. The posted mechanism is viable, and one measurement decided against it. Rebuilt with a
derived table it serves 161 names and passes the surface guards (373 passed across
test_export_surface_is_derived,test_public_api,test_adopter_public_pathsandtest_collision_guard). The cost is thatexec_moduleinjection MUTATES the canonical module:adcp.types.domains.brand.Countrydoes not exist, and then does, permanently, as soon asanything in the process touches a deprecated import. +161 attributes on a canonical module no
generator emitted, load-order dependent, and invisible to this repo's own drift detector because
snapshot_entryreads__all__rather thandir()— a phantom public surface no guard canpolice, in a PR whose thesis is that the generated tree IS the public tree. The posted hand table
avoids that only by serving 3 of the 93 relocated names and leaving 90 broken. (A
dir()-basedderivation is worse again: it injects
Annotated,Any,AnyUrl,AwareDatetimeand 15 otherimports, which is how 180 becomes 161 once filtered on the defining module.)
_SplitAliasModuleserves 193 names with no mutation of anything, and keeps pre-9.0 fidelity onthe 12 names both halves define by letting the discovery half win. Its costs are real and stated:
the one split name loses
sys.modulessingle-object identity — replaced by the stronger gradedclaim that it is one CLASS object, which is what
isinstancereads — and three per-pair testsmove onto
UNSPLIT_PAIRS, derived by subtracting the same split set.4. If any part of the posted mechanism ships instead, its test needs one line. An
anti-vacuity assertion before the loop:
Its absence is the defect, and it costs one line whichever mechanism lands.
5. Where the derivation can and cannot live.
scripts/is not packaged(
packages.find where=["src"], confirmed against an installedadcp-9.0.0wheel), so theshipped module cannot read
ROOT_DISCOVERY_SCHEMAS. What it derives at runtime is thegenerator's naming convention; the test ties that convention back to the dict so a divergence
fails the build. That split is forced by packaging, not chosen.
The legacy format reference was a subclass that weakened its own parent (#1398)
adcp.types.LegacyFormatIdwas a subclass of the generatedFormatReferenceStructuredObject,and no public name on
adcp.typesresolved to the generated class at all — while generatedmodel fields are annotated with it. The subclass was written when
agent_urlwas anAnyUrlthat normalized the wire bytes. #1384 moved it to
WireUrl = Annotated[StrictStr, AfterValidator(_require_absolute_url), WithJsonSchema(...)],which is that subclass's entire purpose, and left every redeclaration a weakening of the
parent it was written to narrow:
LegacyFormatIdwidth: 300.0height: 250.0agent_urlrelative, scheme-relative, bare hostThe flat surface and the type the SDK's own model fields declare disagreed about one wire
document. The parent already carries
extra='allow', the sameidpattern andpixel_ratio;the subclass's
_validate_agent_urlduplicated_require_absolute_url, and itsmodel_dumpoverride was a bare
super()call.Nothing needed adding —
adcp.types._generated.FormatReferenceStructuredObjectwas alreadythe canonical class, routed there by the
KNOWN_COLLISIONS["FormatId"]entry. The fix is thedeletion of the clone, so the
Legacy*name is the generated class as every other name inthat module already is. Net 41 lines removed against 22 added.
The
Legacy*arm set had five holes, not four (#1399)The rule. For every generated
<tool>_response.pywhose tool has anyLegacy*name onadcp.types, every arm of the<Tool>Responseunion it declares must be reachable under apublic name there. The arms come from
typing.get_argsof the union the generated moduleitself declares — not from a name pattern, and not from
__module__, which names the canonicalspec even for a duplicate and so cannot tell a copy from an original. It is keyed on whether a
bare public name already reaches the GENERATED arm, which is why
sync_creativesneeds noLegacy*arm names: there the generated class already is the public one.The four it confirmed.
LegacyUpdateMediaBuy{Success,Error}Responseshipped;LegacyUpdateMediaBuySubmittedResponseand all threeLegacyCreateMediaBuy*Responsearms didnot. A seller holding an arm read off the wire could then only name it through
adcp.types.domains.media_buy— the private tree this release retires — because the barepublic name is the canonical SUBCLASS and
isinstanceagainst it is False for a generatedinstance.
The fifth it found, which no enumeration of the four would have closed. Measured on the
installed wheel for this branch:
Three siblings with two public names each and the fourth with none.
preview_creativegained afourth arm and nobody named it. So "why not just add the four names" answers itself: adding
four names was the plan, and the plan was already one short — in the PR that exists to stop
exactly this.
adcp.typesgoes from 46Legacy*names to 52, and the required set is computedin
tests/test_legacy_surface_names_every_arm.pyrather than listed.What stays enumerated, and why
The
Legacy*names themselves are still spelled out. This SDK's public surface is enumeratedby design —
adcp.types.__all__,_eager.py, theTYPE_CHECKINGre-export block, andpublic_api_snapshot.json's 7,840 recorded resolutions — all undermypy --strictwith azero-
type: ignoreadopter suite. Aglobals()loop is invisible to all four and would tradea visible list for an invisible one. What makes the sixth hole impossible is that the required
set is derived and the surface is refused when it does not satisfy it.
The scope of that guarantee, stated rather than implied. The obligation is keyed on the arms
of a response union, so it closes the hole class this issue is about and no more. A
Legacy*hole in a non-response type — a request, an enum, a bare model — remains possible and is
ungraded.
Both new tests were probed by reintroducing the defect, and one of them failed
Every test added here was fire-probed: the defect it targets was put back and the test had to
go red. Eleven probes; two changed the work rather than confirming it.
The
Legacy*test was blind, and the probe is what found it. The first version assertedthat no
Legacy*name shadows a base with the SAME class name. #1398's clone wasclass LegacyFormatId(FormatReferenceStructuredObject)— a different name — so that rulereported nothing, and only a
__module__check was catching it. One line defeated that(
Clone.__module__ = "adcp.types.domains.core.format_id") and the test went green on the livedefect.
__module__is a weak witness here by construction:module_from_specstamps thecanonical spec onto a duplicate, so it cannot tell a copy from an original. The assertion is
now a ROUND TRIP — the class must be the attribute the module it claims actually binds under
its own qualname — which fails for a shim declared anywhere else and for a shim that lies about
where it was declared. Re-armed against the same probe: 4 red.
The alias's
__path__turned out not to be load-bearing on its own. Dropping it changednothing, because the attribute fallback reaches the same object. It is still bound explicitly —
a package attribute the import system reads on every submodule import is not a compatibility
lookup — but its comment no longer claims to be the thing that makes leaf modules resolve. A
comment that overstates what a line does is how the next person deletes the wrong one.
What the alias deliberately does NOT serve
Relative to this PR's merge base,
bundled.protocol.get_adcp_capabilities_responselost 94class names, now resident under
formats.canonical.*. The alias does not serve them, and mustnot: those are renumbered union arms, so a name that matches is a DIFFERENT class for a
different schema node, and handing an adopter the wrong class silently is worse than an honest
ImportError. The same holds more broadly — 387 (module, class) pairs the old tree exported arenot on the identically-named new module, 243 live elsewhere and 144 exist nowhere at all
(
Status148,Action2,CommittedMetrics6,Type12— arm renumbering across a schemare-sync). A test asserting "every name the old tree exported is importable through the alias"
would be red on 144 names no alias can serve, and would be asserting something false. That is
why the derived test is keyed on the generator's schema-to-module placement rule, which names
the one case where a name that still EXISTS is unreachable from its old address.
One defect filed against this branch is NOT fixed here
#1401 —
adcp.types.ProductFormatDeclaration is adcp.types.Format, and the generated 16-armunion the schema describes is exported as
LegacyProductFormatDeclaration. The premise isright:
canonical_decl.pyjustifies its own existence on "datamodel-code-generatorcollapsesthis shape to a single class ...
format_kindandparamsdisappear entirely", whichfcbf4229fin this PR made false. But the prescribed rename drops a normative constraint:product-format-declaration.json'sallOf.notclause forbids that combination; thehand-rolled class enforces it and the generated union does not. It also carries
params_as(),credential-shaped-key scanning and
extra='allow'round-tripping, and there is a pre-existingcollision underneath it —
canonical_creative.ProductFormatDeclaration = Formatshadowscanonical_decl.ProductFormatDeclaration, two unrelated hand-rolled classes competing for onepublic name — that any rename has to resolve first. Left for a maintainer's decision rather
than settled in passing.
Leaving it open costs the one adopter we can measure nothing: it imports neither
ProductFormatDeclarationnorLegacyProductFormatDeclaration, so folding the rename wouldhave moved none of its pinned aliases either. "It breaks consumers" is not a reason to rush
this one, in either direction.
BREAKING CHANGE: adcp.types.generated_poc is removed. The generated models are defined at adcp.types.domains.., with every module stem unchanged, so the migration is a prefix rename: adcp.types.generated_poc. -> adcp.types.domains. . Importing the old path raises ImportError carrying that instruction rather than ModuleNotFoundError, and the module itself is removed in v10. The move itself changes no public name's resolution, and adcp.types.domains is no longer an internal layer: it is the definition site and a supported import for a name the flat adcp.types namespace cannot bind. every public canonical creative model is now a subclass of the generated class it refines rather than a runtime copy of its fields. isinstance and issubclass against the generated class are now True where they were False, so code using the negative to tell the two layers apart needs a different test. src/adcp/types/canonical_creative.pyi is deleted and the real source is what mypy reads, so previously hidden required fields are enforced at type-check time: ListCreativesResponse requires query_summary and pagination, and a construction that type-checked against the stub may now fail. Validators the generation pipeline injects into a generated class now reach the public canonical model, so a public model refuses documents it previously accepted: CreativeManifest requires one of format_id or format_kind. format_kind is typed str rather than the CanonicalFormatKind enum on every model generated from core/canonical-format-kind.json, and the SDK refuses no value in either direction; CanonicalFormatKind remains exported as the vocabulary and is_canonical_format_kind is the check, so a consumer that relied on the field's type to reject a non-canonical kind must call the predicate instead. adcp_version and adcp_major_version are now plain str and int rather than the AdcpVersion and AdcpMajorVersion RootModel wrappers on eight request classes whose schemas declared them as JSON-pointer refs: AcceptProposalRequest, BuyProductsRequest, ControlMediaBuyRequest, DeclineProposalsRequest, ListProductsRequest, RefineProposalsRequest, RequestProposalsRequest and SyncReportingStatusRequest. Validation is unchanged; drop .root where you read those two fields on those eight classes. AdcpVersion and AdcpMajorVersion remain exported, so no name is removed. Every task request and response also gains AdcpRequest or AdcpResponse as its first base, so code pinning an exact bases or mro shape needs updating. adcp.types.LegacyFormatId and adcp.types.LegacyFormatReferenceStructuredObject now resolve to the generated FormatReferenceStructuredObject instead of a subclass of it, so a model field annotated with either name accepts the JSON-schema spelling of an integer width and height - 300.0 - which the subclass refused, and the subclass's own name no longer appears in a validation error. Six Legacy names are added and none is removed.
BEGIN_COMMIT_OVERRIDE
refactor(codegen)!: generated models are defined at adcp.types.domains.., and adcp.types.generated_poc.* is now a deprecated alias of the same module objects (DeprecationWarning, removed in v10); migrate with the prefix rename adcp.types.generated_poc. -> adcp.types.domains.
feat(types)!: public canonical creative models are real subclasses of the generated classes they refine, and canonical_creative.pyi is deleted, so mypy now enforces previously hidden required fields (ListCreativesResponse requires query_summary and pagination) and generated validators now apply (CreativeManifest requires format_id or format_kind)
feat(types)!: format_kind and format_kinds are open str on every generated and canonical model and no value is refused; compare with == instead of
is CanonicalFormatKind.xand check the vocabulary with adcp.types.is_canonical_format_kindfeat(types)!: adcp_version and adcp_major_version are plain str and int on AcceptProposalRequest, BuyProductsRequest, ControlMediaBuyRequest, DeclineProposalsRequest, ListProductsRequest, RefineProposalsRequest, RequestProposalsRequest and SyncReportingStatusRequest (drop .root), and every task request and response gains AdcpRequest or AdcpResponse as its first base
feat(types): add the AdcpRequest and AdcpResponse markers, AdcpVersionEnvelope on adcp.types.protocol, and the missing Legacy* media-buy response names; LegacyFormatId now resolves to the generated format reference
END_COMMIT_OVERRIDE