Merge pull request #1416 from Open-Source-Legal/claude/resolve-issue-1408-SnImO

Tighten shared protocols + protocol tests (Closes #1408)

Author: JSv4

CHANGELOG.md

@@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- **Shared-protocol contract drift surfaced by PR #1400 follow-up review** (Issue #1408):
+  - **`PermissionedQueryManagerProtocol.visible_to_user` signature mismatch** (`opencontractserver/types/protocols.py:147`): the protocol declared `visible_to_user(self, user: Any = None)` but `PermissionManager.visible_to_user` and `UserFeedbackManager.visible_to_user` have no default. A caller holding a protocol-typed reference could call `.visible_to_user()` with no args and trigger a runtime `TypeError`, and mypy would also reject the concrete classes as structural matches. Dropped the `= None` default so the protocol pins the strictest contract; the docstring now explains that callers must pass an `AnonymousUser` when no authenticated principal is available. Lenient managers (e.g. `BaseVisibilityManager` with `user=None`) still satisfy the protocol — verified with both `isinstance` and `issubclass` checks.
+  - **`ToolProtocol` `@property` descriptors against a `@dataclass`** (`opencontractserver/types/protocols.py:97`): `CoreTool` is a `@dataclass` exposing `name` / `description` / `parameters` / `requires_approval` as plain instance attributes. Mypy accepted dataclass fields against the property-shaped protocol, but the asymmetry was a footgun for future implementors who would reach for `@property` based on the protocol surface. Converted to plain attribute declarations matching the concrete class.
+  - **`StreamObserverProtocol` drift hazard** (`opencontractserver/types/protocols.py:152`): the protocol duplicated `opencontractserver.llms.types.StreamObserver` with no automated enforcement. A naive re-export creates a real circular-import risk because `protocols.py` is imported by `opencontractserver.shared.Managers` during Django app loading and `opencontractserver.llms.__init__` eagerly pulls in the heavy LLM stack (`api` → conversation models, agent factories). Kept duplicated definitions but added explicit "Must be kept in sync with…" notices on both sides (`opencontractserver/llms/types.py:18` and `opencontractserver/types/protocols.py:152`) explaining the rationale, and aligned the `__call__` return type on `llms.types.StreamObserver` (`-> None`, dropping the redundant `Awaitable[None]` wrapper that was already implicit in `async def`).
+  - **`VectorStoreProtocol` test missed renames** (`opencontractserver/tests/test_protocols.py:24`): the canonical-implementation test used `hasattr` against hard-coded method names instead of the `@runtime_checkable` machinery used by the other three tests in the file. Renaming `search` / `async_search` would have left the test passing as long as the attribute names happened to exist via some other code path. Switched to `issubclass(CoreAnnotationVectorStore, VectorStoreProtocol)`, which executes the protocol's structural check without instantiating the class. The negative test for plain `object` was tightened from a manual `hasattr` to `issubclass(object, VectorStoreProtocol)` for symmetry.
+
 ### Added
 
 - **VCR.py wrapper for LLM calls in `doc_extract_query_task`** — `opencontractserver/utils/vcr_replay.py` exposes a `maybe_vcr_cassette()` context manager that, when `OC_LLM_VCR_MODE` and `OC_LLM_VCR_CASSETTE` are set on the celery worker, records or replays every HTTP call to LLM provider hosts (currently `api.openai.com` / `api.anthropic.com`). A custom request-body matcher strips volatile values (millisecond timestamps, Django document PKs, OpenAI tool-call IDs, UUIDs) so a cassette recorded against one DB replays cleanly against another. With the env vars unset the wrapper is a no-op — production behavior is unchanged. Pre-recorded cassette for the E2E extract spec lives at `opencontractserver/tests/fixtures/cassettes/e2e_extract_pdf_workflow/extract.yaml`. Replay was verified end-to-end against a deliberately-fake `OPENAI_API_KEY` to confirm no real network call is made. See `docs/development/e2e_vcr.md` for record / replay / debug instructions.

opencontractserver/llms/types.py

@@ -1,6 +1,5 @@
 """Shared types and enums for the OpenContracts LLM framework."""
 
-from collections.abc import Awaitable
 from enum import Enum
 from typing import Any, Protocol
 
@@ -22,6 +21,12 @@ class StreamObserver(Protocol):
     Framework adapters will call this observer *whenever* they emit a
     chunk, allowing the host application (e.g. WebSocket layer) to forward
     nested or cross-agent events in real time.
+
+    .. important::
+       Must be kept in sync with
+       :class:`opencontractserver.types.protocols.StreamObserverProtocol`,
+       which mirrors this contract for non-LLM consumers that cannot
+       import the ``llms`` package without circular-import risk.
     """
 
-    async def __call__(self, event: Any) -> Awaitable[None]: ...
+    async def __call__(self, event: Any) -> None: ...

opencontractserver/tests/test_protocols.py

@@ -14,6 +14,7 @@
     PermissionedQueryManagerProtocol,
     PipelineComponentProtocol,
     ToolProtocol,
+    VectorStoreProtocol,
 )
 
 
@@ -25,14 +26,13 @@ def test_vector_store_protocol_accepts_core_vector_store(self) -> None:
             CoreAnnotationVectorStore,
         )
 
-        # Don't instantiate (would require a real corpus); class-level
-        # ``isinstance`` is enough since ``runtime_checkable`` just checks
-        # for attribute presence.  Verify the four required methods exist.
-        for attr in ("search", "async_search"):
-            self.assertTrue(
-                hasattr(CoreAnnotationVectorStore, attr),
-                f"CoreAnnotationVectorStore missing {attr}",
-            )
+        # ``issubclass`` works on ``@runtime_checkable`` protocols
+        # without instantiating the class (avoids needing a real corpus)
+        # and matches the style of the other protocol tests below.
+        # Catches regressions where a method is renamed rather than
+        # deleted, which a ``hasattr`` loop over hard-coded names would
+        # miss.
+        self.assertTrue(issubclass(CoreAnnotationVectorStore, VectorStoreProtocol))
 
     def test_pipeline_component_protocol_accepts_base(self) -> None:
         # Validate that a registered concrete component satisfies the
@@ -71,11 +71,9 @@ def test_plain_object_is_not_a_tool(self) -> None:
         self.assertNotIsInstance(object(), ToolProtocol)
 
     def test_plain_object_is_not_a_vector_store(self) -> None:
-        # CoreAnnotationVectorStore has both ``search`` and ``async_search``;
-        # a bare object has neither.
-        self.assertFalse(
-            hasattr(object(), "search") and hasattr(object(), "async_search")
-        )
+        # ``object`` has neither ``search`` nor ``async_search``; the
+        # runtime-checkable protocol must reject it.
+        self.assertFalse(issubclass(object, VectorStoreProtocol))
 
     def test_plain_object_is_not_a_permissioned_manager(self) -> None:
         self.assertNotIsInstance(object(), PermissionedQueryManagerProtocol)

opencontractserver/types/protocols.py

@@ -106,27 +106,17 @@ class ToolProtocol(Protocol):
 
     See ``docs/architecture/llms/README.md`` for the
     ``CoreTool`` → framework-specific wrapper architecture.
-    """
-
-    @property
-    def name(self) -> str:
-        """Tool name as exposed to the LLM."""
-        ...
-
-    @property
-    def description(self) -> str:
-        """Human-readable description used in tool schemas."""
-        ...
 
-    @property
-    def parameters(self) -> dict[str, Any]:
-        """JSONSchema-style parameters dict for the tool."""
-        ...
+    Members are declared as plain attributes (not ``@property``
+    descriptors) because ``CoreTool`` is a ``@dataclass`` exposing them
+    as instance attributes. Attribute declarations match the concrete
+    surface and make the contract obvious to future implementors.
+    """
 
-    @property
-    def requires_approval(self) -> bool:
-        """``True`` if invocation must be gated behind an approval step."""
-        ...
+    name: str
+    description: str
+    parameters: dict[str, Any]
+    requires_approval: bool
 
 
 # ---------------------------------------------------------------------------
@@ -144,13 +134,17 @@ class PermissionedQueryManagerProtocol(Protocol):
     "permissioned manager" (e.g. for analytics or graphene resolvers)
     should depend on this protocol, not on a concrete manager class.
 
-    The argument default is ``None`` so managers that accept anonymous
-    callers still satisfy the protocol; ``Any`` is used because the
-    concrete user type varies across managers (``AbstractUser``,
-    ``AnonymousUser``, or a ``Q`` for compound filters).
+    The ``user`` argument is required: not all concrete managers accept
+    a missing user (e.g. :class:`PermissionManager.visible_to_user` has
+    no default), so the protocol pins the strictest contract. Callers
+    should pass an :class:`~django.contrib.auth.models.AnonymousUser`
+    when no authenticated principal is available. ``Any`` is used
+    because the concrete user type varies across managers
+    (``AbstractUser``, ``AnonymousUser``, or a ``Q`` for compound
+    filters).
     """
 
-    def visible_to_user(self, user: Any = None) -> QuerySet[Any]:
+    def visible_to_user(self, user: Any) -> QuerySet[Any]:
         """Return a queryset filtered to objects visible to ``user``."""
         ...
 
@@ -163,6 +157,18 @@ def visible_to_user(self, user: Any = None) -> QuerySet[Any]:
 class StreamObserverProtocol(Protocol):
     """Callable invoked by framework adapters as streaming events emit.
 
+    .. important::
+       Must be kept in sync with
+       :class:`opencontractserver.llms.types.StreamObserver`. The two
+       definitions are duplicated (rather than re-exported) on purpose:
+       ``protocols.py`` is imported by
+       ``opencontractserver.shared.Managers`` during Django app loading,
+       and ``opencontractserver.llms.types`` lives in a package whose
+       ``__init__`` eagerly pulls in heavy LLM machinery
+       (``api`` → conversation models, agent factories). Re-exporting
+       would force every importer of this lightweight types module to
+       execute the LLM stack at startup and risks circular imports.
+
     Mirrors :class:`opencontractserver.llms.types.StreamObserver` so it
     can be referenced from non-LLM code (notifications, websockets)
     without importing ``llms.types``.