fix(adagents): address review on publisher_properties resolution (#750)

bokelley · bokelley · commit 0b70d338be88 · 2026-05-21T08:18:59.000-04:00
Tightens authorization-decision semantics in `_resolve_agent_properties`:

- Dedup key is now `(publisher_domain, property_id)` instead of bare `property_id`,
  so the same id under different parent domains stays distinct and missing-id
  properties no longer collapse to a single entry. Missing `property_id` now
  raises `AdagentsValidationError` (fail-fast per CLAUDE.md).
- New `_validate_publisher_properties_selector` rejects two illegal selector
  shapes per adcp#4827: mixing scalar `publisher_domain` with array
  `publisher_domains`, and pairing `selection_type='by_id'` with the compact
  `publisher_domains[]` fan-out form (no defined per-domain id partitioning).
- `_resolve_inline` fails closed on unknown `selection_type` (returns `[]`
  instead of all candidates) and on `selection_type='by_tag'` with empty
  `property_tags` — both are authorization decisions where a permissive
  fallback would silently grant access on schema drift.
- Documents that parent-file `revoked_publisher_domains[]` (when implemented)
  is honored transparently because revoked domains are pre-filtered out of the
  per-domain index before resolution.

Adds tests covering each rejection path, the new fail-closed behaviors, and
the missing-property_id fail-fast. Adds a 2.0s wall-clock bound to the
cafemedia scale test to catch O(N×M) regressions without depending on
pytest-timeout (not currently a project dep).
diff --git a/src/adcp/adagents.py b/src/adcp/adagents.py
@@ -955,6 +955,11 @@ def _resolve_agent_properties(
     # fetch. Federated fetch (per-domain HTTP) is a follow-up; this change
     # fixes the primary bug of returning raw selector dicts instead of resolved
     # property objects.
+    #
+    # Revocation: parent-file `revoked_publisher_domains[]` (when implemented)
+    # pre-filters revoked domains from the top-level properties[] before this
+    # function is called; inline resolution then honors revocation transparently
+    # because the per-domain index built below never sees revoked-domain entries.
     if authorization_type == "publisher_properties":
         selectors = agent.get("publisher_properties", [])
         if not isinstance(selectors, list):
@@ -969,17 +974,30 @@ def _resolve_agent_properties(
                 if isinstance(d, str) and d:
                     domain_index.setdefault(d, []).append(p)
         resolved: list[dict[str, Any]] = []
-        seen_ids: set[str | None] = set()
+        # Key dedup on (publisher_domain, property_id) — same property_id on
+        # different domains is a legitimate distinct property, and missing
+        # property_id collapses to a single entry under bare `str` keying.
+        seen_keys: set[tuple[str, str]] = set()
         for selector in selectors:
             if not isinstance(selector, dict):
                 continue
+            _validate_publisher_properties_selector(selector)
             for domain in _selector_domains(selector):
                 inline = _resolve_inline(selector, domain_index, domain)
                 if inline is not None:
                     for prop in inline:
                         pid = prop.get("property_id")
-                        if pid not in seen_ids:
-                            seen_ids.add(pid)
+                        if not isinstance(pid, str) or not pid:
+                            raise AdagentsValidationError(
+                                f"property under domain={domain!r} is missing "
+                                "required 'property_id'"
+                            )
+                        prop_domain = prop.get("publisher_domain")
+                        if not isinstance(prop_domain, str):
+                            prop_domain = domain
+                        key = (prop_domain, pid)
+                        if key not in seen_keys:
+                            seen_keys.add(key)
                             resolved.append(prop)
                     # inline succeeded; skip federated fetch for this domain
                 # inline is None → no parent-file data for domain; federated
@@ -989,6 +1007,33 @@ def _resolve_agent_properties(
     return []
 
 
+def _validate_publisher_properties_selector(selector: dict[str, Any]) -> None:
+    """Reject illegal publisher_properties selector shapes per adcp#4827.
+
+    Raises ``AdagentsValidationError`` for shapes that have no valid semantics:
+
+    * Both ``publisher_domain`` (scalar) and ``publisher_domains`` (compact array
+      form) are set — the spec forbids mixing the two domain encodings on the
+      same selector.
+    * ``selection_type == "by_id"`` combined with the compact ``publisher_domains``
+      array — property_ids are publisher-scoped and the compact fan-out form does
+      not carry per-domain id partitioning, so this combination has no defined
+      meaning.
+    """
+    has_scalar = "publisher_domain" in selector and selector.get("publisher_domain")
+    has_array = "publisher_domains" in selector and selector.get("publisher_domains")
+    if has_scalar and has_array:
+        raise AdagentsValidationError(
+            "publisher_properties selector may not set both 'publisher_domain' "
+            "and 'publisher_domains'"
+        )
+    if selector.get("selection_type") == "by_id" and has_array:
+        raise AdagentsValidationError(
+            "publisher_properties selector with selection_type='by_id' may not "
+            "use the compact 'publisher_domains' form"
+        )
+
+
 def _selector_domains(selector: dict[str, Any]) -> list[str]:
     """Extract publisher domain(s) from a publisher_properties selector.
 
@@ -1021,7 +1066,9 @@ def _resolve_inline(
     filter — this is a real empty set; do NOT fall back.
 
     Handles ``selection_type`` values: ``"all"``, ``"by_tag"``, ``"by_id"``.
-    Unknown types are treated permissively (return all domain candidates).
+    Unknown ``selection_type`` values fail closed (return ``[]``) — this is an
+    authorization-decision path; permissive fallbacks would silently grant
+    access on schema drift or typos.
     """
     candidates = domain_index.get(domain)
     if not candidates:
@@ -1033,16 +1080,21 @@ def _resolve_inline(
     if selection_type == "by_tag":
         required_tags = {t for t in selector.get("property_tags", []) if isinstance(t, str)}
         if not required_tags:
-            return list(candidates)
+            # No tags supplied → no authorization. Fail-closed; the spec's
+            # `minItems: 1` should reject this upstream, but we don't rely on it.
+            return []
         return [
-            p for p in candidates
+            p
+            for p in candidates
             if required_tags & {t for t in p.get("tags", []) if isinstance(t, str)}
         ]
     if selection_type == "by_id":
         required_ids = {i for i in selector.get("property_ids", []) if isinstance(i, str)}
         return [p for p in candidates if p.get("property_id") in required_ids]
-    # Unknown selection_type — permissive fallback
-    return list(candidates)
+    # Unknown selection_type → fail closed. This is an authorization-decision
+    # path; per CLAUDE.md "no fallbacks", an unrecognized selection type must
+    # grant nothing.
+    return []
 
 
 def get_all_properties(adagents_data: dict[str, Any]) -> list[dict[str, Any]]:
diff --git a/tests/test_adagents.py b/tests/test_adagents.py
@@ -1478,32 +1478,171 @@ def test_get_properties_by_agent_publisher_properties_by_id(self):
         properties = get_properties_by_agent(adagents_data, "https://agent1.example.com")
         assert {p["property_id"] for p in properties} == {"ctv-001"}
 
+    def test_selector_with_both_domain_forms_raises(self):
+        """Selector setting both publisher_domain and publisher_domains is rejected."""
+        adagents_data = {
+            "properties": [
+                {"property_id": "a-001", "publisher_domain": "site-a.com", "name": "Site A"},
+            ],
+            "authorized_agents": [
+                {
+                    "url": "https://agent1.example.com",
+                    "authorization_type": "publisher_properties",
+                    "authorized_for": "Test",
+                    "publisher_properties": [
+                        {
+                            "publisher_domain": "site-a.com",
+                            "publisher_domains": ["site-b.com"],
+                            "selection_type": "all",
+                        },
+                    ],
+                },
+            ],
+        }
+        with pytest.raises(AdagentsValidationError, match="may not set both"):
+            get_properties_by_agent(adagents_data, "https://agent1.example.com")
+
+    def test_by_id_with_publisher_domains_raises(self):
+        """selection_type='by_id' combined with publisher_domains[] is rejected."""
+        adagents_data = {
+            "properties": [
+                {"property_id": "a-001", "publisher_domain": "site-a.com", "name": "Site A"},
+            ],
+            "authorized_agents": [
+                {
+                    "url": "https://agent1.example.com",
+                    "authorization_type": "publisher_properties",
+                    "authorized_for": "Test",
+                    "publisher_properties": [
+                        {
+                            "publisher_domains": ["site-a.com", "site-b.com"],
+                            "selection_type": "by_id",
+                            "property_ids": ["a-001"],
+                        },
+                    ],
+                },
+            ],
+        }
+        with pytest.raises(
+            AdagentsValidationError, match="selection_type='by_id'.*publisher_domains"
+        ):
+            get_properties_by_agent(adagents_data, "https://agent1.example.com")
+
+    def test_unknown_selection_type_returns_empty(self):
+        """Unknown selection_type fails closed — no authorization granted."""
+        adagents_data = {
+            "properties": [
+                {"property_id": "a-001", "publisher_domain": "site-a.com", "name": "Site A"},
+                {"property_id": "a-002", "publisher_domain": "site-a.com", "name": "Site A2"},
+            ],
+            "authorized_agents": [
+                {
+                    "url": "https://agent1.example.com",
+                    "authorization_type": "publisher_properties",
+                    "authorized_for": "Test",
+                    "publisher_properties": [
+                        {
+                            "publisher_domain": "site-a.com",
+                            "selection_type": "by_category",
+                        },
+                    ],
+                },
+            ],
+        }
+        properties = get_properties_by_agent(adagents_data, "https://agent1.example.com")
+        assert properties == []
+
+    def test_by_tag_with_empty_property_tags_returns_empty(self):
+        """selection_type='by_tag' with empty property_tags grants nothing."""
+        adagents_data = {
+            "properties": [
+                {
+                    "property_id": "a-001",
+                    "publisher_domain": "site-a.com",
+                    "name": "Site A",
+                    "tags": ["news"],
+                },
+            ],
+            "authorized_agents": [
+                {
+                    "url": "https://agent1.example.com",
+                    "authorization_type": "publisher_properties",
+                    "authorized_for": "Test",
+                    "publisher_properties": [
+                        {
+                            "publisher_domain": "site-a.com",
+                            "selection_type": "by_tag",
+                            "property_tags": [],
+                        },
+                    ],
+                },
+            ],
+        }
+        properties = get_properties_by_agent(adagents_data, "https://agent1.example.com")
+        assert properties == []
+
+    def test_property_missing_property_id_raises(self):
+        """Parent property without property_id under publisher_properties resolution
+        fails fast — every resolved property must carry an id (used for dedup).
+        """
+        adagents_data = {
+            "properties": [
+                {
+                    # NB: no property_id
+                    "publisher_domain": "site-a.com",
+                    "name": "Site A",
+                    "tags": ["news"],
+                },
+            ],
+            "authorized_agents": [
+                {
+                    "url": "https://agent1.example.com",
+                    "authorization_type": "publisher_properties",
+                    "authorized_for": "Test",
+                    "publisher_properties": [
+                        {
+                            "publisher_domain": "site-a.com",
+                            "selection_type": "all",
+                        },
+                    ],
+                },
+            ],
+        }
+        with pytest.raises(AdagentsValidationError, match="missing required 'property_id'"):
+            get_properties_by_agent(adagents_data, "https://agent1.example.com")
+
     def test_get_properties_by_agent_cafemedia_scale(self):
         """Cafemedia/interchange.io canonical fixture: 6,843 inline properties across
         6,800 child domains, all raptive_managed, one authorized agent.
 
         Sized to catch O(N×M) regressions — at this scale an unindexed
         implementation (~46 M ops) would cause a multi-second timeout.
         """
+        import time
+
         # 6,800 child publisher domains (cafemedia fan-out shape)
         child_domains = [f"site{i:04d}.raptive.com" for i in range(6800)]
         properties: list[dict] = []
         # One property per child domain
         for i, domain in enumerate(child_domains):
-            properties.append({
-                "property_id": f"p-{i:05d}",
-                "publisher_domain": domain,
-                "name": f"Site {i} — Raptive Managed",
-                "tags": ["raptive_managed"],
-            })
+            properties.append(
+                {
+                    "property_id": f"p-{i:05d}",
+                    "publisher_domain": domain,
+                    "name": f"Site {i} — Raptive Managed",
+                    "tags": ["raptive_managed"],
+                }
+            )
         # 43 extra properties on the first 43 domains (total: 6,843)
         for i in range(43):
-            properties.append({
-                "property_id": f"extra-{i:03d}",
-                "publisher_domain": child_domains[i],
-                "name": f"Site {i} Extra Property",
-                "tags": ["raptive_managed", "ctv"],
-            })
+            properties.append(
+                {
+                    "property_id": f"extra-{i:03d}",
+                    "publisher_domain": child_domains[i],
+                    "name": f"Site {i} Extra Property",
+                    "tags": ["raptive_managed", "ctv"],
+                }
+            )
 
         adagents_data = {
             "properties": properties,
@@ -1523,7 +1662,13 @@ def test_get_properties_by_agent_cafemedia_scale(self):
             ],
         }
 
+        start = time.perf_counter()
         result = get_properties_by_agent(adagents_data, "https://interchange.io")
+        elapsed = time.perf_counter() - start
+        assert elapsed < 2.0, (
+            f"resolution took {elapsed:.2f}s at cafemedia scale "
+            "(>2s indicates O(N×M) regression)"
+        )
         assert len(result) == 6843
         result_domains = {p["publisher_domain"] for p in result}
         assert result_domains == set(child_domains)  # all 6,800 must appear, not just a subset
