From 359e5e661ae22e8df877469fc8108a8318ba4d9d Mon Sep 17 00:00:00 2001 From: Vishal Katyal Date: Fri, 18 Sep 2026 01:49:05 -0400 Subject: [PATCH] fix(rest/python): bind Complete Checkout to the specified request body The handler declared two required body members, payment and risk_signals, and tucked the real SDK CheckoutCompleteRequest under an optional nested checkout_complete key. Three consequences: 1. risk_signals is not in the specification. git grep returns no matches at the served tag v2026-04-08, on main, or at release/2026-08-25, while the control term signals returns 14 lines at that tag. It was replaced by signals in 1b3e7228 and the deprecated property was deleted in 681d692c, both ancestors of the served tag. A spec-shaped complete request was answered 422 Field required at risk_signals. 2. Top-level signals and attribution were silently dropped, because FastAPI binds only the members the handler declares. 3. PaymentCreateRequest was constructed inside the handler body, so a malformed payment raised a pydantic ValidationError that is not a RequestValidationError and surfaced as a bare 500. The body now binds to the SDK CheckoutCompleteRequest, whose fields are payment required plus signals and attribution optional, matching checkout.json. Its extra=allow keeps a legacy risk_signals harmless, so callers sending the old shape still succeed. Malformed payment is now rejected by framework validation as a 422 in the UCP envelope. The service takes the request rather than a separate risk_signals argument, and the idempotency fingerprint covers the whole body. The fingerprint composition changes with the body, so a retry that reuses an idempotency key recorded by the previous build answers 409 after upgrade rather than replaying. That is the same class of change as #166. The happy-path client sent risk_signals too, so the sample taught the non-spec shape. It now sends signals, and the recorded sample output was regenerated to match. Out of scope: the a2a sample uses the DataPart key a2a.ucp.checkout.risk_signals where the spec at the served tag names a2a.ucp.checkout.signals. Same class, different binding and different sample, to be filed separately. Local: pytest 261 passed, was 245. The 4 new tests are collected by 4 test classes across 3 modules. Restoring the required risk_signals member with the tests kept turns 65 tests red. pre-commit run --all-files passes, and the happy-path client completes against the branch with no 500s. --- .../sample_output/happy_path_dialog.md | 6 +- .../flower_shop/simple_happy_path_client.py | 6 +- rest/python/server/integration_test.py | 127 +++++++++++++++++- .../server/routes/ucp_implementation.py | 18 ++- .../server/services/checkout_service.py | 5 +- 5 files changed, 145 insertions(+), 17 deletions(-) diff --git a/rest/python/client/flower_shop/sample_output/happy_path_dialog.md b/rest/python/client/flower_shop/sample_output/happy_path_dialog.md index 4b482ca..a9b4b35 100644 --- a/rest/python/client/flower_shop/sample_output/happy_path_dialog.md +++ b/rest/python/client/flower_shop/sample_output/happy_path_dialog.md @@ -1353,9 +1353,9 @@ export RESPONSE=$(curl -s -X POST $SERVER_URL/checkout-sessions/$CHECKOUT_ID/com -H 'request-id: f6478591-3679-4459-baa7-7ecddb6a5c95' \ -H 'Content-Type: application/json' \ -d '{ - "risk_signals": { - "ip": "127.0.0.1", - "browser": "python-httpx" + "signals": { + "com.example.ip": "127.0.0.1", + "com.example.browser": "python-httpx" }, "payment": { "instruments": [ diff --git a/rest/python/client/flower_shop/simple_happy_path_client.py b/rest/python/client/flower_shop/simple_happy_path_client.py index 3527b3b..2913b8e 100644 --- a/rest/python/client/flower_shop/simple_happy_path_client.py +++ b/rest/python/client/flower_shop/simple_happy_path_client.py @@ -861,9 +861,9 @@ def main() -> int: } ] }, - "risk_signals": { - "ip": "127.0.0.1", - "browser": "python-httpx", + "signals": { + "com.example.ip": "127.0.0.1", + "com.example.browser": "python-httpx", }, } diff --git a/rest/python/server/integration_test.py b/rest/python/server/integration_test.py index 6f9d095..465d991 100644 --- a/rest/python/server/integration_test.py +++ b/rest/python/server/integration_test.py @@ -319,7 +319,6 @@ def _create_payment_payload(self) -> dict: } ] ), - risk_signals={}, ) return payload.model_dump(mode="json", exclude_none=True) @@ -2224,6 +2223,132 @@ def test_checkout_readiness_lifecycle(self) -> None: self.assertEqual(complete_res.status_code, 200, complete_res.text) self.assertEqual(complete_res.json()["status"], "completed") + def _create_and_get_id(self, checkout_id: str, key: str) -> str: + """Create a checkout and return the server-assigned id.""" + payload = self._create_checkout_payload( + checkout_id, [("rose", "Red Rose", 1000, 1)] + ) + response = self.client.post( + "/checkout-sessions", + headers=self._get_headers(idempotency_key=key, request_id=key), + json=payload.model_dump(mode="json", exclude_none=True), + ) + self.assertEqual(response.status_code, 201, response.text) + return self.get_resource_id(TestCheckout.model_validate(response.json()).id) + + def test_complete_accepts_the_spec_shaped_body(self) -> None: + """Complete Checkout takes the body the spec defines. + + checkout.json marks `payment` complete: required and `signals` optional. + There is no `risk_signals` member anywhere in the specification; it was + replaced by `signals` before the 2026-04-08 release. The handler declared + `risk_signals` as a required body member, so a spec-shaped request was + rejected with 422. + """ + with self.client: + checkout_sid = self._create_and_get_id("spec_body_1", "sb1") + body = { + "payment": { + "instruments": [ + { + "id": "instr_1", + "handler_id": "mock_payment_handler", + "type": "card", + "display": {"brand": "Visa", "last_digits": "1234"}, + "credential": {"type": "token", "token": "success_token"}, + } + ] + }, + "signals": {"com.example.session": "xts-marker-2030"}, + } + response = self.client.post( + f"/checkout-sessions/{checkout_sid}/complete", + headers=self._get_headers(idempotency_key="sb2", request_id="sb2"), + json=body, + ) + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual( + TestCheckout.model_validate(response.json()).status, "completed" + ) + + def test_complete_rejects_a_malformed_payment_with_4xx(self) -> None: + """A malformed payment is a client error, not a server fault. + + The handler built PaymentCreateRequest inside its own body, so pydantic + raised a ValidationError that was not a RequestValidationError and the + request surfaced as a bare 500. + """ + with self.client: + checkout_sid = self._create_and_get_id("bad_payment_1", "bp1") + response = self.client.post( + f"/checkout-sessions/{checkout_sid}/complete", + headers=self._get_headers(idempotency_key="bp2", request_id="bp2"), + json={ + # risk_signals is sent so the request reaches the payment parsing + # rather than being turned away for the missing member. It is a + # tolerated extra once the body binds to the SDK model. + "risk_signals": {}, + "payment": { + "instruments": [ + {"id": "instr_1", "handler_id": 12345, "type": "card"} + ] + }, + }, + ) + self.assertEqual(response.status_code, 422, response.text) + self.assertEqual(response.json()["ucp"]["status"], "error") + + def test_complete_rejects_a_non_list_instruments_with_4xx(self) -> None: + """The same holds when instruments is not an array at all.""" + with self.client: + checkout_sid = self._create_and_get_id("bad_payment_2", "bp3") + response = self.client.post( + f"/checkout-sessions/{checkout_sid}/complete", + headers=self._get_headers(idempotency_key="bp4", request_id="bp4"), + json={ + "risk_signals": {}, + "payment": {"instruments": "xts-not-a-list"}, + }, + ) + self.assertEqual(response.status_code, 422, response.text) + + def test_complete_carries_signals_into_the_request_identity(self) -> None: + """`signals` is part of the request, so it must reach the service. + + The handler bound only `payment` and `risk_signals`, so a top-level + `signals` member was silently discarded. Reusing one idempotency key with + a different `signals` value is then indistinguishable from a replay. With + the body bound to the SDK model it is a conflict. + """ + with self.client: + checkout_sid = self._create_and_get_id("signals_1", "sg1") + base = { + "payment": { + "instruments": [ + { + "id": "instr_1", + "handler_id": "mock_payment_handler", + "type": "card", + "display": {"brand": "Visa", "last_digits": "1234"}, + "credential": {"type": "token", "token": "success_token"}, + } + ] + }, + } + first = self.client.post( + f"/checkout-sessions/{checkout_sid}/complete", + headers=self._get_headers(idempotency_key="sg2", request_id="sg2"), + json=dict(base, signals={"com.example.session": "first-xts"}), + ) + self.assertEqual(first.status_code, 200, first.text) + + second = self.client.post( + f"/checkout-sessions/{checkout_sid}/complete", + headers=self._get_headers(idempotency_key="sg2", request_id="sg3"), + json=dict(base, signals={"com.example.session": "second-xts"}), + ) + self.assertEqual(second.status_code, 409, second.text) + if __name__ == "__main__": absltest.main() diff --git a/rest/python/server/routes/ucp_implementation.py b/rest/python/server/routes/ucp_implementation.py index 1a0460e..77ae161 100644 --- a/rest/python/server/routes/ucp_implementation.py +++ b/rest/python/server/routes/ucp_implementation.py @@ -207,8 +207,7 @@ async def update_checkout( async def complete_checkout( checkout_id: Annotated[str, Path(..., alias="id")], - payment: Annotated[dict[str, Any], Body(...)], - risk_signals: Annotated[dict[str, Any], Body(...)], + checkout_complete: Annotated[CheckoutCompleteRequest, Body(...)], common_headers: Annotated[ dependencies.CommonHeaders, Depends(dependencies.common_headers) ], @@ -216,18 +215,23 @@ async def complete_checkout( checkout_service: Annotated[ CheckoutService, Depends(dependencies.get_checkout_service) ], - checkout_complete: Annotated[CheckoutCompleteRequest | None, Body()] = None, ) -> models.UnifiedCheckout: - """Complete Checkout Implementation.""" + """Complete Checkout Implementation. + + The body is the checkout complete projection: `payment` is required and + `signals` and `attribution` are optional. Binding it to the SDK model means + a malformed payment is answered by the framework as a 422 in the UCP + envelope, rather than raising inside the handler as a 500. + """ del common_headers # Unused - # Parse payment into PaymentCreateRequest - payment_req = PaymentCreateRequest(**payment) + payment_req = PaymentCreateRequest( + **checkout_complete.payment.model_dump(mode="json", exclude_none=True) + ) return await checkout_service.complete_checkout( checkout_id, payment_req, - risk_signals, idempotency_key, checkout_complete=checkout_complete, ) diff --git a/rest/python/server/services/checkout_service.py b/rest/python/server/services/checkout_service.py index f6c1669..4cc90fe 100644 --- a/rest/python/server/services/checkout_service.py +++ b/rest/python/server/services/checkout_service.py @@ -716,7 +716,6 @@ async def complete_checkout( self, checkout_id: str, payment: PaymentCreateRequest, - risk_signals: dict[str, Any], idempotency_key: str, checkout_complete: CheckoutCompleteRequest | None = None, ) -> Checkout: @@ -724,10 +723,10 @@ async def complete_checkout( logger.info("Completing checkout session %s", checkout_id) # Idempotency Check - # Include risk_signals and checkout_complete in the hash + # The whole request body is part of the request identity, so signals and + # attribution are included through checkout_complete. combined_data = { "payment": payment.model_dump(mode="json"), - "risk_signals": risk_signals, "checkout_complete": checkout_complete.model_dump(mode="json") if checkout_complete else None,