diff --git a/docs/handler-authoring.md b/docs/handler-authoring.md index 74fa0de0b..935be353e 100644 --- a/docs/handler-authoring.md +++ b/docs/handler-authoring.md @@ -792,6 +792,33 @@ except ADCPTaskError as exc: Custom error codes (outside `STANDARD_ERROR_CODES`) default to `recovery="terminal"`. Override with `adcp_error("MY_CODE", recovery="correctable")`. +### Rejecting a request with every problem + +A `DecisioningPlatform` method that validates a whole request raises one +`AdcpError` carrying every problem, so the buyer fixes the request in one +pass. `AdcpError.from_errors` puts the first entry in the envelope +`adcp_error` and all of them in the payload `errors[]`, on MCP and A2A: + +```python +from adcp.decisioning import AdcpError + +problems = [] +if package_budget < floor: + problems.append(AdcpError("BUDGET_TOO_LOW", message="budget below minimum", + recovery="correctable", field="packages[0].budget")) +if end_time <= start_time: + problems.append(AdcpError("INVALID_REQUEST", message="end_time before start_time", + recovery="correctable", field="end_time")) +if problems: + raise AdcpError.from_errors(problems) +``` + +Entries may also be generated `Error` models or wire mappings; one without +`recovery` takes the raised error's. To keep a summary envelope instead, pass +the list as `AdcpError("INVALID_REQUEST", message="2 problems", errors=problems)`. +A list in `details["errors"]` whose entries carry a string `code` and +`message` is lifted into `errors[]` too. + ## Response builders Manual `model_dump()` on response Pydantic objects is error-prone — diff --git a/src/adcp/decisioning/types.py b/src/adcp/decisioning/types.py index 3eee00efd..5be0d1ed7 100644 --- a/src/adcp/decisioning/types.py +++ b/src/adcp/decisioning/types.py @@ -21,7 +21,7 @@ from __future__ import annotations -from collections.abc import Awaitable, Callable +from collections.abc import Awaitable, Callable, Mapping, Sequence from dataclasses import dataclass, field from typing import TYPE_CHECKING, Any, Generic, Literal @@ -50,6 +50,7 @@ AccountScope, BusinessEntity, CreditLimit, + Error, GovernanceAgent, PaymentTerms, ReportingBucket, @@ -105,8 +106,28 @@ class AdcpError(Exception): error. :param retry_after: Seconds to wait before retrying. Only meaningful with ``recovery='transient'``. - :param details: Free-form extras for codes that need them - (e.g. ``{'errors': [...]}`` for multi-error preflight). + :param details: Free-form extras for codes that need them. + :param errors: Every problem found, for a request rejected with + several at once (a whole-request preflight). Each entry is an + :class:`AdcpError`, a generated ``Error`` model or a wire mapping + with string ``code`` and ``message``; an entry without + ``recovery`` takes this error's. The framework writes the list to + the payload ``errors[]`` and this error to the envelope + ``adcp_error``. :meth:`from_errors` builds the error from the + list, so ``adcp_error`` is the first entry. Without ``errors``, + ``errors[]`` carries this error alone, unless ``details['errors']`` + holds entries of that shape (the slot this class documented for + multi-error preflight before ``errors`` existed), which are + lifted into ``errors[]``. + + Example — reject a whole request with every problem:: + + raise AdcpError.from_errors([ + AdcpError("BUDGET_TOO_LOW", message="budget below minimum", + recovery="correctable", field="packages[0].budget"), + AdcpError("INVALID_REQUEST", message="end_time before start_time", + recovery="correctable", field="end_time"), + ]) """ def __init__( @@ -121,6 +142,7 @@ def __init__( suggestion: str | None = None, retry_after: int | None = None, details: dict[str, Any] | None = None, + errors: Sequence[AdcpError | Error | Mapping[str, Any]] | None = None, ) -> None: super().__init__(message or code) self.code = code @@ -129,6 +151,38 @@ def __init__( self.suggestion = suggestion self.retry_after = retry_after self.details = details or {} + if errors is not None: + self.errors: list[dict[str, Any]] = [ + _error_entry_to_wire(entry, recovery) for entry in errors + ] + else: + self.errors = _lift_detail_errors(self.details, recovery) + + @classmethod + def from_errors(cls, errors: Sequence[AdcpError | Error | Mapping[str, Any]], /) -> AdcpError: + """Build one raisable error that rejects a request for every entry. + + The first entry fills the envelope ``adcp_error``; all of them, + in order, fill the payload ``errors[]``. Entries are as for the + ``errors`` parameter; one without ``recovery`` takes the first + entry's (``terminal`` when the first has none). + + :raises ValueError: if ``errors`` is empty. + """ + if not errors: + raise ValueError("AdcpError.from_errors needs at least one error") + first = _error_entry_to_wire(errors[0], "terminal") + details = first.get("details") + return AdcpError( + first["code"], + message=first["message"], + recovery=first["recovery"], + field=first.get("field"), + suggestion=first.get("suggestion"), + retry_after=first.get("retry_after"), + details=dict(details) if isinstance(details, Mapping) else None, + errors=errors, + ) def __str__(self) -> str: return f"AdcpError[{self.code} / {self.recovery}]: {self.args[0]}" @@ -158,6 +212,53 @@ def to_wire(self) -> dict[str, Any]: out["details"] = details return out + def errors_to_wire(self) -> list[dict[str, Any]]: + """Project to the payload ``errors[]``: every entry, else this error.""" + if self.errors: + return [dict(entry) for entry in self.errors] + return [self.to_wire()] + + +def _error_entry_to_wire(entry: Any, recovery: str) -> dict[str, Any]: + """Normalize one ``errors`` entry to its wire dict.""" + if isinstance(entry, AdcpError): + return entry.to_wire() + if hasattr(entry, "model_dump"): + entry = entry.model_dump(mode="json", exclude_none=True) + if not isinstance(entry, Mapping): + raise TypeError( + "AdcpError errors entries must be AdcpError, Error or a mapping, " + f"got {type(entry).__name__}" + ) + code, message = entry.get("code"), entry.get("message") + if not isinstance(code, str) or not code or not isinstance(message, str): + raise ValueError("AdcpError errors entries need a non-empty string code and a message") + out = {key: value for key, value in entry.items() if value is not None} + out.setdefault("recovery", recovery) + if out.get("details"): + details = sanitize_error_details(code, out["details"]) + if details: + out["details"] = details + else: + del out["details"] + return out + + +def _lift_detail_errors(details: Mapping[str, Any], recovery: str) -> list[dict[str, Any]]: + """Read the documented ``details['errors']`` multi-error slot, if well formed.""" + entries = details.get("errors") + if not isinstance(entries, Sequence) or isinstance(entries, (str, bytes)) or not entries: + return [] + if not all( + isinstance(entry, Mapping) + and isinstance(entry.get("code"), str) + and entry["code"] + and isinstance(entry.get("message"), str) + for entry in entries + ): + return [] + return [_error_entry_to_wire(entry, recovery) for entry in entries] + #: Per-platform metadata generic. Defaults to ``dict[str, Any]`` for #: adopters who don't define a typed metadata shape; multi-tenant adopters diff --git a/src/adcp/server/a2a_server.py b/src/adcp/server/a2a_server.py index 7ef9769e3..926d8243f 100644 --- a/src/adcp/server/a2a_server.py +++ b/src/adcp/server/a2a_server.py @@ -839,7 +839,7 @@ async def _send_adcp_error( # Lazy import — ``translate.py`` pulls in heavier server deps # (mcp.types) which the A2A module doesn't otherwise need. from adcp.server.helpers import inject_context - from adcp.server.translate import _extract_structured_fields + from adcp.server.translate import _extract_structured_fields, _payload_errors code, message, recovery, field, suggestion, details, _errors = _extract_structured_fields( exc @@ -863,7 +863,10 @@ async def _send_adcp_error( # A fatal failure populates both layers (L3 error handling): the payload # carries the protocol-neutral ``errors[]`` and the envelope ``adcp_error``. - data: dict[str, Any] = {"adcp_error": adcp_error, "errors": [dict(adcp_error)]} + data: dict[str, Any] = { + "adcp_error": adcp_error, + "errors": _payload_errors(exc, adcp_error), + } if params is not None: inject_context(params, data) diff --git a/src/adcp/server/translate.py b/src/adcp/server/translate.py index e18b30cc7..cdc486bbc 100644 --- a/src/adcp/server/translate.py +++ b/src/adcp/server/translate.py @@ -167,7 +167,7 @@ def _extract_structured_fields( suggestion = decisioning_exc.suggestion recovery = decisioning_exc.recovery details = decisioning_exc.details or None - errors = None + errors = decisioning_exc.errors_to_wire() if decisioning_exc.errors else None field = decisioning_exc.field elif isinstance(exc, ADCPError): code = _error_code_for_exception(exc) @@ -192,6 +192,17 @@ def _extract_structured_fields( return code, message, recovery, field, suggestion, details, errors +def _payload_errors(exc: Any, adcp_error: dict[str, Any]) -> list[dict[str, Any]]: + """The payload ``errors[]`` for a fatal failure. + + Every error a raised decisioning ``AdcpError`` carries (see its + ``errors`` parameter), else a copy of the envelope ``adcp_error``. + """ + if isinstance(exc, _get_decisioning_adcp_error_types()) and cast(Any, exc).errors: + return cast("list[dict[str, Any]]", cast(Any, exc).errors_to_wire()) + return [dict(adcp_error)] + + def build_mcp_error_result( exc: ADCPError | Error | Any, *, @@ -208,7 +219,8 @@ def build_mcp_error_result( same envelope — the transport-errors.mdx §MCP Binding ``adcp_error`` plus the payload ``errors[]`` that L3 error handling says a fatal failure SHOULD also carry (storyboards read both ``/adcp_error/code`` - and ``errors[0].field``). + and ``errors[0].field``). A decisioning ``AdcpError`` raised with + several ``errors`` puts all of them in ``errors[]``. The text fallback in ``content[]`` preserves human-readable display for clients that do not consume ``structuredContent`` (LLM tool-use @@ -263,7 +275,10 @@ def build_mcp_error_result( # A fatal failure populates both layers (L3 error handling): the payload # carries the protocol-neutral ``errors[]`` and the envelope ``adcp_error``. - structured: dict[str, Any] = {"adcp_error": adcp_error, "errors": [dict(adcp_error)]} + structured: dict[str, Any] = { + "adcp_error": adcp_error, + "errors": _payload_errors(exc, adcp_error), + } if params is not None: inject_context(params, structured) diff --git a/tests/test_adcp_error_multi.py b/tests/test_adcp_error_multi.py new file mode 100644 index 000000000..65e167564 --- /dev/null +++ b/tests/test_adcp_error_multi.py @@ -0,0 +1,303 @@ +"""A raised ``AdcpError`` can reject a request with every problem (#1780). + +AdCP's payload ``errors[]`` exists so a seller that validates a whole request +returns all of its problems in one response. ``AdcpError(errors=[...])`` and +:meth:`AdcpError.from_errors` carry the list; both transports write it to +``errors[]`` and keep the envelope ``adcp_error``. The ``details['errors']`` +slot the class documented for multi-error preflight is lifted too. +""" + +from __future__ import annotations + +from typing import Any + +import pytest +from a2a import types as pb +from a2a.auth.user import UnauthenticatedUser +from a2a.server.agent_execution.context import RequestContext as A2ARequestContext +from a2a.server.context import ServerCallContext +from a2a.server.events.event_queue import EventQueueLegacy as EventQueue +from google.protobuf.json_format import MessageToDict, ParseDict +from google.protobuf.struct_pb2 import Value +from mcp.types import CallToolResult + +from adcp.decisioning import AdcpError, ValidationError +from adcp.server import ADCPHandler +from adcp.server.a2a_server import ADCPAgentExecutor +from adcp.server.translate import build_mcp_error_result, translate_error +from adcp.types import Error + + +def _budget() -> AdcpError: + return AdcpError( + "BUDGET_TOO_LOW", + message="budget below minimum", + recovery="correctable", + field="packages[0].budget", + ) + + +def _dates() -> AdcpError: + return AdcpError( + "INVALID_REQUEST", + message="end_time before start_time", + recovery="correctable", + field="end_time", + ) + + +class TestAdcpErrorErrors: + def test_single_error_has_no_list(self) -> None: + error = _budget() + assert error.errors == [] + assert error.errors_to_wire() == [error.to_wire()] + + def test_from_errors_first_entry_fills_the_envelope(self) -> None: + error = AdcpError.from_errors([_budget(), _dates()]) + assert error.code == "BUDGET_TOO_LOW" + assert error.field == "packages[0].budget" + assert error.recovery == "correctable" + assert error.errors_to_wire() == [_budget().to_wire(), _dates().to_wire()] + assert error.errors_to_wire()[0] == error.to_wire() + + def test_from_errors_accepts_mappings_and_models(self) -> None: + error = AdcpError.from_errors( + [ + {"code": "INVALID_REQUEST", "message": "unknown product", "field": "packages[1]"}, + Error(code="BUDGET_TOO_LOW", message="too low", recovery="correctable"), + ] + ) + wire = error.errors_to_wire() + # A mapping without ``recovery`` takes the first entry's, ``terminal`` here. + assert wire[0] == { + "code": "INVALID_REQUEST", + "message": "unknown product", + "field": "packages[1]", + "recovery": "terminal", + } + assert wire[1] == { + "code": "BUDGET_TOO_LOW", + "message": "too low", + "recovery": "correctable", + } + assert error.code == "INVALID_REQUEST" + + def test_from_errors_returns_the_base_class_for_a_subclass_entry(self) -> None: + error = AdcpError.from_errors([ValidationError(field="name"), _dates()]) + assert type(error) is AdcpError + assert error.code == "VALIDATION_ERROR" + assert [entry["code"] for entry in error.errors] == ["VALIDATION_ERROR", "INVALID_REQUEST"] + + def test_from_errors_rejects_an_empty_list(self) -> None: + with pytest.raises(ValueError): + AdcpError.from_errors([]) + + def test_errors_keyword_keeps_a_summary_envelope(self) -> None: + error = AdcpError( + "INVALID_REQUEST", + message="2 problems", + recovery="correctable", + errors=[_budget(), {"code": "INVALID_REQUEST", "message": "bad end_time"}], + ) + assert error.to_wire()["message"] == "2 problems" + assert [entry["message"] for entry in error.errors_to_wire()] == [ + "budget below minimum", + "bad end_time", + ] + # An entry without recovery takes the raised error's. + assert error.errors_to_wire()[1]["recovery"] == "correctable" + + @pytest.mark.parametrize( + "entry", + [ + {"message": "no code"}, + {"code": "", "message": "empty code"}, + {"code": "INVALID_REQUEST"}, + {"code": 7, "message": "numeric code"}, + ], + ) + def test_malformed_mapping_entry_is_refused(self, entry: dict[str, Any]) -> None: + with pytest.raises(ValueError): + AdcpError("INVALID_REQUEST", errors=[entry]) + + def test_non_mapping_entry_is_refused(self) -> None: + with pytest.raises(TypeError): + AdcpError("INVALID_REQUEST", errors=["budget below minimum"]) + + def test_entry_details_are_sanitized_like_to_wire(self) -> None: + details = {"connection": {"type": "oauth", "secret": "hunter2"}} + entry = AdcpError("AUTHORIZATION_REQUIRED", message="connect", details=details) + error = AdcpError.from_errors( + [_budget(), {"code": "AUTHORIZATION_REQUIRED", "message": "x", "details": details}] + ) + assert error.errors_to_wire()[1].get("details") == entry.to_wire().get("details") + + def test_errors_to_wire_returns_copies(self) -> None: + error = AdcpError.from_errors([_budget(), _dates()]) + error.errors_to_wire()[0]["code"] = "MUTATED" + assert error.errors_to_wire()[0]["code"] == "BUDGET_TOO_LOW" + + +class TestDetailsErrorsSlot: + def test_well_formed_details_errors_are_lifted(self) -> None: + error = AdcpError( + "INVALID_REQUEST", + message="2 problems", + recovery="correctable", + details={ + "errors": [ + {"code": "INVALID_REQUEST", "message": "budget", "field": "packages[0].budget"}, + {"code": "INVALID_REQUEST", "message": "dates", "field": "end_time"}, + ] + }, + ) + assert [entry["field"] for entry in error.errors_to_wire()] == [ + "packages[0].budget", + "end_time", + ] + assert {entry["recovery"] for entry in error.errors_to_wire()} == {"correctable"} + # The envelope keeps its details unchanged. + assert len(error.to_wire()["details"]["errors"]) == 2 + + @pytest.mark.parametrize( + "value", + [ + ["budget below minimum"], + [{"code": "INVALID_REQUEST"}], + [{"path": "x", "msg": "bad"}], + [], + "not a list", + ], + ) + def test_other_details_errors_are_left_alone(self, value: Any) -> None: + error = AdcpError("INVALID_REQUEST", message="bad", details={"errors": value}) + assert error.errors == [] + assert error.errors_to_wire() == [error.to_wire()] + + def test_explicit_errors_win_over_details(self) -> None: + error = AdcpError( + "INVALID_REQUEST", + details={"errors": [{"code": "A", "message": "a"}]}, + errors=[_dates()], + ) + assert [entry["code"] for entry in error.errors_to_wire()] == ["INVALID_REQUEST"] + + +class TestMcpProjection: + def test_issue_repro_returns_both_problems(self) -> None: + error = AdcpError( + "INVALID_REQUEST", + message="2 problems", + recovery="correctable", + details={ + "errors": [ + { + "code": "INVALID_REQUEST", + "message": "budget below minimum", + "field": "packages[0].budget", + }, + { + "code": "INVALID_REQUEST", + "message": "end_time before start_time", + "field": "end_time", + }, + ] + }, + ) + structured = build_mcp_error_result(error).structured_content + assert [entry.get("field") for entry in structured["errors"]] == [ + "packages[0].budget", + "end_time", + ] + assert structured["adcp_error"]["code"] == "INVALID_REQUEST" + + def test_from_errors_envelope_is_errors_zero(self) -> None: + structured = build_mcp_error_result( + AdcpError.from_errors([_budget(), _dates()]) + ).structured_content + assert structured["adcp_error"] == structured["errors"][0] + assert structured["adcp_error"] is not structured["errors"][0] + assert [entry["field"] for entry in structured["errors"]] == [ + "packages[0].budget", + "end_time", + ] + + def test_single_error_still_mirrors(self) -> None: + structured = build_mcp_error_result(_budget()).structured_content + assert structured["errors"] == [structured["adcp_error"]] + + def test_errors_survive_context_echo_and_enhancer(self) -> None: + seen: list[int] = [] + + def enhancer(response: dict[str, Any]) -> None: + seen.append(len(response["errors"])) + + structured = build_mcp_error_result( + AdcpError.from_errors([_budget(), _dates()]), + params={"context": {"correlation_id": "c-1"}}, + response_enhancer=enhancer, + ).structured_content + assert seen == [2] + assert structured["context"] == {"correlation_id": "c-1"} + + @pytest.mark.asyncio + async def test_through_tool_registration(self) -> None: + from mcp.server import MCPServer + + from adcp.server.serve import _register_tool + + async def caller(_kwargs: dict[str, Any], *, context: Any = None) -> Any: + raise AdcpError.from_errors([_budget(), _dates()]) + + mcp = MCPServer("test-1780") + _register_tool(mcp, "create_media_buy", "test", {"type": "object"}, caller) + result = await mcp.call_tool("create_media_buy", {}) + + assert isinstance(result, CallToolResult) + assert result.is_error is True + assert [entry["code"] for entry in result.structured_content["errors"]] == [ + "BUDGET_TOO_LOW", + "INVALID_REQUEST", + ] + + def test_translate_error_a2a_data_carries_every_error(self) -> None: + a2a_error = translate_error(AdcpError.from_errors([_budget(), _dates()]), "a2a") + data: Any = a2a_error.data # type: ignore[attr-defined] + assert [entry["field"] for entry in data["errors"]] == ["packages[0].budget", "end_time"] + + def test_translate_error_a2a_single_error_has_no_errors_key(self) -> None: + a2a_error = translate_error(_budget(), "a2a") + data: Any = a2a_error.data # type: ignore[attr-defined] + assert "errors" not in data + + +class _Raiser(ADCPHandler): + async def get_adcp_capabilities(self, params: Any, context: Any = None) -> dict[str, Any]: + return {"adcp": {"major_versions": [3]}, "supported_protocols": ["media_buy"]} + + async def get_products(self, params: Any, context: Any = None) -> Any: + raise AdcpError.from_errors([_budget(), _dates()]) + + +@pytest.mark.asyncio +async def test_a2a_failed_task_carries_every_error(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("ADCP_SANDBOX", "1") + value = Value() + ParseDict({"skill": "get_products", "parameters": {}}, value) + message = pb.Message(message_id="m-1", role=pb.Role.ROLE_USER, parts=[pb.Part(data=value)]) + request = A2ARequestContext( + call_context=ServerCallContext(user=UnauthenticatedUser()), + request=pb.SendMessageRequest(message=message), + ) + queue = EventQueue() + await ADCPAgentExecutor(_Raiser(), validation=None).execute(request, queue) + task = await queue.dequeue_event() + + payloads = [ + MessageToDict(part.data) + for part in task.artifacts[0].parts + if part.WhichOneof("content") == "data" + ] + payload = next(p for p in payloads if "adcp_error" in p) + assert payload["adcp_error"]["code"] == "BUDGET_TOO_LOW" + assert [entry["field"] for entry in payload["errors"]] == ["packages[0].budget", "end_time"]