Skip to content

feat: adopt the pinned spec target and the shared base, and close the conformance gaps - #9

Merged
KristofersOzolinsMagebit merged 55 commits into
developfrom
feat/spec-library-adoption
Sep 10, 2026
Merged

KristofersOzolinsMagebit merged 55 commits into
developfrom
feat/spec-library-adoption

Conversation

@KristofersOzolinsMagebit

Copy link
Copy Markdown
Member

Brings this module onto the pinned specification target and the shared base, and closes the
conformance gaps that surfaced along the way. Conformance is 61 of 65 against the official suite
with its own three defects patched, 39 of 65 against an unmodified checkout, which cannot
complete a purchase against any server.

Talking the protocol properly

  • Version negotiation. The UCP-Agent header is read and an unsupported version is refused with
    unsupported_version. Checked inside idempotency handling rather than after it, because an agent
    that cannot read this version's payloads cannot read a replayed one either.
  • Refusals name what to fix. A refused checkout used to answer with a server error and one raw
    Magento sentence. It now reports every blocker as a typed message with the JSONPath of the field:
    a missing email, no delivery option chosen, a product that does not exist, a quantity the store
    cannot supply. Validation answers 422 rather than 400, because the body parsed and the objection is
    to what it says.
  • A quantity the store will not sell is reported separately from one it does not have, asked
    before the item is added. Magento only reports a refused quantity while collecting totals, by which
    point nothing can say which item it was about.
  • An extension the store does not implement is reported as a warning rather than silently
    dropped.
  • A second discount code is no longer thrown away without a word. Magento carries one coupon, and
    the extras now come back as not_applied.
  • Order updates. PUT /orders/{id} accepts an adjustment request, records it and puts a note on
    the order for staff. It does not refund anything, and it refuses an agent-declared shipment with
    not_accepted: only the store can say a parcel was sent.
  • Buyer consent, a completed or cancelled checkout guard, and a billing address filled from the
    delivery address when the cart carries only one.

Signed webhooks

Order events are delivered with RFC 9421 message signatures over the receiver's own authority and
path, not ours, so a relayed delivery does not still verify. Per-store ES256 keys are generated on
first use and published as JWKs with a key id in the discovery profile, with rotation and a grace
window. Retries reuse the event's identity and occurrence time; the first retry is almost immediate,
then the waits grow hard.

Taken from the shared base

Request validation and hydration, idempotency arbitration, the stock check, the buyer writer, the
total label, the scheduled dispatch and the JSON endpoint plumbing all moved to
Magebit_AgenticCore and are consumed from there. Nothing about either protocol lives in that
module. Validation rules now come from the generated specification interfaces rather than being
restated here, which is what removed the hand-written checks.

Two latent bugs fell out of that comparison:

  • The street was never checked. Magento returns one empty line rather than an empty array, so a
    missing street was reported by nothing until the order failed with a message naming no field.
  • A region the country does not have is now named as invalid instead of surfacing at completion.

Notes for review

  • d/check passes: phpcs, phpstan and 340 unit tests.
  • The four remaining conformance failures are each deliberate or external: two are the store
    correctly refusing to let an agent mark an order shipped, one wants two coupons on one cart, and
    one expects a card payment to be declined by a handler the store does not advertise. All four are
    written up in dev/tests/conformance/ucp/README.md.
  • Do not merge before the spec library release. This depends on the CONSTRAINTS constant the
    generator now emits; see fix: carry the rules a list declares on its entries ucp-php-spec#11.
  • The contract migration that drops the legacy tables is deliberately not here. It must not be
    cut until the new tables are confirmed populated on a real install.

…-of-stock case and the three

coupons the conformance run expects. Re-running updates in place.
…p, and follow the OrderPlatformSchema rename.
…s open string no longer defines them, and only DI compilation caught the XML references.
…bit_AgenticCore, deleting the duplicated price converter.
…pping from claim outcome to this module's error envelope. A data patch copies rows still inside their TTL window; the old table stays declared because declarative schema runs before data patches.
…n map becomes two routes with different methods, and its any-shared-segment match is replaced by an anchored pattern.
…ethod writer and shipping option resolver. The fulfillment option total now carries the incl-tax amount, which is what the schema's fulfillment total means.
…n unfiltered backfill copies every existing link; ucp_checkout_meta.order_id stays declared and stops being read.
…26-01-23 in discovery, the merchant profile, both capabilities and every checkout response while building payloads from the released library's 2026-04-08 types, so agents negotiated against the wrong version. A test now fails if the constant and the installed library's target drift apart.
…module. The read is gated on the encryptor's envelope rather than decrypting speculatively: it returns binary garbage instead of failing on a value it never encrypted, so rows written before this change would have replayed corruption.
…rser had no caller at all, so the URL it extracts was parsed and thrown away, and ucp_checkout_meta.webhook_url — the column that exists for it — was never written. Also binds the platform schema, which nothing had ever built.
…0, Webhook-Id, Webhook-Timestamp and UCP-Agent, dispatched through the shared queue on its own cron. The event timestamp is when the order changed rather than when the attempt is made, since a retry is the same event. No credential is sent and delivery ships disabled: the protocol offers three authentication schemes and none has been agreed, and the body needs the Order representation that does not exist yet, so the enqueue site is deliberately unwired.
…get types total amounts as

signed_amount and constrains discount and items_discount to exclusiveMaximum 0, where the 2026-01-23
snapshot the code was written against typed every amount minimum 0.
…026-04-08 defines the shape and

forbids extra properties, so the old top-level status was a violation; the four hand-built messages it
carried were also missing content and severity.
… order with quantity tracking,

fulfillment expectations, the shipment event log and refund adjustments. An order is only disclosed when
the shared link shows this protocol's checkout produced it, and the region rule now follows the store's
configuration instead of demanding one everywhere.
…d keep every scrubbed id consistent

so a fixture's cross-references still resolve. The echoed selection named an identifier the response had
renamed to the quote address id, and the scrubber renumbered each id occurrence independently.
…d no body to carry. The agent's

webhook URL was never resolved because the parser looked for a `name` field inside the capability
registry, which is keyed by name; shipments and refunds are read through their repositories so an event
does not report the order as it was before the document it is announcing.
@magebit-automation

magebit-automation Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

✅ Magebit Code Review — Review complete

Reviewed up to 21f34a9062ff1430476a26aea38bd971651c5be6 (incremental pass).

No issues met the confidence threshold to publish.

Magebit Review: This follow-up is safe to merge for the catalog search crash and the stock wording bugs it set out to fix. I read the new link-table load, the stock join on the product page, the in-stock check that treats a database "0" as out of stock, and the tests that cover a variant shared by two products. Those paths now do what the comments say they do.

Findings checklist

  • Fixed — [High] Anyone who knows an order number can read or change a UCP order (Model/Service/Shopping/OrderHandler.php:141)
  • Fixed — [High] Order webhooks can be sent to any URL the agent names (Model/Service/Shopping/AgentProfileParser.php:285)
  • Fixed — [Medium] Catalog search loads every matching product id before paging (Model/Service/Shopping/CatalogHandler.php:141)
  • Fixed — [High] Catalog lookup loads one product per id and does not cap the list (Model/Service/Shopping/CatalogHandler.php:176)
  • Fixed — [High] Building an order response loads each product again for its thumbnail (Model/Service/Shopping/Converter/OrderToOrderResponse.php:136)
  • Fixed — [High] Catalog search and lookup have no unit tests (Model/Service/Shopping/CatalogHandler.php:156)
  • Fixed — [Medium] Lookup and product read return products the storefront would hide (Model/Service/Shopping/CatalogHandler.php:322)
  • Fixed — [Medium] Catalog search pages joined rows, not products (Model/Service/Shopping/CatalogHandler.php:142)
  • Open — [Medium] No test checks that an order is advertised by session id (Model/Service/Shopping/Converter/OrderToOrderResponse.php:77)
  • Open — [High] Idempotency conflicts still use the old error envelope (Model/IdempotencyHandler.php:132)
  • Open — [Medium] Order update crashes when the request has no fulfillment (Model/Service/Shopping/OrderHandler.php:91)
  • Fixed — [Medium] Catalog search loads children and stock one product at a time (Model/Service/Shopping/CatalogHandler.php:154)
  • Open — [Low] Several commits are not Conventional Commits (-:0)
  • Fixed — [Medium] Shared variants crash catalog search (Model/Service/Shopping/CatalogHandler.php:368)
  • Fixed — [High] Stock is still read once per product (Model/Service/Shopping/CatalogHandler.php:389)
  • Fixed — [Medium] Out-of-stock can show as in stock (Model/Service/Shopping/Converter/ProductToUcpProduct.php:158)
Changed files (180)

.github/

  • .github/workflows/ci.yml +95 −0

Api/

  • Api/Data/CheckoutMetaInterface.php +14 −0
  • Api/Data/IdempotencyKeyInterface.php +0 −125
  • Api/Data/OrderAdjustmentInterface.php +76 −0
  • Api/Data/TotalTypeInterface.php +30 −0
  • Api/IdempotencyKeyRepositoryInterface.php +0 −98
  • Api/OrderAdjustmentRepositoryInterface.php +37 −0
  • Api/Service/Shopping/BuyerWithConsentInterface.php +36 −0
  • Api/Service/Shopping/CartHandlerInterface.php +47 −0
  • Api/Service/Shopping/CatalogHandlerInterface.php +43 −0
  • Api/Service/Shopping/CheckoutCreateRequestInterface.php +8 −0
  • Api/Service/Shopping/CheckoutResponseInterface.php +16 −2
  • Api/Service/Shopping/CheckoutUpdateRequestInterface.php +8 −0
  • Api/Service/Shopping/OrderHandlerInterface.php +39 −0
  • Api/Service/Shopping/RestHandlerInterface.php +9 −0
  • Api/UniversalCommerceProtocolInterface.php +9 −1

Console/

  • Console/Command/DispatchWebhooks.php +72 −0
  • Console/Command/SeedConformance.php +84 −0

Controller/

  • Controller/ApiController.php +89 −71
  • Controller/Router.php +0 −183
  • Controller/Service/Shopping/Cancel.php +10 −13
  • Controller/Service/Shopping/Cart/Cancel.php +96 −0
  • Controller/Service/Shopping/Cart/Create.php +104 −0
  • Controller/Service/Shopping/Cart/Get.php +96 −0
  • Controller/Service/Shopping/Cart/Update.php +110 −0
  • Controller/Service/Shopping/Catalog/Lookup.php +90 −0
  • Controller/Service/Shopping/Catalog/Product.php +96 −0
  • Controller/Service/Shopping/Catalog/Search.php +94 −0
  • Controller/Service/Shopping/Complete.php +15 −14
  • Controller/Service/Shopping/Create.php +13 −6
  • Controller/Service/Shopping/Get.php +10 −13
  • Controller/Service/Shopping/GetOrder.php +83 −0
  • Controller/Service/Shopping/Update.php +15 −14
  • Controller/Service/Shopping/UpdateOrder.php +100 −0
  • Controller/Testing/SimulateShipping.php +148 −0

Cron/

  • Cron/DispatchWebhooks.php +45 −0

Exception/

  • Exception/UcpException.php +8 −18
  • Exception/UcpMessagesException.php +52 −0

Model/

  • Model/CheckoutMeta/Model.php +18 −0
  • Model/Config.php +18 −0
  • Model/Discovery/MerchantProfileBuilder.php +58 −17
  • Model/IdempotencyHandler.php +33 −124
  • Model/IdempotencyKey/Cleanup.php +6 −22
  • Model/IdempotencyKey/Model.php +0 −138
  • Model/IdempotencyKey/Repository.php +0 −143
  • Model/IdempotencyKey/ResourceModel.php +0 −118
  • Model/Order/AdjustmentRecorder.php +89 −0
  • Model/Order/SessionOrderLookup.php +61 −0
  • Model/OrderAdjustment/Collection.php +1 −6
  • Model/OrderAdjustment/Model.php +111 −0

+130 more files…

Reviewed by ~x-ai/grok-latest

Comment thread Model/Service/Shopping/OrderHandler.php
Comment thread Model/Service/Shopping/AgentProfileParser.php
Comment thread Model/Service/Shopping/CatalogHandler.php Outdated
Comment thread Model/Service/Shopping/CatalogHandler.php
Comment thread Model/Service/Shopping/Converter/OrderToOrderResponse.php
Comment thread Model/Service/Shopping/CatalogHandler.php
Comment thread Model/Service/Shopping/CatalogHandler.php Outdated
@KristofersOzolinsMagebit

Copy link
Copy Markdown
Member Author

All seven findings are addressed. One correction to the reasoning, and one severity I disagree with.

Order enumeration — fixed. Orders are now addressed by their checkout session id, which is
unguessable, rather than the sequential increment id. The spec settles the shape: id is only
"unique order identifier", checkout_id is already a separate required field, and label is
documented as a human-readable identifier the business provides, so the merchant order number lives
there and stays visible to the agent. The checkout completion response had to change too, since
that is where an agent learns the id it later reads the order with. A test asserts an order number
now resolves to the same 404 as presenting nothing.

Webhook SSRF — fixed, the URL goes through ProfileUrlValidator::assertFetchable before it is
stored, and a URL that fails is dropped rather than failing the checkout. One detail in the report
was wrong: a data: profile cannot be used, because only https is accepted. The attacker needs a
public HTTPS host, which is a trivial bar, so the finding stood.

Catalog lookup — capped at the existing MAX_LIMIT of 50 and read in one collection query.

Catalog search — paged in SQL. The total now comes from a COUNT(DISTINCT entity_id), which
preserves the reason the original comment gave for avoiding getSize(). A stable sort was added:
paging in SQL without a fixed row order makes a cursor meaningless.

Hidden products — the single-SKU path now applies the same status, visibility and store rules as
search. Worth noting the finding understated itself: that path was also missing the store scope, so
it could return default-scope values.

Catalog tests — added, 12 tests covering the cap, an empty query, a missing id, and a
non-visible product, and asserting the full id scan is gone.

Thumbnails — fixed, but I disagree with High. The loop is bounded by how many lines a real order
has, not by anything a caller sends, and the repository caches within a request. That is a Medium
inefficiency, not a denial-of-service vector. A twenty-line order now costs one collection query
instead of twenty loads, and fewer when lines repeat a product.

Verification: d/check green (361 tests), and the conformance suite holds at 61/65 with the same
four pre-existing failures, so nothing regressed. All six webhook structure tests still pass, which
also confirms the new SSRF guard does not block the suite's own receiver.

Also added on this branch: a CI workflow running PHPStan and the unit tests against a real Magento
install, matching the one in magento2-mcp-module.

Comment thread Model/Service/Shopping/CatalogHandler.php Outdated
// The order is addressed by its checkout session, which nobody can guess. The store's own
// order number is only a label, because it runs in sequence and anyone could count up to it.
$response->setId($checkoutId);
$response->setLabel((string) $order->getIncrementId());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Medium] No test checks that an order is advertised by session id
maintainability · general profile · confidence 75%

The whole point of this follow-up is that agents must come back with the checkout session, not the sequential order number. setId($checkoutId) and setLabel(...) do that, but nothing asserts it.

OrderToOrderResponseTest only checks that pictures load once. QuoteToCheckoutResponse does the same mapping for the checkout confirmation and has no test at all. Putting the increment id back on id would still pass this change’s tests.

Assert that convert() sets id (and the confirmation’s id) to the session, and label to the increment id.

… products

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er number

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@KristofersOzolinsMagebit

Copy link
Copy Markdown
Member Author

@magebit-automation review

Comment thread Model/Service/Shopping/OrderHandler.php
Comment thread Model/Service/Shopping/CatalogHandler.php
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nnot require

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread Model/Service/Shopping/CatalogHandler.php
Comment thread Model/Service/Shopping/CatalogHandler.php Outdated
Comment thread Model/Service/Shopping/Converter/ProductToUcpProduct.php Outdated
…t a factory

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…break search

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@KristofersOzolinsMagebit
KristofersOzolinsMagebit merged commit 52506bf into develop Sep 10, 2026
3 checks passed
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