fix: approximate discriminator array item retyping - #93
Open
vishkaty wants to merge 4 commits into
Open
Conversation
…uite The HAVE_SDK import gate at the top of test_codegen_pipeline.py still imported Description/Totals from ucp_sdk.models.schemas.shopping.types, paths that moved to ucp_sdk.models.schemas.common.types when Universal-Commerce-Protocol#87 (the 2026-08-25 UCP release regen) restructured the schema tree. The stale paths raise ModuleNotFoundError, which the surrounding try/except catches and sets HAVE_SDK = False, so every test gated on HAVE_SDK skips instead of running (unittest reports a skip as OK, so the suite reads green while ~37% of it never executes). Fix every stale shopping.types.* reference in the file: description, totals (+ its request variants), signals (+ its request variants), and error_response moved to common.types under the same class names, so those tests now run and pass unmodified. Two targets did not survive the schema restructuring at all -- card_payment_instrument.Constraints (a uniqueItems brands field) and merchant_fulfillment_config's nested additionalProperties:false object (now business_fulfillment_config, reshaped) -- so their four tests become documented unittest.skip with the reason recorded in the schema, not silently deleted. Before: 89 tests, 33 skipped (26 on the HAVE_SDK gate, 7 on 'executing the module needs pydantic', which also reads HAVE_SDK). After: 89 tests, 4 skipped, all four with a stated schema-shape reason. No production code changes; test-only.
fulfillment_method.json's destinations property is typed via
items.$ref to fulfillment_destination.json (bare type: str, id: str),
but two if/then allOf branches retype it per the method's own type:
when type is shipping, destinations should really be
shipping_destination.json items (postal address fields, type const
shipping_address); when type is pickup, destinations should really be
location_destination.json items (type const business_location). The
committed FulfillmentMethod model ignores both branches entirely, so
a shipping method can list a destination typed business_location (or
vice versa) and it validates in violation of the schema.
This is a third if/then shape neither find_conditional_required
(adds required fields) nor find_conditional_bounds (narrows numeric
ranges) handles: a discriminator retyping an ARRAY PROPERTY's items
to a different referenced schema file entirely. No scanner in
postprocess_models.py ever looked for it.
Adds, mirroring the existing conditional-rule injector tests:
- ConditionalArrayRetypingInjectorTest: injector-level unit tests
against synthetic fixtures mirroring fulfillment_method.json's
exact shape (method/destination/shipping_destination/
location_destination), covering the schema scan (both branches
read, a branch whose ref matches the base is not a retype, a
stripped request-variant field is inapplicable not malformed, an
unresolvable $ref warns), injection idempotency, and the injected
validator's runtime behavior against synthetic Method/Destination
classes.
- FulfillmentMethodDestinationRetypingSemanticTest: exercises the
real committed FulfillmentMethod and FulfillmentDestination
models. Includes negative controls for an open-vocabulary type
(no rule applies) and no destinations at all (unconstrained).
RED (test-only; find_conditional_array_retyping and
inject_conditional_array_retyping do not exist on this commit -- the
generator fix that adds them, developed alongside these tests per
the exact schema shape confirmed against the pinned 2026-08-25 UCP
spec, follows in the next commit): 101 tests, 2 failures + 6 errors,
4 documented skips (unchanged from the root-cause-0 commit).
Adds an eighth constraint family: a discriminator retyping an array PROPERTY's items to a schema file different from the property's own base $ref, a shape neither find_conditional_required (adds required fields) nor find_conditional_bounds (narrows numeric ranges) handles. fulfillment_method.json's destinations stays typed to the base FulfillmentDestination (bare type: str, id: str) regardless of type, even though a shipping method's destinations are really ShippingDestination (postal address fields, type const shipping_address) and a pickup method's are really LocationDestination (type const business_location). Pydantic has no clean way to retype a field's item type from a source-text splice the way the other families rewrite an annotation or add a validator against the field's own declared type, so this is enforced with a runtime check instead of a static type change: find_conditional_array_retyping scans for the shape (single-key const/enum discriminator naming a property on the enclosing object, then narrowing exactly one array property, also on the enclosing object, to a different items.$ref) and resolves the retyped file's own (root-level, post-merge) required keys and const-pinned properties via _resolve_referenced_shape. inject_conditional_array_retyping then injects a model_validator that, for each item in the array field when the discriminator matches, checks the item carries every required key (via model_fields_set | model_extra, same key-counting idiom used throughout this module) and that every const-pinned field matches its expected value. This is deliberately an approximation, not a full re-derivation of the retyped type: only the referenced schema's own required/const fields are checked, not fields it in turn allOf-references (shipping_ destination.json's own allOf-ref to postal_address.json is not inspected). For fulfillment_method.json the meaningful check is primarily the type const pin -- id/type were already required by the base FulfillmentDestination, so checking them again is redundant here, but the mechanism is general and both required-keys and const-pins are checked mechanically from whatever the referenced schema declares, not hardcoded to this one case. A request variant that omits the retyped field entirely (fulfillment_ method_create_request.json never carries destinations at all -- ucp_request: "omit") makes the rule inapplicable rather than malformed, silently, mirroring find_conditional_bounds's existing "stripped field" handling. Generator-level change only (postprocess_models.py); no generated model files touched in this commit. Regeneration follows in a separate commit, which is what turns the two still-red semantic tests green (all injector-level unit tests from the RED commit -- which exercise find_conditional_array_retyping/inject_conditional_array_ retyping directly against synthetic fixtures mirroring the real schema shape, not the committed models -- already pass). 101 tests, 2 failures (FulfillmentMethodDestinationRetypingSemanticTest), 4 documented skips.
Regenerates via ./generate_models.sh 2026-08-25 (the same command the model-drift CI job runs) to pick up the postprocessing fix in the prior commit. One file changes: FulfillmentMethod gains an _enforce_conditional_item_retyping validator covering both the shipping and pickup destination retyping rules. Verified: - Full suite: 101 tests, 0 failures, 4 documented skips (both new semantic tests from the RED commit now pass). - Double-regen: ran generate_models.sh 2026-08-25 twice; diff -rq between both outputs (excluding __pycache__) is empty. - Kill-test: reverted postprocess_models.py to its pre-fix state, regenerated, reinstalled -- the same 2 failures + 6 errors from the RED commit reappeared verbatim. Restored the fix and regenerated again to confirm the suite returns to green. - pre-commit run on the changed file: clean. Not committed: README.md, which ruff format also reformats as a pre-existing docstring-code-block spacing drift in main, unrelated to this fix (see the equivalent note on the jwk-conditional-rules branch).
damaz91
added
the
status:needs-triage
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
3 participants
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.
Fixes #92
What this changes
Adds an eighth constraint family to postprocess_models.py:
find_conditional_array_retyping and inject_conditional_array_retyping,
for a discriminator that retypes an array property items to a schema
file different from the property own base ref, described in the linked
issue.
find_conditional_array_retyping scans for the shape mechanically: a
single key const or enum discriminator naming a property on the
enclosing object, and a then that narrows exactly one array property,
also on the enclosing object, to a different items ref. For each match
it resolves the retyped file own required keys and const pinned
properties, as the pipeline preprocessing leaves them after merging
that file own inline allOf branches, through a new helper,
_resolve_referenced_shape.
inject_conditional_array_retyping then injects a model_validator that,
for each item in the array field when the discriminator matches, checks
the item carries every required key (through model_fields_set union
model_extra, the same key counting idiom used throughout this module)
and that every const pinned field matches its expected value.
This is deliberately an approximation rather than a full re-derivation
of the retyped type. Only the referenced schema own required and const
fields are checked, not fields it in turn allOf references (for example
shipping_destination.json own allOf ref to postal_address.json is not
inspected). For fulfillment_method.json the meaningful check is
primarily the type const pin, id and type were already required by the
base FulfillmentDestination, so checking them again there is redundant,
but the mechanism itself is general and both required keys and const
pins are read mechanically from whatever the referenced schema
declares, nothing is hardcoded to this one case.
A request variant that omits the retyped field entirely
(fulfillment_method_create_request.json never carries destinations at
all) makes the rule inapplicable rather than malformed, silently,
mirroring find_conditional_bounds existing handling of a stripped
field.
Test plan
against synthetic fixtures mirroring fulfillment_method.json exact
shape, plus FulfillmentMethodDestinationRetypingSemanticTest against
the real committed models, including negative controls for an open
vocabulary method type and for a method with no destinations at all.
generator fix. One file changes, fulfillment_method.py.
pycache. No difference.
regenerated, reinstalled. The same failures reappeared exactly.
Restored the fix and regenerated again to confirm green.
Not included
README.md, which ruff format also reformats when generate_models.sh
runs, unrelated to this fix and present before this PR. Left untouched
to keep the diff scoped.