typing: graduate tests.permissioning.* subpackage out of the mypy baseline

[JSv4]

Summary

Continues the mypy baseline drain tracked in #1738 (the #1331 → #1335 → #1447 cadence). After the config tail (PR #1748), this is the next cohesive batch called out in the issue's batching plan: the 18-module opencontractserver.tests.permissioning.* subpackage.

Removes all 18 [mypy-…] ignore_errors blocks from mypy.ini, prunes the corresponding 313 lines from docs/typing/mypy_baseline.txt (5592 → 5279), and fixes the 162 errors that surface.

After this PR: opencontractserver.mcp.tests.test_mcp and the opencontractserver.tests.* leaf-file modules remain on the baseline, per the issue's batching plan.

Production-code finding

One real type-correctness gap surfaced — the kind of thing #1738 explicitly invites graduation passes to look for:

• CoreAgent Protocol was missing its public config: AgentConfig attribute (opencontractserver/llms/agents/core_agents.py:383). Five tests read agent.config.tools / agent.config._approval_bypass_allowed directly off agents handed back by llm_agents.for_corpus(...) / for_document(...). The factory returns the CoreAgent Protocol; the concrete CoreAgentBase initializes self.config = config in __init__, but the Protocol never declared it. Tests worked at runtime (the attribute exists on every concrete agent) but failed type-checking. Added config: AgentConfig to the Protocol body — captures the existing public API contract without a behaviour change.

Per-file test fixes

Most errors were the standard Django setUpTestData(cls) class-attribute pattern flagged by mypy (recommended fix from #1479: annotate the assigned names as class-level attributes before the @classmethod body). The breakdown:

File	Errors	Fix pattern
test_query_optimizer_methods.py	108	Class-level annotations on AnalysisServicePermissionTestCase + DocumentPermissionFilterQueryCountTestCase; switched User = get_user_model() to direct from opencontractserver.users.models import User; replaced three qs.first().<attr> with qs[0].<attr> after the existing qs.count() == 1 guard.
test_metadata_query_optimizer.py	15	assert status is not None narrowings before indexing into the dict | None return of MetadataService.get_metadata_completion_status; one column is not None narrowing in test_validate_column_valid.
test_permissioning.py	10	Same setUpTestData annotation pattern on SetPermissionsIsNewTests; same get_user_model() → direct import switch.
test_permissioned_querysets.py	8	Narrowed qs.query.select_related (dict | bool) via assert isinstance(..., dict) before three assertIn calls; gave two PermissionQuerySet constructions explicit type annotations.
test_public_and_permissions.py	7	Removed six unused # type: ignore[attr-defined] comments on Model.objects.acount() calls (django-stubs now ships proper async-manager typing); one real fix on CoreAgent.config (covered above).
test_version_aware_query_optimizer.py	7	Replaced six result.first().id accesses with result[0].id after the pre-existing result.count() == 1 guard.
test_custom_permission_filters.py	4	Replaced two assertIsNotNone calls with assert narrowings.
test_creator_based_permissions.py	2	Narrow # type: ignore[arg-type] on two get_users_permissions_for_obj(instance=mock_instance, ...) calls (the test deliberately passes a non-Model mock to exercise the no-guardian-tables fallback).
test_comment_permission.py	1	Replaced one assertIsNotNone with assert for narrowing.
Total	162

All test behaviour is preserved — the changes are either no-ops at runtime (class-level annotations, assert vs assertIsNotNone) or behaviour-equivalent refactors (.first().id after a count() == 1 guard → [0].id).

Test plan

• mypy --config-file mypy.ini opencontractserver config → Success: no issues found in 1157 source files (the acceptance gate per Continue draining the mypy baseline established in #1331 — test suite + config tail #1738)
• black / isort / flake8 / pyupgrade clean on all changed files
• All 18 permissioning test modules still import cleanly (Django setup smoke test)
• Backend CI (backend.yml linter + test jobs) — the authoritative runtime check

Note on local testing: the backend test suite could not be exercised in the authoring environment — docker compose -f test.yml is unavailable in this sandbox. mypy was run instead via a Python env mirroring the pre-commit hook's pinned dependency set. The changes are behaviour-neutral (the test diffs preserve every existing assertion, the CoreAgent.config Protocol addition captures an attribute that's been on every concrete agent since its introduction); the backend test job is relied on for the runtime check.

Refs #1738

[claude]

Code Review — PR #1768: typing: graduate tests.permissioning.* out of the mypy baseline

Overview

Continues the incremental mypy graduation cadence (#1738 / #1331 → #1335 → #1447). Removes 18 ignore_errors = True blocks from mypy.ini, prunes 313 stale lines from docs/typing/mypy_baseline.txt, and fixes the 162 actual errors that surface when those modules are checked from scratch. One genuine production-code gap was caught in the process.

---

Production Code Change (core_agents.py)

CoreAgent Protocol — config: AgentConfig addition ✅

# opencontractserver/llms/agents/core_agents.py:383
class CoreAgent(Protocol):
    config: AgentConfig   # ← new
    ...

This is a legitimate API-contract fix. CoreAgentBase.__init__ has always set self.config = config, but the Protocol never declared it, so the five test callers (agent.config.tools, agent.config._approval_bypass_allowed) were working only by runtime duck-typing. Adding the attribute to the Protocol body is correct.

One thing worth confirming: Protocol instance-attribute declarations are satisfied by structural subtyping, so any concrete subclass that initialises self.config in __init__ (as CoreAgentBase does) is fine. No issues here.

The inline comment (# Public configuration attribute…) is appropriate — it explains the "why" without narrating what the code already shows.

---

Test-Side Fixes

assertIsNotNone → assert ... is not None ✅

Used in test_comment_permission.py, test_custom_permission_filters.py, test_metadata_query_optimizer.py. This is the correct technique to give mypy a type narrowing it can flow through. The change is runtime-equivalent (assertIsNotNone raises AssertionError with the same semantics; assert does too).

Minor observation: a handful of assertIsNotNone calls in the same files are left unchanged (where narrowing wasn't needed for subsequent attribute access). This is intentional and correct — no need to convert them all mechanically.

qs.first().<attr> → qs[0].<attr> ✅

Applied in test_query_optimizer_methods.py and test_version_aware_query_optimizer.py, always after a self.assertEqual(qs.count(), 1) guard. The change is behavior-equivalent for the happy path. One subtle difference worth noting for awareness:

• qs.first() → SELECT … ORDER BY pk LIMIT 1 (adds an ORDER BY)
• qs[0] → SELECT … LIMIT 1 OFFSET 0 (no ORDER BY)

In test fixtures with a single matching row the result is the same. If a queryset happens to have no default ordering and a future test creates two matching rows, qs[0] would be non-deterministic. This is very low risk in these controlled-fixture tests, and IndexError is actually a clearer failure signal than AttributeError: 'NoneType' object has no attribute '…'. No action needed.

setUpTestData class-level annotations ✅

Correct fix for the setUpTestData class-attribute pattern (per #1479 guidance). AnalysisServicePermissionTestCase and DocumentPermissionFilterQueryCountTestCase both get complete attribute coverage. No runtime impact.

One minor nit: in test_query_optimizer_methods.py the existing local imports inside setUpTestData:

-from opencontractserver.analyzer.models import Analysis, Analyzer
-from opencontractserver.documents.models import Document

are removed. These are redundant with the module-level imports already present (confirmed by the fact that the newly-added class-level analysis: Analysis etc. annotations would fail at import time if those names weren't already in scope). This cleanup is correct.

get_user_model() → from opencontractserver.users.models import User ✅

Established project pattern for mypy-compatibility per #1479. Acceptable in test code (single user-model project).

select_related narrowing (dict | bool) ✅

# test_permissioned_querysets.py
select_related = qs.query.select_related
assert isinstance(select_related, dict)
self.assertIn("creator", select_related)

Correct: Django sets select_related to True (fetch-all), False (off), or a dict of specific relations. The narrowing via isinstance is the right technique, and the test already expects specific field names in the dict, so asserting isinstance(select_related, dict) is a meaningful and non-vacuous guard.

PermissionQuerySet explicit type annotations ✅

qs: PermissionQuerySet = PermissionQuerySet(model=Document, using=connection.alias)

Correct fix for generic-type capture. No behavioral change.

Removed stale # type: ignore[attr-defined] on acount() ✅

-await ChatMessage.objects.all().acount()  # type: ignore[attr-defined]
+await ChatMessage.objects.all().acount()

These suppression comments are now unused because django-stubs properly types async managers. Removing them is correct; leaving stale ignores would suppress real future errors.

# type: ignore[arg-type] in test_creator_based_permissions.py ✅

Only two calls get the ignore — the two that deliberately pass MockPublicModel / MockPrivateModel instances (not real Model subclasses) to test the no-guardian-tables fallback. The inline comment could be slightly more explicit, but the PR description covers it. Acceptable.

---

Baseline Arithmetic

The PR description says 162 errors were fixed but 313 baseline lines were pruned. The difference (~151 lines) comes from baseline entries for those 18 modules that were already stale — the underlying functions (e.g. set_permissions_for_obj_to_user signature) or stubs (django-stubs async managers) had been improved in earlier PRs, so those errors simply no longer appear when mypy runs fresh on these modules. Pruning all 313 is correct: once ignore_errors = True is removed, the stale baseline entries for those modules are irrelevant.

---

Concerns / Suggestions

1. CI gate is the authoritative check. The PR correctly flags that the test suite could not be run locally. All changes are behavior-neutral by construction, but the backend CI job (backend.yml) should be green before merge. No blocker, just a reminder.

2. CHANGELOG entry is verbose. The existing entries in this section are concise bullet points. The new entry is much longer (covers per-file breakdown). Consider trimming the per-file table to a one-liner in the changelog and leaving the detail in the PR description / commit message, to keep the changelog readable.

3. test_query_optimizer_methods.py — DocumentPermissionFilterQueryCountTestCase annotations include list[Document] and dict[int, Annotation]. These are correct type annotations, but confirm that list and dict are imported (Python 3.9+ built-in generics) or that List/Dict from typing is available. Given the project targets Python 3.10+, list[...] / dict[...] are fine.

---

Summary

This is a clean, mechanical PR that follows the project's established graduation cadence. The production fix to CoreAgent is correct and properly motivated. All test-side changes are behavior-neutral and apply the right mypy-narrowing techniques consistently. No security, performance, or correctness issues found.

Verdict: Approve pending green CI.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!