Skip to content

feat(types): widen extension-point list[X] to Sequence[X] (#624)#640

Merged
bokelley merged 3 commits intomainfrom
claude/issue-624-sequence-extension-points
May 10, 2026
Merged

feat(types): widen extension-point list[X] to Sequence[X] (#624)#640
bokelley merged 3 commits intomainfrom
claude/issue-624-sequence-extension-points

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Closes #624.

Summary

Adopters following the documented Critical Pattern #1 (subclass a library response type, override a parent's `list[X]` field with `list[ChildX]` carrying internal-only fields) carry `# type: ignore[assignment]` on every override under mypy --strict. `list[T]` is invariant in T; `Sequence[T]` is covariant, so a `Sequence[Parent]` parent permits `list[Child]` override cleanly.

This PR adds a post-codegen processor that rewrites `list[X]` → `Sequence[X]` for fields on a small allowlist of documented extension points.

What's in the diff

File Change
`scripts/post_generate_fixes.py` New `widen_extension_point_lists_to_sequence()` + helpers, wired into `main()`. Allowlist keyed on (file, class, field).
`src/adcp/types/generated_poc/media_buy/update_media_buy_response.py` `UpdateMediaBuyResponse1.affected_packages` widened. `Sequence` import added.
`src/adcp/types/generated_poc/bundled/media_buy/update_media_buy_response.py` `UpdateMediaBuyResponse3.affected_packages` widened. `Sequence` import added.
`tests/type_checks/extend_response_with_sequence.py` Regression gate — subclasses `UpdateMediaBuyResponse1` with overridden `list[_InternalPackage]` and asserts mypy --strict accepts it.

Allowlist status

Confirmed (1):

  • `UpdateMediaBuyResponse.affected_packages` — matches salesagent `_base.py:360`. Two emitted variants both rewritten.

Placeholders (9 TODOs):

  • `_base.py:875-878` (geo exclusion lists)
  • `delivery.py:251`, `delivery.py:440`
  • `creative.py` list_creatives asset list
  • `_base.py` get_products products list
  • `_base.py` sync_creatives result list
  • `_base.py` list_accounts accounts list
  • `_base.py` get_signals signals list
  • `_base.py` list_creative_formats formats list

Blocker on filling these in: I need salesagent's actual `# type: ignore[assignment]` line list to map them accurately. Once the list is dropped, expanding the allowlist is a 5-line change per entry.

Validation

  • 2-file mypy --strict spike confirmed Pydantic plugin accepts `Sequence[Parent]` + `list[Child]` override (see issue comment).
  • Regression test `tests/type_checks/extend_response_with_sequence.py` runs under `mypy --strict` and proves `.append()` ergonomics survive on the child class.
  • Local `mypy src/adcp/` clean (784 source files, no issues).
  • Existing tests untouched.

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.

Open questions for review

  1. Allowlist scope: should I expand the placeholders now with best-guess field paths, or wait for the actual salesagent list to avoid rework?
  2. `_ergonomic.py`: BeforeValidator updates probably aren't needed (Sequence accepts list at construction), but worth a sanity check.
  3. Naming: `widen_extension_point_lists_to_sequence` is verbose — open to a shorter name.

bokelley and others added 2 commits May 10, 2026 12:27
… 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>
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>
@bokelley
Copy link
Copy Markdown
Contributor Author

Allowlist expanded to all 15 Category A entries

Refactored the rewriter from `(file, class, field)` to `(class, field)` and made it walk every generated `.py` file. `datamodel-codegen` emits bundled response files that each inline copies of subordinate types (`Placement`, `TargetingOverlay`, etc.) — every emission gets rewritten so all paths stay consistent.

47 fields widened across 21 generated files.

Library type Field
`UpdateMediaBuyResponse` (1, 3) `affected_packages`
`GetMediaBuyDeliveryResponse` `media_buy_deliveries`
`GetCreativeDeliveryResponse` `creatives`
`Signal` `deployments`
`GetSignalsResponse` `signals`
`GetMediaBuysResponse` `media_buys`
`ListCreativesResponse` `creatives`
`PackageRequest` `creatives`
`CreateMediaBuyRequest` `packages`
`UpdateMediaBuyRequest` `packages`
`Placement` `format_ids`
`TargetingOverlay` `geo_countries_exclude`, `geo_regions_exclude`, `geo_metros_exclude`, `geo_postal_areas_exclude`

Local checks: `mypy src/adcp/` clean (784 source files), `mypy --strict tests/type_checks/` clean (9 source files), test sanity sweep on `test_decisioning_dispatch` + `test_pre_validation_hooks` 63 passed.

The walker also emits a `WARN` for any `(class, field)` pair that doesn't match anywhere in the tree, so future allowlist drift (renamed/removed fields) surfaces fast on regen.

CI rolling now.

@bokelley
Copy link
Copy Markdown
Contributor Author

Noted — scope expansion looks solid. The (class, field) keyed walker covering all generated files is the right call given datamodel-codegen's bundled-emission pattern, and the drift-warning on unmatched allowlist pairs is a good safety net for future regens. Standing by for CI green; happy to re-run expert review on the expanded diff once it lands.


Generated by Claude Code

…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>
@bokelley
Copy link
Copy Markdown
Contributor Author

Fold candidate from #642: Issue #642 (codegen type-identity mismatch — Creative re-emitted per response file, so adcp.types.Creative and the field-local Creative are type-distinct under mypy) was just filed as a companion to #624. The proposed fix is a post-processor step in post_generate_fixes.py that replaces local re-emissions with cross-file imports from the canonical module (Approach B from the issue). File scope overlaps with this PR's existing widen_extension_point_lists_to_sequence() addition. If you're still iterating here, folding #642's allowlist rewrite in before merge would close both halves of the adopter-override friction in one PR — no type: ignore[assignment] on type-identity grounds, no type: ignore[assignment] on variance grounds.


Generated by Claude Code

@bokelley bokelley marked this pull request as ready for review May 10, 2026 17:04
@bokelley bokelley merged commit 96ccfd4 into main May 10, 2026
16 checks passed
@bokelley bokelley deleted the claude/issue-624-sequence-extension-points branch May 10, 2026 17:05
@bokelley
Copy link
Copy Markdown
Contributor Author

Investigated the fold. The two Creative classes aren't nominally identical — they're genuinely different schemas:

  • get_creative_delivery_response.Creative (what adcp.types.Creative resolves to): delivery-metrics type — creative_id, media_buy_id, totals, variant_count, variants
  • list_creatives_response.Creative: full creative-asset record — name, status, assets, tags, variables, assignments, etc.

Approach B as written (replace local re-emission with an import from the canonical module) would wire the delivery-metrics Creative into ListCreativesResponse.creatives, which is structurally wrong. The real problem in #642 is that codegen assigned the name Creative to two distinct types, and _generated.py happens to export the delivery one as the public alias — not a simple identity mismatch fixable with a cross-file import.

Recommendation: proceed with #640 as-is (Sequence widening still eliminates the variance half of the friction) and leave #642 open as a separate track requiring either a codegen rename (CreativeDelivery vs Creative) or a _generated.py export fix to surface the right canonical type.


Generated by Claude Code

@bokelley
Copy link
Copy Markdown
Contributor Author

Understood — the investigation confirms these are genuinely distinct schemas, not an identity mismatch, so the fold would have wired the wrong type. #640 ships the Sequence widening as intended; #642 stays open as a separate codegen-rename track (CreativeDelivery vs Creative or a _generated.py export fix). No further action needed here.


Triaged by Claude Code. Session: https://claude.ai/code/session_01FGyjS99jvAz6F58M9qpH35


Generated by Claude Code

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.

feat(types): adopter inheritance pattern triggers ~50 type: ignore[assignment]

1 participant