From 568945cb34662f2d4be1e704368499254e9ac2fd Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 26 Oct 2026 00:13:09 +0000 Subject: [PATCH 1/3] fix(compat): never downgrade into a payload the older schema refuses Projection dropped what the target release could not hold but never checked that the parent it left behind was still valid. A notification config whose event types were all 3.2-only kept event_types: [] below 3.1's minItems: 1, and a structured 3.2 geo_regions capability became {} where 3.1 allows only a boolean. When a node loses or coarsens a direct child, its own shape keywords (required, minItems, minProperties, unions, if/then) are now checked against the older schema. If the newer schema accepted the original and the older one refuses the remainder: - a boolean | object capability whose older contract is boolean maps to true when the structure declares any support (coarsened_to_boolean) - an optional field is omitted (unrepresentable_in_target) - a response array item is omitted (unrepresentable_in_target) - a request array item is never dropped: a shortened sync list can read as removal under delete_missing, so the nearest optional field holding it is omitted instead - a required chain to the root raises ADCPVersionConversionError at the refused value Every downgrade then validates against the older bundled schema and raises a structured error for anything still refused, such as a required enum member the older release cannot spell. Payloads already invalid in their own release are projected but neither repaired nor rejected. Also fixes a crash resolving $ref pointers that index into a list (#/oneOf/N, comply_test_controller). A new sweep converts every bundled 3.2 schema example, the canonical parity cases, the audience-evidence vectors and new fixtures down to 3.1 and 3.0. Before: 21 of 168 conversions failed (19 refused by the target schema, 2 crashes). After: 0; 10 are listed, intentional structured errors. Fixes #1621 Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/cross-version-conversion.md | 36 ++ src/adcp/compat/versions.py | 342 ++++++++++++++++--- tests/fixtures/version_conversion_sweep.json | 105 ++++++ tests/test_cross_version_conversion.py | 137 ++++++++ tests/test_version_conversion_sweep.py | 146 ++++++++ 5 files changed, 720 insertions(+), 46 deletions(-) create mode 100644 tests/fixtures/version_conversion_sweep.json create mode 100644 tests/test_version_conversion_sweep.py diff --git a/docs/cross-version-conversion.md b/docs/cross-version-conversion.md index f287ba1aa..890d78caf 100644 --- a/docs/cross-version-conversion.md +++ b/docs/cross-version-conversion.md @@ -110,6 +110,42 @@ results supplied on `list_tasks` entries. Loss pointers include `/result` or `/tasks//result`. Unknown vendor task results remain opaque. Nested polling wrappers remain opaque as well, bounding conversion to one wrapper level. +## Downgrades never emit a payload the older schema refuses + +Dropping a newer field or enum member can leave its parent in a shape the +older release refuses: a notification config whose `event_types` were all +3.2-only becomes an empty list below 3.1's `minItems: 1`, and an account row +identified only by a 3.2 `account` reference loses its identity. When a +projected node fails its own constraints in the older schema (`required`, +`minItems`, `minProperties`, unions or `if`/`then`) and the newer schema +accepted the original, the SDK resolves it by one rule: + +| Situation | Result | Loss `reason` | +| --- | --- | --- | +| Newer `boolean \| object` capability, older boolean only (3.2 `geo_regions`) | `true` when the structure declares any support, otherwise `false` | `coarsened_to_boolean` | +| Refused node is an optional field | Field omitted | `unrepresentable_in_target` | +| Refused node is a response array item | Item omitted | `unrepresentable_in_target` | +| Refused node is a request array item | The nearest optional field holding the list is omitted | `unrepresentable_in_target` | +| Only required fields lead to the root | `ADCPVersionConversionError` at the refused value | — | + +Requests never send a shortened list. Sync tasks support `delete_missing`, +so a partial list could read as removal; omitting the field leaves that state +unchanged. Losses already recorded inside a dropped node stay in the report, +explaining why it was dropped. + +Every downgrade then validates the result against the older release's bundled +schema. A remaining refusal, such as a field the older release requires and +the payload lacks or a required enum member it cannot spell, fails with +`ADCPVersionConversionError` instead of reaching the peer invalid. A payload +that was already invalid in its own release is projected but neither repaired +nor rejected; the caller's validation policy decides. The server returns these +errors as `VERSION_UNSUPPORTED`; the client returns a failed `TaskResult` with +`adcp_error.code="UNSUPPORTED_FEATURE"`. + +`tests/test_version_conversion_sweep.py` converts every bundled 3.2 schema +example and fixture down to 3.1 and 3.0 and requires each result to validate, +or to fail with one of the structured errors it lists. + ## Conversion reports Client results expose conversion details in diff --git a/src/adcp/compat/versions.py b/src/adcp/compat/versions.py index 88afac718..99952c9ee 100644 --- a/src/adcp/compat/versions.py +++ b/src/adcp/compat/versions.py @@ -14,6 +14,9 @@ from functools import lru_cache from typing import Any +from jsonschema.exceptions import best_match +from jsonschema.validators import validator_for + from adcp._version import ( get_supported_adcp_versions, normalize_to_release_precision, @@ -27,6 +30,9 @@ _OPAQUE_FIELDS = frozenset({"context", "ext"}) _ARRAY_INDEX = re.compile(r"/\d+(?=/|$)") _SUMMARY_POINTER_LIMIT = 10 +#: Compiled node validators keyed by schema identity; see :func:`_refusal`. +_NODE_VALIDATORS: dict[tuple[int, int], tuple[Any, Any, Any]] = {} +_NODE_VALIDATOR_LIMIT = 4096 @dataclass(frozen=True) @@ -50,6 +56,19 @@ def metadata(self) -> list[dict[str, Any]]: return [asdict(loss) for loss in self.losses] +class _UnrepresentableError(Exception): + """A projected node the target schema refuses; its parent decides what survives. + + ``pointer`` locates the refused value for the structured error; the + parent drops the whole child it was projecting, not just that value. + """ + + def __init__(self, pointer: str, message: str) -> None: + super().__init__(message) + self.pointer = pointer + self.message = message + + class ADCPVersionConversionError(ADCPFeatureUnsupportedError): """A payload cannot be represented without inventing state or changing a clear.""" @@ -131,7 +150,8 @@ def _resolve(schema: Any, root: dict[str, Any]) -> dict[str, Any]: if isinstance(ref, str) and ref.startswith("#/"): resolved: Any = root for segment in ref[2:].split("/"): - resolved = resolved[segment.replace("~1", "/").replace("~0", "~")] + segment = segment.replace("~1", "/").replace("~0", "~") + resolved = resolved[int(segment) if isinstance(resolved, list) else segment] return {**resolved, **{key: value for key, value in schema.items() if key != "$ref"}} return schema @@ -209,6 +229,114 @@ def _property(schema: dict[str, Any], key: str) -> Any: return additional if isinstance(additional, dict) else None +#: Keywords a node's remaining shape can fail once a direct child is dropped or +#: coarsened, e.g. a required identity whose only newer spelling was removed. +_SHAPE_KEYWORDS = frozenset( + { + "anyOf", + "contains", + "dependencies", + "if", + "minItems", + "minProperties", + "not", + "oneOf", + "required", + } +) + + +def _shape_keywords(schema: Any, root: dict[str, Any]) -> frozenset[str]: + """The node's own constraints that removing children could violate.""" + resolved = _resolve(schema, root) + found = _SHAPE_KEYWORDS & resolved.keys() + for branch in resolved.get("allOf", []): + found |= _shape_keywords(branch, root) + return frozenset(found) + + +def _shape_schema(schema: Any, root: dict[str, Any]) -> dict[str, Any]: + """The node's shape keywords alone, without ``properties`` or ``items``.""" + resolved = _resolve(schema, root) + own = {key: resolved[key] for key in _SHAPE_KEYWORDS if key in resolved} + if "if" in own: + own.update({key: resolved[key] for key in ("then", "else") if key in resolved}) + branches = [_shape_schema(branch, root) for branch in resolved.get("allOf", [])] + return {**own, "allOf": [branch for branch in branches if branch]} if branches else own + + +def _refusal(schema: Any, root: dict[str, Any], value: Any) -> tuple[str, str] | None: + """Return where and why the node's own shape refuses ``value``, or ``None``. + + Only the node's shape keywords run (see :func:`_shape_schema`); its + children are checked where they were projected. The pointer is relative + to ``value``. Draft-07 ignores ``$ref`` siblings, so the wrapper + validates only the node while every ``#/...`` pointer in it still + resolves against the original document. + """ + if not isinstance(schema, dict) or not schema: + return None + key = (id(schema), id(root)) + cached = _NODE_VALIDATORS.get(key) + if cached is None or cached[0] is not schema or cached[1] is not root: + if len(_NODE_VALIDATORS) >= _NODE_VALIDATOR_LIMIT: + _NODE_VALIDATORS.clear() + document = { + **root, + "$ref": "#/$defs/__adcp_conversion_node", + "$defs": { + **root.get("$defs", {}), + "__adcp_conversion_node": _shape_schema(schema, root), + }, + } + cached = (schema, root, validator_for(root)(document)) + _NODE_VALIDATORS[key] = cached + try: + error = best_match(cached[2].iter_errors(value)) + except Exception: # noqa: BLE001 - an unresolvable node is checked whole later + return None + if error is None: + return None + pointer = "" + for key in error.absolute_path: + pointer = _pointer(pointer, key) + return pointer, error.message + + +def _coarsens_to_boolean(source: Any, target: dict[str, Any], root: dict[str, Any]) -> bool: + """A ``boolean | object`` capability whose older contract kept only the boolean. + + The union itself says how to map down: the boolean is the coarse spelling + of the structured declaration (e.g. 3.2 ``geo_regions``). + """ + types = target.get("type") + if types != "boolean" and not ( + isinstance(types, list) and "boolean" in types and "object" not in types + ): + return False + resolved = _resolve(source, root) + arms = resolved.get("oneOf", resolved.get("anyOf", [])) + return any(_resolve(arm, root).get("type") == "boolean" for arm in arms) and any( + _resolve(arm, root).get("type") == "object" or "properties" in _resolve(arm, root) + for arm in arms + ) + + +def _declares_support(value: Any) -> bool: + """Whether a structured capability declares anything supported.""" + if isinstance(value, dict): + return any(_declares_support(child) for child in value.values()) + if isinstance(value, list): + return any(_declares_support(child) for child in value) + if isinstance(value, bool): + return value + return value is not None + + +def _parent_pointer(pointer: str) -> str: + return pointer.rsplit("/", 1)[0] + + def _project( value: Any, source: Any, @@ -220,10 +348,15 @@ def _project( target_version: str, losses: list[ConversionLoss], *, - drop_unavailable_nulls: bool = False, + response: bool = False, ) -> Any: + raw_source, raw_target = source, target + first_loss = len(losses) source = _view(source, value, source_root) target = _view(target, value, target_root) + if isinstance(value, dict) and _coarsens_to_boolean(raw_source, target, source_root): + losses.append(ConversionLoss(path, source_version, target_version, "coarsened_to_boolean")) + return _declares_support(value) if isinstance(value, dict): if target_version == "3.0" and source.get("title") in { "Postal Area", @@ -266,7 +399,7 @@ def _project( target_child = _property(target, key) if source_child is not None and target_child is None: if ( - drop_unavailable_nulls + response and not path and key == "message" and target.get("additionalProperties", True) is not False @@ -276,7 +409,7 @@ def _project( projected[key] = child continue if ( - not drop_unavailable_nulls + not response and child is None and _allows_null(_view(source_child, child, source_root)) ): @@ -299,7 +432,7 @@ def _project( source_view = _view(source_child, child, source_root) target_view = _view(target_child, child, target_root) if ( - (not drop_unavailable_nulls or key in target["required"]) + (not response or key in target["required"]) and child is None and _allows_null(source_view) and _rejects_null(target_view) @@ -310,12 +443,12 @@ def _project( target_version=target_version, reason=( "the older contract cannot express an explicit null clear" - if not drop_unavailable_nulls + if not response else "the older contract requires a value unavailable in this response" ), ) if ( - drop_unavailable_nulls + response and child is None and key not in target["required"] and _allows_null(source_view) @@ -326,8 +459,8 @@ def _project( ) continue # An optional enum member introduced by the newer release cannot - # be sent to the old peer. Required values remain for validation - # to reject rather than manufacturing a different meaning. + # be sent to the old peer. A required value is never replaced by + # a different meaning; the target check fails it structurally. if ( not isinstance(child, (dict, list)) and "enum" in source_view @@ -340,18 +473,38 @@ def _project( ConversionLoss(child_path, source_version, target_version, "enum_unavailable") ) continue - projected[key] = _project( - child, - source_child, - target_child, - source_root, - target_root, - child_path, - source_version, - target_version, - losses, - drop_unavailable_nulls=drop_unavailable_nulls, - ) + try: + projected[key] = _project( + child, + source_child, + target_child, + source_root, + target_root, + child_path, + source_version, + target_version, + losses, + response=response, + ) + except _UnrepresentableError: + if key in target["required"]: + raise + losses.append( + ConversionLoss( + child_path, source_version, target_version, "unrepresentable_in_target" + ) + ) + _check_representable( + projected, + value, + raw_source, + raw_target, + source_root, + target_root, + path, + losses, + first_loss, + ) return projected if isinstance(value, list): projected_items = [] @@ -371,24 +524,78 @@ def _project( ) ) continue - projected_items.append( - _project( - child, - source.get("items", {}), - target.get("items", {}), - source_root, - target_root, - _pointer(path, index), - source_version, - target_version, - losses, - drop_unavailable_nulls=drop_unavailable_nulls, + try: + projected_items.append( + _project( + child, + source.get("items", {}), + target.get("items", {}), + source_root, + target_root, + _pointer(path, index), + source_version, + target_version, + losses, + response=response, + ) ) - ) + except _UnrepresentableError: + if not response: + # A shortened request list can read as removal, e.g. a + # sync with delete_missing. Omit the optional field that + # holds it instead, or fail when none is optional. + raise + losses.append( + ConversionLoss( + _pointer(path, index), + source_version, + target_version, + "unrepresentable_in_target", + ) + ) + _check_representable( + projected_items, + value, + raw_source, + raw_target, + source_root, + target_root, + path, + losses, + first_loss, + ) return projected_items return value +def _check_representable( + projected: Any, + value: Any, + source: Any, + target: Any, + source_root: dict[str, Any], + target_root: dict[str, Any], + path: str, + losses: list[ConversionLoss], + first_loss: int, +) -> None: + """Refuse a node whose own projection left a shape the target rejects. + + Only nodes that lost or coarsened a direct child are checked, and only + when the source accepted the original: projection must not turn valid + data invalid, but it is not a validator for data that arrived invalid. + The parent drops an optional field or array item and records the loss; + a required chain up to the root becomes a structured conversion error. + """ + if not any(_parent_pointer(loss.pointer) == path for loss in losses[first_loss:]): + return + if not _shape_keywords(target, target_root): + return + refusal = _refusal(target, target_root, projected) + if refusal is not None and _refusal(source, source_root, value) is None: + raise _UnrepresentableError(path + refusal[0], refusal[1]) + + def _allows_null(schema: dict[str, Any]) -> bool: types = schema.get("type", []) return ( @@ -706,18 +913,28 @@ def _convert_payload( ) result.setdefault("cache_scope", scope) if step < 0: - result = _project( - result, - source_schema, - target_schema, - source_schema, - target_schema, - "", - source, - target, - losses, - drop_unavailable_nulls=direction != "request", - ) + try: + result = _project( + result, + source_schema, + target_schema, + source_schema, + target_schema, + "", + source, + target, + losses, + response=direction != "request", + ) + except _UnrepresentableError as exc: + raise ADCPVersionConversionError( + pointer=exc.pointer, + source_version=source, + target_version=target, + reason=( + f"the older contract cannot represent the remaining value: {exc.message}" + ), + ) from None minor += step # The version envelope describes the boundary's contract, including # upgrades before application code. 3.0 has only the major envelope. @@ -729,4 +946,37 @@ def _convert_payload( result["adcp_version"] = target_version if direction != "request": _convert_embedded_results(tool, result, source_version, target_version, losses) + if step < 0: + _require_target_valid(tool, direction, payload, result, source_version, target_version) return ConversionResult(result, tuple(losses)) + + +def _require_target_valid( + tool: str, + direction: Direction, + payload: dict[str, Any], + result: dict[str, Any], + source_version: str, + target_version: str, +) -> None: + """A downgrade never emits a payload the older schema refuses. + + Projection repairs what it removed. Anything else the older contract + requires and the payload cannot supply, such as a newly optional field + or a required enum member it lacks, fails as a structured error rather + than reaching the peer invalid. Payloads that arrived invalid are left + to the caller's validation policy. + """ + from adcp.validation.schema_validator import validate_request, validate_response + + validate = validate_request if direction == "request" else validate_response + outcome = validate(tool, result, version=target_version) + if outcome.valid or not validate(tool, payload, version=source_version).valid: + return + issue = outcome.issues[0] + raise ADCPVersionConversionError( + pointer=issue.pointer, + source_version=source_version, + target_version=target_version, + reason=f"the older contract refuses the converted payload: {issue.message}", + ) diff --git a/tests/fixtures/version_conversion_sweep.json b/tests/fixtures/version_conversion_sweep.json new file mode 100644 index 000000000..a5322dfa0 --- /dev/null +++ b/tests/fixtures/version_conversion_sweep.json @@ -0,0 +1,105 @@ +{ + "description": "Valid AdCP 3.2 payloads converted down by tests/test_version_conversion_sweep.py, in addition to every bundled schema example. Each covers a shape the bundled examples do not.", + "cases": [ + { + "id": "issue1621.list_accounts.notification_configs", + "tool": "list_accounts", + "direction": "sync", + "payload": { + "status": "completed", + "accounts": [ + { + "account_id": "a1", + "name": "Acct", + "status": "active", + "notification_configs": [ + { + "subscriber_id": "only_new_events", + "url": "https://buyer.example/hooks/a", + "event_types": ["account.change_recorded"] + }, + { + "subscriber_id": "mixed_events", + "url": "https://buyer.example/hooks/b", + "event_types": ["account.change_recorded", "product.updated"] + } + ] + } + ] + } + }, + { + "id": "issue1621.get_adcp_capabilities.geo_regions", + "tool": "get_adcp_capabilities", + "direction": "sync", + "payload": { + "status": "completed", + "adcp": { + "major_versions": [3], + "supported_versions": ["3.1", "3.2"], + "idempotency": {"supported": false} + }, + "supported_protocols": ["media_buy"], + "media_buy": { + "execution": { + "targeting": { + "geo_countries": true, + "geo_regions": {"countries": {"US": {"values": ["US-NY", "US-CA"]}}}, + "geo_postal_areas": {"US": ["zip"]} + } + } + } + } + }, + { + "id": "get_adcp_capabilities.geo_regions.with_account", + "tool": "get_adcp_capabilities", + "direction": "sync", + "payload": { + "status": "completed", + "adcp": { + "major_versions": [3], + "supported_versions": ["3.0", "3.1", "3.2"], + "idempotency": {"supported": false} + }, + "supported_protocols": ["media_buy"], + "account": {"supported_billing": ["operator"]}, + "media_buy": { + "execution": { + "targeting": { + "geo_countries": true, + "geo_regions": {"countries": {"US": {"all_values": true}}} + } + } + } + } + }, + { + "id": "sync_accounts.request.notification_configs", + "tool": "sync_accounts", + "direction": "request", + "payload": { + "idempotency_key": "sync-accounts-notification-configs-0001", + "accounts": [ + { + "brand": {"domain": "acme.example"}, + "operator": "agency.example", + "billing": "operator", + "notification_configs": [ + { + "subscriber_id": "only_new_events", + "url": "https://buyer.example/hooks/a", + "event_types": ["account.change_recorded"] + }, + { + "subscriber_id": "mixed_events", + "url": "https://buyer.example/hooks/b", + "event_types": ["account.change_recorded", "product.updated"] + } + ] + } + ] + } + } + ] +} diff --git a/tests/test_cross_version_conversion.py b/tests/test_cross_version_conversion.py index 453ddafcd..88a0c8099 100644 --- a/tests/test_cross_version_conversion.py +++ b/tests/test_cross_version_conversion.py @@ -793,3 +793,140 @@ async def test_in_flight_creative_dialect_survives_capability_refresh() -> None: ) assert client._creative_dialect(request) is CreativeDialect.CANONICAL assert client._creative_dialect(request, result=result) is CreativeDialect.LEGACY + + +def _notification_accounts(*event_types: list[str]) -> dict[str, Any]: + return { + "status": "completed", + "accounts": [ + { + "account_id": "a1", + "name": "Acct", + "status": "active", + "notification_configs": [ + { + "subscriber_id": f"s{index}", + "url": f"https://buyer.example/hooks/{index}", + "event_types": events, + } + for index, events in enumerate(event_types) + ], + } + ], + } + + +def test_response_drops_a_notification_config_left_without_older_events() -> None: + payload = _notification_accounts( + ["account.change_recorded"], ["account.change_recorded", "product.updated"] + ) + result = convert_payload( + "list_accounts", payload, direction="sync", source_version="3.2", target_version="3.1" + ) + assert result.payload["accounts"][0]["notification_configs"] == [ + { + "subscriber_id": "s1", + "url": "https://buyer.example/hooks/1", + "event_types": ["product.updated"], + } + ] + assert [(loss.pointer, loss.reason) for loss in result.losses] == [ + ("/accounts/0/notification_configs/0/event_types/0", "enum_unavailable"), + ("/accounts/0/notification_configs/0", "unrepresentable_in_target"), + ("/accounts/0/notification_configs/1/event_types/0", "enum_unavailable"), + ] + assert validate_response("list_accounts", result.payload, version="3.1").valid + + +def test_request_omits_a_list_it_cannot_send_whole() -> None: + # A shortened sync list can read as removal, so the optional field goes. + request = { + "idempotency_key": "sync-accounts-notification-configs-0001", + "accounts": [ + { + "brand": {"domain": "acme.example"}, + "operator": "agency.example", + "billing": "operator", + **_notification_accounts(["account.change_recorded"], ["product.updated"])[ + "accounts" + ][0], + } + ], + } + for key in ("account_id", "name", "status"): + request["accounts"][0].pop(key) + assert validate_request("sync_accounts", request, version="3.2").valid + result = convert_payload( + "sync_accounts", request, direction="request", source_version="3.2", target_version="3.1" + ) + assert "notification_configs" not in result.payload["accounts"][0] + assert ( + "/accounts/0/notification_configs", + "unrepresentable_in_target", + ) in [(loss.pointer, loss.reason) for loss in result.losses] + assert validate_request("sync_accounts", result.payload, version="3.1").valid + + +def test_request_without_an_optional_holder_fails_structurally() -> None: + request = { + "idempotency_key": "sync-accounts-by-reference-0001", + "accounts": [{"account": {"account_id": "acct_1"}, "name": "Acme"}], + } + with pytest.raises(ADCPVersionConversionError) as caught: + convert_payload( + "sync_accounts", + request, + direction="request", + source_version="3.2", + target_version="3.0", + ) + assert caught.value.pointer == "/accounts/0" + + +@pytest.mark.parametrize( + ("declaration", "expected"), + [ + ({"countries": {"US": {"values": ["US-NY", "US-CA"]}}}, True), + ({"countries": {"US": {"all_values": True}}}, True), + ({"countries": {}}, False), + ], +) +def test_structured_capability_coarsens_to_the_older_boolean(declaration, expected) -> None: + capabilities = { + "status": "completed", + "adcp": { + "major_versions": [3], + "supported_versions": ["3.1", "3.2"], + "idempotency": {"supported": False}, + }, + "supported_protocols": ["media_buy"], + "media_buy": {"execution": {"targeting": {"geo_regions": declaration}}}, + } + result = convert_payload( + "get_adcp_capabilities", + capabilities, + direction="sync", + source_version="3.2", + target_version="3.1", + ) + assert result.payload["media_buy"]["execution"]["targeting"]["geo_regions"] is expected + assert [(loss.pointer, loss.reason) for loss in result.losses] == [ + ("/media_buy/execution/targeting/geo_regions", "coarsened_to_boolean") + ] + assert validate_response("get_adcp_capabilities", result.payload, version="3.1").valid + + +def test_payloads_that_arrive_invalid_are_projected_not_repaired() -> None: + # Missing required product fields are the sender's problem: projection + # still drops the 3.2-only field but neither discards the product nor + # turns the response into a conversion error. + product = {"product_id": "p1", "audience_evidence": [{"evidence_id": "e1"}]} + result = convert_payload( + "get_products", + {"status": "completed", "cache_scope": "public", "products": [product]}, + direction="sync", + source_version="3.2", + target_version="3.1", + ) + assert result.payload["products"] == [{"product_id": "p1"}] + assert [loss.pointer for loss in result.losses] == ["/products/0/audience_evidence"] diff --git a/tests/test_version_conversion_sweep.py b/tests/test_version_conversion_sweep.py new file mode 100644 index 000000000..8dcc99f17 --- /dev/null +++ b/tests/test_version_conversion_sweep.py @@ -0,0 +1,146 @@ +"""Every valid 3.2 payload converts down to a valid older payload, or fails structurally. + +Sources: each bundled 3.2 schema example, the canonical-valid validation +parity cases, the ESA audience-evidence product and capabilities vectors, and +``tests/fixtures/version_conversion_sweep.json``. A conversion may raise +``ADCPVersionConversionError`` only where ``EXPECTED_STRUCTURED_ERRORS`` says +so; any other result must validate against the target release's bundled schema. +""" + +from __future__ import annotations + +import json +import logging +from copy import deepcopy +from importlib.resources import files +from pathlib import Path +from typing import Any + +import pytest + +from adcp.compat.versions import ADCPVersionConversionError, convert_payload +from adcp.validation import validate_request, validate_response +from adcp.validation.schema_loader import get_portable_schema, list_validator_keys + +SOURCE = "3.2" +TARGETS = ("3.1", "3.0") +FIXTURES = Path(__file__).parent / "fixtures" + +#: (case id, target) -> pointer of the intentional structured error. +EXPECTED_STRUCTURED_ERRORS = { + # Test-controller scenarios added after 3.0 have no 3.0 spelling. + ("comply_test_controller::request::example-10", "3.0"): "/scenario", + ("comply_test_controller::sync::example-0", "3.0"): "/scenarios/1", + # convert_payload cannot invent a legacy format_id; the creative dialect + # adapters supply one before conversion on real 3.0 boundaries. A request + # fails, while a response drops the creative and records the loss. + ("sync_creatives::request::example-0", "3.0"): "/creatives/0", + ("sync_creatives::request::example-1", "3.0"): "/creatives/0", + ("sync_creatives::request::example-2", "3.0"): "/creatives/0", + # 3.0 accounts need a brand/operator key; an account_id reference has none. + ("sync_accounts::request::example-5", "3.0"): "/accounts/0", + ("sync_accounts::request::example-6", "3.0"): "/accounts/0", + # Wholesale discovery does not exist in 3.0. + ("parity::get_signals.valid_wholesale", "3.0"): "", + # 3.0 media_buy sellers must declare account capabilities. + ("fixture::issue1621.get_adcp_capabilities.geo_regions", "3.0"): "", + ("vector::audience_evidence.capabilities", "3.0"): "", +} + + +def _validate(tool: str, direction: str, payload: dict[str, Any], version: str) -> Any: + validator = validate_request if direction == "request" else validate_response + return validator(tool, payload, version=version) + + +def _cases() -> list[tuple[str, str, str, dict[str, Any]]]: + previous = logging.root.manager.disable + logging.disable(logging.CRITICAL) + try: + cases = [] + for key in list_validator_keys(version=SOURCE): + tool, direction = key.split("::") + schema = get_portable_schema(tool, direction, version=SOURCE) or {} + for index, example in enumerate(schema.get("examples") or []): + data = example.get("data") if isinstance(example, dict) else None + if isinstance(data, dict): + cases.append((f"{key}::example-{index}", tool, direction, data)) + finally: + logging.disable(previous) + for case in json.loads((FIXTURES / "validation-parity-cases.json").read_text()): + if case["canonical_valid"]: + cases.append( + (f"parity::{case['id']}", case["tool"], case["direction"], case["payload"]) + ) + vectors = json.loads( + files("adcp") + .joinpath("_compliance/3.2.4/test-vectors/audience-evidence/vectors.json") + .read_text() + ) + product = deepcopy(vectors["product"]) + product["audience_evidence"] = [vectors["evidence"]] + product["included_signals"] = [ + { + "signal_ref": {"scope": "product", "signal_id": "adults_25_34"}, + "name": "Adults 25-34", + "value_type": "binary", + "demographic_predicate": {"age": {"min": 25, "max": 34, "include_unknown": False}}, + "restricted_attributes": ["age"], + } + ] + cases.append( + ( + "vector::audience_evidence.get_products", + "get_products", + "sync", + {"status": "completed", "cache_scope": "public", "products": [product]}, + ) + ) + cases.append( + ( + "vector::audience_evidence.capabilities", + "get_adcp_capabilities", + "sync", + vectors["capability_response"], + ) + ) + for case in json.loads((FIXTURES / "version_conversion_sweep.json").read_text())["cases"]: + cases.append((f"fixture::{case['id']}", case["tool"], case["direction"], case["payload"])) + return cases + + +CASES = _cases() + + +def test_sweep_covers_every_bundled_example_and_fixture() -> None: + assert len({case_id for case_id, *_ in CASES}) == len(CASES) + assert sum(case_id.startswith("fixture::") for case_id, *_ in CASES) == 4 + assert len(CASES) > 90 + + +@pytest.mark.parametrize("target", TARGETS) +@pytest.mark.parametrize( + ("case_id", "tool", "direction", "payload"), CASES, ids=[case[0] for case in CASES] +) +def test_downgrade_is_valid_in_the_target_or_a_structured_error( + case_id: str, tool: str, direction: str, payload: dict[str, Any], target: str +) -> None: + assert _validate(tool, direction, payload, SOURCE).valid, "sweep inputs must be valid 3.2" + if get_portable_schema(tool, direction, version=target) is None: + pytest.skip(f"{tool} {direction} does not exist in AdCP {target}") + expected = EXPECTED_STRUCTURED_ERRORS.get((case_id, target)) + try: + result = convert_payload( + tool, + payload, + direction=direction, + source_version=SOURCE, + target_version=target, + ) + except ADCPVersionConversionError as exc: + assert expected is not None, f"unexpected structured error: {exc}" + assert exc.pointer == expected + return + assert expected is None, "expected a structured error; update EXPECTED_STRUCTURED_ERRORS" + outcome = _validate(tool, direction, result.payload, target) + assert outcome.valid, outcome.issues From 5f6d79b80ab7d41605635fb9cbdb358a30bd780c Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 26 Oct 2026 00:32:50 +0000 Subject: [PATCH 2/3] fix(compat): blame only drops that cause a refusal; never repair requests Review follow-ups: - A node is dropped only when the drop caused the refusal: the target must accept it with the dropped children restored. Otherwise the final check raises, so a 3.1 delivery row missing spend is not silently discarded just because a 3.2-only sibling was also dropped. - Requests are never repaired by omission: an absent filter widens the search and a shortened sync list can read as removal. They fail with a structured error instead. - The final check validates against the same portable schemas the projection used, without the validator's payload size cap. - Structured-error reasons are value-free; pointers from the final check locate the value in the converted payload. - Chained $ref nodes resolve fully for the shape checks. - convert_payload(require_valid_target=False) for evidence reconstruction, used by the negotiated catalog purchase round-trip. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/cross-version-conversion.md | 51 +++++--- .../compat/negotiated_catalog_purchase.py | 3 + src/adcp/compat/versions.py | 122 +++++++++++++----- tests/test_cross_version_conversion.py | 80 +++++++++--- tests/test_version_conversion_sweep.py | 21 ++- 5 files changed, 201 insertions(+), 76 deletions(-) diff --git a/docs/cross-version-conversion.md b/docs/cross-version-conversion.md index 890d78caf..2fadf6b4c 100644 --- a/docs/cross-version-conversion.md +++ b/docs/cross-version-conversion.md @@ -114,33 +114,42 @@ wrappers remain opaque as well, bounding conversion to one wrapper level. Dropping a newer field or enum member can leave its parent in a shape the older release refuses: a notification config whose `event_types` were all -3.2-only becomes an empty list below 3.1's `minItems: 1`, and an account row -identified only by a 3.2 `account` reference loses its identity. When a -projected node fails its own constraints in the older schema (`required`, -`minItems`, `minProperties`, unions or `if`/`then`) and the newer schema -accepted the original, the SDK resolves it by one rule: +3.2-only becomes an empty list below 3.1's `minItems: 1`. When a node loses or +coarsens a direct child, the SDK checks the node's own constraints in the older +schema (`required`, `minItems`, `minProperties`, unions, `if`/`then`). A +refusal counts as caused by the drop only when the newer schema accepted the +original and the older schema accepts the node with the dropped children put +back. Such a node is resolved by one rule: | Situation | Result | Loss `reason` | | --- | --- | --- | | Newer `boolean \| object` capability, older boolean only (3.2 `geo_regions`) | `true` when the structure declares any support, otherwise `false` | `coarsened_to_boolean` | -| Refused node is an optional field | Field omitted | `unrepresentable_in_target` | -| Refused node is a response array item | Item omitted | `unrepresentable_in_target` | -| Refused node is a request array item | The nearest optional field holding the list is omitted | `unrepresentable_in_target` | -| Only required fields lead to the root | `ADCPVersionConversionError` at the refused value | — | - -Requests never send a shortened list. Sync tasks support `delete_missing`, -so a partial list could read as removal; omitting the field leaves that state -unchanged. Losses already recorded inside a dropped node stay in the report, -explaining why it was dropped. +| Response: refused node is an optional field | Field omitted | `unrepresentable_in_target` | +| Response: refused node is an array item | Item omitted | `unrepresentable_in_target` | +| Request: any refused node | `ADCPVersionConversionError` | none | +| Response: only required fields lead to the root | `ADCPVersionConversionError` | none | + +Requests are never repaired by omission, because omitting a request field +changes its meaning: a shortened sync list can read as removal under +`delete_missing`, and an absent filter matches everything. A request whose +`filters.required_metrics` were all 3.2-only therefore fails rather than +widening the search. Optional newer members are still dropped one at a time +(`enum_unavailable`) when others remain, as before. Losses already recorded +inside a dropped response node stay in the report, explaining why it went. Every downgrade then validates the result against the older release's bundled -schema. A remaining refusal, such as a field the older release requires and -the payload lacks or a required enum member it cannot spell, fails with -`ADCPVersionConversionError` instead of reaching the peer invalid. A payload -that was already invalid in its own release is projected but neither repaired -nor rejected; the caller's validation policy decides. The server returns these -errors as `VERSION_UNSUPPORTED`; the client returns a failed `TaskResult` with -`adcp_error.code="UNSUPPORTED_FEATURE"`. +schema. A refusal the projection did not cause fails with +`ADCPVersionConversionError` instead of reaching the peer invalid. Examples are +a field the older release requires and the payload never had, such as 3.1 +delivery `totals.spend`, and a required enum member the older release cannot +spell. That error's pointer locates the value in the converted payload. Its +reason never echoes payload values. A payload already invalid in its own +release is projected but neither repaired nor rejected; the caller's +validation policy decides. The server returns these errors as +`VERSION_UNSUPPORTED`, including after a handler has run. The client returns a +failed `TaskResult` with `adcp_error.code="UNSUPPORTED_FEATURE"`. Code that +reconstructs a peer's own evidence, rather than sending a payload, can pass +`require_valid_target=False` to `convert_payload` to skip the final check. `tests/test_version_conversion_sweep.py` converts every bundled 3.2 schema example and fixture down to 3.1 and 3.0 and requires each result to validate, diff --git a/src/adcp/compat/negotiated_catalog_purchase.py b/src/adcp/compat/negotiated_catalog_purchase.py index 0896a3278..ea30ac0a9 100644 --- a/src/adcp/compat/negotiated_catalog_purchase.py +++ b/src/adcp/compat/negotiated_catalog_purchase.py @@ -1014,6 +1014,9 @@ def _completed_payload( # Round-trips the client's own lossless upgrade back to # the served contract; only upgrade defaults can drop. loss_log_level=logging.DEBUG, + # Reconstructs the seller's evidence faithfully, even + # where the seller's own response was invalid. + require_valid_target=False, ).payload return payload diff --git a/src/adcp/compat/versions.py b/src/adcp/compat/versions.py index 99952c9ee..571c0f347 100644 --- a/src/adcp/compat/versions.py +++ b/src/adcp/compat/versions.py @@ -31,7 +31,7 @@ _ARRAY_INDEX = re.compile(r"/\d+(?=/|$)") _SUMMARY_POINTER_LIMIT = 10 #: Compiled node validators keyed by schema identity; see :func:`_refusal`. -_NODE_VALIDATORS: dict[tuple[int, int], tuple[Any, Any, Any]] = {} +_NODE_VALIDATORS: dict[tuple[int, int, bool], tuple[Any, Any, Any]] = {} _NODE_VALIDATOR_LIMIT = 4096 @@ -246,9 +246,19 @@ def _property(schema: dict[str, Any], key: str) -> Any: ) +def _resolve_chain(schema: Any, root: dict[str, Any]) -> dict[str, Any]: + """Follow ``$ref`` until it reaches a node with its own keywords.""" + resolved = _resolve(schema, root) + for _ in range(32): + if "$ref" not in resolved: + break + resolved = _resolve(resolved, root) + return resolved + + def _shape_keywords(schema: Any, root: dict[str, Any]) -> frozenset[str]: """The node's own constraints that removing children could violate.""" - resolved = _resolve(schema, root) + resolved = _resolve_chain(schema, root) found = _SHAPE_KEYWORDS & resolved.keys() for branch in resolved.get("allOf", []): found |= _shape_keywords(branch, root) @@ -257,7 +267,7 @@ def _shape_keywords(schema: Any, root: dict[str, Any]) -> frozenset[str]: def _shape_schema(schema: Any, root: dict[str, Any]) -> dict[str, Any]: """The node's shape keywords alone, without ``properties`` or ``items``.""" - resolved = _resolve(schema, root) + resolved = _resolve_chain(schema, root) own = {key: resolved[key] for key in _SHAPE_KEYWORDS if key in resolved} if "if" in own: own.update({key: resolved[key] for key in ("then", "else") if key in resolved}) @@ -265,18 +275,21 @@ def _shape_schema(schema: Any, root: dict[str, Any]) -> dict[str, Any]: return {**own, "allOf": [branch for branch in branches if branch]} if branches else own -def _refusal(schema: Any, root: dict[str, Any], value: Any) -> tuple[str, str] | None: - """Return where and why the node's own shape refuses ``value``, or ``None``. +def _refusal( + schema: Any, root: dict[str, Any], value: Any, *, shape: bool = True +) -> tuple[str, str] | None: + """Return where and why ``schema`` refuses ``value``, or ``None``. - Only the node's shape keywords run (see :func:`_shape_schema`); its - children are checked where they were projected. The pointer is relative - to ``value``. Draft-07 ignores ``$ref`` siblings, so the wrapper - validates only the node while every ``#/...`` pointer in it still - resolves against the original document. + With ``shape`` only the node's own shape keywords run (see + :func:`_shape_schema`); its children are checked where they were + projected. Otherwise the whole subtree is validated. The pointer is + relative to ``value``. Draft-07 ignores ``$ref`` siblings, so the + wrapper validates only the node while every ``#/...`` pointer in it + still resolves against the original document. """ if not isinstance(schema, dict) or not schema: return None - key = (id(schema), id(root)) + key = (id(schema), id(root), shape) cached = _NODE_VALIDATORS.get(key) if cached is None or cached[0] is not schema or cached[1] is not root: if len(_NODE_VALIDATORS) >= _NODE_VALIDATOR_LIMIT: @@ -286,7 +299,7 @@ def _refusal(schema: Any, root: dict[str, Any], value: Any) -> tuple[str, str] | "$ref": "#/$defs/__adcp_conversion_node", "$defs": { **root.get("$defs", {}), - "__adcp_conversion_node": _shape_schema(schema, root), + "__adcp_conversion_node": _shape_schema(schema, root) if shape else schema, }, } cached = (schema, root, validator_for(root)(document)) @@ -300,7 +313,21 @@ def _refusal(schema: Any, root: dict[str, Any], value: Any) -> tuple[str, str] | pointer = "" for key in error.absolute_path: pointer = _pointer(pointer, key) - return pointer, error.message + return pointer, _describe(error) + + +def _describe(error: Any) -> str: + """A value-free reason: diagnostics never echo payload contents.""" + keyword = error.validator + if keyword == "required": + missing = [name for name in error.validator_value if name not in error.instance] + return f"missing required field(s): {', '.join(map(str, missing))}" + if keyword in {"minItems", "minProperties"}: + unit = "item" if keyword == "minItems" else "field" + return f"needs at least {error.validator_value} {unit}(s)" + if keyword in {"enum", "const"}: + return "value has no spelling in the older release" + return f"fails the older schema's {keyword} constraint" def _coarsens_to_boolean(source: Any, target: dict[str, Any], root: dict[str, Any]) -> bool: @@ -487,7 +514,9 @@ def _project( response=response, ) except _UnrepresentableError: - if key in target["required"]: + if key in target["required"] or not response: + # Omitting a request field changes its meaning, e.g. an + # absent filter matches everything; the caller decides. raise losses.append( ConversionLoss( @@ -542,8 +571,7 @@ def _project( except _UnrepresentableError: if not response: # A shortened request list can read as removal, e.g. a - # sync with delete_missing. Omit the optional field that - # holds it instead, or fail when none is optional. + # sync with delete_missing. raise losses.append( ConversionLoss( @@ -582,17 +610,28 @@ def _check_representable( """Refuse a node whose own projection left a shape the target rejects. Only nodes that lost or coarsened a direct child are checked, and only - when the source accepted the original: projection must not turn valid - data invalid, but it is not a validator for data that arrived invalid. - The parent drops an optional field or array item and records the loss; - a required chain up to the root becomes a structured conversion error. + when the drop caused the refusal: the source accepted the original and + the target accepts the node with those children put back. A refusal + the drop did not cause, such as a field the older release requires and + the payload never had, is left to :func:`_require_target_valid`. + Projection must not turn valid data invalid, but it is not a validator + for data that arrived invalid. """ - if not any(_parent_pointer(loss.pointer) == path for loss in losses[first_loss:]): - return - if not _shape_keywords(target, target_root): + dropped = [ + loss.pointer.rsplit("/", 1)[1].replace("~1", "/").replace("~0", "~") + for loss in losses[first_loss:] + if _parent_pointer(loss.pointer) == path + ] + if not dropped or not _shape_keywords(target, target_root): return refusal = _refusal(target, target_root, projected) - if refusal is not None and _refusal(source, source_root, value) is None: + if refusal is None or _refusal(source, source_root, value) is not None: + return + if isinstance(value, dict): + restored: Any = {**projected, **{key: value[key] for key in dropped if key in value}} + else: + restored = [*projected, *(value[int(index)] for index in dropped)] + if _refusal(target, target_root, restored) is None: raise _UnrepresentableError(path + refusal[0], refusal[1]) @@ -713,6 +752,7 @@ def _convert_embedded_results( source_version: str, target_version: str, losses: list[ConversionLoss], + require_valid_target: bool, ) -> None: """Use task identity for opaque polling artifacts, bounded to one wrapper level.""" if tool == "get_task_status": @@ -751,6 +791,7 @@ def _convert_embedded_results( direction=direction, source_version=source_version, target_version=target_version, + require_valid_target=require_valid_target, ) except ADCPVersionConversionError as exc: raise ADCPVersionConversionError( @@ -777,6 +818,7 @@ def convert_payload( target_version: str, request: dict[str, Any] | None = None, loss_log_level: int = logging.WARNING, + require_valid_target: bool = True, ) -> ConversionResult: """Chain adjacent minor conversions without modifying the caller's data. @@ -789,6 +831,11 @@ def convert_payload( a buyer losing a seller's data keeps ``WARNING``, while a seller dropping newer fields for an older buyer expects those losses and logs them at ``DEBUG`` (see :func:`log_conversion_summary`). + + A downgrade raises :class:`ADCPVersionConversionError` rather than return + a payload the older schema refuses. ``require_valid_target=False`` skips + only that final check, for callers reconstructing a peer's own evidence + rather than sending a payload; projection repairs still apply. """ converted = _convert_payload( tool, @@ -797,6 +844,7 @@ def convert_payload( source_version=source_version, target_version=target_version, request=request, + require_valid_target=require_valid_target, ) for loss in converted.losses: logger.log( @@ -849,6 +897,7 @@ def _convert_payload( source_version: str, target_version: str, request: dict[str, Any] | None = None, + require_valid_target: bool = True, ) -> ConversionResult: """Convert without logging; :func:`convert_payload` logs the final losses once.""" source_version, target_version = _release(source_version), _release(target_version) @@ -945,8 +994,10 @@ def _convert_payload( elif direction != "request" or "adcp_version" in payload: result["adcp_version"] = target_version if direction != "request": - _convert_embedded_results(tool, result, source_version, target_version, losses) - if step < 0: + _convert_embedded_results( + tool, result, source_version, target_version, losses, require_valid_target + ) + if step < 0 and require_valid_target: _require_target_valid(tool, direction, payload, result, source_version, target_version) return ConversionResult(result, tuple(losses)) @@ -965,18 +1016,19 @@ def _require_target_valid( requires and the payload cannot supply, such as a newly optional field or a required enum member it lacks, fails as a structured error rather than reaching the peer invalid. Payloads that arrived invalid are left - to the caller's validation policy. + to the caller's validation policy. Validation uses the same bundled + schemas the projection used, with no payload size cap. """ - from adcp.validation.schema_validator import validate_request, validate_response - - validate = validate_request if direction == "request" else validate_response - outcome = validate(tool, result, version=target_version) - if outcome.valid or not validate(tool, payload, version=source_version).valid: + target_schema = _schema(tool, direction, target_version) + refusal = _refusal(target_schema, target_schema, result, shape=False) + if refusal is None: + return + source_schema = _schema(tool, direction, source_version) + if _refusal(source_schema, source_schema, payload, shape=False) is not None: return - issue = outcome.issues[0] raise ADCPVersionConversionError( - pointer=issue.pointer, + pointer=refusal[0], source_version=source_version, target_version=target_version, - reason=f"the older contract refuses the converted payload: {issue.message}", + reason=f"the older contract refuses the converted payload at that pointer: {refusal[1]}", ) diff --git a/tests/test_cross_version_conversion.py b/tests/test_cross_version_conversion.py index 88a0c8099..df1d3dec2 100644 --- a/tests/test_cross_version_conversion.py +++ b/tests/test_cross_version_conversion.py @@ -838,8 +838,9 @@ def test_response_drops_a_notification_config_left_without_older_events() -> Non assert validate_response("list_accounts", result.payload, version="3.1").valid -def test_request_omits_a_list_it_cannot_send_whole() -> None: - # A shortened sync list can read as removal, so the optional field goes. +def test_request_is_never_repaired_by_omission() -> None: + # Omitting a request field changes its meaning: a shortened sync list can + # read as removal, an absent filter matches everything. request = { "idempotency_key": "sync-accounts-notification-configs-0001", "accounts": [ @@ -847,24 +848,71 @@ def test_request_omits_a_list_it_cannot_send_whole() -> None: "brand": {"domain": "acme.example"}, "operator": "agency.example", "billing": "operator", - **_notification_accounts(["account.change_recorded"], ["product.updated"])[ - "accounts" - ][0], + "notification_configs": _notification_accounts( + ["account.change_recorded"], ["product.updated"] + )["accounts"][0]["notification_configs"], } ], } - for key in ("account_id", "name", "status"): - request["accounts"][0].pop(key) assert validate_request("sync_accounts", request, version="3.2").valid - result = convert_payload( - "sync_accounts", request, direction="request", source_version="3.2", target_version="3.1" - ) - assert "notification_configs" not in result.payload["accounts"][0] - assert ( - "/accounts/0/notification_configs", - "unrepresentable_in_target", - ) in [(loss.pointer, loss.reason) for loss in result.losses] - assert validate_request("sync_accounts", result.payload, version="3.1").valid + with pytest.raises(ADCPVersionConversionError) as caught: + convert_payload( + "sync_accounts", + request, + direction="request", + source_version="3.2", + target_version="3.1", + ) + assert caught.value.pointer == "/accounts/0/notification_configs/0/event_types" + + +def test_request_filter_is_not_widened_by_omission() -> None: + request = { + "buying_mode": "brief", + "brief": "video", + "filters": {"required_metrics": ["viewable_impressions"], "channels": ["olv"]}, + } + assert validate_request("get_products", request, version="3.2").valid + with pytest.raises(ADCPVersionConversionError) as caught: + convert_payload( + "get_products", + request, + direction="request", + source_version="3.2", + target_version="3.1", + ) + assert caught.value.pointer == "/filters/required_metrics" + + +def test_a_drop_does_not_take_the_blame_for_an_unrelated_refusal() -> None: + # spend became optional in 3.2. Dropping the 3.2-only sibling did not + # cause its absence, so the delivery row is not silently discarded. + def delivery(**totals: Any) -> dict[str, Any]: + return { + "status": "completed", + "reporting_period": {"start": "2026-10-01T00:00:00Z", "end": "2026-10-02T00:00:00Z"}, + "currency": "USD", + "media_buy_deliveries": [ + { + "media_buy_id": "mb1", + "status": "active", + "totals": {"impressions": 100, **totals}, + "by_package": [], + } + ], + } + + for payload in (delivery(), delivery(commissionable_value=5)): + assert validate_response("get_media_buy_delivery", payload, version="3.2").valid + with pytest.raises(ADCPVersionConversionError) as caught: + convert_payload( + "get_media_buy_delivery", + payload, + direction="sync", + source_version="3.2", + target_version="3.1", + ) + assert caught.value.pointer == "/media_buy_deliveries/0/totals" def test_request_without_an_optional_holder_fails_structurally() -> None: diff --git a/tests/test_version_conversion_sweep.py b/tests/test_version_conversion_sweep.py index 8dcc99f17..9d07117df 100644 --- a/tests/test_version_conversion_sweep.py +++ b/tests/test_version_conversion_sweep.py @@ -32,19 +32,32 @@ ("comply_test_controller::request::example-10", "3.0"): "/scenario", ("comply_test_controller::sync::example-0", "3.0"): "/scenarios/1", # convert_payload cannot invent a legacy format_id; the creative dialect - # adapters supply one before conversion on real 3.0 boundaries. A request - # fails, while a response drops the creative and records the loss. + # adapters supply one before conversion on real 3.0 boundaries. + ("list_creatives::sync::example-0", "3.0"): "/creatives/1", + ("list_creatives::sync::example-1", "3.0"): "/creatives/1", ("sync_creatives::request::example-0", "3.0"): "/creatives/0", ("sync_creatives::request::example-1", "3.0"): "/creatives/0", - ("sync_creatives::request::example-2", "3.0"): "/creatives/0", - # 3.0 accounts need a brand/operator key; an account_id reference has none. + ("sync_creatives::request::example-2", "3.0"): "/creatives/1", + # Older accounts need a brand/operator key; an account_id reference has none. ("sync_accounts::request::example-5", "3.0"): "/accounts/0", ("sync_accounts::request::example-6", "3.0"): "/accounts/0", + ("sync_accounts::sync::example-5", "3.1"): "", + ("sync_accounts::sync::example-5", "3.0"): "", # Wholesale discovery does not exist in 3.0. ("parity::get_signals.valid_wholesale", "3.0"): "", # 3.0 media_buy sellers must declare account capabilities. ("fixture::issue1621.get_adcp_capabilities.geo_regions", "3.0"): "", ("vector::audience_evidence.capabilities", "3.0"): "", + # Requests are never repaired: a config left without older event types + # fails instead of silently changing the subscription. + ( + "fixture::sync_accounts.request.notification_configs", + "3.1", + ): "/accounts/0/notification_configs/0/event_types", + ( + "fixture::sync_accounts.request.notification_configs", + "3.0", + ): "/accounts/0/notification_configs/0/event_types", } From bbfafae638c0cb71d7df98295ad40ed5b725da58 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 26 Oct 2026 00:41:43 +0000 Subject: [PATCH 3/3] fix(compat): keep the final check on for status-less responses The SDK validators default a missing status envelope; the final check now does the same, so a direct caller converting a status-less 3.2 response still gets the guarantee. Also restore only removed (not coarsened) list items when attributing a refusal, and word false-schema refusals. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/adcp/compat/versions.py | 16 +++++++++++++- tests/test_version_conversion_sweep.py | 30 ++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/adcp/compat/versions.py b/src/adcp/compat/versions.py index 571c0f347..65aae532e 100644 --- a/src/adcp/compat/versions.py +++ b/src/adcp/compat/versions.py @@ -327,6 +327,8 @@ def _describe(error: Any) -> str: return f"needs at least {error.validator_value} {unit}(s)" if keyword in {"enum", "const"}: return "value has no spelling in the older release" + if keyword is None: + return "the older schema allows no value here" return f"fails the older schema's {keyword} constraint" @@ -630,7 +632,13 @@ def _check_representable( if isinstance(value, dict): restored: Any = {**projected, **{key: value[key] for key in dropped if key in value}} else: - restored = [*projected, *(value[int(index)] for index in dropped)] + # Only removed items are restored; a coarsened item is still present. + removed = { + int(loss.pointer.rsplit("/", 1)[1]) + for loss in losses[first_loss:] + if _parent_pointer(loss.pointer) == path and loss.reason != "coarsened_to_boolean" + } + restored = [*projected, *(value[index] for index in sorted(removed))] if _refusal(target, target_root, restored) is None: raise _UnrepresentableError(path + refusal[0], refusal[1]) @@ -1019,6 +1027,12 @@ def _require_target_valid( to the caller's validation policy. Validation uses the same bundled schemas the projection used, with no payload size cap. """ + from adcp.validation._response_envelope import uses_task_status_envelope + + if direction != "request" and uses_task_status_envelope(tool): + # Match the SDK validators, which default the status envelope. + payload = {"status": "completed", **payload} + result = {"status": "completed", **result} target_schema = _schema(tool, direction, target_version) refusal = _refusal(target_schema, target_schema, result, shape=False) if refusal is None: diff --git a/tests/test_version_conversion_sweep.py b/tests/test_version_conversion_sweep.py index 9d07117df..24c548f4a 100644 --- a/tests/test_version_conversion_sweep.py +++ b/tests/test_version_conversion_sweep.py @@ -157,3 +157,33 @@ def test_downgrade_is_valid_in_the_target_or_a_structured_error( assert expected is None, "expected a structured error; update EXPECTED_STRUCTURED_ERRORS" outcome = _validate(tool, direction, result.payload, target) assert outcome.valid, outcome.issues + + +STATUSLESS = [case for case in CASES if case[2] != "request" and "status" in case[3]] + + +@pytest.mark.parametrize("target", TARGETS) +@pytest.mark.parametrize( + ("case_id", "tool", "direction", "payload"), + STATUSLESS, + ids=[case[0] for case in STATUSLESS], +) +def test_status_defaulted_responses_get_the_same_guarantee( + case_id: str, tool: str, direction: str, payload: dict[str, Any], target: str +) -> None: + # The SDK validators default a missing status envelope; so does the + # downgrade's final check, which must not switch off without it. + payload = {key: value for key, value in payload.items() if key != "status"} + if not _validate(tool, direction, payload, SOURCE).valid: + pytest.skip("not valid 3.2 without its status") + if get_portable_schema(tool, direction, version=target) is None: + pytest.skip(f"{tool} {direction} does not exist in AdCP {target}") + try: + result = convert_payload( + tool, payload, direction=direction, source_version=SOURCE, target_version=target + ) + except ADCPVersionConversionError: + assert (case_id, target) in EXPECTED_STRUCTURED_ERRORS + return + assert (case_id, target) not in EXPECTED_STRUCTURED_ERRORS + assert _validate(tool, direction, result.payload, target).valid