Skip to content

Regenerate models from ucp@c5c6139 (Cart, Catalog, identity-linking, signed totals)#39

Closed
caohy1988 wants to merge 2 commits intoUniversal-Commerce-Protocol:mainfrom
caohy1988:regen-ucp-c5c6139
Closed

Regenerate models from ucp@c5c6139 (Cart, Catalog, identity-linking, signed totals)#39
caohy1988 wants to merge 2 commits intoUniversal-Commerce-Protocol:mainfrom
caohy1988:regen-ucp-c5c6139

Conversation

@caohy1988
Copy link
Copy Markdown

@caohy1988 caohy1988 commented May 6, 2026

What

Regenerates the SDK against Universal-Commerce-Protocol/ucp@c5c61396ebf9a5afb9c4169bfa3af1409daa6fd6 — the identity-linking OAuth 2.0 foundation merge from PR #354 plus all v2026-04-08 features. Adds the small generator/preprocessor changes that the regen depends on, plus semantic smoke tests so this regression mode is caught next time.

Opening as draft to give maintainers a chance to weigh in on (a) the --ref flag shape and (b) whether you'd prefer to wait for the next dated release/* cut before landing identity-linking models. Happy to rebase on whichever ref you prefer.

Why

Downstream observability work (Universal-Commerce-Protocol-Analytics issue #8) needs typed access to:

  • Cart (POST /carts, GET /carts/{id}, …) — was missing from the previous snapshot
  • Catalog Search + Lookup, including GetProductRequest / GetProductResponse / DetailProduct from catalog_lookup.json#/$defs
  • common/identity_linking (config.scopes, scope_policy) introduced by ucp PR #354
  • All four Context fieldsintent, language, currency, eligibility (the previous snapshot exposed intent only)
  • Signed totalsTotal.amount is now SignedAmount (a signed integer) so refunds and discounts can carry negative values without losing the sign
  • Open total / error / info / warning vocabularies — the spec documents these as well-known values that businesses MAY extend, so the generated types must be str / RootModel[str], not closed Literal[...]
  • Standalone info_code / warning_code models alongside error_code

release/2026-04-08 is not sufficient because identity-linking, info_code, and warning_code merged after the release branch was cut.

caohy1988 added 2 commits May 6, 2026 09:32
Three small but coupled changes that together let the SDK regenerate
from an arbitrary ucp commit (not only release branches) and produce
correct catalog models.

* generate_models.sh
  * New --ref <sha|ref> mode for arbitrary commits / tags / branches.
    Existing positional release-name and no-arg main behavior preserved.
  * Records the resolved upstream HEAD into src/ucp_sdk/_schema_ref.py
    (UCP_SCHEMA_REF + GENERATE_COMMAND constants) so downstream
    consumers can verify provenance after upstream main moves.
* preprocess_schemas.py
  * Add response_catalog_schema to the ucp.json oneOf union.
    Both catalog_search.json and catalog_lookup.json $ref this schema;
    without it the regenerated catalog models lose their ucp metadata
    discriminator.
* tests/test_schema_smoke.py + .github/workflows/test.yml
  * Semantic smoke tests against the regenerated models: open Total
    type vocabulary, signed Total amount accepting negatives, all four
    Context fields (intent/language/currency/eligibility), open
    error/info/warning code vocabularies, cart/catalog/identity-linking
    importability, and the provenance constant.
  * Matrix-tests on Python 3.10/3.11/3.12.
* pyproject.toml
  * pytest>=8 added to the dev dependency group; pytest config block.

Motivation: identity-linking, info_code, and warning_code schemas
were merged after release/2026-04-08 was cut; downstream observability
work needs those models, and a closed Literal[...] / Field(ge=0) on
totals would silently corrupt refund/discount analytics.
Regenerated tree from Universal-Commerce-Protocol/ucp at commit
c5c61396ebf9a5afb9c4169bfa3af1409daa6fd6 — the identity-linking OAuth
2.0 foundation merge (PR #354) plus all earlier v2026-04-08 features.

Net additions vs the previous snapshot:

* shopping/cart.py + cart_create_request.py + cart_update_request.py
* shopping/catalog_search.py
* shopping/catalog_lookup.py (with GetProductRequest/Response and
  DetailProduct as nested defs from catalog_lookup.json#/$defs)
* common/identity_linking/dev/ucp/common.py (capability_identity_config,
  config.scopes, scope_policy)
* shopping/types/info_code.py + warning_code.py + error_code.py +
  error_response.py
* shopping/types/signed_amount.py (RootModel[int], no ge=0 constraint
  — discounts and refunds can be negative)
* shopping/types/amount.py + adjustment.py + attribution.py +
  available_payment_instrument.py + category.py + description.py +
  detail_option_value.py + media.py + option_value.py + price.py +
  variant.py + product.py + signing_key.py + signals.py + several
  ECP transport schemas
* shopping/types/context.py grows intent / language / currency /
  eligibility (was intent only)
* shopping/types/total.py: type is now `str` (open vocabulary) and
  amount is `SignedAmount`; well-known values per the spec are
  subtotal / items_discount / discount / fulfillment / tax / fee /
  total, with explicit business-defined extension support
* AP2 mandate, buyer consent, and discount become package directories
  rather than single modules due to nested $defs

Generated with:
  ./generate_models.sh --ref c5c61396ebf9a5afb9c4169bfa3af1409daa6fd6

Provenance recorded at src/ucp_sdk/_schema_ref.py.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the UCP SDK by regenerating schema models to support new capabilities including Cart, Catalog, and identity linking, alongside a transition to signed totals. It also introduces provenance tracking for generated models and adds semantic smoke tests to prevent regressions. Feedback identifies a potential failure point in the generation script where an invalid git reference would not halt execution, potentially leading to models being generated from the wrong version.

I am having trouble creating individual review comments. Click here to see my feedback.

generate_models.sh (50)

high

The git checkout command is executed in a subshell without checking its exit status. If the provided ref is invalid, the script will silently continue using the default branch of the cloned repository, resulting in models generated from the wrong version while recording the intended ref in provenance. You should check the exit status or rely on set -e.

(cd ucp && git checkout "$REF_VALUE") || { echo "Error: Failed to checkout ref $REF_VALUE" >&2; exit 1; }

@caohy1988
Copy link
Copy Markdown
Author

Blocking review

I checked PR 1a against issue #8 acceptance and ran the PR tests locally:

uv run pytest -q
9 passed

I found two blockers before this should move out of draft.

  1. generate_models.sh --ref can silently generate from the wrong ref.

    generate_models.sh does not use set -euo pipefail, and the arbitrary-ref path does:

    git clone https://github.com/Universal-Commerce-Protocol/ucp ucp
    (cd ucp && git checkout "$REF_VALUE")
    UCP_SCHEMA_REF="$(cd ucp && git rev-parse HEAD)"

    If git checkout "$REF_VALUE" fails, the script keeps going on the clone's default branch and records that HEAD as provenance. That undermines the main reason issue chore: add CODEOWNERS file #8 asked for explicit-ref/SHA support.

    Fix: add strict shell mode and/or explicitly check every critical command:

    set -euo pipefail
    ...
    git clone https://github.com/Universal-Commerce-Protocol/ucp ucp
    git -C ucp checkout "$REF_VALUE"

    Also add a small generator test or shell smoke check proving ./generate_models.sh --ref definitely-not-a-real-ref exits nonzero.

  2. identity_linking is importable but not semantically generated.

    Issue chore: add CODEOWNERS file #8 acceptance requires common/identity_linking.py with config.scopes / scope_policy. This PR's generated module is only:

    ScopePolicy and ScopeToken exist, but there is no typed business identity-linking model exposing config.scopes. The current test only imports IdentityLinking, so it passes even though downstream cannot use typed access for the field issue chore: add CODEOWNERS file #8 explicitly called out.

    Fix options:

    • adjust preprocessing/codegen so the business schema under $defs["dev.ucp.common.identity_linking"].business_schema produces a typed class with config.scopes, or
    • if the nested reverse-domain $defs layout prevents that, add a targeted generated/helper model for this schema and cover it with a semantic test.

    Acceptance test should instantiate/access something like:

    model = IdentityLinkingBusinessSchema(
        config={"scopes": {"dev.ucp.shopping.order:read": {}}}
    )
    assert "dev.ucp.shopping.order:read" in model.config.scopes

Everything else I checked aligns with PR 1a's current scope: Cart/Catalog files are present, Context has intent/language/currency/eligibility, totals use signed amounts with open type, message codes are open RootModel[str], and provenance records c5c61396ebf9a5afb9c4169bfa3af1409daa6fd6.

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.

1 participant