Skip to content

fix: approximate discriminator array item retyping - #93

Merged
carolinerg1 merged 3 commits into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/fulfillment-method-destination-retyping
Sep 1, 2026
Merged

fix: approximate discriminator array item retyping#93
carolinerg1 merged 3 commits into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/fulfillment-method-destination-retyping

Conversation

@vishkaty

@vishkaty vishkaty commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #92

What this changes

Adds a ninth 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

  • Added ConditionalArrayRetypingInjectorTest, injector level unit tests
    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.
  • Full suite: 132 tests, 0 failures, 4 documented skips.
  • Regenerated against release/2026-08-25 as a separate commit from the
    generator fix. One file changes, fulfillment_method.py.
  • Regenerated a second time and diffed the two outputs, excluding
    pycache. No difference.
  • Kill test: reverted postprocess_models.py to its pre fix state,
    regenerated, reinstalled. The same failures reappeared exactly.
    Restored the fix and regenerated again to confirm green.
  • pre-commit run on the changed file: clean.

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.

@damaz91 damaz91 added the status:needs-triage Signal that the PR is ready for human triage label Aug 28, 2026
@carolinerg1 carolinerg1 added status:under-review and removed status:needs-triage Signal that the PR is ready for human triage labels Aug 31, 2026
@vishkaty
vishkaty force-pushed the fix/fulfillment-method-destination-retyping branch from eb5624c to fd438bc Compare September 1, 2026 15:10
@carolinerg1

Copy link
Copy Markdown

Hi @vishkaty, similar comment to PR #91 - thank you for submitting these fixes! If you could please take a look at the branch conflict, really appreciate it.

@vishkaty
vishkaty force-pushed the fix/fulfillment-method-destination-retyping branch from fd438bc to fe4bf05 Compare September 1, 2026 15:44
@vishkaty

vishkaty commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main and resolved the test file conflict by keeping the #89 semantic test classes alongside the new one. The full suite and the model drift regeneration pass locally on the new head.

@carolinerg1

Copy link
Copy Markdown

Hi @vishkaty, looks like there's still a conflict in postprocess_models.py. Could you please take a look.

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): 132 tests, 2 failures + 6 errors,
4 documented skips (unchanged from main).
Adds a ninth 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).

132 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: 132 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).
@vishkaty
vishkaty force-pushed the fix/fulfillment-method-destination-retyping branch from fe4bf05 to df2d341 Compare September 1, 2026 18:08
@vishkaty

vishkaty commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main now that #91 has landed. The only conflict was the return in main(), where both families needed keeping, so it now covers all nine patch passes including maxProperties from #91, and the module docstring count is bumped to nine. Full suite and the model drift regeneration pass locally on the new head.

@carolinerg1
carolinerg1 merged commit 3e1aace into Universal-Commerce-Protocol:main Sep 1, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: fulfillment_method.json destination retyping per method type is dropped

5 participants