fix: constrain getattr to model_fields, add targeting to declared features

Addresses expert review feedback:
- Add model_fields guard on media_buy.features and signals.features getattr
  (consistent with targeting namespace, prevents Pydantic internal access)
- Include targeting features in get_declared_features() for better error messages
- Fix test assertion (or → and) in test_require_error_lists_declared_features
- Add edge case tests: empty require(), nonexistent targeting field, model_fields
  guard, unmapped task, success=True with data=None
- Move repeated in-method imports to module level

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: bokelley

src/adcp/capabilities.py

@@ -101,15 +101,17 @@ def supports(self, feature: str) -> bool:
 
         # Media buy features check
         if caps.media_buy is not None and caps.media_buy.features is not None:
-            val = getattr(caps.media_buy.features, feature, None)
-            if val is True:
-                return True
+            if feature in type(caps.media_buy.features).model_fields:
+                val = getattr(caps.media_buy.features, feature, None)
+                if val is True:
+                    return True
 
         # Signals features check
         if caps.signals is not None and caps.signals.features is not None:
-            val = getattr(caps.signals.features, feature, None)
-            if val is True:
-                return True
+            if feature in type(caps.signals.features).model_fields:
+                val = getattr(caps.signals.features, feature, None)
+                if val is True:
+                    return True
 
         return False
 
@@ -163,6 +165,15 @@ def get_declared_features(self) -> list[str]:
                 if getattr(caps.signals.features, field_name, None) is True:
                     declared.append(field_name)
 
+        # Targeting features
+        if caps.media_buy is not None and caps.media_buy.execution is not None:
+            targeting = caps.media_buy.execution.targeting
+            if targeting is not None:
+                for field_name in type(targeting).model_fields:
+                    val = getattr(targeting, field_name, None)
+                    if val is not None and val is not False:
+                        declared.append(f"targeting.{field_name}")
+
         # Extensions
         if caps.extensions_supported is not None:
             for item in caps.extensions_supported:

tests/test_capabilities.py

@@ -7,8 +7,10 @@
 
 import pytest
 
-from adcp import ADCPClient
+from adcp import AccountReference, ADCPClient, SyncEventSourcesRequest
+from adcp.capabilities import FeatureResolver, validate_capabilities
 from adcp.exceptions import ADCPError, ADCPFeatureUnsupportedError
+from adcp.server.base import ADCPHandler
 from adcp.types.core import AgentConfig, Protocol, TaskResult, TaskStatus
 from adcp.types.generated_poc.core.media_buy_features import MediaBuyFeatures
 from adcp.types.generated_poc.protocol.get_adcp_capabilities_response import (
@@ -91,8 +93,6 @@ def _client_with_caps(
     **kwargs,
 ) -> ADCPClient:
     """Create a client and inject cached capabilities."""
-    from adcp.capabilities import FeatureResolver
-
     client = ADCPClient(_make_config(), **kwargs)
     client._capabilities = caps
     client._feature_resolver = FeatureResolver(caps)
@@ -193,6 +193,27 @@ def test_targeting_no_execution(self):
 
         assert client.supports("targeting.geo_countries") is False
 
+    def test_targeting_nonexistent_field(self):
+        """Targeting field not in model_fields returns False."""
+        caps = _make_capabilities(
+            targeting={"geo_countries": True}
+        )
+        client = _client_with_caps(caps)
+
+        assert client.supports("targeting.nonexistent_field") is False
+        assert client.supports("targeting.__class__") is False
+
+    def test_model_fields_guard_on_features(self):
+        """Pydantic internals are not treated as features."""
+        caps = _make_capabilities(
+            media_buy_features={"audience_targeting": True}
+        )
+        client = _client_with_caps(caps)
+
+        assert client.supports("model_dump") is False
+        assert client.supports("model_fields") is False
+        assert client.supports("__class__") is False
+
     def test_unknown_feature_returns_false(self):
         caps = _make_capabilities(
             media_buy_features={"audience_targeting": True}
@@ -234,6 +255,12 @@ async def test_fetch_then_supports(self):
 class TestRequire:
     """Tests for seller.require(*features)."""
 
+    def test_require_no_features_is_noop(self):
+        """require() with zero arguments does not raise."""
+        caps = _make_capabilities()
+        client = _client_with_caps(caps)
+        client.require()  # Should not raise
+
     def test_require_all_present(self):
         caps = _make_capabilities(
             media_buy_features={"audience_targeting": True, "conversion_tracking": True}
@@ -299,7 +326,7 @@ def test_require_error_lists_declared_features(self):
             client.require("audience_targeting")
 
         error_str = str(exc_info.value)
-        assert "inline_creative_management" in error_str or "conversion_tracking" in error_str
+        assert "inline_creative_management" in error_str and "conversion_tracking" in error_str
 
     def test_require_includes_agent_context(self):
         caps = _make_capabilities()
@@ -402,6 +429,24 @@ async def test_refresh_capabilities_failure_raises(self):
             with pytest.raises(ADCPError, match="Failed to fetch capabilities"):
                 await client.refresh_capabilities()
 
+    @pytest.mark.asyncio
+    async def test_fetch_capabilities_success_true_data_none(self):
+        """success=True but data=None still raises."""
+        client = ADCPClient(_make_config())
+
+        result = TaskResult(
+            status=TaskStatus.COMPLETED,
+            data=None,
+            success=True,
+        )
+        with patch.object(
+            client, "get_adcp_capabilities", new_callable=AsyncMock
+        ) as mock_get:
+            mock_get.return_value = result
+
+            with pytest.raises(ADCPError, match="Failed to fetch capabilities"):
+                await client.fetch_capabilities()
+
 
 # ========================================================================
 # Automatic validation (validate_features) tests
@@ -411,14 +456,20 @@ async def test_refresh_capabilities_failure_raises(self):
 class TestValidateFeatures:
     """Tests for automatic feature validation on task calls."""
 
+    def test_validate_skips_unmapped_task(self):
+        """Tasks not in TASK_FEATURE_MAP are not validated."""
+        caps = _make_capabilities(media_buy_features={})
+        client = _client_with_caps(caps, validate_features=True)
+        # get_products is not in TASK_FEATURE_MAP, should not raise
+        client._validate_task_features("get_products")
+
     @pytest.mark.asyncio
     async def test_sync_event_sources_requires_conversion_tracking(self):
         caps = _make_capabilities(
             media_buy_features={"audience_targeting": True}
         )
         client = _client_with_caps(caps, validate_features=True)
 
-        from adcp import AccountReference, SyncEventSourcesRequest
 
         request = SyncEventSourcesRequest(
             account=AccountReference(account_id="acc1"),
@@ -448,7 +499,6 @@ async def test_validation_passes_when_feature_supported(self):
         )
         client = _client_with_caps(caps, validate_features=True)
 
-        from adcp import AccountReference, SyncEventSourcesRequest
 
         mock_result = TaskResult(
             status=TaskStatus.COMPLETED,
@@ -470,7 +520,6 @@ async def test_validation_skipped_when_not_opted_in(self):
         )
         client = _client_with_caps(caps, validate_features=False)
 
-        from adcp import AccountReference, SyncEventSourcesRequest
 
         mock_result = TaskResult(
             status=TaskStatus.COMPLETED,
@@ -490,7 +539,6 @@ async def test_validation_skipped_when_no_capabilities(self):
         """When capabilities haven't been fetched, skip validation."""
         client = ADCPClient(_make_config(), validate_features=True)
 
-        from adcp import AccountReference, SyncEventSourcesRequest
 
         mock_result = TaskResult(
             status=TaskStatus.COMPLETED,
@@ -560,8 +608,6 @@ class TestFeatureResolver:
     """Tests for FeatureResolver used independently of ADCPClient."""
 
     def test_resolver_supports(self):
-        from adcp.capabilities import FeatureResolver
-
         caps = _make_capabilities(
             protocols=["media_buy", "signals"],
             media_buy_features={"audience_targeting": True},
@@ -577,8 +623,6 @@ def test_resolver_supports(self):
         assert resolver.supports("conversion_tracking") is False
 
     def test_resolver_require_raises(self):
-        from adcp.capabilities import FeatureResolver
-
         caps = _make_capabilities(media_buy_features={})
         resolver = FeatureResolver(caps)
 
@@ -588,12 +632,11 @@ def test_resolver_require_raises(self):
         assert exc_info.value.agent_id == "test"
 
     def test_resolver_get_declared_features(self):
-        from adcp.capabilities import FeatureResolver
-
         caps = _make_capabilities(
             protocols=["media_buy"],
             media_buy_features={"audience_targeting": True, "conversion_tracking": True},
             extensions=["scope3"],
+            targeting={"geo_countries": True},
         )
         resolver = FeatureResolver(caps)
 
@@ -602,10 +645,9 @@ def test_resolver_get_declared_features(self):
         assert "audience_targeting" in declared
         assert "conversion_tracking" in declared
         assert "ext:scope3" in declared
+        assert "targeting.geo_countries" in declared
 
     def test_resolver_capabilities_property(self):
-        from adcp.capabilities import FeatureResolver
-
         caps = _make_capabilities()
         resolver = FeatureResolver(caps)
         assert resolver.capabilities is caps
@@ -620,8 +662,6 @@ class TestValidateCapabilities:
     """Tests for server-side validate_capabilities()."""
 
     def test_warns_on_declared_but_unimplemented(self):
-        from adcp.capabilities import validate_capabilities
-        from adcp.server.base import ADCPHandler
 
         class MyHandler(ADCPHandler):
             pass  # Doesn't override anything
@@ -636,8 +676,6 @@ class MyHandler(ADCPHandler):
         assert any("log_event" in w or "sync_event_sources" in w for w in warnings)
 
     def test_no_warnings_when_handler_overrides(self):
-        from adcp.capabilities import validate_capabilities
-        from adcp.server.base import ADCPHandler
 
         class MyHandler(ADCPHandler):
             async def log_event(self, params, context=None):
@@ -654,8 +692,6 @@ async def sync_event_sources(self, params, context=None):
         assert len(warnings) == 0
 
     def test_no_warnings_when_feature_not_declared(self):
-        from adcp.capabilities import validate_capabilities
-        from adcp.server.base import ADCPHandler
 
         class MyHandler(ADCPHandler):
             pass
@@ -669,8 +705,6 @@ class MyHandler(ADCPHandler):
 
     def test_warns_on_partial_implementation(self):
         """If only some handler methods for a feature are overridden."""
-        from adcp.capabilities import validate_capabilities
-        from adcp.server.base import ADCPHandler
 
         class MyHandler(ADCPHandler):
             async def log_event(self, params, context=None):
@@ -688,8 +722,6 @@ async def log_event(self, params, context=None):
 
     def test_no_warnings_when_mixin_overrides(self):
         """Overrides inherited from an intermediate class are detected."""
-        from adcp.capabilities import validate_capabilities
-        from adcp.server.base import ADCPHandler
 
         class ConversionMixin(ADCPHandler):
             async def log_event(self, params, context=None):
@@ -710,8 +742,6 @@ class MyHandler(ConversionMixin):
 
     def test_multiple_handler_methods_warned(self):
         """All handler methods for a declared feature produce warnings when unoverridden."""
-        from adcp.capabilities import validate_capabilities
-        from adcp.server.base import ADCPHandler
 
         class MyHandler(ADCPHandler):
             pass