diff --git a/src/adcp/decisioning/dispatch.py b/src/adcp/decisioning/dispatch.py index 785201a9..049ad046 100644 --- a/src/adcp/decisioning/dispatch.py +++ b/src/adcp/decisioning/dispatch.py @@ -70,7 +70,7 @@ if TYPE_CHECKING: from collections.abc import Awaitable, Callable - from pydantic import BaseModel + from pydantic import BaseModel, ValidationError from adcp.decisioning.accounts import AccountStore from adcp.decisioning.context import AuthInfo, RequestContext @@ -632,6 +632,47 @@ def _internal_error_details(exc: BaseException) -> dict[str, Any]: return details +# --------------------------------------------------------------------------- +# _validation_error_to_invalid_request — request-validation error wrapper +# --------------------------------------------------------------------------- + + +def _validation_error_to_invalid_request(method_name: str, exc: ValidationError) -> AdcpError: + """Convert a ``pydantic.ValidationError`` raised inside a platform method + to ``AdcpError('INVALID_REQUEST', recovery='correctable')``. + + The generic ``except Exception`` handler wraps all unhandled exceptions as + ``INTERNAL_ERROR``. But a ``ValidationError`` from a platform delegate + almost always means the buyer supplied a request field that failed the + seller's stricter schema — semantically an ``INVALID_REQUEST`` the buyer + can correct. This matches the behaviour of + :func:`_coerce_params_to_platform_type` for the annotation-coercion path. + + Uses :func:`adcp.types.error_narrowing.narrow_union_errors` to strip + discriminated-union noise from the ``details.validation_errors`` list. + Both ``narrow_union_errors`` and ``exc.errors()`` are part of stable + in-repo / Pydantic APIs respectively, so a failure here would be a + genuine bug worth surfacing rather than masking with a fallback. + """ + from adcp.types.error_narrowing import narrow_union_errors + + raw = exc.errors(include_input=False, include_context=False, include_url=False) + errors: list[Any] = list(narrow_union_errors(raw)) + first: dict[str, Any] = dict(errors[0]) if errors else {} + field_path = ".".join(str(loc) for loc in first.get("loc", ())) + msg = first.get("msg", "validation failed") + return AdcpError( + "INVALID_REQUEST", + message=( + f"Request validation failed for {method_name!r}: {msg}" + + (f" (field: {field_path!r})" if field_path else "") + ), + field=field_path or None, + recovery="correctable", + details={"validation_errors": errors}, + ) + + # --------------------------------------------------------------------------- # validate_platform — server-boot fail-fast # --------------------------------------------------------------------------- @@ -1294,6 +1335,10 @@ async def _invoke_platform_method( to release the consumption reservation so the buyer can retry. Hook errors are logged but never block exception propagation. """ + # pydantic is a required dep; import here (not at module level) to mirror + # the lazy-import discipline used throughout this module. + from pydantic import ValidationError as _ValidationError # noqa: PLC0415 + method = getattr(platform, method_name) # Re-validate through the platform method's own annotation when it's a # stricter subclass of the shim's already-deserialized type. Skipped @@ -1409,6 +1454,21 @@ async def _invoke_platform_method( if on_failure is not None: await _safe_on_failure_call(on_failure, wrapped, method_name) raise wrapped from exc + except _ValidationError as exc: + # A ValidationError that escaped the platform delegate is almost + # always the buyer sending a field that fails the seller's stricter + # request schema. Surface it as INVALID_REQUEST (correctable) so + # the buyer knows the payload is fixable and gets the field path. + # Mirrors _coerce_params_to_platform_type for the annotation path. + logger.warning( + "pydantic.ValidationError in platform.%s — wrapping to INVALID_REQUEST", + method_name, + exc_info=True, + ) + wrapped = _validation_error_to_invalid_request(method_name, exc) + if on_failure is not None: + await _safe_on_failure_call(on_failure, wrapped, method_name) + raise wrapped from exc except Exception as exc: # Wrap unexpected exceptions so the wire never sees a stack # trace. Adopter logs the original via observability hooks; diff --git a/tests/test_decisioning_dispatch.py b/tests/test_decisioning_dispatch.py index 8f80e108..747f8d05 100644 --- a/tests/test_decisioning_dispatch.py +++ b/tests/test_decisioning_dispatch.py @@ -784,14 +784,16 @@ async def get_products(self, req, ctx): async def test_invoke_validation_error_surfaces_narrowed_field_paths( executor: ThreadPoolExecutor, ) -> None: - """When the platform method raises ``pydantic.ValidationError`` - directly — typically because the seller constructed an invalid - response model — the wire ``details`` MUST carry the narrowed - field-path list so the buyer agent sees what failed (Stability AI - Emma P1: pre-fix wire said "see details for cause" with empty - details). Field paths are pulled from - ``ValidationError.errors()`` and run through - ``narrow_union_errors`` to filter discriminated-union noise.""" + """Any ``pydantic.ValidationError`` escaping a platform method body + is now wrapped as ``INVALID_REQUEST`` (correctable) with structured + field paths in ``details.validation_errors`` — not ``INTERNAL_ERROR``. + + The framework cannot distinguish a buyer-request validation failure + from a seller-response construction bug at this catch boundary; treating + both as ``INVALID_REQUEST`` is consistent with ``_coerce_params_to_platform_type`` + and gives buyers the field information they need to fix the request. + Field paths are narrowed via ``narrow_union_errors`` to strip + discriminated-union noise.""" from pydantic import BaseModel class _ResponseModel(BaseModel): @@ -804,10 +806,6 @@ class _CrashingPlatform(DecisioningPlatform): accounts = SingletonAccounts(account_id="x") async def get_products(self, req, ctx): - # Seller-side bug: building a response with missing fields. - # Realistic shape: an adopter calling - # CreativeManifest(...) with a missing required ``url`` on - # ImageContent. _ResponseModel.model_validate({"creative_id": "cr-1"}) ctx = _build_request_context(ToolContext(), Account(id="x"), None) @@ -820,17 +818,65 @@ async def get_products(self, req, ctx): executor=executor, registry=InMemoryTaskRegistry(), ) - assert exc_info.value.code == "INTERNAL_ERROR" - # ``caused_by`` still surfaces the exception class for triage. - assert exc_info.value.details["caused_by"]["type"] == "ValidationError" - # NEW: ``validation_errors`` is populated with structured field - # paths so the buyer agent (and the seller's wire log) see the - # actual missing fields. + assert exc_info.value.code == "INVALID_REQUEST" + assert exc_info.value.recovery == "correctable" + # ``validation_errors`` carries structured field paths. validation_errors = exc_info.value.details["validation_errors"] missing_fields = {err["loc"][-1] for err in validation_errors if err["type"] == "missing"} assert "width" in missing_fields and "height" in missing_fields +@pytest.mark.asyncio +async def test_invoke_validation_error_from_manual_model_validate_is_invalid_request( + executor: ThreadPoolExecutor, +) -> None: + """Platform method manually calls ``RequestModel.model_validate()`` + with a stricter subclass (``extra='forbid'``) and the buyer sends an + extra field — the resulting ValidationError must surface as + ``INVALID_REQUEST`` with ``recovery='correctable'`` and a populated + ``field`` pointing at the first failing loc (issue #652).""" + from pydantic import BaseModel, ConfigDict + + class _StrictRequest(BaseModel): + model_config = ConfigDict(extra="forbid") + product_id: str + + class _LooseRequest(BaseModel): + product_id: str + extra_field: str + + class _Platform(DecisioningPlatform): + capabilities = DecisioningCapabilities() + accounts = SingletonAccounts(account_id="x") + + async def get_products(self, req, ctx): + # Seller re-validates with stricter schema; buyer sent extra_field. + _StrictRequest.model_validate(req.model_dump()) + return {} + + ctx = _build_request_context(ToolContext(), Account(id="x"), None) + req = _LooseRequest(product_id="p1", extra_field="unexpected") + + with pytest.raises(AdcpError) as exc_info: + await _invoke_platform_method( + _Platform(), + "get_products", + req, + ctx, + executor=executor, + registry=InMemoryTaskRegistry(), + ) + err = exc_info.value + assert err.code == "INVALID_REQUEST" + assert err.recovery == "correctable" + # field should reference the rejected extra field + assert err.field is not None + assert "extra_field" in (err.field or "") + # full error list in details + assert "validation_errors" in err.details + assert any("extra_field" in str(e.get("loc", "")) for e in err.details["validation_errors"]) + + @pytest.mark.asyncio async def test_invoke_arg_projector_signature_drift_projects_invalid_request( executor: ThreadPoolExecutor,