From 286f20ef5d103bc30ab7bce5aab02961e071fc46 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Thu, 30 Apr 2026 19:03:29 -0400 Subject: [PATCH] fix(migrate): per-symbol replacement for generated_poc reach-ins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-5 adopter feedback (salesagent v3→v4 experiment, 270 files / 161 collection failures): 82 of 156 codemod findings were ``adcp.types.generated_poc`` flag-only output that forced adopters to hand-grep the public-API module to find each replacement. Fixes: - ``GENERATED_POC_SYMBOL_MAP`` covers the 8 unambiguous reach-ins with explicit "Symbol → adcp.types.Symbol" replacements: AccountReference, BrandReference, ContextObject, CreativeAsset, Error, MediaBuyStatus, ProductFilters, ReportingWebhook. The ``test_generated_poc_symbol_map_covers_publicly_exported_names`` test guards drift between the codemod and the SDK's public surface. - Multi-symbol single-line imports (``from … import A, B, C``) emit one Finding per known symbol; unknown symbols and multiline imports fall back to the generic "private module" flag. - Text report shows per-symbol replacement on the header line so adopters fix without leaving the codemod output. - ``import X as Y`` aliases are handled — the codemod keys off the canonical LHS name. Intentionally NOT mapped: ``CreditLimit``, ``Setup``, ``GovernanceAgent``. These names appear in 8+ generated files (per-schema variants like ``core/account.Setup`` vs. ``account/sync_accounts_response.Setup``); a blanket ``adcp.types.Setup`` hint would be ambiguous. Adopters reaching for these get the generic flag; landing them in the public API is a separate design decision. Migration guide additions: - "Fix removed-type imports first — they cascade" section documents salesagent's pattern of centralizing SDK imports in a barrel module (``_base.py`` re-export) where one broken import crashes ~140 tests during pytest collect-only. Calls out the priority order: barrel modules first, then generated_poc reach-ins, then numbered Assets, then per-call-site shape changes. - "a2a-sdk transitive bump" section flags the ``a2a-sdk>=1.0.0`` surprise migration for adopters with direct ``a2a.utils.errors`` / ``a2a.types`` imports. Co-Authored-By: Claude Opus 4.7 (1M context) --- MIGRATION_v3_to_v4.md | 52 +++++++++++++++++ src/adcp/migrate/v3_to_v4.py | 103 +++++++++++++++++++++++++++++++-- tests/test_migrate_v3_to_v4.py | 87 ++++++++++++++++++++++++++-- 3 files changed, 234 insertions(+), 8 deletions(-) diff --git a/MIGRATION_v3_to_v4.md b/MIGRATION_v3_to_v4.md index f8acd9d28..7b406e56c 100644 --- a/MIGRATION_v3_to_v4.md +++ b/MIGRATION_v3_to_v4.md @@ -46,6 +46,58 @@ dependencies = [ ] ``` +## Fix removed-type imports first — they cascade + +Real-adopter feedback (salesagent v3→v4 experiment, 270 files scanned, 161 +test-collection failures): consumers tend to centralize SDK imports in one +schema module, so a single broken import there crashes test collection across +the whole codebase. salesagent re-exported through `src/core/schemas/_base.py` +— **one missing `FormatCategory` import there cascaded into ~140 test failures +during pytest collect-only**, and stubbing it revealed the next ~140-test +cascade from `BrandManifest`, then the next from the `generated_poc` reach-ins. + +The codemod's static finding count understates this by 100x+. To minimize the +felt blast radius, work in this order: + +1. **Removed types in central re-export modules first.** Find every + `BrandManifest`, `FormatCategory`, `DeliverTo`, `PromotedProducts`, + `PromotedOfferings`, `Pricing`, `PackageStatus` import in modules that + re-export to the rest of your codebase (e.g. a `_base.py` schema barrel). + Fix those before anything else — most of your test-collection failures + disappear. +2. **`adcp.types.generated_poc` reach-ins.** The codemod's per-symbol mapping + tells you the public alias for each (`ContextObject` → + `adcp.types.ContextObject`, etc.). Mechanical lookup once you know the + pattern. +3. **Numbered `Assets` imports.** Switch to the semantic alias from + `adcp.types`. See [Numbered discriminated-union classes + shifted](#numbered-discriminated-union-classes-shifted) below. +4. **Per-call-site shape changes** (e.g. `BrandManifest(name=..., logo_url=...)` + → `BrandReference(domain=...)`). The codemod can't auto-rewrite these — + the data is different. + +## a2a-sdk transitive bump (only matters if you have direct `a2a` imports) + +v4 of this SDK requires `a2a-sdk>=1.0.0`. If your codebase has any direct +imports from the `a2a` package — typically `a2a.utils.errors.ServerError`, +`a2a.types.DataPart`, or other surface — those are a separate migration that +this SDK forces on you transitively. + +Symptoms during pytest collection after upgrading: + +- `cannot import name 'ServerError' from 'a2a.utils.errors'` +- `cannot import name 'DataPart' from 'a2a.types'` + +If you don't import from `a2a` directly (only via `adcp.server` / +`adcp.protocols.a2a`), this section doesn't apply — the SDK's wrapper layer +absorbs the change. + +If you do, see the [a2a-sdk +1.0 release notes](https://github.com/a2aproject/a2a-python/releases) for the +import-path moves. The most common pattern is `a2a.utils.errors.ServerError` +→ `a2a.types.A2AError` and `a2a.types.DataPart` → `a2a.types.MessagePart`, +but verify against your specific usage. + ## Removed types The following types were removed from the AdCP spec and have no replacement diff --git a/src/adcp/migrate/v3_to_v4.py b/src/adcp/migrate/v3_to_v4.py index 9fc411c30..0607f141c 100644 --- a/src/adcp/migrate/v3_to_v4.py +++ b/src/adcp/migrate/v3_to_v4.py @@ -114,6 +114,50 @@ } +# Per-symbol mapping for the most common ``generated_poc`` reach-ins +# salesagent surfaced during their v3→v4 experiment (and any other +# adopter would hit). The codemod scans for ``from +# adcp.types.generated_poc. import `` lines and emits an +# explicit "before → after" hint per symbol so adopters don't have to +# hand-grep the public-API module to find the canonical alias. +# +# Mapping shape: `` → adcp.types.``. Every +# symbol listed here is already exported from ``adcp.types``; the +# ``test_generated_poc_symbol_map_covers_publicly_exported_names`` test +# guards drift between this map and the SDK's public surface. +# +# Intentionally NOT in the map (yet): ``CreditLimit``, ``Setup``, +# ``GovernanceAgent``. These names appear in 8+ generated files +# (``core/account.py``, ``account/sync_accounts_response.py``, +# ``media_buy/sync_event_sources_response.py``, ``bundled/...``) — the +# codegen emits one independent class per containing schema, so a +# blanket "import from adcp.types" hint would be ambiguous about +# which variant. Adopters reaching for these get the generic +# private-module flag; landing them in the public API is a separate +# design decision (which canonical variant to expose / whether to +# expose schema-namespaced aliases like ``AccountSetup``). +GENERATED_POC_SYMBOL_MAP: dict[str, str] = { + "AccountReference": "adcp.types.AccountReference", + "BrandReference": "adcp.types.BrandReference", + "ContextObject": "adcp.types.ContextObject", + "CreativeAsset": "adcp.types.CreativeAsset", + "Error": "adcp.types.Error", + "MediaBuyStatus": "adcp.types.MediaBuyStatus", + "ProductFilters": "adcp.types.ProductFilters", + "ReportingWebhook": "adcp.types.ReportingWebhook", +} + + +# ``from adcp.types.generated_poc.<...> import `` — +# captures the symbol list so we can emit per-symbol replacement hints. +# Multiline imports (parenthesized) aren't covered by this regex; they +# fall through to the generic "private module" flag, which still +# surfaces the issue and prints the migration anchor. +_GENERATED_POC_FROM_IMPORT = re.compile( + r"from\s+adcp\.types\.generated_poc(?:\.[\w.]+)?\s+import\s+([\w\s,]+)" +) + + # Regex for numbered Assets direct imports (``Assets5``, ``Assets14``, etc). # Bare ``Assets`` (no digits) is a legitimate base class alias; the # regex requires at least one digit to avoid false positives. @@ -283,10 +327,51 @@ def scan_file(path: Path, *, apply_changes: bool) -> tuple[list[Finding], str | ) ) - # adcp.types.generated_poc imports. + # adcp.types.generated_poc imports. When the line is a + # single-line ``from adcp.types.generated_poc. import + # `` and any of the imported symbols are in + # GENERATED_POC_SYMBOL_MAP, emit one per-symbol Finding with the + # public-API replacement (e.g. "ContextObject → adcp.types.ContextObject"). + # Otherwise fall back to the generic "private module" flag so + # multiline / star imports still surface. for private_path, hint in PRIVATE_IMPORT_PATHS.items(): - if private_path in line: - col = line.index(private_path) + 1 + if private_path not in line: + continue + col = line.index(private_path) + 1 + from_match = _GENERATED_POC_FROM_IMPORT.search(line) + mapped_any = False + if from_match: + # Symbols list — handles ``A``, ``A, B``, ``A as X``. + # ``as`` aliases are rare in practice for these reach-ins + # but treat the LHS as the canonical symbol when present. + raw_symbols = [s.strip() for s in from_match.group(1).split(",")] + for raw in raw_symbols: + if not raw: + continue + symbol = raw.split(" as ")[0].strip() + replacement = GENERATED_POC_SYMBOL_MAP.get(symbol) + if replacement is None: + continue + sym_col = line.find(symbol, from_match.start(1)) + 1 + findings.append( + Finding( + kind="flag_private", + path=str(path), + line=lineno, + column=sym_col, + before=symbol, + after=replacement, + hint=( + f"private module — import {symbol} from " + "adcp.types (stable public API) instead" + ), + ) + ) + mapped_any = True + if not mapped_any: + # Generic flag — multiline imports, star imports, or + # symbols without a known public alias. Adopter does the + # lookup; codemod still surfaces the issue. findings.append( Finding( kind="flag_private", @@ -373,7 +458,17 @@ def _format_text_report(report: Report, *, apply_changes: bool) -> str: for f in report.flagged: by_name.setdefault(f.before, []).append(f) for name, hits in sorted(by_name.items()): - lines.append(f" {name} ({len(hits)} hit{'s' if len(hits) != 1 else ''})") + # Per-symbol mapping ("ContextObject → adcp.types.ContextObject") + # — print the explicit replacement on the header line so + # adopters fix without leaving the report. Falls back to + # bare name when no replacement is mapped. + replacement = hits[0].after + header = ( + f" {name} → {replacement} ({len(hits)} hit{'s' if len(hits) != 1 else ''})" + if replacement + else f" {name} ({len(hits)} hit{'s' if len(hits) != 1 else ''})" + ) + lines.append(header) hint = hits[0].hint if hint: lines.append(f" → {hint}") diff --git a/tests/test_migrate_v3_to_v4.py b/tests/test_migrate_v3_to_v4.py index 34e3ce6d5..941973ef8 100644 --- a/tests/test_migrate_v3_to_v4.py +++ b/tests/test_migrate_v3_to_v4.py @@ -160,13 +160,16 @@ def test_bare_assets_is_not_flagged_as_numbered(tmp_path: Path) -> None: assert numbered == [] -def test_flags_generated_poc_imports(tmp_path: Path) -> None: - """``adcp.types.generated_poc`` is a private module — flag imports - from it and point at the public alias path.""" +def test_flags_generated_poc_imports_unknown_symbol_falls_back_to_generic_hint( + tmp_path: Path, +) -> None: + """A ``generated_poc`` import for a symbol not in the per-symbol map + falls back to the generic 'private module' flag — still surfaces the + issue, adopter does the lookup.""" _write( tmp_path, "code.py", - "from adcp.types.generated_poc.core.account import Account\n", + "from adcp.types.generated_poc.core.something import Unknown\n", ) report = v3_to_v4.run(tmp_path, apply_changes=False) @@ -176,6 +179,82 @@ def test_flags_generated_poc_imports(tmp_path: Path) -> None: assert private[0].before == "adcp.types.generated_poc" +def test_flags_generated_poc_imports_per_symbol_mapping(tmp_path: Path) -> None: + """Round-5 adopter feedback (salesagent v3→v4 experiment): the + ``generated_poc`` flag-only output forced 82 of 156 findings into + hand-grep territory. Each known reach-in now emits an explicit + "Symbol → adcp.types.Symbol" replacement so adopters apply the fix + without leaving the codemod report.""" + _write( + tmp_path, + "code.py", + "from adcp.types.generated_poc.core.context import ContextObject\n" + "from adcp.types.generated_poc.core.brand_ref import BrandReference\n" + "from adcp.types.generated_poc.enums.media_buy_status import MediaBuyStatus\n" + "from adcp.types.generated_poc.core.error import Error as AdCPResponseError\n", + ) + + report = v3_to_v4.run(tmp_path, apply_changes=False) + + private = [f for f in report.flagged if f.kind == "flag_private"] + by_symbol = {f.before: f for f in private} + + # Each known symbol gets a per-symbol replacement, NOT the generic + # "adcp.types.generated_poc" flag. + assert by_symbol["ContextObject"].after == "adcp.types.ContextObject" + assert by_symbol["BrandReference"].after == "adcp.types.BrandReference" + assert by_symbol["MediaBuyStatus"].after == "adcp.types.MediaBuyStatus" + # ``import Error as AdCPResponseError`` — codemod keys off the LHS + # canonical name, ignoring the local alias. + assert by_symbol["Error"].after == "adcp.types.Error" + # The generic private-module flag MUST NOT also fire when the + # per-symbol mapping handled the line — would double-count and + # confuse the report. + assert "adcp.types.generated_poc" not in by_symbol + + +def test_flags_generated_poc_multiple_symbols_one_line(tmp_path: Path) -> None: + """``from adcp.types.generated_poc.core.x import A, B, C`` emits + one Finding per symbol so the report surfaces every replacement.""" + _write( + tmp_path, + "code.py", + "from adcp.types.generated_poc.core.x import " + "BrandReference, ContextObject, MediaBuyStatus\n", + ) + + report = v3_to_v4.run(tmp_path, apply_changes=False) + + private = [f for f in report.flagged if f.kind == "flag_private"] + by_symbol = {f.before: f.after for f in private} + assert by_symbol == { + "BrandReference": "adcp.types.BrandReference", + "ContextObject": "adcp.types.ContextObject", + "MediaBuyStatus": "adcp.types.MediaBuyStatus", + } + + +def test_generated_poc_symbol_map_covers_publicly_exported_names() -> None: + """Every entry in ``GENERATED_POC_SYMBOL_MAP`` MUST point at a real + public-API symbol on ``adcp.types`` — otherwise the hint sends + adopters to a NameError. Guards against drift between the codemod's + map and the SDK's __all__.""" + import importlib + + types_module = importlib.import_module("adcp.types") + for symbol, replacement in v3_to_v4.GENERATED_POC_SYMBOL_MAP.items(): + # Every replacement is exactly ``adcp.types.``. + assert replacement == f"adcp.types.{symbol}", ( + f"GENERATED_POC_SYMBOL_MAP[{symbol!r}] = {replacement!r} but " + f"the convention is adcp.types.{symbol}" + ) + assert hasattr(types_module, symbol), ( + f"GENERATED_POC_SYMBOL_MAP claims adcp.types.{symbol} exists " + "but it's not on the public types module — drop the entry " + "or add the public alias." + ) + + def test_flags_removed_attribute_accesses(tmp_path: Path) -> None: """``.brand_manifest`` on ResolvedBrand was removed — flag the attribute access with a hint."""