feat: feature capability validation API (supports/require)#158
Conversation
KonstantinMirin
left a comment
There was a problem hiding this comment.
Great feature — clean architecture, solid test coverage.
A few observations:
1. MRO override detection in validate_capabilities() (bug)
type(handler).__dict__ only checks the leaf class. If a handler inherits overrides from an intermediate class, it'll produce false warnings:
class ConversionMixin(ADCPHandler):
async def log_event(self, ...): ...
class MyHandler(ConversionMixin): pass # inherits the override
validate_capabilities(MyHandler(), caps) # warns incorrectlyShould walk the MRO up to (excluding) ADCPHandler:
mro_classes = [cls for cls in type(handler).__mro__ if cls is not ADCPHandler and not issubclass(ADCPHandler, cls)]
is_overridden = any(method_name in cls.__dict__ for cls in mro_classes)2. Feature namespace prefixes — worth establishing now
Since this is forward-looking, it's worth establishing unambiguous prefixes before more features land. Currently supports("audience_targeting") checks media_buy.features first, then signals.features — if both namespaces ever share a field name, media buy silently wins. Consider requiring signals.catalog_signals (matching the existing targeting. and ext: prefix conventions) so the lookup is unambiguous from the start.
3. TASK_FEATURE_MAP coverage — track what's missing
Only conversion_tracking → {sync_event_sources, log_event} is mapped today. Worth opening a tracking issue (or adding a note here) enumerating the unmapped features so it's clear what still needs wiring up:
audience_targeting→sync_audiences(when added)catalog_management→sync_catalogs(when added)inline_creative_management→sync_creatives,list_creativesproperty_list_filtering→ (already works viaget_productsfiltering, may not need a gate)content_standards→ content standards handler methods
This makes it easy for someone to pick up the remaining work.
4. Sales agent impact
No breaking changes — all new constructor args have backward-compatible defaults. The sales agent doesn't use ADCPHandler subclasses (it uses FastMCP tools directly), so validate_capabilities() isn't directly applicable today, but the buyer-side supports()/require() API works correctly against the sales agent's existing capabilities response. No immediate action needed on the sales agent side.
|
Thanks for the thorough review @KonstantinMirin! 1. MRO override detection — fixed ✅Good catch on the mixin pattern. Updated 2. Feature namespace prefixesAgree this is worth thinking about. Today media_buy and signals feature names are disjoint in the spec (media_buy has I'd lean toward adding 3. TASK_FEATURE_MAP coverageAdded a comment in the code documenting the intentional scope. Opening a tracking issue for the remaining mappings makes sense — I'll create one after this merges so we can enumerate what's needed as the handler methods land. 4. Sales agent impactThanks for confirming no breaking changes on that side. Agreed that |
Adds FeatureResolver for resolving feature support from capabilities responses, shared by both client (buyer) and server (seller) sides. Client-side: supports(), require(), fetch_capabilities() with TTL caching, and opt-in automatic validation on task calls via validate_features=True. Server-side: validate_capabilities() checks that declared features have corresponding handler method overrides, walking the MRO for mixin support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
55d62af to
d550c08
Compare
Summary
Implements #149. Adds a
FeatureResolverclass andsupports()/require()API for feature capability validation, shared by both buyer (client) and seller (server) sides of the SDK.FeatureResolver— resolves feature support across multiple namespaces: protocols ("media_buy"), extensions ("ext:scope3"), targeting ("targeting.geo_countries"), media buy features ("audience_targeting"), and signals features ("catalog_signals")ADCPClient.supports(feature)— check if a seller supports a featureADCPClient.require(*features)— fail fast withADCPFeatureUnsupportedErrorif features are missingfetch_capabilities()with configurable TTL (default 1hr),refresh_capabilities()to bypass cachevalidate_features=Truechecks feature requirements before task calls (e.g.,sync_event_sourcesrequiresconversion_tracking)validate_capabilities()— development-time check that handler implementations match declared featuresADCPFeatureUnsupportedError— includes unsupported features, declared features, and agent contextTest plan
model_fieldsconstraint ongetattr, derivedFEATURE_HANDLER_MAP,type(handler).__dict__for override detection, logger.warning on misconfigured auto-validationCloses #149
🤖 Generated with Claude Code