Repository navigation
feat(codegen)!: compile what a schema root states, and route every arm it declares - #1522
Open
KonstantinMirin wants to merge 3 commits into
Open
KonstantinMirin wants to merge 3 commits into
KonstantinMirin wants to merge 3 commits into
Conversation
KonstantinMirin
force-pushed
the
integration/all-of-ours
branch
2 times, most recently
from
October 17, 2026 01:48
f341c4a to
42e4cd0
Compare
…m it declares
Carries the canonical-format and codegen work developed on the sales-agent
integration branch onto main as one change, plus four smaller fixes in the same
surface. The generated tree here is REGENERATED from the generator in this
commit, not copied from the branch it came from.
## Why this is one change rather than a series
The generator and the types that consume it were co-developed. Grouping the
source commits by subject gives eight themes, but they are interleaved rather
than sequential: of 85 commits carrying reviewable content, the constraint
reader/compiler theme spans positions 0..80 and the split-root theme spans
6..82, in 60 contiguous runs. Thirty-three files appear in more than one theme,
three of them (`scripts/post_generate_fixes.py`, `types/_forward_compat.py`,
`types/canonical_creative.py`) in five. Any thematic split would reorder commits
that depend on each other, so the themes describe the work without being able to
schedule it.
The review burden is far smaller than the file count suggests, because most of
it is the generator:
hand-written src/ 60 files
scripts/ 13 files
generated output 250 files (follows from the generator; see below)
## What it does
Compile a schema root's constraint content instead of a third of it: one reader
for what a root states, a vocabulary the compiled constraints are written in, and
a compiler that names an arm and plans a branch class. Split roots then get a
shared public base (`<Root>Shape`), one dispatch hook mixed in rather than a copy
per root, and construction dispatched as well as validation, so a public name
resolves to the branch a document satisfies.
Around that: `error.field` carries the JSONPath-lite the spec requires, taken
from a located issue and resolved against the document the peer actually sent,
never from the document root; recovery text is derived from the code the pin
registers, deleting seventy literals that restated it; narrowing stops giving up
on the inputs that most need it; and pin gaps and compatibility annotations are
declared in the schema at the field rather than patched onto classes.
## The four smaller fixes
- **Format references compare every parameter they declare.** `core/format-id.json`
declares four parameters beyond the `(agent_url, id)` identity, and three
helpers each named three of them in a hand-written tuple, so `pixel_ratio` was
never compared: a product requiring 2x accepted a creative claiming no density,
and accepted one claiming a conflicting density, while the legacy-size upgrade
discarded a density it was handed. The set is now derived from `model_fields`.
`pixel_ratio` keeps one deliberate rule, because the schema gives it one —
omitting it selects the backward-compatible 1x variant, so an absent density
reads as 1x rather than as unspecified.
- **Open asset unions are generated, and route by a derived vocabulary.** The
callable discriminator read a hand-written frozenset of 14 individual and 12
group types beside generated unions of 21 and 21, and it read that frozenset
before pydantic reached the arms — so seven individual and nine group asset
types the pinned schema declares parsed as the unknown fallback with their
generated classes unused. The vocabulary now comes from the arms' own literals.
- **A declaration's v1 refs are read in either spelling.** The generated
`ProductFormatDeclaration` arms declare `v1_format_ref`; `canonical_creative.Format`
exposes `legacy_format_refs` over private state. Five functions read the second
and raised `AttributeError` on the first, which made the v2-to-v1 product
projection unusable against the generated types, and `client.py` read it behind
a `getattr(..., ())` default that answered "asserts no v1 refs" with no error.
All six reads go through one accessor that refuses a type declaring neither.
- **Four `Legacy*` names are re-exported from `adcp.types.legacy`.**
`LegacyProposalTotalBudgetGuidance` and the three
`LegacyUpdateMediaBuy{Success,Error,Submitted}Response` names were reachable
only from `adcp.types.aliases`, so an adopter following the migration module's
own docstring could not reach them.
Note the first two are the same defect: a hand-written list beside a generated
truth, where the hand-written one was short. They were found independently.
## Two of these tests were reconciled, not the refusal narrowed
`test_canonical_boundary_serialization.py::...[parent]` and
`test_canonical_creatives_rc3.py::test_primary_dump_cannot_emit_legacy_identity_from_private_state_or_extras`
are from the branch this work came from, and both failed here. Each builds its
fixture with `model_construct` -- deliberately, since the subject is what a DUMP
can emit and the fixture is a document validation refuses by design -- and then
reached its last assertion through `Envelope(catalog=...)` / `Container(declaration=...)`,
which validates that fixture and is refused by the wrap validator on the
canonical boundary base.
Measured before changing anything, and measured twice independently by two
people: the refusal fires on that exact document at TOP LEVEL
(`Format.model_validate`, `Catalog.model_validate`) as well as nested. That is a
fact about production behaviour rather than a preference about what the test
should say, which is what makes editing our own test reviewable -- a reader who
doubts it can re-run those two calls. Since the refusal is correct at top level
it is not over-firing on re-validation, so narrowing it is out; and the document
is never legal in the first place, because `model_construct` bypasses validation
entirely. The expectation was always wrong and the older serialization path
simply never reached the check.
`test_canonical_creatives_rc3` was checked separately rather than assumed to
share the cause. It does, and more plainly: that test's own first assertion is
`pytest.raises(ValidationError, match="legacy creative identity")` on a normal
construction, so it proves validation refuses this shape and then asks
validation to accept it.
**No production code was changed on either side** -- not the refusal, not
`adcontextprotocol#1521`'s serialization path. Only the two tests' plumbing: each now builds its
containing model with `model_construct`, like the four sibling parameters that
already passed. `[parent]` is kept rather than deleted, because it is the only
case covering "the serializer does not add a fourth pass over the full tree when
nested", and its assertions are untouched -- `roots == 3` still fails if the
strip stops running once per root, verified by breaking it to 99 and watching
all five parameters fail.
## Not included, deliberately
- **No spec-version move.** `ADCP_VERSION` stays at main's 3.2.1 and no file
under `schemas/` or `_compliance/` is touched. The branch this came from also
carried a 3.2.3 adoption; that overlaps adcontextprotocol#1479 (signed 3.2.4) on 1230 files and
does not cherry-pick onto main, so it is left to that PR.
- **No interpreter or lockfile policy.** The source branch pinned
`.python-version` to 3.12 and committed a 3240-line `uv.lock`. Main has
neither and its CI tests 3.10/3.11/3.13, so carrying an interpreter-pin
proposal inside this change would invite rejection for a reason unrelated to
the work. Excluded as a decision, not an oversight; it remains available as a
separate proposal.
## Verification
- `generate_versioned_stubs.py`, `generate_types.py`, `generate_versioned_bases.py`
all pass `--check` after regeneration, so the 250 generated files follow from
the generator in this commit rather than from the tree they were copied out of.
- `ruff check src/` clean; `mypy src/adcp/` clean over 1538 modules.
- Formatting: this branch leaves 7 files unformatted, all of which are
unformatted on main too; it formats the two of main's nine that it touches.
- Every public symbol and model shape a downstream consumer resolves is unchanged
against main: 323 of 323 imported names resolve, and all 164 pydantic models
carry an identical (field, required) set.
BREAKING CHANGE: `adcp.types.response_dispatch` is removed, and with it
`ResponseArmDispatchMixin`. A per-root copy of the dispatch hook is replaced by
one mixed-in hook, `adcp.types._branch_dispatch.BranchDispatchMixin`, which the
generated response classes now inherit directly -- so a consumer importing
`from adcp.types.response_dispatch import ResponseArmDispatchMixin` must stop;
nothing needs to be imported in its place, because the generated classes carry
the behaviour themselves. `adcp.types._ergonomic` is removed in the same way,
with the one coercion that was live declared at its field instead.
KonstantinMirin
force-pushed
the
integration/all-of-ours
branch
from
October 17, 2026 01:52
42e4cd0 to
7de831d
Compare
… not one arm
``_DeliveryCreativeManifest`` refined
``CreativeManifestCanonicalFormatManifest`` -- oneOf[1] of the manifest root --
while the public ``CreativeManifest`` refines the root itself. Both are
reached through the SAME ``$ref`` to ``core/creative-manifest.json``:
``core/creative-variant.json`` for the input manifest,
``core/get-creative-delivery-response.json`` for the readback one. One schema
node refined twice, so the two owe a document one verdict, and they gave two.
The root's ``required`` is ``['assets']``; oneOf[1] additionally requires
``format_kind``. The disagreement showed up in two places, neither of them the
format-kind vocabulary:
* statically, ``format_kind`` read ``str`` on the readback manifest and
``str | None`` on the input one. That is the ``mypy --strict`` error
``tests/type_checks/creative_asset_binding.py:82`` carried, and the only
reason the ``Test Python 3.12`` leg was red.
* in the refusal of ``{"assets": {}}``: the root-bound class names the root's
combinator (``type='oneOf'``), the arm-bound one reported a ``missing``
field and never reached the combinator. ``core/error.json`` requires an
issue to carry a JSON Schema keyword and ``missing`` is not one.
Binding the root relaxes nothing at runtime. The root's ``oneOf`` is enforced
by the root's own conjunct validator, not by ``BranchDispatchMixin``'s branch
union -- the mixin hands the union only to the class that DECLARES
``_branch_models``, and a refinement declared in ``canonical_creative`` does
not. Measured against the input manifest on all three documents: canonical arm
accepted by both, neither-arm refused ``oneOf`` by both, both-arms refused
``value_error`` by both.
Note what the previous comments on this binding claimed and what is actually
true. They said binding the root "would make it validate as the root's UNION";
it does not, for the reason above. The real cost is narrower: a root-bound
instance is an instance of the root and of NEITHER arm, so revalidating the
INSTANCE through the generated root refuses it as ``model_type`` where it used
to be taken. That is the obligation adcontextprotocol#1241 asked for, and it now holds on all
four (response model x input class) pairs instead of three -- so
``test_delivery_manifest_cannot_bypass_strict_input``'s conditional collapsed
in the strengthening direction. Its own docstring already called the accepted
case "the instance-revalidation bypass that the subclassing opened".
Two new tests grade the fix rather than leaving it asserted only by mypy:
``test_readback_manifest_grades_a_document_exactly_as_the_input_manifest``
(behavioural, three documents) and
``test_readback_manifest_refines_the_manifest_root_not_one_arm``
(structural). Re-pointing the class at the arm turns 3 of those 4 cases red --
verified by doing it.
The shrink-only ceiling in ``tests/test_type_check_fixtures.py`` held exactly
this one error and is removed, not lowered: the residual is fixed, and the CI
step it mirrors tolerates nothing either, so a ceiling could only hide
something that reddens there.
…tion
``test_core_reconciliation_refuses_a_managed_delivery_ledger`` pinned
``destination_ref`` onto a HEALTHY period and expected
``MANAGED_DELIVERY_NOT_ENABLED``. It got ``STATUS_READ_FAILED``, because
``core/reporting-obligation.json``'s ``allOf[4]`` fires on the CONJUNCTION
``if: {properties: {health: {enum: [healthy, complete]}}, required: [health,
destination_ref]}`` and then requires ``materialization_count >= 1``,
``successful_materialization_count >= 1`` and ``resource_retained_until``. The
seller's payload was invalid against the pin, ``load_reporting_ledger``
refused it first, and the tier check under test never ran -- so the assertion
graded the read, not the tier.
Supplying those counts was tried and reverted, correctly: a Core ledger HAS no
materializations, so a fixture reporting one asserts something false about the
very tier under test. The pin says so in ``destination_ref``'s own
description -- "Core (API-delivered) obligations omit every destination and
materialization field".
The schema-valid, Core-legal state is to falsify ``allOf[4]``'s HEALTH
conjunct instead. The obligation is now ``delayed``: the source is left
unscripted, the obligation commits when the period closes, no revision exists,
and the read boundary sits after ``expected_at`` but inside the declared
automated recovery window -- step 8 of this module's own lifecycle narrative.
Nothing is fabricated. No materialization field is required or supplied,
``allOf[8]`` wants ``issues`` non-empty and an overdue period already carries
``REPORT_OVERDUE``, and ``destination_ref`` is left as the ONE thing in the
payload the pin forbids for Core.
Validated against the PIN rather than against this branch's compiler --
``jsonschema`` draft-07 over ``schemas/cache/3.2/`` with the whole cache as a
``referencing`` registry:
delayed + destination_ref PIN-VALID <- the new fixture
healthy + destination_ref PIN-INVALID <- the old fixture, failing on
exactly allOf[4]'s three
required materialization keys
delayed, no destination_ref PIN-VALID <- control
Two guards against a fixture that passes for the wrong reason: an assertion
that the read boundary is inside the recovery window, so the obligation cannot
drift to ``action_required`` unnoticed, and a reconcile of the SAME ledger
through the unmodified handler, which must succeed and report
``destination_ref=None`` with ``health='delayed'``. Without the second, a
fixture that broke the read for any unrelated reason would still raise and
still be graded green. Removing the ``destination_ref`` injection now fails
with DID NOT RAISE -- verified by doing it.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1517
Closes #1486
Carries the canonical-format and codegen work developed on the sales-agent
integration branch onto main as one change, plus four smaller fixes in the same
surface. The generated tree here is REGENERATED from the generator in this
commit, not copied from the branch it came from.
Why this is one change rather than a series
The generator and the types that consume it were co-developed. Grouping the
source commits by subject gives eight themes, but they are interleaved rather
than sequential: of 85 commits carrying reviewable content, the constraint
reader/compiler theme spans positions 0..80 and the split-root theme spans
6..82, in 60 contiguous runs. Thirty-three files appear in more than one theme,
three of them (
scripts/post_generate_fixes.py,types/_forward_compat.py,types/canonical_creative.py) in five. Any thematic split would reorder commitsthat depend on each other, so the themes describe the work without being able to
schedule it.
The review burden is far smaller than the file count suggests, because most of
it is the generator:
What it does
Compile a schema root's constraint content instead of a third of it: one reader
for what a root states, a vocabulary the compiled constraints are written in, and
a compiler that names an arm and plans a branch class. Split roots then get a
shared public base (
<Root>Shape), one dispatch hook mixed in rather than a copyper root, and construction dispatched as well as validation, so a public name
resolves to the branch a document satisfies.
Around that:
error.fieldcarries the JSONPath-lite the spec requires, takenfrom a located issue and resolved against the document the peer actually sent,
never from the document root; recovery text is derived from the code the pin
registers, deleting seventy literals that restated it; narrowing stops giving up
on the inputs that most need it; and pin gaps and compatibility annotations are
declared in the schema at the field rather than patched onto classes.
The four smaller fixes
Format references compare every parameter they declare.
core/format-id.jsondeclares four parameters beyond the
(agent_url, id)identity, and threehelpers each named three of them in a hand-written tuple, so
pixel_ratiowasnever compared: a product requiring 2x accepted a creative claiming no density,
and accepted one claiming a conflicting density, while the legacy-size upgrade
discarded a density it was handed. The set is now derived from
model_fields.pixel_ratiokeeps one deliberate rule, because the schema gives it one —omitting it selects the backward-compatible 1x variant, so an absent density
reads as 1x rather than as unspecified.
Open asset unions are generated, and route by a derived vocabulary. The
callable discriminator read a hand-written frozenset of 14 individual and 12
group types beside generated unions of 21 and 21, and it read that frozenset
before pydantic reached the arms — so seven individual and nine group asset
types the pinned schema declares parsed as the unknown fallback with their
generated classes unused. The vocabulary now comes from the arms' own literals.
A declaration's v1 refs are read in either spelling. The generated
ProductFormatDeclarationarms declarev1_format_ref;canonical_creative.Formatexposes
legacy_format_refsover private state. Five functions read the secondand raised
AttributeErroron the first, which made the v2-to-v1 productprojection unusable against the generated types, and
client.pyread it behinda
getattr(..., ())default that answered "asserts no v1 refs" with no error.All six reads go through one accessor that refuses a type declaring neither.
Four
Legacy*names are re-exported fromadcp.types.legacy.LegacyProposalTotalBudgetGuidanceand the threeLegacyUpdateMediaBuy{Success,Error,Submitted}Responsenames were reachableonly from
adcp.types.aliases, so an adopter following the migration module'sown docstring could not reach them.
Note the first two are the same defect: a hand-written list beside a generated
truth, where the hand-written one was short. They were found independently.
Two of these tests were reconciled, not the refusal narrowed
test_canonical_boundary_serialization.py::...[parent]andtest_canonical_creatives_rc3.py::test_primary_dump_cannot_emit_legacy_identity_from_private_state_or_extrasare from the branch this work came from, and both failed here. Each builds its
fixture with
model_construct-- deliberately, since the subject is what a DUMPcan emit and the fixture is a document validation refuses by design -- and then
reached its last assertion through
Envelope(catalog=...)/Container(declaration=...),which validates that fixture and is refused by the wrap validator on the
canonical boundary base.
Measured before changing anything, and measured twice independently by two
people: the refusal fires on that exact document at TOP LEVEL
(
Format.model_validate,Catalog.model_validate) as well as nested. That is afact about production behaviour rather than a preference about what the test
should say, which is what makes editing our own test reviewable -- a reader who
doubts it can re-run those two calls. Since the refusal is correct at top level
it is not over-firing on re-validation, so narrowing it is out; and the document
is never legal in the first place, because
model_constructbypasses validationentirely. The expectation was always wrong and the older serialization path
simply never reached the check.
test_canonical_creatives_rc3was checked separately rather than assumed toshare the cause. It does, and more plainly: that test's own first assertion is
pytest.raises(ValidationError, match="legacy creative identity")on a normalconstruction, so it proves validation refuses this shape and then asks
validation to accept it.
No production code was changed on either side -- not the refusal, not
#1521's serialization path. Only the two tests' plumbing: each now builds itscontaining model with
model_construct, like the four sibling parameters thatalready passed.
[parent]is kept rather than deleted, because it is the onlycase covering "the serializer does not add a fourth pass over the full tree when
nested", and its assertions are untouched --
roots == 3still fails if thestrip stops running once per root, verified by breaking it to 99 and watching
all five parameters fail.
Not included, deliberately
ADCP_VERSIONstays at main's 3.2.1 and no fileunder
schemas/or_compliance/is touched. The branch this came from alsocarried a 3.2.3 adoption; that overlaps fix(protocol): adopt signed AdCP 3.2.4 on SDK 9 beta #1479 (signed 3.2.4) on 1230 files and
does not cherry-pick onto main, so it is left to that PR.
.python-versionto 3.12 and committed a 3240-lineuv.lock. Main hasneither and its CI tests 3.10/3.11/3.13, so carrying an interpreter-pin
proposal inside this change would invite rejection for a reason unrelated to
the work. Excluded as a decision, not an oversight; it remains available as a
separate proposal.
Verification
generate_versioned_stubs.py,generate_types.py,generate_versioned_bases.pyall pass
--checkafter regeneration, so the 250 generated files follow fromthe generator in this commit rather than from the tree they were copied out of.
ruff check src/clean;mypy src/adcp/clean over 1538 modules.unformatted on main too; it formats the two of main's nine that it touches.
against main: 323 of 323 imported names resolve, and all 164 pydantic models
carry an identical (field, required) set.
Verification
The generated tree is regenerated from the generator in this commit rather than copied.
Regeneration is what surfaced that
types/response_dispatch.pyandtypes/_ergonomic.pyare correctly deleted — main's generated tree still imported the former.
Known failing checks, with reasons
Two checks fail deliberately rather than being papered over. Both are filed.
Test Python 3.12— one[assert-type]error intests/type_checks/creative_asset_binding.py.The readback manifest type binds one arm of a split root, where the pin
(
core/creative-manifest.json,required: ['assets']) makesformat_kindoptional on theroot. Binding the root instead fixes this error and breaks
test_delivery_manifest_cannot_bypass_strict_input— measured in both directions. One type iscarrying two incompatible obligations, tolerant readback and strict input; the fix is two
types, filed separately rather than buried in a change this size.
Postgres conformance (Postgres 16, core)— one test,test_core_reconciliation_refuses_a_managed_delivery_ledger. It injectsdestination_refintoa healthy period, which
core/reporting-obligation.json'sallOf/4/then/requiredthen requiresmaterialization counts alongside, so the read refuses with
STATUS_READ_FAILEDbefore the tiercheck runs. Supplying those counts is not available either: a Core ledger has none, and
fabricating them makes the fixture assert something false. The fixture needs a redesign, not a
patch. The
Postgres conformance (Postgres 16)row is the aggregator and mirrors this one.Three other failures in this lane were real and are fixed: the
ReportingObligationhelperdeclared
health="healthy", production_status="published"while passingrevisions=(), sorevision_countderived to 0 against a schema minimum of 1. Verified against the pin withjsonschemaindependently of this branch's compiler. The core lane is 2766 passed, up from 2763.BREAKING CHANGE:
adcp.types.response_dispatchis removed, and with itResponseArmDispatchMixin. The generated response classes now inheritBranchDispatchMixindirectly, so a consumer needs no replacement import.