Make the virtual-concept re-entrancy rule enforceable

src/backend/vontology/virtual_concept_providers.py

@@ -19,10 +19,20 @@
 - A provider must declare ``serves_public_concepts``. Claiming a concept
   currently grants unconditional visibility in access control, so a provider
   serving scoped content must not use this seam until visibility is modelled.
+- ``owns`` must be cheap and self-contained. Access control calls it, so
+  anything it reaches that consults access control, tool metadata, or Vontology
+  closes a loop back into resolution. That cycle already shipped once: the tool
+  provider built the contract registry in ``owns``, which loaded metadata, which
+  ran an access check, which asked again. ``lru_cache`` does not guard
+  re-entrancy, so each level rebuilt from scratch and a single fetch never
+  returned. ``owning_provider`` now refuses to recurse and logs when it does,
+  but the guard only bounds the damage — it does not make such a provider
+  correct, because the suppressed call answers "not virtual".
 """
 
 from __future__ import annotations
 
+import logging
 import threading
 from typing import Any, Dict, Iterable, Iterator, List, Optional, Protocol, runtime_checkable
 
@@ -51,16 +61,38 @@ def source_id(self) -> str:
     def serves_public_concepts(self) -> bool:
         """Whether every concept served is readable by any actor."""
 
-    def owns(self, concept_id: str) -> bool: ...
+    def owns(self, concept_id: str) -> bool:
+        """Whether this provider is authoritative for the id.
+
+        Must be cheap and self-contained. Access control asks this question, so
+        anything reached from here that itself consults access control, tool
+        metadata, or Vontology closes a loop back into resolution. Decide from
+        the id's shape and an in-memory set; do not build registries, read the
+        database, or resolve contracts. ``get`` may do the expensive work,
+        because it runs only after ``owns`` has already said yes.
+        """
 
     def get(self, concept_id: str) -> Optional[Dict[str, Any]]: ...
 
     def iter_concepts(self) -> Iterable[Dict[str, Any]]: ...
 
 
+logger = logging.getLogger(__name__)
+
 _providers: List[VirtualConceptProvider] = []
 _lock = threading.RLock()
 _resolving = threading.local()
+_guard_trips = 0
+
+
+def guard_trip_count() -> int:
+    """How often resolution has refused to recurse.
+
+    Expected to stay at zero. A non-zero count means some provider's ``owns``
+    is not self-contained; see the re-entrancy rule in the module docstring.
+    """
+    with _lock:
+        return _guard_trips
 
 
 def register_provider(provider: VirtualConceptProvider) -> None:
@@ -122,8 +154,21 @@ def owning_provider(concept_id: str) -> Optional[VirtualConceptProvider]:
         return None
     # A provider may reach code that asks about virtual concepts again — access
     # control and tool metadata call into each other. Refuse to recurse rather
-    # than rebuild the world at every level.
+    # than rebuild the world at every level. This bounds the damage but does not
+    # repair it: the suppressed call answers "not virtual", which can surface as
+    # a missing concept, so it is recorded loudly rather than swallowed.
     if getattr(_resolving, "active", False):
+        global _guard_trips
+        with _lock:
+            _guard_trips += 1
+        logger.warning(
+            "[virtual_concepts] Re-entered resolution while resolving %r; "
+            "refusing to recurse and answering 'not virtual'. A provider's "
+            "owns() reached code that consults access control, tool metadata, "
+            "or Vontology. Make that owns() decide from the id shape and an "
+            "in-memory set.",
+            concept_id,
+        )
         return None
     _resolving.active = True
     try:

tests/backend/test_virtual_concept_providers.py

@@ -299,6 +299,89 @@ def _fail():
     assert provider.owns("#V#no_such_tool_tool") is False
 
 
+def test_no_registered_provider_re_enters_resolution(monkeypatch):
+    """Generic cover for the cycle that shipped once.
+
+    The specific regression tests pin McpToolConceptProvider. This one holds for
+    any provider added later.
+
+    Tested structurally rather than by reproducing the hang. The cycle only
+    closes when the metadata load finds concepts to run access checks on, so it
+    depends on database state and does not reproduce in a unit test — an
+    earlier dynamic version of this test passed happily with the bug
+    reintroduced. Tripwires on the forbidden calls encode the rule directly.
+    """
+    from src.backend.integrations.internal_mcp import tool_contract_registry
+    from src.backend.security import access_control
+    import src.backend.services.tool_metadata_service as tms
+
+    reached = []
+
+    def _tripwire(label):
+        def _raise(*args, **kwargs):
+            reached.append(label)
+            raise AssertionError(f"owns() reached {label}")
+
+        return _raise
+
+    # Providers use call-time imports, so patching the module attribute catches
+    # them wherever they import from.
+    monkeypatch.setattr(
+        tool_contract_registry,
+        "get_canonical_tool_registry",
+        _tripwire("get_canonical_tool_registry"),
+    )
+    monkeypatch.setattr(tms, "get_tool_metadata", _tripwire("get_tool_metadata"))
+    monkeypatch.setattr(
+        tms, "_load_from_vontology", _tripwire("_load_from_vontology")
+    )
+    monkeypatch.setattr(
+        access_control, "can_access_concept", _tripwire("can_access_concept")
+    )
+
+    probes = [
+        _a_code_concept_id(),
+        tool_concept_id("fetch_concept"),
+        tool_concept_id("find_subconcepts"),
+        "#V#person",
+        "#V#no_such_thing_tool",
+        "not-a-concept-id",
+    ]
+
+    vcp.registered_source_ids()  # ensure providers are bootstrapped
+    offenders = []
+    for provider in tuple(vcp._providers):
+        for concept_id in probes:
+            try:
+                provider.owns(concept_id)
+            except AssertionError as exc:
+                offenders.append(f"{provider.source_id}: {exc}")
+
+    assert not offenders, (
+        "owns() must decide from the id shape and an in-memory set, never by "
+        "building registries or reading Vontology:\n  " + "\n  ".join(offenders)
+    )
+
+
+def test_guard_is_observable_when_it_trips(monkeypatch, stub_registered):
+    """A tripped guard must be countable, not silent.
+
+    The guard answers 'not virtual', which can present as a missing concept, so
+    a reintroduced cycle has to leave a trace someone can find.
+    """
+
+    def _reentrant(concept_id):
+        vcp.is_virtual_concept_id("#V#stub_thing")
+        return False
+
+    monkeypatch.setattr(stub_registered, "owns", _reentrant)
+
+    before = vcp.guard_trip_count()
+    vcp.owning_provider("#V#stub_thing")
+
+    assert vcp.guard_trip_count() > before
+
+
 def test_cheap_tool_name_set_matches_the_full_registry():
     """The name set used by owns() must not drift from the built contracts."""
     from src.backend.integrations.internal_mcp.tool_contract_registry import (