Repository navigation
fix(types): generate the open asset unions, and derive the vocabulary they route by - #1501
KonstantinMirin wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean fix for #1486, no blocking or medium findings.
This PR derives FormatAssetUnion/GroupFormatAssetUnion and their routed vocabulary from the generated base slot classes, fixing the case where 7/21 individual and 9/21 group asset_types silently downgraded to UnknownFormatAsset.
What I checked:
- Forward-compat preserved: the
_unknownfallback arm is retained, so genuine novel discriminator values still parse. No fallback arm removed, no discriminator key changed on the wire — not ahighfinding under the forward-compat rule. - Not a breaking change: no public name added/removed; newly-routed typed classes add no required fields beyond the base, so no previously-valid payload now fails to deserialize. Snapshot repoint is the only public-API movement.
fix:prefix is appropriate — semver signal is correct. - Import layering: the numbered
Assets94import sits underTYPE_CHECKINGinaliases.py, which the layering allowlist permits. - Fail-closed derivation: empty scan, duplicate asset_type, and reserved-tag collisions each raise
ImportError. - Vocabulary-drift and corrected pixel_tracker→derived-novel tests are thorough.
Decision rationale: 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 via a (modified) entry on aliases.py, but with zero medium-or-higher findings rows 4/5 do not fire. No no-auto-approve team match. Falls through to row 9 → approve.
|
Moved to draft — please don't spend review time on this shape yet. This derives the arms and the discriminator vocabularies at the consumer, in The version I should have opened does it in codegen: emit each slot's open union, its I'm porting that onto current |
… they route by Closes adcontextprotocol#1486. `Format.assets` routed 14 of the 21 `asset_type` values the pinned schema declares, and the repeatable group's `assets` routed 12 of its 21. The missing ones did not fail: they matched the `_unknown` Tag and validated as `UnknownFormatAsset` / `UnknownGroupAsset`, so a consumer got an untyped object with the asset's fields in `__pydantic_extra__` where the schema promises a typed one -- while the generated class for every missing value sat in `domains/core/format.py`, unused. Format.assets missing card daast_tracker display_tag pixel_tracker published_post vast_tracker zip RepeatableAssetGroup.assets missing those, plus brief catalog The cause was a hand-written vocabulary beside a generated union. `aliases.py` declared both unions, both fallback classes, and two frozensets of 14 and 12 `asset_type` values; `_forward_compat._apply_forward_compat()` then rewrote `Format.assets` and the group's `assets` over the generated annotation at import time. The frozensets are what went stale, and they decide first: the callable `Discriminator` reads the frozenset and returns `_unknown` BEFORE pydantic reaches the arms, so adding arms would have changed routing not at all. `_forward_compat`'s own docstring used `pixel_tracker` as its EXAMPLE of a "novel asset_type"; by then the example was an instance of the defect. So the union is now emitted where its arms are declared. `_OPEN_ASSET_UNIONS` in `scripts/post_generate_fixes.py` writes, per slot, the fallback class, the `asset_type` vocabulary, the callable discriminator and the tagged union into `domains/core/format.py`, and repoints the carrying field. The generator knows each arm's `asset_type` literal because it emitted that literal, so nothing is declared and nothing can go stale: a schema that adds an asset type adds its class AND its tag in the same run. `_forward_compat`'s asset patch is deleted rather than rewired -- with the generated annotation correct there is nothing left to patch -- and `aliases.py` re-exports the four names instead of defining them. The arms keep their genuine asymmetry. Each union is derived from its own base class, because a group slot carries no nested `repeatable_group`. The group container is derived too, by its `item_type` discriminator rather than by name, for the reason `restore_format_asset_numbered_aliases` already derives it: the generator renumbers that class (`Assets94` once, `Assets29` now). `_unknown` is still an arm, so completeness costs none of the openness adcontextprotocol#744 bought: a value the schema does not declare still reaches the fallback. Six derived refusals, each fire-probed on a mutated copy of the generated module rather than on the schema, so none of them can pass vacuously: * an arm the derivation cannot see while the field still carries it, and an arm the field carries that the base does not own -- the membership check requires the field's current element union to resolve to exactly the arm set the alias was built from, and names the symmetric difference; * a duplicate `asset_type` across two arms (a `Tag` must be unique); * a carrying field that is renamed, or that stops being a list; * no class declaring `item_type='repeatable_group'`. Four tests in `tests/test_forward_compat_assets.py` asserted that `pixel_tracker` parses as `UnknownFormatAsset`. The obligation is right and the value was wrong -- they agreed with the stale frozenset rather than with the schema -- so the novel value is now derived from the generated classes: a hardcoded "novel" type can become declared by a schema refresh without the test noticing, which is how those four came to be wrong. A fifth restated the stale 14-value list as its own assertion and now reads the derivation. Added `TestDeclaredAssetTypesAreNotDegraded`, parametrized over the generated classes on both slots, asserting at the PARSE level that each declared type reaches its own class with no untyped extras -- plus one case comparing that population against the two arms of the pinned schema, read at their own depths and selected by `item_type` const, so a generator that stopped emitting a class would shrink the population loudly rather than grade fewer rows in silence. `adcp.types.UnknownFormatAsset` and `UnknownGroupAsset` now resolve to the classes in `domains.core.format` rather than the ones in `aliases`. The public name tracks the class the unions actually route to, as it did before, so `isinstance` against the public name is unaffected; `tests/fixtures/ public_api_snapshot.json` records the repoint and the gained arms.
629c1b5 to
bd4257c
Compare
|
Replaced with the generator-side fix and ready for review. The defect, the schema citation and the measurement are unchanged; the mechanism is different, and it is the one I should have opened first. What changed from the shape you may have glanced at earlier:
Evidence, with the control reported separately because an instrument that refuses every row prints a clean "nothing downgraded" while grading nothing: Arm counting would not have caught this and is worth stating explicitly: the callable One unexpected result: this improves adopter typing rather than merely preserving it. On Gates are matched on both sides, same environment and flags: identical FAILED sets, the pass delta is exactly the 43 tests added, and both sides reconcile against their own |
fix(types): generate the open asset unions, and derive the vocabulary they route by
Closes #1486.
The defect
Format.assetsrouted 14 of the 21asset_typevalues the pinnedschema declares, and
RepeatableAssetGroup.assetsrouted 12 of its 21.The missing values did not fail. They matched the
_unknownTagand validatedas
UnknownFormatAsset/UnknownGroupAsset, so a consumer received an untypedobject with the asset's fields in
__pydantic_extra__where the schema promisesa typed one — while the generated class for every missing value sat in
src/adcp/types/domains/core/format.py, unused.This is the unpaid half of #742. That issue correctly identified that a closed
generated union breaks consumers when the spec grows additively, and #744 fixed
it by replacing the generated union with a hand-written open one carrying a
fallback arm. Openness became permanent and completeness became manual.
_forward_compat's own docstring usedpixel_trackeras its EXAMPLE of a"novel asset_type"; by then the example was an instance of the gap.
Why adding arms would not have fixed it
There were four hand-maintained lists in
aliases.py, not two: the twounions, and two frozensets of 14 and 12
asset_typevalues. The frozensetsdecide first — the callable
Discriminatorreads one and returns_unknownBEFORE pydantic reaches the arms:
So an annotation can show 21 arms while every document for the other seven
still downgrades. Arm counting is not evidence here, and that is demonstrable
rather than asserted: drop one value from the generated vocabulary on this
branch and leave all 21
Tagarms in place, and the two instruments disagree —The acceptance evidence below is at the parse level for that reason.
What changed
The union is emitted where its arms are declared.
_OPEN_ASSET_UNIONSinscripts/post_generate_fixes.pywrites, per slot, intodomains/core/format.py:extra='allow',asset_type: str),asset_typevocabulary, as a frozenset of the literals the arms carry,Tagper arm plus_unknown,and then repoints the carrying field at the alias.
The generator knows each arm's
asset_typeliteral because it emitted thatliteral, so nothing is declared and nothing can go stale: a schema that adds
an asset type adds its class AND its tag in the same run.
src/adcp/types/_forward_compat.py's asset patch is deleted, not rewired —with the generated annotation correct there is nothing left to patch — and
aliases.pyre-exports the four names instead of defining them. That module nolonger imports
aliasesat all; the asset patch was its only reason to.The two slots keep their genuine asymmetry: each union is derived from its own
base class, because a group slot carries no nested
repeatable_group. The groupcontainer is derived too — by its
item_typediscriminator rather than by name,for the reason
restore_format_asset_numbered_aliasesalready derives it: thegenerator renumbers that class (
Assets94once,Assets29now)._unknownis still an arm, so completeness costs none of the openness #744bought: a value the schema does not declare still reaches the fallback.
Acceptance evidence: parse level, with a control
Every declared
asset_typevalidated through a real document on both slots, andthe class it lands in. The control row carries an
asset_typethe schema doesnot declare and must land in the FALLBACK — a probe whose rows all refuse would
print a clean-looking table while grading nothing.
The vocabulary the probe iterates comes from
schemas/cache/<bundle>/core/format.json, not from generated code. Both arms ofproperties.assets.items.oneOfare selected by theiritem_typeconst ratherthan by position, and read at their own depth — the individual arm carries its
asset
oneOfdirectly, the group arm one level further down underproperties.assets.items— because a fixed-depth or positional reader gets oneright and the other empty.
The guards, and what breaks them
Six derived refusals in the generator, each fire-probed on a mutated copy of the
generated module (never on the schema: mutating the authority side reddens any
comparison against that authority and proves only that the plumbing is
connected):
Format.assets and FormatAssetUnion disagree … Carried but not an arm of the union: ['Assets26']… Carried but not an arm of the union: ['Renders']asset_type 'image' is declared by both Assets and Assets26; a discriminated union cannot tag two arms the sameFormat.assets is not declared, so FormatAssetUnion has nothing to openFormat.assets is not a list annotation (…); FormatAssetUnion assumes it isitem_type='repeatable_group'… the asset unions have no group arm and no group containerThe membership check is the one that keeps the derivation from passing
vacuously: the field's current element union must resolve to exactly the arm
set the alias was built from. A schema refresh that adds an asset type moves
both sides together and passes; anything else fails here rather than leaving the
field closed.
Tests
Four tests in
tests/test_forward_compat_assets.pyasserted thatpixel_trackerparses asUnknownFormatAsset. The obligation (a novel valuereaches the fallback) is right; the value was wrong — they agreed with the stale
frozenset rather than with the schema, and the gap's own symptom is what made
them pass. A fifth restated the stale 14-value list as its own assertion. The
novel value is now derived from the generated classes, so a schema refresh
cannot turn the test's novel value into a declared one without the test
noticing.
TestDeclaredAssetTypesAreNotDegradedis new: parametrized over the generatedclasses on both slots (21 + 21), asserting at the parse level that each declared
type reaches its own class with no untyped extras. Dropping one entry from the
generated vocabulary fails exactly one case, naming it:
One further case compares that population against the two arms of the pinned
schema. Without it, a generator that stopped emitting a class would shrink the
parametrization and stay green — grading fewer rows and saying nothing about the
one that vanished.
Public surface
adcp.types.UnknownFormatAssetandUnknownGroupAssetnow resolve to theclasses in
domains.core.formatrather than the ones inaliases. The publicname tracks the class the unions actually route to, exactly as before, so
isinstanceagainst the public name is unaffected.tests/fixtures/public_api_snapshot.jsonrecords the repoint, regenerated withscripts/regenerate_public_api_snapshot.py.Both unions stay syntactic type aliases — the generator writes
FormatAssetUnion = Annotated[…]as source into the generated module — so astatic checker reads them directly. That makes the adopter-facing typing better,
not merely unregressed.
aliases.pybuilt the unions out of_format_asset_class("image"), a runtime call returningtype, so pyright saw avariable rather than a type alias; an adopter writing
list[FormatAssetUnion]gets four errors on main today:
Measured on the same fixture, same environment,
--no-incrementalon both sides:Out of scope
The public
<Type>FormatAsset/<Type>FormatGroupAssetaliases still name 14and 12 of the 21 slot classes — there is no
CardFormatAsset,ZipFormatAsset,DisplayTagFormatAsset,PixelTrackerFormatAsset,VastTrackerFormatAsset,DaastTrackerFormatAssetorPublishedPostFormatAsset. Routing no longerdepends on that list, so the gap is now only a naming one, and adding public
names is an API decision rather than a bug fix. It should probably be generated
from the same derivation rather than extended by hand, or it drifts the way the
vocabulary did. Happy to follow up separately.