Merge pull request #2051 from Open-Source-Legal/claude/zen-shannon-oxuzju

typing: graduate the conversation & threading test domain out of the mypy baseline

Author: JSv4

changelog.d/1738-conversation-tests-mypy.changed.md

@@ -0,0 +1,40 @@
+- **Graduated the `conversation & threading` test domain out of the mypy
+  baseline (#1738).** Removed the 7 `[mypy-opencontractserver.tests.test_*]`
+  `ignore_errors` blocks for the chat/threads feature:
+  `test_conversation_mutations_graphql`, `test_conversation_permissions`,
+  `test_conversation_query`, `test_conversation_search`,
+  `test_long_conversation_api`, `test_thread_corpus_actions`,
+  `test_threading`. Pruned the corresponding 433 lines from
+  `docs/typing/mypy_baseline.txt` (3375 → 2942) and fixed the 361 errors that
+  actually surface. The baseline had drifted: e.g. `test_conversation_search`
+  fell 77 → 23 (its historical `set_permissions_for_obj_to_user` arg-type
+  errors no longer reproduce under `django-stubs==6.0.5`), while
+  `test_conversation_permissions` grew 86 → 159 after recent edits. Fixes use
+  the established patterns — class-level annotations for
+  `setUpClass`/`setUpTestData` attributes (the recommended fix from #1479), the
+  graphene `self.client` → `self.graphene_client` rename (3 files), and
+  `assert ... is not None` narrowing of Optional ORM/embedding returns. The
+  three `setUpClass`-heavy files also swapped their module-level
+  `User = get_user_model()` alias for the concrete
+  `from opencontractserver.users.models import User` import, since mypy rejects
+  a `get_user_model()` variable as a type annotation.
+- **Declared the `_skip_signals` fixture flag on `InstanceUserCanMixin`
+  (`opencontractserver/shared/user_can_mixin.py`).** `_skip_signals` is an
+  out-of-band attribute that tests/fixtures and one production path
+  (`llms/tools/moderation_tools.py`) set on model instances so the signal
+  handlers in `*/signals.py` skip their side effects (notifications, badge
+  awards, corpus-action triggers). It was undeclared, which forced a
+  `# type: ignore[attr-defined]` in `moderation_tools.py` and produced 26
+  `attr-defined` errors in `test_thread_corpus_actions`. Declaring it once as
+  `_skip_signals: bool` on the mixin shared by `BaseOCModel` *and* the
+  `TreeNode`-rooted `Corpus` / `CorpusFolder` (a type-only bare annotation;
+  Django's model metaclass ignores it at runtime, so the presence-based
+  `hasattr`/`getattr` guards still behave identically) removes the existing
+  ignore, clears all 26 test errors, and types the flag across both model
+  lineages — future-proofing the other still-baselined tests that use it on
+  `Conversation`/`ChatMessage` (`test_badges`, `test_leaderboard`,
+  `test_notifications`) as well as on `Corpus` (`test_caml_intelligence_block`).
+  The full
+  project surface (`mypy --config-file mypy.ini opencontractserver config`)
+  stays clean under both the pre-commit pin (`mypy==2.0.0`) and CI's pin
+  (`mypy==2.1.0`).

docs/typing/mypy_baseline.txt

@@ -136,6 +136,22 @@ ignore_errors = True
 # but its body already handles MIME strings -- widened to
 # ``Optional[Union[str, FileTypeEnum]]`` (removing a now-unused
 # ``# type: ignore[arg-type]`` in ``tasks/doc_tasks.py``).
+#
+# Issue #1738 also graduated the 7-module ``conversation & threading`` domain
+# chunk (the chat/threads feature): ``test_conversation_mutations_graphql``,
+# ``test_conversation_permissions``, ``test_conversation_query``,
+# ``test_conversation_search``, ``test_long_conversation_api``,
+# ``test_thread_corpus_actions``, ``test_threading`` (433 lines pruned). Mostly
+# the ``setUpClass``/``setUpTestData`` class-attribute pattern plus the
+# ``self.client`` → ``self.graphene_client`` rename and ``assert ... is not
+# None`` narrowing; the three ``setUpClass``-heavy files also swapped their
+# ``User = get_user_model()`` alias for the concrete
+# ``from opencontractserver.users.models import User`` import (mypy rejects a
+# ``get_user_model()`` variable as a type annotation). Surfaced the
+# undeclared ``_skip_signals`` fixture flag — now declared once as
+# ``_skip_signals: bool`` on ``InstanceUserCanMixin`` (``shared/user_can_mixin.py``,
+# shared by ``BaseOCModel`` + ``Corpus``/``CorpusFolder``), removing a now-unused
+# ``# type: ignore[attr-defined]`` in ``llms/tools/moderation_tools.py``.
 
 [mypy-opencontractserver.tests.base]
 ignore_errors = True
@@ -212,18 +228,6 @@ ignore_errors = True
 [mypy-opencontractserver.tests.test_compact_pawls]
 ignore_errors = True
 
-[mypy-opencontractserver.tests.test_conversation_mutations_graphql]
-ignore_errors = True
-
-[mypy-opencontractserver.tests.test_conversation_permissions]
-ignore_errors = True
-
-[mypy-opencontractserver.tests.test_conversation_query]
-ignore_errors = True
-
-[mypy-opencontractserver.tests.test_conversation_search]
-ignore_errors = True
-
 [mypy-opencontractserver.tests.test_core_agents]
 ignore_errors = True
 
@@ -314,9 +318,6 @@ ignore_errors = True
 [mypy-opencontractserver.tests.test_llm_tools]
 ignore_errors = True
 
-[mypy-opencontractserver.tests.test_long_conversation_api]
-ignore_errors = True
-
 [mypy-opencontractserver.tests.test_mcp_extended]
 ignore_errors = True
 
@@ -479,12 +480,6 @@ ignore_errors = True
 [mypy-opencontractserver.tests.test_text_thumbnails]
 ignore_errors = True
 
-[mypy-opencontractserver.tests.test_thread_corpus_actions]
-ignore_errors = True
-
-[mypy-opencontractserver.tests.test_threading]
-ignore_errors = True
-
 [mypy-opencontractserver.tests.test_thumbnails]
 ignore_errors = True
 

mypy.ini

@@ -457,7 +457,7 @@ def add_thread_message(
         creator_id=creator_id,
         agent_configuration=agent_config,
     )
-    message._skip_signals = True  # type: ignore[attr-defined]
+    message._skip_signals = True
     message.save()
 
     logger.info(

opencontractserver/llms/tools/moderation_tools.py

@@ -153,6 +153,24 @@ class InstanceUserCanMixin:
     or fix the manager — never paper over the ``AttributeError``.
     """
 
+    # Out-of-band signal-suppression flag (NOT a DB field). The signal
+    # handlers in ``*/signals.py`` check it on the instance to skip their
+    # side effects (notifications, badge awards, corpus-action triggers)
+    # during fixture creation and a handful of internal saves (e.g.
+    # ``llms/tools/moderation_tools.py``). Declared here — on the mixin shared
+    # by ``BaseOCModel`` *and* the ``TreeNode``-rooted ``Corpus`` /
+    # ``CorpusFolder`` — so the attribute is typed across both model lineages
+    # (a bare annotation Django's metaclass ignores at runtime).
+    #
+    # The protocol is PRESENCE-based: set it to ``True`` to suppress. Most
+    # handlers use ``hasattr(instance, "_skip_signals")``, so assigning
+    # ``False`` does NOT re-enable signals — ``del instance._skip_signals`` is
+    # the only reliable re-enable. (The lone exception is
+    # ``notifications/signals.py``'s ``emit_analysis_status_notification``,
+    # which reads ``getattr(instance, "_skip_signals", False)`` and so IS
+    # value-sensitive.) Normally absent at runtime.
+    _skip_signals: bool
+
     def __getstate__(self) -> Any:
         """Strip the Tier 1 permission cache before pickling.
 

opencontractserver/shared/user_can_mixin.py

@@ -60,7 +60,7 @@ def setUp(self):
         )
 
         # Create GraphQL client
-        self.client = Client(schema)
+        self.graphene_client = Client(schema)
 
     def _execute_with_user(self, query, user, variables=None):
         """Execute a GraphQL query with a specific user context."""
@@ -72,7 +72,7 @@ def __init__(self, user):
                 self.META = {}
 
         context_value = MockRequest(user)
-        return self.client.execute(
+        return self.graphene_client.execute(
             query, variables=variables, context_value=context_value
         )
 
@@ -122,7 +122,9 @@ def test_create_thread_mutation(self):
         # Verify initial message was created
         messages = ChatMessage.objects.filter(conversation=conversation)
         self.assertEqual(messages.count(), 1)
-        self.assertEqual(messages.first().content, "This is the first message")
+        first_message = messages.first()
+        assert first_message is not None
+        self.assertEqual(first_message.content, "This is the first message")
 
     def test_create_thread_without_permission(self):
         """Test creating a thread without corpus permission."""

opencontractserver/tests/test_conversation_mutations_graphql.py

@@ -17,7 +17,6 @@
 from types import SimpleNamespace
 from unittest.mock import patch
 
-from django.contrib.auth import get_user_model
 from django.contrib.auth.models import AnonymousUser, Group
 from django.test import TestCase
 from guardian.shortcuts import assign_perm
@@ -32,15 +31,19 @@
 from opencontractserver.corpuses.models import Corpus
 from opencontractserver.documents.models import Document
 from opencontractserver.types.enums import PermissionTypes
-
-User = get_user_model()
+from opencontractserver.users.models import User
 
 
 class TestConversationBifurcatedPermissions(TestCase):
     """
     Test the bifurcated permission model for Conversation visibility.
     """
 
+    superuser: User
+    alice: User
+    bob: User
+    charlie: User
+
     @classmethod
     def setUpClass(cls):
         super().setUpClass()
@@ -479,6 +482,10 @@ class TestChatMessageInheritedPermissions(TestCase):
     Test that ChatMessage visibility inherits from Conversation visibility.
     """
 
+    alice: User
+    bob: User
+    charlie: User
+
     @classmethod
     def setUpClass(cls):
         super().setUpClass()
@@ -691,6 +698,11 @@ class TestConversationGroupGrants(TestCase):
     extracts).
     """
 
+    owner: User
+    member: User
+    outsider: User
+    group: Group
+
     @classmethod
     def setUpClass(cls):
         super().setUpClass()
@@ -898,6 +910,10 @@ class TestConversationService(TestCase):
     centralization roadmap.
     """
 
+    alice: User
+    bob: User
+    superuser: User
+
     @classmethod
     def setUpClass(cls):
         super().setUpClass()
@@ -1123,6 +1139,8 @@ class TestEdgeCases(TestCase):
     Test edge cases and boundary conditions.
     """
 
+    alice: User
+
     @classmethod
     def setUpClass(cls):
         super().setUpClass()

opencontractserver/tests/test_conversation_permissions.py

@@ -42,7 +42,7 @@ def setUp(self) -> None:
         )
 
         # Graphene client with context as self.user
-        self.client = Client(schema, context_value=TestContext(self.user))
+        self.graphene_client = Client(schema, context_value=TestContext(self.user))
 
         # Create a test corpus and document
         self.corpus = Corpus.objects.create(
@@ -167,7 +167,7 @@ def test_resolve_conversations_with_corpus_id(self):
         corpus_global_id = to_global_id("CorpusType", self.corpus.id)
         variables = {"corpusId": corpus_global_id}
 
-        response = self.client.execute(query, variables=variables)
+        response = self.graphene_client.execute(query, variables=variables)
         self.assertIsNone(
             response.get("errors"),
             f"GraphQL returned errors: {response.get('errors')}",
@@ -212,7 +212,7 @@ def test_conversations_title_search_is_case_insensitive(self):
             }
         }
         """
-        response = self.client.execute(
+        response = self.graphene_client.execute(
             query, variables={"titleContains": "conversation with corpus"}
         )
         self.assertIsNone(
@@ -258,7 +258,7 @@ def test_resolve_conversations_with_document_id(self):
         document_global_id = to_global_id("DocumentType", self.doc.id)
         variables = {"documentId": document_global_id}
 
-        response = self.client.execute(query, variables=variables)
+        response = self.graphene_client.execute(query, variables=variables)
         self.assertIsNone(
             response.get("errors"),
             f"GraphQL returned errors: {response.get('errors')}",
@@ -307,7 +307,7 @@ def test_user_cannot_see_others_conversations(self):
         }
         """
 
-        response = self.client.execute(query)
+        response = self.graphene_client.execute(query)
         self.assertIsNone(
             response.get("errors"),
             f"GraphQL returned errors: {response.get('errors')}",