Skip to content

Commit 84e43be

Browse files
committed
Merge remote-tracking branch 'origin/main' into claude/add-tests-issue-1276-cTfRS
# Conflicts: # CHANGELOG.md
2 parents 2724145 + be5bcfc commit 84e43be

130 files changed

Lines changed: 719 additions & 360 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

CHANGELOG.md

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,33 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
2424
- `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.
2525
- `frontend/tests/CorpusAgentManagement.ct.tsx` (+8 tests): query loading state, query error state, multi-tool badge overflow, inactive-status badge, update-mutation happy path, create-mutation backend-error toast, tool deselection, and edit-modal cancel.
2626
- `frontend/tests/CorpusDescriptionEditor.ct.tsx` (+7 tests): save failure (`ok: false`), save network-error path, reapply of snapshot-less version, twice-click collapse, Cancel Version Edit reset, fetch-md URL failure, and version-count pluralization.
27+
- **Return-type annotations across core models and import/export pipeline** (Issue #1334, follow-up to #1331): The mypy gate wired in by #1331 recorded a 7208-error baseline frozen across 357 files. This PR pays down the annotation deficit on the core domain models and the bulk import/export tasks without touching runtime behavior or adding validators. Coverage jumped from the pre-issue numbers to:
28+
- `opencontractserver/corpuses/` 61.5% → 88.4% (target ≥80%)
29+
- `opencontractserver/annotations/` 48.1% → 93.8% (target ≥80%)
30+
- `opencontractserver/documents/` 51.9% → 84.9% (target ≥80%)
31+
- `opencontractserver/extracts/` 47.1% → 94.1% (target ≥75%)
32+
- `tasks/import_tasks.py`, `tasks/import_tasks_v2.py`, `tasks/export_tasks.py`, `tasks/export_tasks_v2.py` all at 100% function-signature coverage.
33+
- **Files touched** (annotations only — zero behavior changes, zero new comments, zero renames): `annotations/{models,signals,admin}.py`, `corpuses/{models,signals,managers}.py`, `documents/{models,signals}.py`, `extracts/{models,signals}.py`, `tasks/{import_tasks,import_tasks_v2,export_tasks,export_tasks_v2}.py`.
34+
- Each file adopts `from __future__ import annotations` so forward references work without string quoting, and uses a `TYPE_CHECKING` block for the handful of cross-app imports (`AbstractBaseUser`, `Corpus`, `Document`, `Annotation`, etc.) that would otherwise be circular at runtime.
35+
- **TypedDict adoption** — the import/export task signatures now consume existing TypedDicts from `opencontractserver/types/dicts.py` directly instead of bare `dict`: `OpenContractsExportDataJsonPythonType` / `OpenContractsExportDataJsonV2Type` for the data.json payloads, `OpenContractDocExport` for per-document payloads, `OpenContractsAnnotationPythonType` / `OpenContractsRelationshipPythonType` for annotation/relationship lists, `IngestionSourceExport` / `DocumentPathExport` / `StructuralAnnotationSetExport` / `CorpusFolderExport` / `AgentConfigExport` / `DescriptionRevisionExport` / `ConversationExport` / `ChatMessageExport` / `MessageVoteExport` for V2-specific shapes, and `OpenContractsAnnotatedDocumentImportType` for single-document imports. No new TypedDicts were introduced — all were already defined and previously unused in callers.
36+
- Signal handlers (`annotations/signals.py`, `corpuses/signals.py`, `documents/signals.py`, `extracts/signals.py`) now have typed `sender` / `instance` / `created` / `**kwargs` parameters with `TYPE_CHECKING` imports of the sender model classes.
37+
- **Manager methods**: `CorpusActionExecutionManager.visible_to_user` now types the optional `user` param as `Optional[AbstractBaseUser]` (previously bare default), and `summary_by_status` / `summary_by_action` tightened from bare `dict` / `QuerySet` to `dict[str, int]` / `QuerySet[Any]`.
38+
- **Model method signatures**: `save()`/`clean()`/`delete()` overrides are now `-> None` (Django's `Model.save` returns None) or `-> tuple[int, dict[str, int]]` (Django's `Model.delete` signature); `__str__` returns `str`; `@property` accessors have explicit scalar returns; classmethod factories return `"ClassName"`; tree/versioning helpers on `CorpusFolder` type their descendant queries as `QuerySet[CorpusFolder]` / `list[CorpusFolder]` where applicable (falling back to `Any` where the CTE queryset class isn't easily importable).
39+
- **Graduation from the mypy baseline is deferred.** The `mypy.ini` `[mypy-…] ignore_errors = True` sections for these modules still contain pre-existing Django-plugin-specific errors (field descriptor `assignment` mismatches, `misc` lambdas in model field declarations, base-class variable override warnings on `embedder_path`/`creator`) that are not caused by missing annotations and therefore cannot be fixed under this issue's "annotations only, no bugfixes" constraint. The annotations landed here make those modules ready for a follow-up PR that either `# type: ignore`s or refactors the remaining Django-model type issues and then removes the baseline entries.
2740
- **Mypy type-checking wired into pre-commit and CI** (Issue #1331): The existing `[mypy]` block in `setup.cfg` and the `mypy==1.20.1` / `django-stubs==6.0.2` / `djangorestframework-stubs==3.16.9` pins in `requirements/local.txt` were never actually enforced, so the investment was drifting (48 pre-existing `# type: ignore` markers, many modules at 0% annotation coverage). This PR turns on the gate without requiring the 7208 existing errors across 357 files to be fixed first — `mypy.ini` lists each of those files under its own `[mypy-<module>] ignore_errors = True` section, so new modules added outside the baseline **are** type-checked and CI / the hook fails on their errors.
2841
- `mypy.ini` (split out of `setup.cfg` because the per-module baseline is ~1000 lines): `python_version` bumped `3.9``3.11` to match the runtime, plugins kept, `django_settings_module` pointed at the new `config/settings/mypy.py` (dummy `DATABASE_URL` default so contributors don't need env vars).
2942
- `.pre-commit-config.yaml`: new `mirrors-mypy@v1.20.1` hook with `pass_filenames: false` and fully pinned stubs + Django runtime in `additional_dependencies` (pre-commit autoupdate only bumps `rev`, so unpinned stubs would drift).
3043
- `.github/workflows/backend.yml`: `Run mypy` step added to the `linter` job; the preceding `pip install -r requirements/local.txt` step satisfies the django-stubs plugin.
3144
- `docs/typing/README.md`: how to run locally / via pre-commit / in the test container, plus the per-file graduation workflow.
3245
- `docs/typing/mypy_baseline.txt`: frozen error list (sorted for stable diffs) so follow-up issues can measure progress.
46+
- **Mypy: typed the auth, users, and notifications packages** (Issue #1333, follow-up to #1331): Brought `config/admin_auth`, `config/graphql_api_token_auth`, `opencontractserver/users`, and `opencontractserver/notifications` to full return-annotation coverage and graduated them out of the mypy baseline. These packages sit on hot paths for authentication, token verification, session handling, and notification dispatch; typing them documents invariants that used to live only in `CLAUDE.md`.
47+
- Signal handlers (`opencontractserver/users/signals.py`, `opencontractserver/notifications/signals.py`) now declare `sender: type[Model]`, `instance: Model`, `created: bool`, `**kwargs: Any`, and `-> None` so drive-by edits can't silently change the signal contract.
48+
- `opencontractserver/notifications/__init__.py` gained a module-level docstring calling out that the app deliberately diverges from `AnnotatePermissionsForReadMixin` — a single `recipient` FK is the authoritative visibility gate. The design note in `CLAUDE.md` is referenced so changes to one file flag the other.
49+
- `mypy.ini`: dropped `[mypy-config.admin_auth.*]`, `[mypy-config.graphql_api_token_auth.backends]`, and `[mypy-opencontractserver.users.models]` `ignore_errors` sections. Added a narrow `disable_error_code = django-manager-missing` on `opencontractserver.users.models` only, documented inline: `django-tree-queries`' `as_manager(with_tree_fields=True)` pattern used by `Corpus`, `CorpusFolder`, and `DocumentPath` creates manager classes at runtime that the `mypy_django_plugin` can't introspect, so reverse `_set` accessors on `User` can't resolve — a known plugin limitation, not a code error. Graduating those three models with explicit manager typing will let us delete the disable.
50+
- `docs/typing/mypy_baseline.txt`: pruned all 20 entries for the graduated modules.
51+
- Bug fixes uncovered by typing:
52+
- `config/graphql_api_token_auth/backends.py`: `ApiKeyBackend.authenticate_header` referenced `self.keyword` but the attribute was never declared. DRF calls this on 401 responses to build the `WWW-Authenticate` header, so an unauthenticated request would have hit `AttributeError` at response time. Added `keyword: str = "Token"` as a class attribute.
53+
- `opencontractserver/notifications/signals.py`: `create_moderation_notification` dereferenced `action.moderator.username` unconditionally, but the `ModerationAction.moderator` FK permits `NULL`. A moderation action created without a moderator would crash the signal handler (and therefore the originating save). Added an explicit early return with a debug log when `action.moderator is None`.
3354

3455
### Changed
3556

config/admin_auth/backends.py

Lines changed: 32 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,18 @@
77
"""
88

99
import logging
10+
from typing import TYPE_CHECKING, Any, Optional
1011

1112
from django.conf import settings
1213
from django.contrib.auth import get_user_model
1314
from django.contrib.auth.backends import ModelBackend
15+
from django.http import HttpRequest
16+
17+
if TYPE_CHECKING:
18+
from opencontractserver.users.models import User
1419

1520
logger = logging.getLogger(__name__)
16-
User = get_user_model()
21+
UserModel = get_user_model()
1722

1823

1924
class Auth0AdminBackend(ModelBackend):
@@ -27,32 +32,47 @@ class Auth0AdminBackend(ModelBackend):
2732
the frontend Auth0 login flow, then back to admin.
2833
"""
2934

30-
def authenticate(self, request, auth0_user_id=None, **kwargs):
35+
def authenticate(
36+
self,
37+
request: Optional[HttpRequest],
38+
username: Optional[str] = None,
39+
password: Optional[str] = None,
40+
**kwargs: Any,
41+
) -> Optional["User"]:
3142
"""
3243
Authenticate a user by their Auth0 user ID (sub claim).
3344
3445
This is called after the user has authenticated via Auth0 on the
35-
frontend and the token has been validated.
46+
frontend and the token has been validated. ``username`` and
47+
``password`` are declared to match :class:`ModelBackend`'s
48+
signature (Django calls every backend with those kwargs) but are
49+
ignored here — credential-based login falls through to the
50+
default backend. The Auth0-specific ``auth0_user_id`` is passed
51+
via ``**kwargs``.
3652
3753
Args:
3854
request: The HTTP request object.
39-
auth0_user_id: The Auth0 user ID (sub claim from JWT).
40-
**kwargs: Additional keyword arguments (ignored).
55+
username: Accepted for :class:`ModelBackend` compatibility; ignored.
56+
password: Accepted for :class:`ModelBackend` compatibility; ignored.
57+
**kwargs: Additional credentials. The ``auth0_user_id`` key
58+
(Auth0 JWT ``sub`` claim) drives the lookup; other keys
59+
are ignored.
4160
4261
Returns:
43-
User: The authenticated user if valid and is_staff, None otherwise.
62+
The authenticated user if valid and ``is_staff``, otherwise ``None``.
4463
"""
4564
if not getattr(settings, "USE_AUTH0", False):
4665
return None
4766

67+
auth0_user_id: Optional[str] = kwargs.get("auth0_user_id")
4868
if not auth0_user_id:
4969
return None
5070

5171
try:
52-
user = User.objects.get(username=auth0_user_id)
72+
user = UserModel.objects.get(username=auth0_user_id)
5373
if user.is_active and user.is_staff:
5474
logger.info(
55-
"Auth0 admin authentication successful for user ID %s", user.id
75+
"Auth0 admin authentication successful for user ID %s", user.pk
5676
)
5777
return user
5878
else:
@@ -63,13 +83,13 @@ def authenticate(self, request, auth0_user_id=None, **kwargs):
6383
user.is_staff,
6484
)
6585
return None
66-
except User.DoesNotExist:
86+
except UserModel.DoesNotExist:
6787
logger.warning("Auth0 admin auth failed: user %s not found", auth0_user_id)
6888
return None
6989

70-
def get_user(self, user_id):
90+
def get_user(self, user_id: int) -> Optional["User"]:
7191
"""Retrieve user by primary key."""
7292
try:
73-
return User.objects.get(pk=user_id)
74-
except User.DoesNotExist:
93+
return UserModel.objects.get(pk=user_id)
94+
except UserModel.DoesNotExist:
7595
return None

config/admin_auth/views.py

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,22 @@
77
"""
88

99
import logging
10+
from typing import TYPE_CHECKING, Optional
1011
from urllib.parse import urlencode
1112

1213
from django.conf import settings
1314
from django.contrib import admin, messages
1415
from django.contrib.auth import authenticate, login, logout
15-
from django.http import HttpResponse, HttpResponseNotAllowed, JsonResponse
16+
from django.http import (
17+
HttpRequest,
18+
HttpResponse,
19+
HttpResponseBase,
20+
HttpResponseNotAllowed,
21+
JsonResponse,
22+
)
23+
24+
if TYPE_CHECKING:
25+
from opencontractserver.users.models import User
1626
from django.shortcuts import redirect, render
1727
from django.urls import reverse
1828
from django.utils.decorators import method_decorator
@@ -35,17 +45,17 @@
3545
ADMIN_LOGIN_PAGE_RATE = RateLimits.ADMIN_LOGIN_PAGE
3646

3747

38-
def _get_login_url():
48+
def _get_login_url() -> str:
3949
"""Get the admin login URL using reverse() for proper URL resolution."""
4050
return reverse("admin_auth0_login")
4151

4252

43-
def _get_admin_index_url():
53+
def _get_admin_index_url() -> str:
4454
"""Get the admin index URL using reverse() for proper URL resolution."""
4555
return reverse("admin:index")
4656

4757

48-
def _get_next_url_from_request(request):
58+
def _get_next_url_from_request(request: HttpRequest) -> Optional[str]:
4959
"""
5060
Extract the 'next' parameter from request, checking POST first then GET.
5161
@@ -60,7 +70,11 @@ def _get_next_url_from_request(request):
6070
return request.POST.get("next") or request.GET.get("next")
6171

6272

63-
def _get_safe_redirect_url(request, url=None, default=None):
73+
def _get_safe_redirect_url(
74+
request: HttpRequest,
75+
url: Optional[str] = None,
76+
default: Optional[str] = None,
77+
) -> str:
6478
"""
6579
Validate and return a safe redirect URL.
6680
@@ -103,7 +117,7 @@ def _get_safe_redirect_url(request, url=None, default=None):
103117
return default
104118

105119

106-
def _get_safe_logout_return_url(request):
120+
def _get_safe_logout_return_url(request: HttpRequest) -> str:
107121
"""
108122
Get a safe return URL for Auth0 logout.
109123
@@ -163,7 +177,7 @@ class Auth0AdminLoginView(View):
163177

164178
@method_decorator(csrf_protect)
165179
@method_decorator(view_ratelimit(rate=ADMIN_LOGIN_PAGE_RATE, block=False))
166-
def get(self, request):
180+
def get(self, request: HttpRequest) -> HttpResponseBase:
167181
"""Display the appropriate login form."""
168182
if getattr(request, "limited", False):
169183
logger.warning("Rate limit exceeded for admin login page GET")
@@ -203,7 +217,7 @@ def get(self, request):
203217

204218
@method_decorator(csrf_protect)
205219
@method_decorator(view_ratelimit(rate=ADMIN_LOGIN_RATE, block=False))
206-
def post(self, request):
220+
def post(self, request: HttpRequest) -> HttpResponseBase:
207221
"""Handle token-based login via POST or password authentication."""
208222
if getattr(request, "limited", False):
209223
logger.warning("Rate limit exceeded for admin login POST")
@@ -236,7 +250,7 @@ def post(self, request):
236250
return redirect(_get_login_url())
237251

238252
@staticmethod
239-
def _get_csp_nonce(request):
253+
def _get_csp_nonce(request: HttpRequest) -> str:
240254
"""Return the CSP nonce from the request, warning if it is missing.
241255
242256
An empty nonce would produce an invalid CSP directive that silently
@@ -252,12 +266,14 @@ def _get_csp_nonce(request):
252266
return ""
253267
return nonce
254268

255-
def _authenticate_with_token(self, request, token):
269+
def _authenticate_with_token(
270+
self, request: HttpRequest, token: str
271+
) -> HttpResponseBase:
256272
"""Authenticate user with Auth0 JWT token."""
257273
from config.jwt_utils import get_user_from_jwt_token
258274

259275
try:
260-
user = get_user_from_jwt_token(token)
276+
user: Optional["User"] = get_user_from_jwt_token(token)
261277

262278
if user and user.is_active:
263279
# Sync admin claims from token (only during admin login, not API requests)
@@ -282,12 +298,12 @@ def _authenticate_with_token(self, request, token):
282298
)
283299
# Validate redirect URL to prevent open redirect attacks
284300
next_url = _get_safe_redirect_url(request)
285-
logger.info("Admin login successful for user ID %s", user.id)
301+
logger.info("Admin login successful for user ID %s", user.pk)
286302
return redirect(next_url)
287303
else:
288304
logger.warning(
289305
"User ID %s denied admin access",
290-
user.id if user else "unknown",
306+
user.pk if user else "unknown",
291307
)
292308
messages.error(
293309
request, "You do not have permission to access the admin."
@@ -299,7 +315,7 @@ def _authenticate_with_token(self, request, token):
299315
messages.error(request, "Authentication failed. Please try again.")
300316
return redirect(_get_login_url())
301317

302-
def _sync_admin_claims(self, user, token):
318+
def _sync_admin_claims(self, user: "User", token: str) -> bool:
303319
"""
304320
Sync admin claims from Auth0 token to user model.
305321
@@ -324,7 +340,7 @@ def _sync_admin_claims(self, user, token):
324340
return sync_admin_claims_from_payload(user, payload)
325341
except Exception as e:
326342
# Log but don't fail authentication - claim sync is secondary
327-
logger.warning("Failed to sync admin claims for user ID %s: %s", user.id, e)
343+
logger.warning("Failed to sync admin claims for user ID %s: %s", user.pk, e)
328344
return False
329345

330346

@@ -337,7 +353,7 @@ class Auth0AdminLogoutView(View):
337353
"""
338354

339355
@method_decorator(csrf_protect)
340-
def post(self, request):
356+
def post(self, request: HttpRequest) -> HttpResponseBase:
341357
"""Log out the user and redirect appropriately."""
342358
logout(request)
343359

@@ -355,7 +371,7 @@ def post(self, request):
355371

356372
return redirect(_get_admin_index_url())
357373

358-
def get(self, request):
374+
def get(self, request: HttpRequest) -> HttpResponse:
359375
"""
360376
Reject GET requests for logout.
361377

0 commit comments

Comments
 (0)