Merge remote-tracking branch 'origin/main' into claude/resolve-issue-1246-XUuxZ

Author: JSv4

CHANGELOG.md

@@ -64,6 +64,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- **Mypy graduation: typed GraphQL resolvers, mutations, and filters** (Issue #1332): Raised return-annotation coverage in `config/graphql/` from ~4.8% at the start of #1331 to **91.5%** (421/460 function defs) and removed 22 modules from the `mypy.ini` baseline allow-list.
+  - **Root-cause annotation fixes in `opencontractserver/utils/permissioning.py`**: `set_permissions_for_obj_to_user`, `user_has_permission_for_obj`, `get_users_permissions_for_obj`, and `get_permission_id_to_name_map_for_model` were previously annotated with `instance: type[django.db.models.Model]` (a class) despite every call site passing an instance — and with `user: type[User]` instead of the `User` runtime instance. These were annotation bugs (the code was correct, the annotations were inverted), which compounded: every mutation calling `set_permissions_for_obj_to_user(user, obj, ...)` was a single `[arg-type]` error each. Corrected to `instance: django.db.models.Model` / `user: UserModel` (forward-referenced via `TYPE_CHECKING` import of `opencontractserver.users.models.User`). Also added the missing `dict[int, str]` annotation on `this_model_permission_id_map` and removed the `user_instance=User` (class) default on `get_users_group_ids`, which would have exploded at runtime if any caller ever omitted the argument. Module graduated out of the baseline.
+  - **Graduated from `mypy.ini` baseline** (22 modules): `config.graphql.{action_queries, agent_mutations, badge_mutations, base_types, conversation_mutations, conversation_types, corpus_types, document_queries, filters, ingestion_source_mutations, moderation_mutations, og_metadata_queries, pipeline_queries, security, serializers, slug_queries, smart_label_mutations, social_types, user_queries, user_types, voting_mutations}` and `opencontractserver.utils.permissioning`. Each had the underlying mypy errors fixed first (root-cause in `permissioning.py` cleared the `set_permissions_for_obj_to_user` cluster across every mutation file above).
+  - **Per-file type fixes**:
+    - `config/graphql/slug_queries.py` & `config/graphql/user_types.py` & `config/graphql/social_types.py` & `config/graphql/corpus_types.py`: Reversed `.filter(...).visible_to_user(user)` → `.visible_to_user(user).filter(...)` so the custom manager method (typed on the manager) resolves before `.filter()` flattens to the base `QuerySet[Model]` that django-stubs doesn't know carries `visible_to_user`. Semantics are preserved — both orderings AND the conditions. The CLAUDE.md permissioning docs already recommend the manager-first pattern.
+    - `config/graphql/og_metadata_queries.py`: Guarded against `Extract.corpus` being `None` (the FK uses `on_delete=SET_NULL`, so `corpus` is nullable in the DB but the OG metadata resolver was treating it as non-null).
+    - `config/graphql/pipeline_queries.py`: Narrowed the `mimetype` optional before passing to `get_components_by_mimetype_cached` (which required `str`), and typed `components_data` as `dict[str, Sequence[PipelineComponentDefinition]]` so both branches (which return `list[...]` vs `tuple[...]`) type-check against the unified annotation.
+    - `config/graphql/ingestion_source_mutations.py`: Replaced `if error:` with `if pk is None:` in two call sites — the error-then-continue pattern left mypy unable to narrow `pk: str | None` through the conditional. Functionally equivalent (`error is None ⟺ pk is not None` by construction of `_parse_ingestion_source_global_id`).
+    - `config/graphql/conversation_types.py`: Fixed `base64.binascii.Error` → `binascii.Error` with an explicit `import binascii` — `base64` doesn't re-export `binascii` as an attribute, so the reference was broken under `warn_unused_ignores`.
+    - `config/graphql/filters.py`: Coerced `from_global_id(value)[1]` (returns `str`) to `int` before passing to `folder_id` lookup.
+    - `config/graphql/security.py`: Replaced `CsrfViewMiddleware(lambda req: None)` with a typed `_csrf_noop_get_response(request) -> HttpResponse` so the middleware's `get_response` contract is satisfied in-types; switched `wrapped_view.csrf_exempt = True` and `request._dont_enforce_csrf_checks = True` to `setattr(...)` to avoid typing-only attribute errors against Django stubs that don't carry these internal flags. Behaviour identical.
+    - `config/graphql/moderation_mutations.py`: Added an explicit `Union[ChatMessage, Conversation, None]` annotation on `target` where the surrounding `if/else` mixes the two types (mypy can't unify across branches without the hint).
+    - `config/graphql/action_queries.py`: Coerced `from_global_id(...)[1]` to `int` at the three call sites where it feeds `for_corpus` / `for_document` custom queryset methods (which expect `int` PKs).
+  - **Docs & baseline maintenance**: `mypy.ini` baseline section for `config.graphql` trimmed from 35 modules to 14; 63 matching error lines pruned from `docs/typing/mypy_baseline.txt` so the reference file matches the live baseline.
+  - **Known remaining bugs surfaced by mypy** (filed as separate issues per the scope rules of #1332):
+    - #1359 — `RemoveLabelsFromLabelsetMutation` calls non-existent `labelset.documents`. Silent runtime failure (swallowed by a broad `except Exception`). Blocks `config.graphql.label_mutations` graduation; one-line fix + test needed.
+    - #1360 — `DRFMutation.IOSettings` declares `model: django.db.models.Model = None` and `serializer = None`. Non-trivial refactor of the base mutation class; blocks `config.graphql.base` graduation.
 - **Coverage: raise Corpus Chat & Agent Management component tests** (Issue #1276): added 36 new Playwright CT tests across the four lowest-ROI corpus components to drive coverage toward the ≥60% target. Breakdown:
   - `frontend/tests/CorpusChat.ct.tsx` (+13 tests): `initialQuery` auto-send, tool-call timeline entries (ASYNC_THOUGHT), ASYNC_SOURCES merge, SYNC_CONTENT rendering, ASYNC_RESUME, ask_document sub-tool approval remapping, unknown-type default branch, back-to-list navigation, server-message-with-sources rendering, title-filter debounce, and additional navigation-header coverage. Extended the shared `StubSocket` in `beforeEach` with new query-triggered frame sequences.
   - `frontend/tests/CreateCorpusActionModal.ct.tsx` (+8 tests): analyzer-path validation, inline-agent validation (empty name / empty instructions), existing-agent-selection validation, successful inline-agent mutation, backend error toast, analyzer edit-mode pre-population, and legacy trigger-casing normalization fallback.
@@ -120,6 +137,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **DRFMutation/DRFDeletion `IOSettings` misconfiguration surfaces as a generic internal error** (Issue #1360, `config/graphql/base.py`): `DRFMutation.IOSettings` declared `model: django.db.models.Model = None`, `graphene_model: DjangoObjectType = None` and `serializer = None` — the type annotations were wrong (instance types, not class types, holding `None`), and there was no runtime guard. A subclass that forgot to override one of those would fail deep inside `mutate()` with an `AttributeError` (`'NoneType' object has no attribute 'objects'`) or `TypeError` (`'NoneType' object is not callable`), which the broad `except Exception` then masks as a generic "internal error" message back to the API caller. `DRFDeletion.IOSettings` had the inverse problem — it didn't declare `model` at all, even though `DRFDeletion.mutate` dereferences `cls.IOSettings.model.objects`. Discovered while working on #1332 (typing): mypy flagged four errors on `base.py:128/204/238/252` plus `base.py:93 "type[IOSettings]" has no attribute "model"`.
+  - Retyped both `IOSettings` declarations with `ClassVar[Optional[type[...]]] = None` so the annotation matches reality (holding a class, not an instance, and nullable by default). `pk_fields` narrowed from `list[str | int]` to `list[str]` — the only call sites use it as a dict key.
+  - Added `_require_io_setting(mutation_cls, name)` helper that fails fast with `NotImplementedError("<Subclass>.IOSettings.<name> must be set by the subclass.")` when a required attribute is `None` or missing. Called for `model`/`serializer`/`graphene_model` in `DRFMutation.mutate` and for `model` in `DRFDeletion.mutate`. In `DRFMutation` the error is caught by the broad `except Exception` and surfaced as "internal error" to the API, with the full descriptive message in the server log via `logger.error(traceback.format_exc())`. `DRFDeletion.mutate` has no equivalent `try/except` (matching its pre-PR behavior), so the `NotImplementedError` propagates to graphene as a GraphQL execution error — also a fail-fast outcome, just on a different surface.
+  - Tightened `DRFDeletion.mutate` to raise `ValueError` when `kwargs[lookup_field]` is missing (the `id` GraphQL argument is `required=False`), instead of passing `None` into `from_global_id` and getting an opaque `AttributeError`. Same propagation surface as the `_require_io_setting` guard above.
+  - Fixed pre-existing `to_global_id(graphene_model.__class__.__name__, obj.id)` in both `DRFMutation` create/update return paths: `graphene_model` is the `DjangoObjectType` class, so `.__class__` referenced graphene's metaclass (`SubclassWithMeta_Meta`) and the resulting global id used the wrong type name. Switched to `graphene_model.__name__` to match the GraphQL type name (e.g. `"CorpusType"`) used everywhere else in `config/graphql/`. Surfaced by the new explicit `ClassVar[Optional[type[...]]]` annotations — both call sites stayed dormant because the returned `obj_id` is rarely consumed by the frontend (which re-queries by global id from its own cache).
+  - Graduated `config.graphql.base` out of the mypy baseline: removed the `[mypy-config.graphql.base]` section from `mypy.ini` and pruned the ten corresponding lines from `docs/typing/mypy_baseline.txt`. `mypy --config-file mypy.ini` is now clean on this module.
+  - Regression tests in `opencontractserver/tests/test_security_hardening.py::TestIOSettingsRequiredFieldsGuard`: the helper raises `NotImplementedError` for each of `model`/`serializer`/`graphene_model` when missing or `None`, returns the configured value when present, and the base classes expose the `None` defaults the guard relies on.
 - **`CorpusChat` dropped `SYNC_CONTENT` messages from the visible chat** (Issue #1276, `frontend/src/components/corpuses/CorpusChat.tsx:468-505`): The `SYNC_CONTENT` WebSocket frame is a standalone, non-streaming assistant reply used for synchronous server responses. `ChatTray` (document chat) appends these directly to its `chat` state; the corpus-level chat only forwarded the content to `handleCompleteMessage`, which stores sources in `ChatSourceAtom` but never pushes a message to the visible list. As a result, any `SYNC_CONTENT` the backend sent over the corpus socket rendered nothing. Fixed by mirroring the `ChatTray` pattern — push a new complete assistant message into `chat` before persisting sources/timeline. The fallback `crypto.randomUUID()` is also now captured in a single local variable so the visible chat entry and the `ChatSourceAtom` record share the same id when the server omits `message_id`. New regression test in `frontend/tests/CorpusChat.ct.tsx` ("SYNC_CONTENT renders a complete message immediately") pins the behavior.
 - **CAML article preview crashed when inserting an extract grid embed** (`frontend/src/utils/camlComponents.ts`, `frontend/src/hooks/useCamlComponentRenderer.tsx`, `frontend/src/components/corpuses/CamlArticleEditor.tsx`, `frontend/src/components/corpuses/caml/CamlDirectiveRenderer.tsx`): the editor wrapped each newly-inserted `[component:TYPE ...]` marker in a `::: prose` fence, but `@os-legal/caml`'s parser has no `case "prose"` in `parseBlock`, so the resulting block carried `body` instead of `content`. `ProseBlock` then crashed inside `splitPullquotes(undefined)`, which unmounted the entire editor modal and made the "ArrowDown then Enter inserts the extract-grid component marker" CT test fail. Switched the fence to a project-specific `::: oc-component` block and routed it through `CamlArticle`'s `customBlocks` slot, where the marker text is handed back to the existing `[component:...]` resolver. The keyboard handler in `CamlArticleEditor` was also tightened to read the active picker index from a `useRef` mirror so back-to-back ArrowDown/Enter keystrokes don't observe a stale closure value of `-1` and bail out before insertion.
 - **PR #1177 follow-up: CAML extract embed polish** (Issue #1227):

config/graphql/action_queries.py

@@ -160,7 +160,7 @@ def resolve_corpus_action_executions(self, info, **kwargs) -> Any:
         # Filter by corpus if provided (with access check)
         corpus_id = kwargs.get("corpus_id")
         if corpus_id:
-            corpus_pk = from_global_id(corpus_id)[1]
+            corpus_pk = int(from_global_id(corpus_id)[1])
             # Defense-in-depth: verify user has access to this corpus
             if not Corpus.objects.visible_to_user(user).filter(pk=corpus_pk).exists():
                 return queryset.none()
@@ -169,7 +169,7 @@ def resolve_corpus_action_executions(self, info, **kwargs) -> Any:
         # Filter by document if provided (with access check)
         document_id = kwargs.get("document_id")
         if document_id:
-            document_pk = from_global_id(document_id)[1]
+            document_pk = int(from_global_id(document_id)[1])
             # Defense-in-depth: verify user has access to this document
             if (
                 not Document.objects.visible_to_user(user)
@@ -231,7 +231,7 @@ def resolve_corpus_action_trail_stats(self, info, corpus_id, since=None) -> Any:
         from opencontractserver.corpuses.models import Corpus, CorpusActionExecution
 
         user = info.context.user
-        corpus_pk = from_global_id(corpus_id)[1]
+        corpus_pk = int(from_global_id(corpus_id)[1])
 
         # Defense-in-depth: verify user has access to this corpus
         if not Corpus.objects.visible_to_user(user).filter(pk=corpus_pk).exists():

config/graphql/analysis_mutations.py

@@ -176,7 +176,7 @@ class Arguments:
         id = graphene.String(required=True)
 
     @login_required
-    def mutate(root, info, id) -> "DeleteAnalysisMutation | None":
+    def mutate(root, info, id) -> "DeleteAnalysisMutation":
 
         # ok = False
         # message = "Could not complete"
@@ -206,4 +206,5 @@ def mutate(root, info, id) -> "DeleteAnalysisMutation | None":
 
         # Kick off an async task to delete the analysis (as it can be very large)
         delete_analysis_and_annotations_task.si(analysis_pk=analysis_pk).apply_async()
-        return None
+
+        return DeleteAnalysisMutation(ok=True, message="SUCCESS")

config/graphql/annotation_types.py

@@ -93,14 +93,14 @@ def resolve_content_modalities(self, info) -> Any:
 
     all_source_node_in_relationship = graphene.List(lambda: RelationshipType)
 
-    def resolve_feedback_count(self, info) -> Any:
+    def resolve_feedback_count(self, info) -> int:
         # If feedback_count was annotated on the queryset, use it
         if hasattr(self, "feedback_count"):
             return self.feedback_count
         # Otherwise, count it (but this triggers N+1)
         return self.user_feedback.count()
 
-    def resolve_all_source_node_in_relationship(self, info) -> Any:
+    def resolve_all_source_node_in_relationship(self, info) -> QuerySet[Relationship]:
         return self.source_node_in_relationships.all()
 
     all_target_node_in_relationship = graphene.List(lambda: RelationshipType)
@@ -131,7 +131,7 @@ def resolve_descendants_tree(self, info) -> Any:
         """
         from django_cte import CTE, with_cte
 
-        def get_descendants(cte) -> Any:
+        def get_descendants(cte):
             base_qs = Annotation.objects.filter(parent_id=self.id).values(
                 "id", "parent_id", "raw_text"
             )
@@ -161,7 +161,7 @@ def resolve_full_tree(self, info) -> Any:
         while root.parent_id is not None:
             root = root.parent
 
-        def get_full_tree(cte) -> Any:
+        def get_full_tree(cte):
             base_qs = Annotation.objects.filter(id=root.id).values(
                 "id", "parent_id", "raw_text"
             )
@@ -197,7 +197,7 @@ def resolve_subtree(self, info) -> Any:
         ancestor_ids = [ancestor.id for ancestor in ancestors]
 
         # Get all descendants of the current node
-        def get_descendants(cte) -> Any:
+        def get_descendants(cte):
             base_qs = Annotation.objects.filter(parent_id=self.id).values(
                 "id", "parent_id", "raw_text"
             )
@@ -357,7 +357,7 @@ def resolve_descendants_tree(self, info) -> Any:
         """
         from django_cte import CTE, with_cte
 
-        def get_descendants(cte) -> Any:
+        def get_descendants(cte):
             base_qs = Note.objects.filter(parent_id=self.id).values(
                 "id", "parent_id", "content"
             )
@@ -387,7 +387,7 @@ def resolve_full_tree(self, info) -> Any:
         while root.parent_id is not None:
             root = root.parent
 
-        def get_full_tree(cte) -> Any:
+        def get_full_tree(cte):
             base_qs = Note.objects.filter(id=root.id).values(
                 "id", "parent_id", "content"
             )
@@ -421,7 +421,7 @@ def resolve_subtree(self, info) -> Any:
         ancestor_ids = [ancestor.id for ancestor in ancestors]
 
         # Get all descendants of the current node
-        def get_descendants(cte) -> Any:
+        def get_descendants(cte):
             base_qs = Note.objects.filter(parent_id=self.id).values(
                 "id", "parent_id", "content"
             )