feat(handlers): expose sync_catalogs through sales-catalog-driven specialism

[bokelley]

Closes #786

Re-cut of #790 from current main (19fde5e). PR #790 was conflicting (dirty) with maintainer edits disabled; this branch is a clean re-apply of the same logic on top of 6.1.0-beta.1.

Summary

• Wire sync_catalogs into SPECIALISM_TO_ADVERTISED_TOOLS so sales-catalog-driven platforms expose the tool via tools/list
• Add PlatformHandler.sync_catalogs dispatcher — full SyncCatalogsRequest passed through (no arg_projector) so adopters can inspect req.delete_missing, req.dry_run, req.validation_mode, and req.catalog_ids
• Add sync_catalogs to SalesPlatform Protocol body (required section, sales-catalog-driven only)
• Add _require_platform_method("sync_catalogs") guard — non-catalog-driven platforms that somehow receive the call surface UNSUPPORTED_FEATURE instead of INTERNAL_ERROR
• Add sync_catalogs to _OPTIONAL_PLATFORM_METHODS (specialism-gated variant — absent on all non-sales-catalog-driven platforms)
• Add examples/hello_seller_catalog.py — runnable minimal adopter with discovery mode, delete_missing guard, and ergonomic list return
• Add MIGRATION_v5_to_v6.md 6.1.0-beta.1 entry documenting SalesPlatform @runtime_checkable isinstance behavior change
• Fix _SyncSalesPlatform test fixture to remain structurally conformant with SalesPlatform

Protocol note: sync_catalogs on the shared SalesPlatform Protocol

Adding sync_catalogs to the shared SalesPlatform Protocol (rather than a separate CatalogDrivenPlatform mixin) is intentional and consistent with how sync_audiences is handled on AudiencePlatform. The consequence is that isinstance(platform, SalesPlatform) now requires sync_catalogs for structural conformance — non-catalog-driven platforms that use this check need a stub. This is documented in MIGRATION_v5_to_v6.md. A separate CatalogDrivenPlatform mixin would be cleaner but is a larger structural change; see code-reviewer sign-off below.

What was tested

• pytest tests/test_decisioning_advertised_per_specialism.py tests/test_decisioning_dispatch.py tests/test_decisioning_handler_shims.py tests/test_decisioning_specialisms.py tests/test_lazy_platform_router.py — 235 passed
• ruff check src/ — clean
• python -m mypy src/adcp/decisioning/handler.py src/adcp/decisioning/specialisms/sales.py — clean

Nits (not fixing in this PR):

• _require_platform_method message says "optional method" — technically imprecise for sync_catalogs on sales-catalog-driven, but validate_platform (called from serve()) hard-fails at boot before any buyer reaches this gate; the backstop is only reachable via programmatic PlatformHandler construction without serve(). Pre-existing pattern used by 8 other methods.
• _maybe_auto_emit_sync_completion fires on dry_run=True requests — pre-existing pattern shared with sync_audiences; sync_catalogs in webhook_emit._SYNC_COMPLETION_METHODS is already present on main.
• MaybeAsync[SyncCatalogsSuccessResponse] Protocol return type doesn't formally include the ergonomic list[SyncCatalogResult] arm — pre-existing mismatch shared with sync_audiences.

Pre-PR review

code-reviewer: No blockers. Design concern noted (shared Protocol body vs. separate mixin) — explained above as intentional pattern. 1 nit (inline fixture duplication across 3 shim tests). Approved.

dx-expert: Approved (2nd pass). Previous blockers resolved: example added, _SyncSalesPlatform fixture fixed, migration note added. No new blockers.

---

Triage-managed PR. This bot does not currently iterate on
review comments or PR conversation threads (only on the source
issue). To unblock:

• Push fixup commits directly: gh pr checkout <num> →
  fix → push.
• Or re-trigger: comment /triage execute on the source
  issue.

See adcp#3121
for context.

Session: https://claude.ai/code/session_01NYBCVdwct1Hhz7GD1eaEHr

---

Generated by Claude Code

[bokelley]

Resolved the current-main conflict by keeping both the canonical-format migration notes from main and this PR's sales-catalog-driven migration note, then pushed merge commit 55ba144.\n\nLocal validation:\n- python3 -m ruff check src/adcp/decisioning/handler.py src/adcp/decisioning/specialisms/sales.py tests/test_decisioning_advertised_per_specialism.py tests/test_decisioning_dispatch.py tests/test_decisioning_handler_shims.py tests/test_decisioning_specialisms.py tests/test_lazy_platform_router.py examples/hello_seller_catalog.py\n- PYTHONPATH=src pytest tests/test_decisioning_advertised_per_specialism.py tests/test_decisioning_dispatch.py tests/test_decisioning_handler_shims.py tests/test_decisioning_specialisms.py tests/test_lazy_platform_router.py -q → 235 passed\n\nLeaving as draft until the refreshed GitHub checks complete; if they stay green, recommendation is to mark ready for review and merge this as the active fix for #786.

[bokelley]

Acknowledged — conflict resolution looks clean (keeping both migration note sections is correct), and 235 passed locally is a good signal. Waiting on the refreshed GitHub checks before marking ready for review is the right call. No action needed from triage; will re-engage if checks flag anything on the source issue.

---

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

---

Generated by Claude Code

[aao-ipr-bot]

LGTM. Follow-ups noted below. Closes the sales-catalog-driven wire-up gap from #786 by mirroring the sync_audiences shim shape — full request through (no arg_projector), list-arm projection to the wire envelope, _require_platform_method gate for the non-serve() construction path.

Things I checked

• Specialism mapping is internally consistent. _CATALOG_ADVERTISED_TOOLS (handler.py:286-291) → unioned into sales-catalog-driven only (handler.py:406-408) → unioned into the class-level universe (handler.py:1045) → and sync_catalogs is in _OPTIONAL_PLATFORM_METHODS (handler.py:378-380). No drift across the four sites that have to agree.
• _require_platform_method gate is structurally meaningful, not dead code. For sales-catalog-driven, validate_platform boot-fails first, so the gate is unreachable on that path. But for programmatic PlatformHandler construction without serve(), the gate converts AttributeError → INTERNAL_ERROR into UNSUPPORTED_FEATURE. The test at tests/test_decisioning_handler_shims.py:439-476 pins this. Good.
• Wire envelope is conformant. _project_sync_catalogs at handler.py:808-822 wraps the list arm as {catalogs: [...]}; upstream sync-catalogs-response.json requires only catalogs on the success arm, dry_run is optional. ad-tech-protocol-expert: sound.
• Discovery mode is upstream-defined, not an SDK invention. sync-catalogs-request.json line 4 and the request required block confirm catalogs is optional and catalogs is None is the spec's discovery semantics. The Protocol docstring at sales.py:286-298 and examples/hello_seller_catalog.py:67-74 match.
• delete_missing=True + catalogs=None guard is upstream-defined. Request schema lines 74-89 enforce this via JSON-Schema if/then with the same errorMessage ("would delete all buyer-managed catalogs"). The example raising AdcpError("INVALID_REQUEST", field="catalogs") surfaces a spec-mandated rejection — defense-in-depth, since the transport-layer validator catches it first.
• isinstance(x, SalesPlatform) behavior change is documented. MIGRATION_v5_to_v6.md:303-322 and the _SyncSalesPlatform test fixture (tests/test_lazy_platform_router.py:87-91) both pin the structural consequence of adding sync_catalogs to the Protocol body. On a 6.1.0-beta.1 pre-release train, feat: is acceptable — feat!: would force a 6.x → 7.x jump under release-please that's wrong for a beta.
• Test additions cover every load-bearing seam. Advertised-tools coverage (advertised_per_specialism.py:359-394), full-request passthrough, discovery passthrough, list-arm projection, all four _project_sync_catalogs arms (handler_shims.py:396-433), and the UNSUPPORTED_FEATURE gate. The "all wire tools" enumeration in test_advertised_tools_covers_every_specialism_wire_tool adds sync_catalogs in both locations (L85-89, L134).

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

• dry_run is silently dropped from the list-arm projection. _project_sync_catalogs (handler.py:808-822) doesn't echo dry_run: true when params.dry_run is set and the adopter returned the ergonomic list. Pre-existing pattern shared with _project_sync_audiences (handler.py:790-806), so not a regression — but an adopter using the list arm in a dry-run path makes the buyer think real mutations happened. Either echo params.dry_run into the wrapper or document the list arm as "non-dry-run only" in sales.py:281. (ad-tech-protocol-expert: caveat #1.)
• validation_mode=strict semantics are adopter-side. The shim passes the field through with zero enforcement; an adopter under validation_mode=strict who returns a list with any action: "failed" row produces a success envelope where the spec calls for a SyncCatalogsError envelope. Worth documenting on sales.py:296-298 or enforcing in the shim. (ad-tech-protocol-expert: caveat #2.)
• _maybe_auto_emit_sync_completion fires under dry_run=True and discovery mode. Pre-existing pattern noted in the PR description; worth pinning a follow-up to make the auto-emit aware of dry-run/discovery and skip the buyer notification.

Minor nits (non-blocking)

1. getattr on a typed Pydantic field in the example. examples/hello_seller_catalog.py:73,76 uses getattr(req, "delete_missing", False) / getattr(req, "catalogs", None) where req: SyncCatalogsRequest is a known Pydantic model. Defensive-getattr is appropriate in framework code crossing the Protocol boundary, but in adopter-facing example code it models the wrong pattern. Replace with req.delete_missing / req.catalogs.
2. Handler shim docstring doesn't cross-link the delete_missing=True + catalogs=None rejection rule. handler.py:2412-2425 describes the two return arms but doesn't mention the spec-undefined combination called out at sales.py:298-301. Buyers reading the handler signature won't see the rule. Cross-link or duplicate the warning.
3. Unversioned schema path in the projection docstring. handler.py:813 cites schemas/cache/media-buy/sync-catalogs-response.json; actual cached versions are 3.0/ and 3.1.0-beta.3/. Consistent with the _project_sync_audiences docstring (L793) so not new churn, but the path rots when the schema cache cycles.
4. Migration-note stub uses dict return. MIGRATION_v5_to_v6.md:316-319 shows return {"catalogs": []} — correct envelope but loses type information. Cross-reference to the typed SyncCatalogsSuccessResponse(catalogs=[]) form for adopters who type-check.

Notable that the third _SyncSalesPlatform test-fixture cleanup (tests/test_lazy_platform_router.py:87-91) lands as part of the same PR — the "add a required Protocol method" pattern wants a structural-conformance helper, but that is a separate refactor and out of scope here.

Approving on the strength of the internally-consistent specialism wiring plus the upstream-conformant discovery and delete_missing semantics.

[aao-ipr-bot]

Hold. The adopter-facing example crashes on import — the single artifact this PR added per dx-expert's previous block does not run as documented.

MUST FIX (blocking)

1. examples/hello_seller_catalog.py:17 — broken import, example crashes at startup. from adcp import AdcpError fails: AdcpError is not exported from adcp top-level (src/adcp/__init__.py has no entry, and the module-level __getattr__ at line 561 raises AttributeError for unknown names). Every other example in the tree uses from adcp.decisioning import AdcpError — examples/hello_mock_seller.py:91, examples/multi_platform_seller/src/account_store.py:25, examples/v3_reference_seller/src/app.py:58. Running the example via the documented uv run python examples/hello_seller_catalog.py raises ImportError before serve() is reached. Fix: change line 17 to from adcp.decisioning import AdcpError (or fold it into the existing from adcp.decisioning import (...) block on line 18).

Things I checked

• Specialism wiring at src/adcp/decisioning/handler.py:406-408 correctly composes _CATALOG_ADVERTISED_TOOLS with _SALES_ADVERTISED_TOOLS | _ACCOUNT_ADVERTISED_TOOLS for sales-catalog-driven only.
• The shim at src/adcp/decisioning/handler.py:2407-2441 omits arg_projector — full SyncCatalogsRequest passes through, so req.delete_missing, req.dry_run, req.validation_mode, and req.catalogs is None discovery mode all reach the adopter intact. _require_platform_method("sync_catalogs") runs before _invoke_platform_method so absent methods surface UNSUPPORTED_FEATURE, not INTERNAL_ERROR.
• _project_sync_catalogs at handler.py:808-820 wraps list-arm into {catalogs: [...]} — ad-tech-protocol-expert confirmed this matches schemas/cache/3.1.0-beta.3/media-buy/sync-catalogs-response.json:165-167 exactly.
• _OPTIONAL_PLATFORM_METHODS entry at handler.py:380 is correct — sync_catalogs is required at boot for sales-catalog-driven (via REQUIRED_METHODS_PER_SPECIALISM in dispatch.py:200-209), and absent on all other sales-* claims, so the runtime AttributeError shield is needed. No naming collision.
• _maybe_auto_emit_sync_completion("sync_catalogs", ...) works because sync_catalogs is already in SPEC_WEBHOOK_TASK_TYPES at webhook_emit.py:89. (Pre-PR; PR body's reference to _SYNC_COMPLETION_METHODS is the wrong constant name — non-blocking.)
• Test coverage: tests/test_decisioning_dispatch.py pins the validate_platform hard-fail at boot for sales-catalog-driven without sync_catalogs; tests/test_decisioning_handler_shims.py covers full-request passthrough, discovery-mode catalogs=None passthrough, list-arm projection, and the UNSUPPORTED_FEATURE gate. _SyncSalesPlatform (test_lazy_platform_router.py:87) and _SalesShim (test_decisioning_specialisms.py:401) fixtures both gain the stub.
• Migration note at MIGRATION_v5_to_v6.md:306-323 documents the @runtime_checkable isinstance consequence clearly with a working snippet.

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

• Sibling-Protocol asymmetry vs. sync_audiences. Per ad-tech-protocol-expert: sync_audiences lives on its own AudiencePlatform Protocol (src/adcp/decisioning/specialisms/audience.py:51-92), but sync_catalogs is grafted onto the shared SalesPlatform body. Wire-equivalent, but five non-catalog sales-* specialisms now carry a method they're not supposed to implement, weakening @runtime_checkable value. A CatalogDrivenPlatform sibling Protocol would mirror the audience pattern. PR body acknowledges this as intentional — fine to defer, but worth filing.
• Docstring wording at src/adcp/decisioning/specialisms/sales.py:301-303. delete_missing=True with catalogs=None is spec-rejected, not spec-undefined — the upstream schemas/.../sync-catalogs-request.json has an explicit if/then (lines 73-88) requiring catalogs when delete_missing=true. Tighten the wording in the next pass.
• The three nits the PR body already enumerates (the _require_platform_method "optional method" message, _maybe_auto_emit_sync_completion firing on dry_run=True, the MaybeAsync return-type mismatch on the list arm) are all pre-existing pattern. Don't drag them into this PR.

Re-request review once the import is fixed.

[aao-ipr-bot]

LGTM. Clean re-cut of #790 — sync_catalogs lands on the same rails as sync_audiences, which is the right precedent for a per-specialism wire tool with a list/envelope return.

Things I checked

• _CATALOG_ADVERTISED_TOOLS unioned only into sales-catalog-driven (src/adcp/decisioning/handler.py:404-407); regression-pinned both ways at tests/test_decisioning_advertised_per_specialism.py:359-395. No other sales-* specialism leaks the tool.
• Wire envelope {catalogs: [...]} in _project_sync_catalogs (handler.py:808-822) matches schemas/cache/3.1.0-beta.3/media-buy/sync-catalogs-response.json SyncCatalogsSuccess arm. Byte-identical structure to _project_sync_audiences at handler.py:785-805.
• Deliberate arg_projector omission on the sync_catalogs shim (handler.py:2407-2441) — adopters get the full SyncCatalogsRequest so req.delete_missing / req.dry_run / req.validation_mode / req.catalog_ids are inspectable. Documented divergence from sync_audiences.
• Discovery mode (req.catalogs is None) is spec-defined per the request schema description, not SDK-invented. delete_missing=true + catalogs=None rejection is correct — the schema's if/then at sync-catalogs-request.json:73-87 makes catalogs required when delete_missing=true.
• Boot-fail-on-missing-method pinned at tests/test_decisioning_dispatch.py:130-160. UNSUPPORTED_FEATURE backstop pinned at tests/test_decisioning_handler_shims.py:440-476 — the gate is only reachable via programmatic PlatformHandler construction without serve(), as the PR body calls out.
• MIGRATION_v5_to_v6.md 6.1.0-beta.1 entry documents the @runtime_checkable isinstance consequence. _SyncSalesPlatform (tests/test_lazy_platform_router.py:87-90) and the test_decisioning_specialisms.py _SalesShim (line 401-402) both updated.
• 235/235 tests pass per PR body, mirroring CI.

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

• Migration-note heading is stale. MIGRATION_v5_to_v6.md:306 is headed ### 6.1.0-beta.1 but pyproject.toml:7 already pins 6.1.1b2. Adopters diffing the lockfile from an older beta will read this entry as historical and skip it. Bump to whatever release-please cuts next.
• "Spec-undefined" wording is wrong. Docstring at src/adcp/decisioning/specialisms/sales.py:300 and the example at examples/hello_seller_catalog.py:78 say delete_missing=True + catalogs=None is "spec-undefined." The request schema's if/then constraint (sync-catalogs-request.json:73-87) explicitly forbids that combination with a normative errorMessage. Behavior is right; comment should say "spec-forbidden" / "schema-rejected."
• List-arm projection drops dry_run. _project_sync_catalogs (handler.py:808-822) wraps the ergonomic list into {catalogs: [...]} but doesn't thread req.dry_run through to the top-level envelope where the response schema places it (sync-catalogs-response.json lines 20-23). Pre-existing pattern shared with sync_audiences, so out of scope here — file once and fix in both places.
• Conventional-commit signal. Adding a required method to a @runtime_checkable Protocol breaks isinstance for adopters not implementing it. The PR title is feat(handlers):. Acceptable on a beta with a documented migration note, but feat(handlers)!: with a BREAKING CHANGE: footer matches the precedent in CHANGELOG.md:14 (the 6.1.0-beta.2 callout). Worth being stricter on the next breaking beta change.

Minor nits (non-blocking)

1. Protocol body uses raise NotImplementedError instead of .... Every other method on SalesPlatform uses ... as the Protocol body; sync_catalogs at src/adcp/decisioning/specialisms/sales.py:310 is the odd one out (added by fixup 75dca7ff to satisfy a lint complaint). Minor consequence: _require_platform_method's hasattr check at handler.py:2211 now silently passes for subclasses that inherit-but-don't-override, so the dispatch-time backstop is slightly weakened. The boot-fail rule still catches sales-catalog-driven adopters; this just narrows the safety net for the programmatic-construction edge case the docstring at handler.py:2199-2210 advertises. Worth aligning to ... if there's a less invasive way to silence the lint.
2. Dead membership. _OPTIONAL_PLATFORM_METHODS (handler.py:357-381) is referenced only by its own docstring — adding sync_catalogs there is documentation, not behavior. Consistent with prior pattern, just noting it's inert.
3. Example: or [] after a None-check is dead. examples/hello_seller_catalog.py:99 has req.catalogs or [] inside the comprehension, but line 96 already early-returned on req.catalogs is None.

The shared SalesPlatform Protocol vs. a CatalogDrivenPlatform mixin trade-off is acknowledged in the PR body and matches the sync_audiences / AudiencePlatform precedent. Reasonable call — mixin refactor is a wider blast radius than this PR should carry.

Safe to merge.