fix: restore JWK conditional rules and unit.json scale pin - #89
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.
profile.json's jwk_public_key def carries five if/then rules (two
conditional-required: an EC key needs crv/x/y, an OKP key needs
crv/x; three conditional const-pins matching curve to algorithm:
P-256/ES256, P-384/ES384, Ed25519/EdDSA), and unit.json pins scale to
0 when unit is C62. All six are dropped by the current generator, so
the committed JwkPublicKey and Unit models validate payloads the spec
rejects.
Two independent scanner bugs cause this, isolated here at both the
injector level (schema-scan unit tests against synthetic fixtures)
and the semantic level (against the real committed models):
(a) find_conditional_required only looks at a branch's own
properties; an allOf branch carrying just {if, then} with no
properties of its own (every JWK rule) is invisible, with no
warning. find_conditional_bounds already threads the enclosing
object's properties into such branches; find_conditional_required
never picked up that fix.
(b) find_conditional_bounds only recognizes the four numeric bound
keywords (minimum/maximum/exclusiveMinimum/exclusiveMaximum) in
a then.properties.<field> constraint. unit.json's scale pin and
all three JWK algorithm pins use const, which is rejected as an
unsupported shape and dropped (with a warning, unlike (a)).
(c) Shared: in both scanners, a rule's own documentation title
(JWK gives each branch a human-readable title, e.g. "EC keys
carry crv, x, y") overwrites current_class_name via the same
code path used for real class-defining titles, misattributing
the rule to a nonexistent class instead of JwkPublicKey.
Kill-rate note: test_schema_scan_skips_else_branches and
test_schema_scan_skips_rules_whose_fields_were_stripped (both
pre-existing) stay green, confirming the new scope-threading and
const-recognition do not loosen the existing else-branch and
stripped-field guards.
RED: 109 tests, 9 failures + 1 error, 4 documented skips (unchanged
from the prior commit). Generator fix and regen follow in separate
commits per repo convention.
Three bugs in the postprocessing scanners were dropping all five of
profile.json's jwk_public_key if/then rules and unit.json's C62 scale
pin, letting the generated JwkPublicKey and Unit models validate
payloads the spec rejects:
(a) find_conditional_required only looked at a branch's own
properties. An allOf branch carrying just {if, then} with no
properties of its own (every JWK rule is exactly this shape) was
invisible, silently, with no warning. find_conditional_bounds
already threads the enclosing object's properties into such
branches via an enclosing_properties parameter;
find_conditional_required now does the same.
(b) find_conditional_bounds only recognized the four numeric bound
keywords (minimum/maximum/exclusiveMinimum/exclusiveMaximum) in a
then.properties.<field> constraint, so a bare {"const": ...}
constraint fell outside _BOUND_KEYWORDS and the whole rule
returned None (unit.json's scale pin, and all three JWK
curve/algorithm pairings, are const-shaped). _BOUND_KEYWORDS gains
a "const" entry mapped to not-equal, reusing the existing
"value <op> limit -> violation" template unchanged; the describe()
type check is split so const may be a string (JWK's algorithm
names) while the numeric keywords keep their existing int/float
requirement.
(c) Shared by both scanners: a rule's own documentation title (JWK
gives each branch a human-readable one, e.g. "EC keys carry crv,
x, y") was adopted as current_class_name via the same code path
used for real class-defining titles, misattributing the rule to a
nonexistent class instead of the enclosing JwkPublicKey. A new
_is_bare_conditional_branch() helper recognizes an if/then node
with no properties of its own as rule documentation, not a type,
and both scanners now skip title adoption for it.
Fixed at the generator level only (postprocess_models.py); no
generated model files touched in this commit. Regeneration against
the pinned 2026-08-25 UCP schema follows in a separate commit, which
is what turns the six still-red semantic tests green (the four
injector-level unit tests added in the prior commit -- which exercise
the scanners directly against synthetic fixtures, not the committed
models -- already pass).
109 tests, 6 failures (JwkConditionalRulesSemanticTest x5,
UnitScaleSemanticTest x1), 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. Six files change: - profile.py: JwkPublicKey gains both a conditional-required validator (EC needs crv/x/y, OKP needs crv/x) and a conditional-bounds validator (P-256/ES256, P-384/ES384, Ed25519/EdDSA pairing). - common/types/unit.py (+ its create/update request variants): Unit gains a conditional-bounds validator pinning scale to 0 when unit is C62. - common/types/total.py, common/types/totals.py: unchanged behavior, picked up only because the conditional-bounds checks dict (embedded verbatim in every class using this validator family) now also carries the const entry. Verified: - Full suite: 109 tests, 0 failures, 4 documented skips (all six new 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 9 failures + 1 error 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 files: clean (ruff, ruff-format, codespell, trailing-whitespace, end-of-file-fixer all pass). Not committed: README.md, which ruff format also reformats (a pre-existing docstring-code-block spacing drift in main, unrelated to this fix -- the model-drift CI job only diffs src/ucp_sdk/models/schemas, so this was never caught there). Left untouched to keep this diff scoped to the constraint fix.
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 #88
What this changes
Two scanner bugs in postprocess_models.py were dropping all five of
profile.json jwk_public_key if/then rules and the unit.json C62 scale
pin, described in the linked issue. This restores all six.
find_conditional_required now threads the enclosing object properties
into an allOf branch that carries no properties of its own, the same
way find_conditional_bounds already does. Every JWK required field
rule is this shape.
find_conditional_bounds now recognizes a bare const constraint in a
then.properties. clause, not only the four numeric bound
keywords. A const entry is added to _BOUND_KEYWORDS mapped to not
equal, reusing the existing value versus limit comparison template
unchanged.
Both scanners now decline to adopt a bare if/then branch own title as
the enclosing class name. A new helper,
is_bare_conditional_branch, recognizes a node that carries if and
then but no properties of its own as rule documentation rather than a
type, so its title is never mistaken for a generated class name. A
node that carries both a conditional rule and its own properties (a
titled type with an inline conditional) keeps adopting its title as
before.
No hardcoded schema knowledge was added. Every rule is still derived
mechanically from the schema tree, the same as the existing conditional
required and conditional bounds families.
Also included
The tests/test_codegen_pipeline.py HAVE_SDK import gate cited
ucp_sdk.models.schemas.shopping.types paths for Description and Totals
that moved to common.types in #87. That silently skipped about a third
of the suite instead of running it, hiding the six failures this PR
fixes. Corrected every stale shopping.types reference in the file
(description, totals and its request variants, signals and its request
variants, error_response), which moved under the same class names and
now pass unmodified. Two targets did not survive the schema restructure
at all, card_payment_instrument Constraints (a uniqueItems brands field)
and merchant_fulfillment_config nested additionalProperties false object
(now business_fulfillment_config, reshaped), so their four tests became
a documented unittest.skip with the reason stated in the schema, rather
than a silent deletion.
Test plan
the real committed models, plus injector level unit tests for both
scanner fixes against synthetic fixtures mirroring the schema shape.
2026-08-25) as a separate commit from the generator fix. Six files
change: profile.py, unit.py with its create and update request
variants, and total.py plus totals.py, the last two only because the
comparison table embedded verbatim in every conditional bounds
validator now carries the const row, with no change to their own
rules.
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 (a spacing difference in an embedded code block, unrelated to this
fix and present before this PR). Left untouched to keep the diff scoped.