Skip to content

fix(codegen): name the JSON Schema keyword an injected check enforces - #1435

Merged
bokelley merged 1 commit into
mainfrom
fix/schema-keyword-on-generated-refusals
Oct 10, 2026
Merged

bokelley merged 1 commit into
mainfrom
fix/schema-keyword-on-generated-refusals

Conversation

@KonstantinMirin

Copy link
Copy Markdown
Collaborator

The defect

A generated required-group validator enforces a root anyOf/oneOf and then discards which
keyword it was. On main (9.0.0-beta.1, bundle 3.2), core/creative_asset.py:

@model_validator(mode='after')
def _require_schema_required_group(self) -> CreativeAsset:
    for group in (('format_id',), ('format_kind',),):
        if all(name in self.model_fields_set for name in group):
            return self
    raise ValueError('CreativeAsset requires at least one of these field groups: format_id | format_kind')

core/error.json requires every issues[] entry to carry a keyword:

Schema keyword that rejected the payload, drawn from the JSON Schema vocabulary (e.g.
'required', 'type', 'format', 'enum', 'pattern', 'minimum', 'maxLength'). […]
Implementers SHOULD use the validator's native keyword name; do not invent custom values here.

A bare ValueError becomes pydantic's value_error, which names no keyword. keyword is in
issues[].required, so a consumer assembling a conformant envelope has nothing to put in it and
has to drop the issue entirely — the buyer is refused with an empty issues[] and no structured
reason.

A seller hit this on exactly this schema. core/creative-asset.json is a oneOf, so a creative
carrying neither format_id nor format_kind must be refused with keyword oneOf; what reached
the buyer was an envelope with value_error and issues[] empty. Their report is that the
inherited validator refuses first, so a subclass cannot restate the rule with a keyword of its own
and still be the one that fires — they are carrying a mode="before" validator that re-raises the
same rule as PydanticCustomError("oneOf", …) purely to get ahead of it. This change is meant to
make that workaround unnecessary.

Measured, on this repo's main rather than the released artifact

before after
raise PydanticCustomError in types/domains/ 0 43 (anyOf 33, oneOf 10)
bare raise ValueError in types/domains/ 90 47
_require_schema_required_group validators 42 42 (all keyworded)

e.errors()[0]["type"], through the models:

document before after
CreativeAsset with neither format reference value_error oneOf
CreateMediaBuyRequest satisfying no group value_error anyOf
list_creatives Creative with both / neither format reference value_error oneOf

Conforming documents still validate unchanged — PydanticCustomError sets the error type and
keeps the message verbatim, so no rule, no field and no accepted shape moved.

One visible side effect, worth knowing before merge: pydantic prefixes a ValueError's message
with "Value error, " and does not prefix a PydanticCustomError's. So errors()[0]["msg"] goes
from Value error, CreativeAsset requires at least one of… to
CreativeAsset requires at least one of…. The sentence is identical; a consumer matching on the
prefix rather than on the type would see that. Nothing in this repo does — the existing tests
match on the sentence.

The change

Two generator sites in scripts/post_generate_fixes.py:

  • _root_required_groups already reads the combinator in order to find the groups, so the
    keyword was known at exactly one point and was being thrown away. It now returns with the
    groups, and enforce_root_required_groups refuses with that keyword.
  • fix_list_creatives_format_reference_xor names oneOf, the combinator its own docstring
    cites ("The schema models list_creatives creative records as a oneOf").

Plus the shared seam: schema_keyword_raise(keyword, message) renders the raise, and
SCHEMA_KEYWORDS is checked rather than documented, because an invented keyword reaches the wire
looking exactly like a real one and issues[].keyword says not to invent values there.

The keyword mapping, and why

Decided from the source schemas in schemas/cache/3.2/, not from the message text — the English
does not distinguish the two cases, and generate_types.flatten_validation_oneof strips the
combinator before codegen, so the original file is the only place it survives.

  • a root anyOf over required-only arms → anyOf (33 schemas)
  • a root oneOf over required-only arms → oneOf (9 schemas, including creative-asset.json,
    creative-manifest.json, catchment.json, list-account-changes-response.json)
  • the list_creatives record's oneOf whose arms carry opposing not: {required: [...]} →
    oneOf (1 module, 3 emitted validators across the merged and split codegen shapes)

Two mapping choices worth stating:

oneOf on a partially-enforced oneOf. enforce_root_required_groups deliberately enforces
only the "at least one group" half for a oneOf (cardinality is not derivable there — several
schemas give two arms the same required set and distinguish them by properties
discriminators). The keyword is still oneOf: the document failed the root oneOf, and which
of its obligations went unmet is what the message says, not what the keyword says. The
schemaPath example in the same core/error.json reads the same way —
#/properties/packages/items/oneOf/1 names the composition as the rejecting keyword, with the
arm as a position under it.

Not not, for the XOR. not: {required: [...]} is the arm-internal clause that makes the
arms disjoint. The keyword a record was rejected by is the oneOf that contains them.

Regeneration

python scripts/generate_types.py from the vendored schemas/cache/3.2/ — no network, and
--check was clean on main before the change, so the committed tree is byte-for-byte what a
regeneration produces. 43 generated modules, +386/-128 including the 43 timestamp lines; no
class renumbering, no field change. The generator is +139 and the tests +170.

Tests

In tests/test_code_generation.py, beside the existing enforce_root_required_groups tests:

  • test_root_required_group_refusal_names_the_root_combinator[anyOf|oneOf] — fixture schema
    through the real injector, exec'd, asserting errors()[0]["type"] is the keyword and that the
    conforming document still validates.
  • test_every_root_required_group_refusal_names_its_own_schema_combinator — over the committed
    tree, each keyword matched against its own schema's combinator. The keyword is derived, so a
    derivation matching nothing would pass silently: there are floors on the population (≥42), on
    both keywords being present, and on CreativeAsset/CreateMediaBuyRequest by name.
  • test_a_consumer_reads_the_schema_keyword_off_the_refusal — the obligation through the models
    rather than their source.
  • test_schema_keyword_raise_refuses_a_keyword_outside_the_vocabulary — the "do not invent custom
    values" rule, held at generation time.
  • the existing test_list_creatives_merged_model_restores_xor_and_legacy_aliases now also asserts
    the keyword and that no bare ValueError survives in that module.

Each was fire-probed: pinning the combinator to anyOf reddens the oneOf case and the
tree-wide test; reverting schema_keyword_raise to emit ValueError reddens all three fixture
tests.

Locally: make lint, make typecheck-all (mypy, the strict adopter fixtures, pyright, the
type-ignore contract) and make validate-generated all pass, and pytest tests/ has no failure
attributable to this change. Seven tests do fail on this macOS box —
tests/test_reporting_interop_matrix.py (5), tests/test_reporting_interop_corpus.py (1) and one
case in tests/conformance/reporting/test_reporting_production_harness.py — all on
FileNotFoundError: /etc/os-release. They fail identically on main with the same seven node
ids, so they are a Linux-only environment dependency rather than anything here.

What this does not cover

47 bare raise ValueError sites remain in types/domains/, emitted by nine bespoke
post-generation fixes. They are not in this PR because each needs its own reading of the schema it
restores — a per-rule judgment, where the two sites here derive their keyword mechanically from
the construct they were generated for. Some of the 47 also have no honest keyword at all:
change-term minimum result exceeds maximum result and change-term monetary constraint currency must match purchases are cross-field rules the schema states with no keyword, and
issues[].keyword forbids inventing one.

The shapes I can see a keyword for, by rule rather than by count (the grouping is a reading, so
I would rather have it reviewed than assert it):

  • reconciled_billing requires managed_delivery and its siblings in
    fix_reporting_capability_defaults, params requires format_kind in
    fix_beta3_package_request_constraints → dependentRequired
  • the must use https / must not contain credentials checks in
    fix_beta3_secure_url_constraints and fix_trusted_match_runtime_validators → format, or
    pattern where the schema states it that way
  • the uniqueness checks in fix_legacy_purchase_accepted_losses and
    fix_reporting_request_selectors → uniqueItems
  • countries is required when identity_match is true and its pair → if/then
  • reporting scope requires exactly one selector → oneOf
  • tmpx_providers keys must be valid provider_id values → propertyNames

Happy to extend this PR to those, or follow up — say which you prefer. schema_keyword_raise is
the seam either way.

One thing I noticed while measuring

The SDK does not itself project a ValidationError into issues[] anywhere. The handler
shims in src/adcp/server/content_standards.py and src/adcp/server/governance.py do
Error(code="VALIDATION_ERROR", message=str(e)) with no issues[], and
src/adcp/compat/purchase_continuation.py reads issue.keyword off an already-built Issue.
So today every consumer writes its own ValidationError → issues[] projection — which is why
the keyword being absent surfaced as a seller bug rather than an SDK test failure. A shared
projection in the SDK looks like the natural follow-on, and it could not have produced a
conformant keyword before this change. Happy to file that separately if you want it.

Related

#1414 is open against the same validator and is a different defect — that one is about the
branch conditions the generator drops; this one is about the keyword it discards on the
condition it does keep. Neither fix depends on the other; both touch
enforce_root_required_groups, so whichever lands second will need a trivial rebase.

A generated required-group validator enforces a root `anyOf`/`oneOf` and then
discards which keyword it was: it refuses with a bare `ValueError`, which
reaches a consumer as pydantic's `value_error`.

`core/error.json` requires every `issues[]` entry to carry a `keyword` "drawn
from the JSON Schema vocabulary" and says not to invent values there. A
`value_error` names no keyword, so a consumer assembling a conformant envelope
has nothing to put in that required field and has to drop the issue -- the buyer
is then refused with an empty `issues[]` and no structured reason. A seller
reported exactly that for `core/creative-asset.json`: a creative carrying
neither `format_id` nor `format_kind` must be refused with keyword `oneOf`, and
the envelope carried `value_error` with `issues[]` empty.

`_root_required_groups` already reads the combinator in order to find the
groups, so the keyword is known at one point and was being thrown away. It now
returns with the groups, and `schema_keyword_raise` renders
`PydanticCustomError(<keyword>, <message>)`, which sets pydantic's error `type`
to the keyword and keeps the message verbatim -- nothing but the type changes.
33 of the 42 roots declare `anyOf` and 9 declare `oneOf`; each refusal names its
own root's combinator, never a keyword read off the English, which does not
distinguish the two. The partial `oneOf` enforcement still names `oneOf`: the
document failed the root `oneOf`, and which of its obligations went unmet is
what the message says.

`fix_list_creatives_format_reference_xor` names `oneOf` for the same reason --
the record's schema states the rule as a `oneOf` whose arms forbid each other.
`not` is the arm-internal clause that makes them disjoint, not the keyword the
record was rejected by.

`SCHEMA_KEYWORDS` is checked rather than documented because an invented keyword
reaches the wire looking exactly like a real one, and `issues[].keyword` says
not to invent values there.

The regeneration touches 43 generated modules -- 42 required-group validators
plus the list_creatives XOR -- and changes no rule, no message and no field.
47 bare `raise ValueError` sites remain in the tree, each from a bespoke
post-generation fix whose keyword needs its own reading of the schema it
restores; `schema_keyword_raise` is the seam those will go through.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ladon verdict: Approve

Approve — codegen fix with paired regeneration output, no blocking or medium findings.

Checked:

  • scripts/post_generate_fixes.py emits raise PydanticCustomError(<combinator>, msg) in place of raise ValueError(msg) for the root-required-group and list_creatives XOR validators, so errors()[0]["type"] carries the JSON Schema keyword (anyOf/oneOf) required by core/error.json's issues[].keyword. schema_keyword_raise uses repr-escaping with a SCHEMA_KEYWORDS membership gate; the import injector fail-closes; indentation correct at both sites.
  • All 43 domains/** changes are regeneration output paired with the generator edit — no hand-edits, no class renumbering, no field/shape change, so the "generated code is not source" gate is satisfied.
  • The only behavior change (value_error → anyOf/oneOf, dropped "Value error, " prefix) touches runtime error metadata, not public model/export shape, so fix: is the correct semver signal — no breaking-change gate triggered.

No finding ∈ {critical, high}; gated_paths is false; high_risk is false; no no-auto-approve team match; zero medium findings. Rows 1–8 do not fire — falls through to row 9 (approve). Note: review_decision is REVIEW_REQUIRED, but with gated_paths: false row 2 does not apply.

@KonstantinMirin

Copy link
Copy Markdown
Collaborator Author

Checking in on this one — is anything blocking it? It has been open since 10-07 with no change requested, review is green, all 45 checks pass on 69c03fcb1, and it is still MERGEABLE against today's main. Happy to change anything; I would just rather it not sit on my misunderstanding of what it still needs.

Two things I can confirm, since a 45-file diff is the obvious reason to want a second look before merging.

The generated output is still current. Nothing on main has touched the generator or the 3.2 tree since this branched — the only commit in that window to go near either is #1454, which moves schemas/cache/3.1/**:

$ git log --oneline 69c03fcb11..upstream/main -- scripts/post_generate_fixes.py src/adcp/types/domains/ schemas/
7d610e1ae fix(validation): sync signed 3.1.27 macro URL schemas (#1454)

So the before/after counts in the body still hold, and the regeneration does not need redoing against a newer bundle.

The 43 regenerated files carry no renumbering and no shape change. Categorising every changed line under src/adcp/types/domains/:

 43  -raise ValueError
 43  +raise PydanticCustomError
 43  +from pydantic_core import PydanticCustomError
 33  +'anyOf',
 10  +'oneOf',
     + regeneration-timestamp headers and the two comment lines

and filtering the same diff for class, field or import lines other than the injected import returns nothing at all. That is the ClassNameN → ClassNameM churn LADON.md warns about, and there is none of it here: 43 raise sites and their one import, which is exactly the generator edit's reach.

Only 2 of the 45 files are hand-written — scripts/post_generate_fixes.py and tests/test_code_generation.py — so the review surface is the generator edit plus its paired output.

@KonstantinMirin

Copy link
Copy Markdown
Collaborator Author

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

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

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

@bokelley
bokelley merged commit 9e5c42d into main Oct 10, 2026
46 checks passed
@bokelley
bokelley deleted the fix/schema-keyword-on-generated-refusals branch October 10, 2026 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants