From ee41cb7bbe901a5effd54c5dae4c52b2a69d78ef Mon Sep 17 00:00:00 2001 From: witbrock Date: Wed, 19 Aug 2026 16:16:35 +0200 Subject: [PATCH] Make the virtual-concept re-entrancy rule enforceable The guardrails shipped with JVNAUTOSCI-2650 did not hold up to inspection. The rule lived in a comment inside McpToolConceptProvider.owns, the one implementation that already followed it. The Protocol's owns() had no docstring at all, the module's resolution rules omitted it, and the regression test monkeypatched that provider specifically, so a third provider was uncovered. The guard itself was silent: it answered "not virtual", which can present as a missing concept, and logged nothing. State the rule where an implementer reads it, on the Protocol method and in the module docstring. Count and log guard trips so a reintroduced cycle leaves a trace instead of quietly changing answers, and expose guard_trip_count for diagnostics. Add a provider-agnostic test. It is structural rather than dynamic: the cycle only closes when the metadata load finds concepts to run access checks on, so it depends on database state. A first, dynamic version of this test passed happily with the bug deliberately reintroduced, and still passed after being changed to clear the caches first. Tripwires on get_canonical_tool_registry, get_tool_metadata, _load_from_vontology and can_access_concept encode the rule directly, and were verified to fail against a reintroduced owns() that consults the contract registry. Co-Authored-By: Claude Opus 5 --- .../vontology/virtual_concept_providers.py | 49 ++++++++++- .../backend/test_virtual_concept_providers.py | 83 +++++++++++++++++++ 2 files changed, 130 insertions(+), 2 deletions(-) diff --git a/src/backend/vontology/virtual_concept_providers.py b/src/backend/vontology/virtual_concept_providers.py index 92a8d504..c4b2cb32 100644 --- a/src/backend/vontology/virtual_concept_providers.py +++ b/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: diff --git a/tests/backend/test_virtual_concept_providers.py b/tests/backend/test_virtual_concept_providers.py index 08fe3faf..da9dfd9d 100644 --- a/tests/backend/test_virtual_concept_providers.py +++ b/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 (