Skip to content

Commit 96ccfd4

Browse files
bokelleyclaude
bokelley
and
claude
authored
feat(types): widen extension-point list[X] to Sequence[X] (#624) (#640)
* feat(types): widen extension-point list[X] to Sequence[X] for adopter inheritance Closes #624. Adopters who follow Critical Pattern #1 (subclass a library response type and override a parent's `list[X]` field with `list[ChildX]`) hit `# type: ignore[assignment]` on every override under mypy --strict — list is invariant in its element type. Sequence is covariant, so a Sequence[Parent] parent permits list[Child] override (where Child <: Parent) cleanly. This PR adds a post-codegen processor (`widen_extension_point_lists_to_sequence` in `scripts/post_generate_fixes.py`) that rewrites annotations on a small allowlist of fields documented as extension points. The allowlist is keyed on (file, class, field) so it survives codegen reformatting and field-order drift. The current allowlist contains one confirmed entry — `UpdateMediaBuyResponse.affected_packages` (matches salesagent `_base.py:360`) — across the two emitted variants (`media_buy/` and `bundled/media_buy/`). Nine TODO entries are placeholders pending the salesagent `# type: ignore[assignment]` line list — fill in as they're mapped. `tests/type_checks/extend_response_with_sequence.py` is the regression gate: subclasses `UpdateMediaBuyResponse1` with an overridden `affected_packages: list[_InternalPackage]` and asserts mypy --strict accepts the override with zero ignores. The Pydantic plugin behavior was validated in a 2-file spike before this PR — see the comment on #624 for the validation result. BREAKING CHANGE: callers passing `affected_packages` into a function typed `def f(x: list[Package])` will see a mypy error and need to migrate to `Sequence[Package]`. Runtime behavior is unchanged; the change is annotation-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(types): expand #624 allowlist with full Category A entries Refactor allowlist from (file, class, field) to (class, field) tuples and walk every generated .py file. datamodel-codegen emits bundled response files that each inline copies of subordinate types (Placement, TargetingOverlay, etc.); the walker rewrites every emission so all paths stay consistent. Allowlist now covers all 15 Category A entries: - Response: UpdateMediaBuyResponse.affected_packages, GetMediaBuyDeliveryResponse.media_buy_deliveries, GetCreativeDeliveryResponse.creatives, Signal.deployments, GetSignalsResponse.signals, GetMediaBuysResponse.media_buys, ListCreativesResponse.creatives - Request: PackageRequest.creatives, CreateMediaBuyRequest.packages, UpdateMediaBuyRequest.packages - Cross-cutting: Placement.format_ids, TargetingOverlay.geo_*_exclude (4) Result: 47 fields widened across 21 generated files. Pairs that match zero files emit a WARN so allowlist drift surfaces fast. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(types): address pre-merge review on #624 — bound regex to class, silent idempotency, document scope Three fixes from expert review: 1. Regex now bounded to current class. _widen_field_annotation slices region up to next ^class before running the field search. Latent bug fix — current corpus has no collisions, but prevents future codegen changes from silently triggering it. 2. Idempotent re-runs are silent. New _field_already_widened helper tracks "already Sequence" separately from "no list found"; WARN fires only when neither is found anywhere (genuine drift). 3. Regression test scope honestly documented. Required-field overrides surface a separate codegen multi-emission issue (filed as #642): datamodel-codegen emits Creative/Package/etc. per response file, so public alias resolves to one emission while parent field references another. Test docstring documents the scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 3160ace commit 96ccfd4

24 files changed

Lines changed: 393 additions & 50 deletions

‎scripts/post_generate_fixes.py‎

Lines changed: 255 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -799,6 +799,260 @@ def _first_subscript_arg(node: ast.Subscript) -> ast.AST | None:
799799
return slice_node
800800

801801

802+
# ---------------------------------------------------------------------------
803+
# #624: widen documented extension-point list[X] fields to Sequence[X].
804+
#
805+
# Adopters who follow Critical Pattern #1 (subclass a library response type
806+
# and override the parent's list field with a more specific element type)
807+
# hit `# type: ignore[assignment]` on every override under mypy --strict —
808+
# list is invariant in its element type. Sequence is covariant, so a
809+
# Sequence[Parent] parent permits list[Child] override cleanly.
810+
#
811+
# Scope is intentionally narrow: only fields the SDK documents as
812+
# extension points (response payloads adopters routinely subclass, plus
813+
# request bodies that compose extendable sub-records like packages and
814+
# creatives). Internal scalars stay as list.
815+
#
816+
# Allowlist format: (class_name, field_name). datamodel-codegen emits
817+
# bundled response files that each inline copies of subordinate types
818+
# (Placement, TargetingOverlay, etc.); the rewriter walks every generated
819+
# .py file and applies the substitution to every emission of the named
820+
# (class, field) pair so all copies stay consistent.
821+
822+
_SEQUENCE_EXTENSION_POINTS: list[tuple[str, str]] = [
823+
# Response payloads adopters subclass to add internal-only fields.
824+
# `UpdateMediaBuySuccessResponse` is the success variant of the
825+
# `UpdateMediaBuyResponse` discriminated union — emitted as
826+
# `UpdateMediaBuyResponse1` (v3.0) and `UpdateMediaBuyResponse3`
827+
# (v3.0.6 bundled).
828+
("UpdateMediaBuyResponse1", "affected_packages"),
829+
("UpdateMediaBuyResponse3", "affected_packages"),
830+
("GetMediaBuyDeliveryResponse", "media_buy_deliveries"),
831+
("GetCreativeDeliveryResponse", "creatives"),
832+
("Signal", "deployments"),
833+
("GetSignalsResponse", "signals"),
834+
("GetMediaBuysResponse", "media_buys"),
835+
("ListCreativesResponse", "creatives"),
836+
# Request bodies that carry extendable sub-records — adopters subclass
837+
# the inner record type and need to override the list element type.
838+
("PackageRequest", "creatives"),
839+
("CreateMediaBuyRequest", "packages"),
840+
("UpdateMediaBuyRequest", "packages"),
841+
# Cross-cutting record types referenced from multiple responses; each
842+
# bundled response file inlines its own copy. The walker rewrites
843+
# every emission.
844+
("Placement", "format_ids"),
845+
("TargetingOverlay", "geo_countries_exclude"),
846+
("TargetingOverlay", "geo_regions_exclude"),
847+
("TargetingOverlay", "geo_metros_exclude"),
848+
("TargetingOverlay", "geo_postal_areas_exclude"),
849+
]
850+
851+
852+
def widen_extension_point_lists_to_sequence():
853+
"""Rewrite ``list[X]`` to ``Sequence[X]`` on documented extension-point fields.
854+
855+
Walks every generated ``.py`` file under :data:`OUTPUT_DIR`. For each
856+
file, applies every ``(class, field)`` pair in
857+
:data:`_SEQUENCE_EXTENSION_POINTS` that matches a class declaration
858+
in that file. The same ``(class, field)`` pair commonly appears in
859+
multiple files because bundled response emission inlines copies of
860+
subordinate types — every emission is rewritten so all paths stay
861+
consistent. Each rewritten file gets ``from collections.abc import
862+
Sequence`` added if it isn't already present.
863+
864+
Pairs that produce zero rewrites across the whole tree emit a WARN
865+
so allowlist drift surfaces fast (a renamed field or removed class
866+
means the override pattern this entry was protecting no longer
867+
exists).
868+
869+
See `adcp-client-python#624 <https://github.com/adcontextprotocol/adcp-client-python/issues/624>`_
870+
for the design rationale and the spike that validated the Pydantic
871+
plugin accepts ``Sequence[Parent]`` parent + ``list[Child]`` child
872+
override under mypy --strict.
873+
"""
874+
print("Widening extension-point list[X] fields to Sequence[X] (#624)...")
875+
876+
# Track total rewrites per (class, field) — a pair with zero hits is
877+
# a stale allowlist entry and surfaces as a WARN.
878+
# Track per-pair state across all files:
879+
# rewrites: how many list[X] sites were rewritten this run
880+
# already_widened: how many sites are already in Sequence[X] form
881+
# A pair with rewrites == 0 AND already_widened == 0 is genuinely stale
882+
# (field renamed/removed) and warrants a WARN. A pair with already_widened
883+
# > 0 is silent — that's the steady-state idempotent run.
884+
rewrites_per_pair: dict[tuple[str, str], int] = {pair: 0 for pair in _SEQUENCE_EXTENSION_POINTS}
885+
already_per_pair: dict[tuple[str, str], int] = {pair: 0 for pair in _SEQUENCE_EXTENSION_POINTS}
886+
files_touched = 0
887+
total_widened = 0
888+
889+
for file_path in sorted(OUTPUT_DIR.rglob("*.py")):
890+
original = file_path.read_text()
891+
content = original
892+
widened_in_file = 0
893+
894+
for class_name, field_name in _SEQUENCE_EXTENSION_POINTS:
895+
# Quick filter — skip files that don't declare this class.
896+
if f"class {class_name}(" not in content and f"class {class_name}:" not in content:
897+
continue
898+
new_content, did_widen = _widen_field_annotation(content, class_name, field_name)
899+
if did_widen:
900+
content = new_content
901+
widened_in_file += 1
902+
rewrites_per_pair[(class_name, field_name)] += 1
903+
elif _field_already_widened(content, class_name, field_name):
904+
already_per_pair[(class_name, field_name)] += 1
905+
906+
if widened_in_file == 0:
907+
continue
908+
909+
content = _ensure_sequence_import(content)
910+
file_path.write_text(content)
911+
files_touched += 1
912+
total_widened += widened_in_file
913+
print(f" ✓ {file_path.relative_to(OUTPUT_DIR)}: widened {widened_in_file} field(s)")
914+
915+
stale = [
916+
pair
917+
for pair in _SEQUENCE_EXTENSION_POINTS
918+
if rewrites_per_pair[pair] == 0 and already_per_pair[pair] == 0
919+
]
920+
for class_name, field_name in stale:
921+
print(
922+
f" WARN: {class_name}.{field_name} — neither list[X] nor Sequence[X] "
923+
"found in any generated file (field renamed or removed?)"
924+
)
925+
926+
if total_widened == 0:
927+
print(" No extension-point fields to widen")
928+
else:
929+
print(
930+
f" ✓ Widened {total_widened} extension-point field(s) "
931+
f"across {files_touched} file(s)"
932+
)
933+
934+
935+
def _widen_field_annotation(content: str, class_name: str, field_name: str) -> tuple[str, bool]:
936+
"""Rewrite ``list[X]`` → ``Sequence[X]`` in one field's annotation.
937+
938+
Locates ``class {class_name}(...):`` then walks forward to the first
939+
`` {field_name}:`` line at class-body indentation, **bounded to the
940+
target class** so a same-named field on a later class in the same
941+
file cannot mis-match. Within the AnnAssign's annotation block (which
942+
may span multiple lines for ``Annotated[..., Field(...)]``), replaces
943+
the first ``list[`` with ``Sequence[``. Idempotent — a second pass
944+
over already-widened content is a no-op.
945+
"""
946+
# Anchor on the class definition.
947+
class_pattern = re.compile(rf"^class {re.escape(class_name)}\b", re.MULTILINE)
948+
class_match = class_pattern.search(content)
949+
if class_match is None:
950+
return content, False
951+
952+
# Bound the search region to the current class body. Scanning past the
953+
# next `^class ` would let `re.search` mis-target a same-named field
954+
# on a sibling class in the same file (the lookahead in
955+
# field_start_pattern terminates a *match*, but `re.search` is free to
956+
# scan past the first class's boundary looking for a hit).
957+
class_body_start = class_match.end()
958+
next_class = re.compile(r"^class ", re.MULTILINE).search(content, class_body_start)
959+
region_end = next_class.start() if next_class is not None else len(content)
960+
region = content[class_body_start:region_end]
961+
962+
# The annotation block runs from the field name to the next class-body
963+
# statement at 4-space indentation (next field, model_config, or method).
964+
field_start_pattern = re.compile(
965+
rf"^( {re.escape(field_name)}: )(.*?)(?=^ [a-zA-Z_]|\Z)",
966+
re.MULTILINE | re.DOTALL,
967+
)
968+
field_match = field_start_pattern.search(region)
969+
if field_match is None:
970+
return content, False
971+
972+
annotation_block = field_match.group(2)
973+
# Replace the first list[ inside the annotation only. Generated
974+
# annotations always have `list[X]` as the outer container; the
975+
# narrow scope of the allowlist (no `dict[str, list[X]]` entries)
976+
# makes this safe in practice. If a future entry has nested list,
977+
# this needs to anchor on the outer container explicitly.
978+
new_annotation = re.sub(r"\blist\[", "Sequence[", annotation_block, count=1)
979+
if new_annotation == annotation_block:
980+
return content, False
981+
982+
# Stitch back. .start()/.end() are relative to `region`; convert to
983+
# absolute offsets in `content`.
984+
abs_start = class_body_start + field_match.start(2)
985+
abs_end = class_body_start + field_match.end(2)
986+
new_content = content[:abs_start] + new_annotation + content[abs_end:]
987+
return new_content, True
988+
989+
990+
def _field_already_widened(content: str, class_name: str, field_name: str) -> bool:
991+
"""Return True when the named field's annotation is already ``Sequence[X]``.
992+
993+
Used to silence the WARN on idempotent re-runs: a pair that's already
994+
widened is the steady state, not allowlist drift.
995+
"""
996+
class_match = re.search(rf"^class {re.escape(class_name)}\b", content, re.MULTILINE)
997+
if class_match is None:
998+
return False
999+
class_body_start = class_match.end()
1000+
next_class = re.compile(r"^class ", re.MULTILINE).search(content, class_body_start)
1001+
region_end = next_class.start() if next_class is not None else len(content)
1002+
region = content[class_body_start:region_end]
1003+
field_match = re.search(
1004+
rf"^( {re.escape(field_name)}: )(.*?)(?=^ [a-zA-Z_]|\Z)",
1005+
region,
1006+
re.MULTILINE | re.DOTALL,
1007+
)
1008+
if field_match is None:
1009+
return False
1010+
return "Sequence[" in field_match.group(2)
1011+
1012+
1013+
def _ensure_sequence_import(content: str) -> str:
1014+
"""Add ``from collections.abc import Sequence`` if not already present.
1015+
1016+
Inserts after the ``from __future__ import annotations`` line so the
1017+
import sits with sibling stdlib imports rather than landing at the top
1018+
of the file.
1019+
"""
1020+
if "from collections.abc import Sequence" in content:
1021+
return content
1022+
# If `collections.abc` is already imported, extend the import line.
1023+
extend_pattern = re.compile(r"^from collections\.abc import ([^\n]+)$", re.MULTILINE)
1024+
match = extend_pattern.search(content)
1025+
if match is not None:
1026+
existing = match.group(1)
1027+
# Maintain alphabetical order if the existing import is sorted.
1028+
names = sorted({*[n.strip() for n in existing.split(",")], "Sequence"})
1029+
new_line = f"from collections.abc import {', '.join(names)}"
1030+
return content[: match.start()] + new_line + content[match.end() :]
1031+
1032+
# Otherwise insert after the typing imports block. Codegen always emits
1033+
# ``from typing import Annotated`` near the top, so anchor on it.
1034+
typing_pattern = re.compile(r"^from typing import [^\n]+$", re.MULTILINE)
1035+
match = typing_pattern.search(content)
1036+
if match is not None:
1037+
return (
1038+
content[: match.end()]
1039+
+ "\nfrom collections.abc import Sequence"
1040+
+ content[match.end() :]
1041+
)
1042+
1043+
# Fallback: prepend after the `from __future__` line.
1044+
future_pattern = re.compile(r"^from __future__ import annotations$", re.MULTILINE)
1045+
match = future_pattern.search(content)
1046+
if match is not None:
1047+
return (
1048+
content[: match.end()]
1049+
+ "\n\nfrom collections.abc import Sequence"
1050+
+ content[match.end() :]
1051+
)
1052+
1053+
return "from collections.abc import Sequence\n\n" + content
1054+
1055+
8021056
def main():
8031057
"""Apply all post-generation fixes."""
8041058
print("Applying post-generation fixes...")
@@ -816,6 +1070,7 @@ def main():
8161070
fix_reuse_model_discriminator_bug()
8171071
restore_format_category_deprecation_shim()
8181072
inject_literal_discriminator_defaults()
1073+
widen_extension_point_lists_to_sequence()
8191074

8201075
print("\n✓ Post-generation fixes complete\n")
8211076

‎src/adcp/types/generated_poc/bundled/creative/get_creative_delivery_response.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
from enum import Enum, IntEnum
88
from typing import Annotated, Any, Literal
9+
from collections.abc import Sequence
910

1011
from adcp.types.base import AdCPBaseModel
1112
from pydantic import AnyUrl, AwareDatetime, ConfigDict, Field, RootModel, StringConstraints
@@ -3612,7 +3613,7 @@ class GetCreativeDeliveryResponse(AdCPBaseModel):
36123613
]
36133614
reporting_period: Annotated[ReportingPeriod, Field(description='Date range for the report.')]
36143615
creatives: Annotated[
3615-
list[Creative], Field(description='Creative delivery data with variant breakdowns')
3616+
Sequence[Creative], Field(description='Creative delivery data with variant breakdowns')
36163617
]
36173618
pagination: Annotated[
36183619
Pagination | None,

‎src/adcp/types/generated_poc/bundled/creative/list_creatives_response.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
from enum import Enum, IntEnum
88
from typing import Annotated, Any, Literal
9+
from collections.abc import Sequence
910

1011
from adcp.types.base import AdCPBaseModel
1112
from pydantic import AnyUrl, AwareDatetime, ConfigDict, EmailStr, Field, RootModel, StringConstraints
@@ -3810,7 +3811,7 @@ class ListCreativesResponse(AdCPBaseModel):
38103811
),
38113812
]
38123813
creatives: Annotated[
3813-
list[Creative], Field(description='Array of creative assets matching the query')
3814+
Sequence[Creative], Field(description='Array of creative assets matching the query')
38143815
]
38153816
format_summary: Annotated[
38163817
dict[Annotated[str, StringConstraints(pattern=r'^[a-zA-Z0-9_-]+$')], int] | None,

‎src/adcp/types/generated_poc/bundled/media_buy/create_media_buy_request.py‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
from enum import Enum, IntEnum
88
from typing import Annotated, Any, Literal
9+
from collections.abc import Sequence
910

1011
from adcp.types.base import AdCPBaseModel
1112
from pydantic import AnyUrl, AwareDatetime, ConfigDict, EmailStr, Field, RootModel, StringConstraints
@@ -2576,7 +2577,7 @@ class TargetingOverlay(AdCPBaseModel):
25762577
),
25772578
] = None
25782579
geo_countries_exclude: Annotated[
2579-
list[GeoCountriesExcludeItem] | None,
2580+
Sequence[GeoCountriesExcludeItem] | None,
25802581
Field(
25812582
description="Exclude specific countries from delivery. ISO 3166-1 alpha-2 codes (e.g., 'US', 'GB', 'DE').",
25822583
min_length=1,
@@ -2590,7 +2591,7 @@ class TargetingOverlay(AdCPBaseModel):
25902591
),
25912592
] = None
25922593
geo_regions_exclude: Annotated[
2593-
list[GeoRegionsExcludeItem] | None,
2594+
Sequence[GeoRegionsExcludeItem] | None,
25942595
Field(
25952596
description="Exclude specific regions/states from delivery. ISO 3166-2 subdivision codes (e.g., 'US-CA', 'GB-SCT').",
25962597
min_length=1,
@@ -2604,7 +2605,7 @@ class TargetingOverlay(AdCPBaseModel):
26042605
),
26052606
] = None
26062607
geo_metros_exclude: Annotated[
2607-
list[GeoMetrosExcludeItem] | None,
2608+
Sequence[GeoMetrosExcludeItem] | None,
26082609
Field(
26092610
description='Exclude specific metro areas from delivery. Each entry specifies the classification system and excluded values. Seller must declare supported systems in get_adcp_capabilities.',
26102611
min_length=1,
@@ -2618,7 +2619,7 @@ class TargetingOverlay(AdCPBaseModel):
26182619
),
26192620
] = None
26202621
geo_postal_areas_exclude: Annotated[
2621-
list[GeoPostalAreasExcludeItem] | None,
2622+
Sequence[GeoPostalAreasExcludeItem] | None,
26222623
Field(
26232624
description='Exclude specific postal areas from delivery. Each entry specifies the postal system and excluded values. Seller must declare supported systems in get_adcp_capabilities.',
26242625
min_length=1,
@@ -5030,7 +5031,7 @@ class CreateMediaBuyRequest(AdCPBaseModel):
50305031
),
50315032
] = None
50325033
packages: Annotated[
5033-
list[Package] | None,
5034+
Sequence[Package] | None,
50345035
Field(
50355036
description="Array of package configurations. Required when not using proposal_id. When executing a proposal, this can be omitted and packages will be derived from the proposal's allocations.",
50365037
min_length=1,

‎src/adcp/types/generated_poc/bundled/media_buy/create_media_buy_response.py‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
from enum import Enum
88
from typing import Annotated, Any, Literal
9+
from collections.abc import Sequence
910

1011
from adcp.types.base import AdCPBaseModel
1112
from pydantic import AnyUrl, AwareDatetime, ConfigDict, EmailStr, Field, RootModel
@@ -2540,7 +2541,7 @@ class TargetingOverlay(AdCPBaseModel):
25402541
),
25412542
] = None
25422543
geo_countries_exclude: Annotated[
2543-
list[GeoCountriesExcludeItem] | None,
2544+
Sequence[GeoCountriesExcludeItem] | None,
25442545
Field(
25452546
description="Exclude specific countries from delivery. ISO 3166-1 alpha-2 codes (e.g., 'US', 'GB', 'DE').",
25462547
min_length=1,
@@ -2554,7 +2555,7 @@ class TargetingOverlay(AdCPBaseModel):
25542555
),
25552556
] = None
25562557
geo_regions_exclude: Annotated[
2557-
list[GeoRegionsExcludeItem] | None,
2558+
Sequence[GeoRegionsExcludeItem] | None,
25582559
Field(
25592560
description="Exclude specific regions/states from delivery. ISO 3166-2 subdivision codes (e.g., 'US-CA', 'GB-SCT').",
25602561
min_length=1,
@@ -2568,7 +2569,7 @@ class TargetingOverlay(AdCPBaseModel):
25682569
),
25692570
] = None
25702571
geo_metros_exclude: Annotated[
2571-
list[GeoMetrosExcludeItem] | None,
2572+
Sequence[GeoMetrosExcludeItem] | None,
25722573
Field(
25732574
description='Exclude specific metro areas from delivery. Each entry specifies the classification system and excluded values. Seller must declare supported systems in get_adcp_capabilities.',
25742575
min_length=1,
@@ -2582,7 +2583,7 @@ class TargetingOverlay(AdCPBaseModel):
25822583
),
25832584
] = None
25842585
geo_postal_areas_exclude: Annotated[
2585-
list[GeoPostalAreasExcludeItem] | None,
2586+
Sequence[GeoPostalAreasExcludeItem] | None,
25862587
Field(
25872588
description='Exclude specific postal areas from delivery. Each entry specifies the postal system and excluded values. Seller must declare supported systems in get_adcp_capabilities.',
25882589
min_length=1,

0 commit comments

Comments
 (0)