Merge remote-tracking branch 'origin/main' into pr-1239-clean

Resolves divergent-history merge against main. PR's prior main-merge
commit (0883061) was tree-identical to main's f3ae088 but had a
different SHA, so naive merge produced 74 spurious "both added"
conflicts. Used the equivalent main commit as the virtual merge base
to reduce that to one real content conflict in
opencontractserver/llms/vector_stores/core_vector_stores.py:
- Combined reranker imports (ours) with VectorStoreProtocol import
  (theirs) at module top.
- Kept the rerank-aware return in the empty-query-text branch (ours)
  and added the __all__ re-export from theirs at module level.

Author: JSv4

CHANGELOG.md

@@ -9,6 +9,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- **Mypy: type analyzer, shared, agents, badges, worker_uploads; introduce shared protocols** (Issue #1335): Brought the five smaller, interface-rich target packages over the ≥70% return-annotation bar called for by the issue and seeded `opencontractserver/types/protocols.py` with the four protocols requested in the scope:
+  - `VectorStoreProtocol` — minimum surface (`search` / `async_search`) implemented by `CoreAnnotationVectorStore` (`opencontractserver/llms/vector_stores/core_vector_stores.py`); imported and re-exported from that module so consumers can annotate against the protocol rather than the concrete dataclass.
+  - `PipelineComponentProtocol` — `title` / `description` / `author` / `dependencies` surface that the pipeline registry duck-types against; imported from `opencontractserver/pipeline/base/base_component.py` so any future parser/embedder/thumbnailer registered outside the inheritance hierarchy still type-checks against the same contract.
+  - `ToolProtocol` — `name` / `description` / `parameters` / `requires_approval` mirror of `CoreTool` (`opencontractserver/llms/tools/tool_factory.py`); framework adapters can accept any object satisfying it, no inheritance required.
+  - `PermissionedQueryManagerProtocol` — `visible_to_user(user) -> QuerySet` contract that `BaseVisibilityManager`, `PermissionManager`, `DocumentManager`, `AnnotationManager`, and `NoteManager` all satisfy (`opencontractserver/shared/Managers.py`); imported there so callers receiving "a permissioned manager" can type against the protocol instead of a concrete class.
+  - `StreamObserverProtocol` — duplicated `__call__(event)` shape from `opencontractserver/llms/types.StreamObserver` for callers in non-LLM modules (notifications, websockets) that need the contract without importing `llms.types`.
+  - **Coverage delta** (return-annotation coverage measured by AST walk, excluding `__init__.py`):
+    - `analyzer/`: 12.5% → **87.5%** (target ≥70%)
+    - `shared/`: 38.1% → **96.3%** (target ≥70%)
+    - `agents/`: 34.4% → **71.9%** (target ≥70%)
+    - `badges/`: 47.1% → **100%** (target ≥70%)
+    - `worker_uploads/`: 46.4% → **100%** (target ≥70%)
+  - **Files touched** (annotations only, zero behavior changes): `analyzer/{apps,checks,signals,startup,utils,admin,admin_views}.py`, `analyzer/management/commands/sync_doc_analyzers.py`, `agents/{apps,admin,memory}.py`, `badges/{apps,signals,models}.py`, `worker_uploads/{apps,auth,serializers,views,models,tasks}.py`, `shared/{utils,defaults,fields,mixins,Managers,QuerySets,decorators}.py`, plus the four protocol-consumer wirings above.
+  - **Bare-generic promotion**: every `dict` / `list` / `set` in newly-touched public signatures was widened to a parametrised form (`dict[str, Any]`, `list[Any]`, `dict[str, int]`, etc.). The `HasEmbeddingMixin` docstring example was tightened to match (`-> dict[str, Any]`).
+  - **`# type: ignore` audit**: every bare `# type: ignore` comment in the codebase now carries a specific error code. `opencontractserver/llms/tools/core_tools.py:413,416,418,420` (channels/asgiref import + `partial` kwarg-aware wrapper) tightened to `[import-not-found]` / `[call-arg]` with a one-line comment explaining why; `opencontractserver/utils/embeddings.py:171` to `[attr-defined]`; `opencontractserver/tests/test_pipeline_utils.py:395,423,431,438,446` cleaned up the duplicated `# type: ignore; type: ignore` markers and scoped them to `[import-not-found]`; `opencontractserver/tests/test_core_tool_factory.py:34,35,47` widened from bare to `[attr-defined]`. The total count went from 62 → 61, and the ratio of bare-to-scoped dropped to zero.
 - **Return-type annotations across `config/graphql/` resolvers and mutations** (Issue #1332, follow-up to #1331): The largest, least-typed subtree in the backend (459 function definitions, ~4.4% return-annotation coverage at baseline) is now at 100% return-annotation coverage. Touched files include every `*_mutations.py`, every `*_queries.py`, every `*_types.py`, plus `filters.py`, `base.py`, `base_types.py`, `security.py`, `optimized_file_resolvers.py`, `permissioning/permission_annotator/{middleware,mixins,utils}.py`, and the small utility modules. No behavioral changes — annotations only.
   - **`mutate(...)` on `graphene.Mutation` subclasses**: typed as forward references to the enclosing class (`-> "ClassName"`). Discovered and fixed the latent bug in `config/graphql/analysis_mutations.py:179` (`DeleteAnalysisMutation.mutate`) where the success path had no `return` statement; annotation is `-> "DeleteAnalysisMutation | None"` and an explicit `return None` was added to preserve the original implicit-None behavior on success.
   - **`resolve_*` methods**: typed as `-> Any` by default, refined where the GraphQL field type makes the runtime return obvious (e.g. `resolve_in_use -> bool`, `resolve_datacell_count -> int`).
@@ -23,6 +38,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **Extraction grounding follow-up** (Issue #1246, follow-up to original #1245 grounding pipeline):
+  - **Bug — silent `page=1` fallback corrupted multi-page PDF grounding** (`opencontractserver/utils/extraction_grounding.py`, `_create_pdf_annotation`): when PlasmaPDF could not determine a page for a span, the previous code logged a warning and saved the annotation on page 1 anyway. For multi-page PDFs this produced a structurally incorrect annotation pinned to the wrong page (and therefore the wrong bounding box context), so users clicking through to the source landed on a different page than the one containing the extracted text. Fixed: `_create_pdf_annotation` now raises `ValueError` inside its `transaction.atomic()` savepoint, the savepoint rolls back, and the outer per-result `try/except` in `_create_grounding_annotations` logs it as a failed grounding attempt. Best-effort grounding is preserved (other annotations in the batch are unaffected) but no annotation is ever saved with a wrong page.
+  - **Bug — label-set lookup outside the per-annotation guard caused all-or-nothing failure** (`opencontractserver/utils/extraction_grounding.py`, `_create_grounding_annotations`): `corpus.ensure_label_and_labelset(...)` was invoked once before the per-annotation `try/with transaction.atomic()` loop. A failure to materialise the label-set (e.g. a transient DB error or a pre-existing constraint conflict) propagated out, was caught by the outer `try/except` in `data_extract_tasks.py`, and silently dropped *all* groundings for the datacell. Moved the call inside the savepoint so a label-lookup failure only skips the affected annotation.
+  - **Bug — duplicate `OC_EXTRACT_SOURCE` annotations on Celery retry** (`opencontractserver/utils/extraction_grounding.py`, `_create_pdf_annotation` & `_create_span_annotation`): nothing prevented the grounding pipeline from creating fresh annotations and re-linking them via `datacell.sources.add(*annotations)` if `ground_extraction_to_annotations` ran twice on the same datacell (Celery retry after partial failure was the realistic trigger). Replaced the construct-then-`save()` flow with `Annotation.objects.get_or_create()` keyed on `(document, annotation_label, annotation_type, raw_text, …)` so retries reuse existing rows. `datacell.sources` is a `ManyToManyField`, so re-linking the same row is already a no-op once the row is shared.
+  - **Constant — extracted `DOCX_MIME_TYPE`** (`opencontractserver/constants/document_processing.py`): the long `application/vnd.openxmlformats-officedocument.wordprocessingml.document` literal previously lived inline in `_load_document_text_and_layer`. Per the project's no-magic-strings rule it now sits next to `MARKDOWN_MIME_TYPE` and is imported from one place.
+  - **Type annotations** (`opencontractserver/utils/extraction_grounding.py`): `Document`, `Corpus`, `Datacell`, `Annotation`, and `AnnotationLabel` parameters and return types added via a `TYPE_CHECKING` block on every public and helper function. No runtime change.
+  - **Documentation** — the `page=1` placeholder for SPAN_LABEL annotations (text/DOCX) is now documented in the function docstring, explaining that the `txt_extract_file` pipeline does not preserve a page-break map and the actual location lives in the character offsets in `json`.
+  - **Tests** — `opencontractserver/tests/test_extraction_grounding.py`:
+    - `TestGroundingPipelinePDFIntegration` (new class): builds a synthetic two-page PAWLS payload (no real PDF binary needed), runs grounding through `build_translation_layer`, and verifies (a) annotations land on the correct page, (b) re-running grounding is idempotent, and (c) when PlasmaPDF returns `page=None` the annotation is **skipped** instead of being saved on page 1.
+    - `test_ground_text_document_is_idempotent`: regression for the duplicate-annotation bug on the SPAN_LABEL path.
+
 - **`CreateCorpusActionModal` opened with the wrong default agent instructions for document triggers** (Issue #1385, `frontend/src/components/corpuses/CreateCorpusActionModal.tsx:136-144,168-171`): the `inlineAgentInstructions` state was initialised with `DEFAULT_MODERATOR_INSTRUCTIONS` even though the default trigger is `add_document` (a document trigger). The trigger-change handler at line 611 swaps to `DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS`, but a user who created an inline agent on the default-selected trigger without first re-selecting the trigger would submit the moderator copy as the new agent's system instructions. Initialised both the `useState` default and `resetForm()` to `DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS` so the pre-interaction value matches the default trigger. Updated `frontend/tests/CreateCorpusActionModal.ct.tsx` "inline-agent create: full happy path" mutation mock to expect `DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS` — the previous mock variable masked this bug because `MockedProvider` was matching the stale moderator default rather than the trigger-appropriate one.
 
 ### Changed

opencontractserver/agents/admin.py

@@ -1,4 +1,6 @@
 from django.contrib import admin
+from django.db.models import QuerySet
+from django.http import HttpRequest
 from django.utils.html import format_html
 from guardian.admin import GuardedModelAdmin
 
@@ -146,7 +148,7 @@ class AgentActionResultAdmin(GuardedModelAdmin):
         ),
     )
 
-    def status_badge(self, obj):
+    def status_badge(self, obj: AgentActionResult) -> str:
         """Display status as a colored badge."""
         colors = {
             "pending": "#6c757d",  # gray
@@ -165,7 +167,7 @@ def status_badge(self, obj):
     status_badge.short_description = "Status"
     status_badge.admin_order_field = "status"
 
-    def corpus_action_link(self, obj):
+    def corpus_action_link(self, obj: AgentActionResult) -> str:
         """Link to the corpus action in admin."""
         if obj.corpus_action:
             name = (
@@ -182,7 +184,7 @@ def corpus_action_link(self, obj):
 
     corpus_action_link.short_description = "Corpus Action"
 
-    def document_link(self, obj):
+    def document_link(self, obj: AgentActionResult) -> str:
         """Link to the document in admin."""
         if obj.document:
             title = (
@@ -199,7 +201,7 @@ def document_link(self, obj):
 
     document_link.short_description = "Document"
 
-    def tools_count(self, obj):
+    def tools_count(self, obj: AgentActionResult) -> str:
         """Display count of tools executed."""
         if obj.tools_executed:
             count = len(obj.tools_executed)
@@ -212,7 +214,7 @@ def tools_count(self, obj):
 
     tools_count.short_description = "Tools"
 
-    def duration_display(self, obj):
+    def duration_display(self, obj: AgentActionResult) -> str:
         """Display execution duration."""
         duration = obj.duration_seconds
         if duration is not None:
@@ -228,7 +230,7 @@ def duration_display(self, obj):
 
     duration_display.short_description = "Duration"
 
-    def agent_response_display(self, obj):
+    def agent_response_display(self, obj: AgentActionResult) -> str:
         """Display agent response with formatting."""
         if obj.agent_response:
             # Truncate very long responses in admin
@@ -240,7 +242,7 @@ def agent_response_display(self, obj):
 
     agent_response_display.short_description = "Agent Response"
 
-    def tools_executed_display(self, obj):
+    def tools_executed_display(self, obj: AgentActionResult) -> str:
         """Display tools executed as formatted JSON."""
         if obj.tools_executed:
             import json
@@ -254,7 +256,7 @@ def tools_executed_display(self, obj):
 
     tools_executed_display.short_description = "Tools Executed"
 
-    def execution_metadata_display(self, obj):
+    def execution_metadata_display(self, obj: AgentActionResult) -> str:
         """Display execution metadata as formatted JSON."""
         if obj.execution_metadata:
             import json
@@ -265,7 +267,7 @@ def execution_metadata_display(self, obj):
 
     execution_metadata_display.short_description = "Execution Metadata"
 
-    def get_queryset(self, request):
+    def get_queryset(self, request: HttpRequest) -> QuerySet[AgentActionResult]:
         """Optimize queryset with select_related."""
         return (
             super()

opencontractserver/agents/apps.py

@@ -3,6 +3,6 @@
 
 
 class AgentsConfig(AppConfig):
-    default_auto_field = "django.db.models.BigAutoField"
-    name = "opencontractserver.agents"
+    default_auto_field: str = "django.db.models.BigAutoField"
+    name: str = "opencontractserver.agents"
     verbose_name = _("Agents")

opencontractserver/agents/memory.py

@@ -11,7 +11,7 @@
 import logging
 import re
 from datetime import datetime, timezone
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Any
 
 from channels.db import database_sync_to_async
 from django.core.files.base import ContentFile
@@ -74,7 +74,7 @@ def _build_empty_memory(corpus_id: int) -> str:
 # ---------------------------------------------------------------------------
 
 
-async def get_or_create_memory_document(corpus: Corpus, user) -> Document:
+async def get_or_create_memory_document(corpus: Corpus, user: Any) -> Document:
     """Get the existing memory document or create a new empty one.
 
     If the corpus already has a valid ``memory_document``, return it.
@@ -95,7 +95,7 @@ async def get_or_create_memory_document(corpus: Corpus, user) -> Document:
 
     from opencontractserver.corpuses.models import Corpus as CorpusModel
 
-    def _get_or_create_sync():
+    def _get_or_create_sync() -> Document:
         # Phase 1: Check under lock whether a memory document already exists.
         with transaction.atomic():
             locked = CorpusModel.objects.select_for_update().get(pk=corpus.pk)
@@ -164,7 +164,7 @@ async def read_memory_content(corpus: Corpus) -> str:
     if not corpus.memory_document_id:
         return ""
 
-    def _read():
+    def _read() -> str:
         doc = corpus.memory_document
         if doc is None or not doc.txt_extract_file:
             return ""
@@ -187,7 +187,9 @@ def _read():
     return await database_sync_to_async(_read)()
 
 
-async def update_memory_content(corpus: Corpus, new_content: str, user) -> Document:
+async def update_memory_content(
+    corpus: Corpus, new_content: str, user: Any
+) -> Document:
     """Update the memory document with new content.
 
     Creates the memory document if it doesn't exist.  Overwrites the
@@ -215,7 +217,7 @@ async def update_memory_content(corpus: Corpus, new_content: str, user) -> Docum
 
     doc = await get_or_create_memory_document(corpus, user)
 
-    def _update():
+    def _update() -> Document:
         from django.db import transaction
 
         with transaction.atomic():
@@ -431,7 +433,7 @@ def merge_curation_into_memory(
     current_content: str,
     collection_patterns: list[str],
     query_patterns: list[str],
-    refinements: list[dict],
+    refinements: list[dict[str, Any]],
 ) -> str:
     """Merge curation results into the existing memory document.
 

opencontractserver/analyzer/admin.py

@@ -1,5 +1,5 @@
 from django.contrib import admin
-from django.urls import path
+from django.urls import URLPattern, path
 from guardian.admin import GuardedModelAdmin
 
 from opencontractserver.analyzer.admin_views import AnalyzerSyncView
@@ -16,7 +16,7 @@ class AnalyzerAdmin(GuardedModelAdmin):
     list_display = ["id", "description", "task_name", "host_gremlin"]
     change_list_template = "admin/analyzer/analyzer_changelist.html"
 
-    def get_urls(self):
+    def get_urls(self) -> list[URLPattern]:
         urls = super().get_urls()
         custom_urls = [
             path(

opencontractserver/analyzer/admin_views.py

@@ -1,5 +1,8 @@
+from typing import Any
+
 from django.contrib import messages
 from django.contrib.admin.views.decorators import staff_member_required
+from django.http import HttpRequest, HttpResponse
 from django.shortcuts import redirect, render
 from django.urls import reverse
 from django.utils.decorators import method_decorator
@@ -17,11 +20,11 @@
 class AnalyzerSyncView(View):
     """Custom admin view for syncing doc analyzer tasks"""
 
-    template_name = "admin/analyzer/analyzer_sync.html"
+    template_name: str = "admin/analyzer/analyzer_sync.html"
 
-    def get_available_analyzers(self):
+    def get_available_analyzers(self) -> list[dict[str, Any]]:
         """Get info about all available doc analyzer tasks"""
-        analyzers = []
+        analyzers: list[dict[str, Any]] = []
 
         for task_name in celery_app.tasks.keys():
             analyzer_task = get_doc_analyzer_task_by_name(task_name)
@@ -48,8 +51,8 @@ def get_available_analyzers(self):
 
         return sorted(analyzers, key=lambda x: (x["exists"], x["task_name"]))
 
-    def get(self, request):
-        context = {
+    def get(self, request: HttpRequest) -> HttpResponse:
+        context: dict[str, Any] = {
             "title": "Sync Doc Analyzer Tasks",
             "analyzers": self.get_available_analyzers(),
             "opts": Analyzer._meta,
@@ -59,7 +62,7 @@ def get(self, request):
         }
         return render(request, self.template_name, context)
 
-    def post(self, request):
+    def post(self, request: HttpRequest) -> HttpResponse:
         if not request.user.has_perm("analyzer.add_analyzer"):
             messages.error(request, "You don't have permission to create analyzers.")
             return redirect(reverse("admin:analyzer_sync"))

opencontractserver/analyzer/apps.py

@@ -3,10 +3,10 @@
 
 
 class AnnotationsConfig(AppConfig):
-    default_auto_field = "django.db.models.BigAutoField"
-    name = "opencontractserver.analyzer"
+    default_auto_field: str = "django.db.models.BigAutoField"
+    name: str = "opencontractserver.analyzer"
 
-    def ready(self):
+    def ready(self) -> None:
         try:
             import opencontractserver.analyzer.signals  # noqa F401
             from opencontractserver.analyzer.models import GremlinEngine

opencontractserver/analyzer/checks.py

@@ -1,13 +1,15 @@
+from typing import Any
+
 from django.core.checks import Warning, register
 
 
 @register()
-def check_unsynced_analyzers(app_configs, **kwargs):
+def check_unsynced_analyzers(app_configs: Any, **kwargs: Any) -> list[Warning]:
     """
     Check if there are doc_analyzer_task decorated functions
     that haven't been synced to the database.
     """
-    warnings = []
+    warnings: list[Warning] = []
 
     try:
         from opencontractserver.analyzer.models import Analyzer

opencontractserver/analyzer/management/commands/sync_doc_analyzers.py

@@ -1,5 +1,7 @@
+from typing import Any
+
 from django.contrib.auth import get_user_model
-from django.core.management.base import BaseCommand
+from django.core.management.base import BaseCommand, CommandParser
 
 from opencontractserver.analyzer.models import Analyzer
 from opencontractserver.analyzer.utils import auto_create_doc_analyzers
@@ -8,14 +10,14 @@
 class Command(BaseCommand):
     help = "Synchronize doc_analyzer_task decorated functions with Analyzer database"
 
-    def add_arguments(self, parser):
+    def add_arguments(self, parser: CommandParser) -> None:
         parser.add_argument(
             "--dry-run",
             action="store_true",
             help="Show what would be created without making changes",
         )
 
-    def handle(self, *args, **options):
+    def handle(self, *args: Any, **options: Any) -> None:
         UserModel = get_user_model()
 
         if options["dry_run"]: