Skip to content

Make the virtual-concept re-entrancy rule enforceable - #403

Merged
witbrock merged 1 commit into
mainfrom
fix/virtual-concept-reentrancy-guardrails
Aug 19, 2026
Merged

Make the virtual-concept re-entrancy rule enforceable#403
witbrock merged 1 commit into
mainfrom
fix/virtual-concept-reentrancy-guardrails

Conversation

@witbrock

Copy link
Copy Markdown
Member

Follow-up to #402 (JVNAUTOSCI-2650), prompted by the question "where is the hazard warning, and will it actually prevent future occurrences?" — the honest answer was no.

What was wrong with the guardrails

The rule lived in a comment inside McpToolConceptProvider.owns, the one implementation that already followed it. Someone writing a third provider would more likely copy CodeConceptProvider, which is clean and says nothing.

  • VirtualConceptProvider.owns had no docstring at all — bare ....
  • The module's "Resolution rules" listed three rules; this wasn't one.
  • test_owns_never_builds_the_contract_registry monkeypatches McpToolConceptProvider._contracts specifically, so a new provider was uncovered.
  • The guard was silent. It answers "not virtual", which can present as a missing concept, and logged nothing — arguably harder to debug than the hang it replaced.

Changes

State the rule where an implementer reads it: on the Protocol method and in the module docstring, including why the guard bounds the damage without making such a provider correct.

Count and log guard trips, and expose guard_trip_count(), so a reintroduced cycle leaves a trace.

Add a provider-agnostic test covering every registered provider.

Why that test is structural, not dynamic

This is the part worth reviewing. 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.

I verified this the hard way, by reintroducing the bug and watching the new test pass:

  1. First version asserted the guard-trip count stayed zero across probes → passed with the bug reintroduced (the contract registry was already warm, so owns() calling it was harmless).
  2. Second version cleared the registry and metadata caches first → still passed (no DB rows in the unit environment, so the access check never fired).
  3. Final version puts tripwires on get_canonical_tool_registry, get_tool_metadata, _load_from_vontology and can_access_concept, and asserts no registered provider's owns() reaches any of them.

Version 3 fails against a reintroduced owns() that consults the contract registry, naming the offending provider and the exact forbidden call:

AssertionError: owns() must decide from the id shape and an in-memory set,
never by building registries or reading Vontology:
  mcp_tool_catalogue: owns() reached get_canonical_tool_registry

Providers use call-time imports, so patching the module attribute catches them wherever they import from.

29 tests passing in the suite; 64 across the touched suites.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>
@witbrock
witbrock merged commit a949f52 into main Aug 19, 2026
4 checks passed
@witbrock
witbrock deleted the fix/virtual-concept-reentrancy-guardrails branch August 19, 2026 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant