Skip to content

fix(server): project postal capabilities by AdCP version#952

Merged
bokelley merged 3 commits into
mainfrom
adcp-client-python-951
Jun 14, 2026
Merged

fix(server): project postal capabilities by AdCP version#952
bokelley merged 3 commits into
mainfrom
adcp-client-python-951

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • add a shared project_geo_postal_areas helper for AdCP version-aware postal capability serialization
  • project get_adcp_capabilities postal declarations to legacy fused booleans for unversioned / 3.0 callers and native country-keyed maps for 3.1+ callers
  • document the automatic projection behavior and add changelog coverage

Expert review

  • ad-tech protocol expert: no blocking protocol issues; requested postal-bearing schema validation tests, now added
  • code reviewer: found future/custom ISO country keys were dropped by the public helper; fixed by preserving two-letter uppercase country keys for native projection

Validation

  • PYTHONPATH=src python3 -m pytest tests/test_decisioning_capabilities_projection.py tests/test_public_api.py tests/test_import_layering.py
  • PYTHONPATH=src mypy src/
  • ruff check tests/test_decisioning_capabilities_projection.py src/adcp/types/projections.py src/adcp/decisioning/handler.py src/adcp/decisioning/platform.py src/adcp/types/__init__.py src/adcp/types/capabilities.py
  • black --check tests/test_decisioning_capabilities_projection.py src/adcp/types/projections.py src/adcp/decisioning/handler.py src/adcp/decisioning/platform.py src/adcp/types/__init__.py src/adcp/types/capabilities.py
  • pre-commit hooks during commit: black, ruff, mypy, type-check fixtures, bandit, JSON/YAML/check hygiene

Closes #951

Comment thread src/adcp/types/projections.py Fixed
@bokelley bokelley marked this pull request as ready for review June 14, 2026 12:34
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes Jun 14, 2026

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clean version-aware projection of geo_postal_areas. Right shape: one typed GeoPostalAreas declaration, the framework emits only what the caller negotiated, and it fail-closes to the legacy 3.0 booleans for unversioned callers.

Things I checked

  • Wire shape, both endpoints. ad-tech-protocol-expert: sound. Diffed the projection output against the cached schemas — 3.0 (schemas/cache/3.0/protocol/get-adcp-capabilities-response.json:266) is legacy-booleans-only with additionalProperties: false, so dropping the alias-less native systems (BR cep, IN pin, ZA postal_code) from 3.0 projections isn't just acceptable, it's the only schema-valid behavior — emitting them would fail validation. 3.1 (postal-area-support.json) blesses native+legacy coexistence, so normalizing a 3.1 caller's deprecated booleans into native shape is a valid choice, and the ^[A-Z]{2}$ passthrough for future country keys matches the schema's propertyNames anyOf.
  • Mapping completeness. _NATIVE_TO_LEGACY_POSTAL covers all 11 LegacyPostalCodeSystem members exactly and inverts bijectively. False/absent booleans treated as absent, so projection never invents support.
  • Version gate. _is_native_geo_postal_version (projections.py): None→legacy, unparseable→legacy (ValueError caught), 3.0→legacy, 3.1/3.1.0-rc.13→native. normalize_to_release_precision strips the patch component first, so minor_raw is always single-digit — no int("1.0") blowup. Fail-closed throughout.
  • Handler dump-then-patch (handler.py:1722-1746): reads geo_postal_areas off the typed model, mutates the model_dump dict, and the cascade pop (targetingexecution) only fires on genuinely empty dicts — sibling keys like trusted_match/creative_specs survive. Correct, and the empty-projection case is covered by test_native_postal_capabilities_without_legacy_alias_are_omitted_for_30_callers.
  • Import layering: projections.py pulls GeoPostalAreas/LegacyPostalCodeSystem from adcp.types.capabilities (allowlisted), not _generated/generated_poc. Compliant.
  • Public surface: project_geo_postal_areas is additive — __init__.py, __all__, and public_api_snapshot.json all updated consistently. Non-breaking, so fix: carries a valid semver signal. Schema-validation tests assert both endpoints against the bundled validator — the right guard.

Follow-ups (non-blocking — file as issues)

  • fix(server): vs the changelog ### Features entry. The commit type and the changelog section disagree — and feat: is the more honest type, since this is new auto-projection behavior, not a bug fix. Semver is non-breaking either way; just retitle or move the line so release-please notes read straight.
  • Handler-path forward-compat is narrower than the PR framing. GeoPostalAreas is extra='ignore', so an unknown country key (NL) survives only on the public-helper raw-Mapping path — through a real typed model in the handler it's dropped at validation before projection runs. Not a regression (consistent with the SDK's forward-compat posture), but the "preserves future native country keys" story only holds for the helper, not the handler.

Minor nits (non-blocking)

  1. _NATIVE_POSTAL_COUNTRIES is dead code. projections.py defines the 11-element tuple but nothing references it — country filtering is done entirely by _COUNTRY_KEY_RE. ruff won't catch a module-level unused constant, so it ships silently. Delete it.
  2. Redundant inner try/except in _is_native_geo_postal_version. normalize_to_release_precision already guarantees digit-only major.minor, so the int() guard never trips. Harmless defensive code — leave it or drop it.

Approving on the strength of the schema-diff confirmation at both version endpoints plus the bijective mapping check. Follow-ups noted.

aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes Jun 14, 2026

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clean version-aware projection of geo_postal_areas with the right shape — one typed declaration, serialized to whatever the caller negotiated. The native↔legacy mapping is symmetric and schema-complete, and the layering holds.

Things I checked

  • Mapping table is complete and inverse-consistent. _NATIVE_TO_LEGACY_POSTAL (11 entries) matches the 11 LegacyPostalCodeSystem members exactly; _LEGACY_TO_NATIVE_POSTAL is its true inverse, so the native-path _LEGACY_TO_NATIVE_POSTAL[legacy.value] can never KeyError. ad-tech-protocol-expert cross-checked against schemas/cache/3.0/.../get-adcp-capabilities-response.json:266-305 (11 booleans, additionalProperties: false) and the 3.1 core/postal-area-support.json.
  • Exactly-one-shape is correct — for 3.0 it's mandatory. The 3.0 schema forbids native country keys (additionalProperties: false), so emitting both shapes to a 3.0 caller would be schema-invalid. BR cep / IN pin / ZA postal_code have no 3.0 boolean and no native representation in 3.0 at all — silent omission is the only valid option, an AdCP 3.0 expressiveness gap, not a PR defect.
  • False booleans never invent support. Handler passes the model; _geo_postal_payload dumps with exclude_none=True; the is True / is not True guards treat False as absent. Projection is additive-only.
  • Handler cleanup block is non-destructive (handler.py:1722-1742). Operates on a fresh model_dump copy and only pops targeting/execution once empty; sibling media_buy.* and other execution.* keys keep those dicts populated. context is None handled.
  • No layering breach. projections.py imports GeoPostalAreas/LegacyPostalCodeSystem from the allowlisted adcp.types.capabilities, not from _generated/generated_poc — so it doesn't need to be on the test_import_layering.py allowlist and won't trip it. capabilities.py (already allowlisted) is the only file adding a direct _generated import, which is permitted. Both names are stable, not codegen-numbered.
  • Public-API audit: project_geo_postal_areas is a non-breaking addition — __all__, the snapshot fixture (public_api_snapshot.json), and CHANGELOG.md all updated. fix(server) prefix is correct for an additive behavior fix.
  • Largest-file rule (examples/v3_reference_seller/src/platform.py, 203 net): read it. The model_construct path at ~L1487 is the one site worth flagging (follow-up below); the rest — _product_format_options, _normalize_echo_urls, the sandbox/valid_actions additions — is example-contract plumbing and safe.

Follow-ups (non-blocking — file as issues)

  • version is None disagrees with the SDK's own unversioned convention. _is_native_geo_postal_version(None) returns False (→ deprecated 3.0 booleans), but src/adcp/server/responses.py:444 projects adcp_version is None as newest/3.1 (adcp_version is None or _is_adcp_31_or_newer(...)), and base.py:132-137 documents A2A-unversioned traffic as "served as the current SDK shape." Net effect on an A2A-unversioned response: 3.1 media_buy_status from responses.py sitting next to fused-boolean geo_postal_areas from this helper — internally inconsistent, and it silently drops BR/IN/ZA for native-capable unversioned callers. Not a hard break (the deprecated booleans stay valid under 3.1's schema, and MCP is pinned to 3.0 upstream of this code), which is why this is a follow-up, not a block. But two projection layers disagreeing on what None means is a wart. test_unversioned_postal_capabilities_fall_back_to_30_projection codifies the current behavior — if None→3.0 is deliberate, justify it against the responses.py precedent; otherwise align on None→native. ad-tech-protocol-expert: sound-with-caveats, this is the caveat.
  • _NATIVE_POSTAL_COUNTRIES is dead code (projections.py). Defined, never referenced — _iter_native_postal_systems gates on _COUNTRY_KEY_RE instead. It also misleads: it lists BR/IN/ZA as if special-cased when the regex accepts any 2-letter uppercase key. Delete it.
  • project_geo_postal_areas is a new public export with no AGENTS.md mention. Minor docs drift.

Minor nits (non-blocking)

  1. The model_construct hack in the example reimplements an existing helper, wrong. examples/v3_reference_seller/src/platform.py:~1487 builds UpdateMediaBuySuccessResponse via model_construct(... status="completed", media_buy_status=...), bypassing validation and leaving affected_packages as raw unvalidated dicts on a field typed as list-of-models. The generated model already has a mode="before" validator that maps legacy statusmedia_buy_status, and the repo ships update_media_buy_response() (src/adcp/server/responses.py:468) that does this correctly — examples/seller_agent.py uses it. A reference example modeling model_construct over the validated path teaches the wrong pattern. Prefer model_validate or the existing helper.
  2. Post-validation product.format_ids = ... # type: ignore mutation (platform.py:~935) works around codegen dropping ProductFormatDeclaration discriminator fields. Safe (the data was validated as part of product_payload), but tie the comment to the upstream codegen issue number so nobody "cleans it up."

Mapping is right, the wire shapes validate on both 3.0 and 3.1 paths, and the only real caveat ships valid bytes. Approving. The None-handling inconsistency is the one to chase — half this diff is example storyboard fixes riding under a postal-projection title, which is its own small adventure.

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clean version-aware postal projection. Right shape: one typed GeoPostalAreas declaration, serialized to whatever shape the caller negotiated — sellers stop hand-forking 3.0 vs 3.1 capability responses.

Things I checked

  • Mapping table is a complete bijection. _NATIVE_TO_LEGACY_POSTAL (11 pairs) maps exactly onto LegacyPostalCodeSystem's 11 members and the named native country properties in core/postal-area-support.json — US/GB/CA carry both sub-systems, DE/CH/AT/FR/AU one each. No missing or extra pair. _LEGACY_TO_NATIVE_POSTAL[legacy.value] at projections.py:652 therefore can't KeyError.
  • Both wire shapes validate. ad-tech-protocol-expert: sound — native {"US":["zip","zip_plus_four"],"BR":["cep"]} matches the 3.1 postal-area-support ref; legacy only ever emits the 11 closed booleans, so it can't violate 3.0's additionalProperties: false. Tests assert both via validate_response for 3.0 and 3.1.
  • Projection never invents support. payload.get(legacy.value) is True / is not True treats False and None as absent. No-alias native systems (BR cep, IN pin, ZA postal_code) drop from 3.0 — correct lossy down-projection, not a failure.
  • Handler mutation is safe. handler.py:1722-1742 projects off the typed model but rewrites the dumped dict, replacing geo_postal_areas wholesale (no stale native+legacy mix), then pops empty targeting/execution under isinstance(..., dict) guards. Empty-projection (3.0 + BR-only) pops cleanly — covered by test_native_postal_capabilities_without_legacy_alias_are_omitted_for_30_callers.
  • Version gate. major > 3 or (major == 3 and minor >= 1), ValueError-guarded, unversioned → legacy. 3.1.0-rc.13 normalizes to release 3.1 → native. Conservative default is right.
  • Security. security-reviewer: none. geo_postal_areas is static class-level capability metadata, not request- or tenant-derived; resolved_adcp_version is a boolean branch selector, never a sink; no ctx_metadata credential-echo path touched.
  • Public surface. project_geo_postal_areas is additive — tracked in public_api_snapshot.json, __all__, and capabilities.py's LegacyPostalCodeSystem re-export. Layering respected: projections.py imports from adcp.types.capabilities, not from generated_poc/_generated.

Follow-ups (non-blocking — file as issues)

  • Open-union country keys are unreachable through the typed path. The 3.1 schema permits arbitrary ISO countries (NL postal_code, custom) via additionalProperties, but the generated GeoPostalAreas is closed — 11 named countries + 11 booleans. project_geo_postal_areas handles open-union keys correctly when fed a raw dict (that's what test_public_postal_projection_preserves_future_native_country_keys exercises), but the handler reads off the typed model, so a seller can't actually declare NL today. Pre-existing codegen limitation, not introduced here. Closing it means a _forward_compat.py patch on GeoPostalAreas analogous to Format.assets.
  • CHANGELOG hand-edit. The new line lands under ### Features of an already-released version section, and a fix(server): commit filed under Features. release-please regenerates this from commit history — the manual edit will be clobbered or misfiled. Drop it and let the tooling generate it.

Minor nits (non-blocking)

  1. update_media_buy switched to model_construct. examples/v3_reference_seller/src/platform.py (update_media_buy) now does cast(Any, UpdateMediaBuySuccessResponse).model_construct(... status="completed", media_buy_status=MediaBuyStatus(response_status) ...). Functionally correct — it hand-replicates the model's legacy-status normalization — but model_construct bypasses validation, so affected_packages stays raw dicts and the cast(Any, ...) defeats field-name type checking. Example code, happy-path smoke test only. An adopter who copies the pattern loses the runtime guard. Worth a comment pointing at the codegen issue it works around.
  2. get_products post-validation reassignment. Same file: after Product.model_validate(...), product.format_ids / product.format_options get reassigned with # type: ignore[assignment] to restore wire declarations the validator drops. Fine for a demo seller; leave a breadcrumb to the upstream codegen issue so it can be removed later.

Approving on the strength of the complete mapping table plus both projections validating against the bundled 3.0 and 3.1 schemas. Follow-ups noted below.

@bokelley bokelley merged commit d395cbb into main Jun 14, 2026
26 checks passed
@bokelley bokelley deleted the adcp-client-python-951 branch June 14, 2026 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Project native postal capabilities to legacy fused shape for AdCP 3.0 callers

1 participant