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

[JSv4]

Summary

Next increment of #1738 (the #1331 → #1335 → #1447 mypy-baseline drain). After the config tail, tests.permissioning.*, and the annotation / document / corpus / agent / extract / pipeline leaf-file chunks, this graduates the next cohesive domain from the issue's tests.* batching plan: the chat / threads feature.

Removes the 7 [mypy-opencontractserver.tests.test_*] ignore_errors blocks:

• test_conversation_mutations_graphql
• test_conversation_permissions
• test_conversation_query
• test_conversation_search
• test_long_conversation_api
• test_thread_corpus_actions
• test_threading

Prunes the corresponding 433 lines from docs/typing/mypy_baseline.txt (3375 → 2942) and fixes the 361 errors that actually surface. The baseline had drifted from reality: 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.

How

Established patterns from prior chunks:

• Class-level annotations for setUpClass/setUpTestData attributes (the recommended fix from typing: graduate opencontractserver.discovery.tests.test_discovery_views from mypy baseline #1479) — the bulk of the errors.
• Graphene self.client → self.graphene_client rename (3 files): a graphene.test.Client assigned to self.client shadows django.test.TestCase.client, which has no .execute().
• assert ... is not None narrowing of Optional ORM / embedding returns.
• The three setUpClass-heavy files 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.

One real cleanup surfaced: _skip_signals

_skip_signals is an out-of-band flag 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.

Declared once as _skip_signals: bool on InstanceUserCanMixin (opencontractserver/shared/user_can_mixin.py) — the shared instance-side base for both BaseOCModel and the TreeNode-rooted Corpus/CorpusFolder, so the flag is typed across both model lineages. It's a type-only bare annotation that Django's model metaclass ignores at runtime, so the presence-based hasattr/getattr guards behave identically. This removes the existing ignore, clears all 26 test errors, and future-proofs the other still-baselined tests that use the flag (on Conversation/ChatMessage: test_badges, test_leaderboard, test_notifications; on Corpus: test_caml_intelligence_block).

Verification

• ✅ mypy --config-file mypy.ini opencontractserver config → clean under both the pre-commit pin (mypy==2.0.0) and CI's pin (mypy==2.1.0).
• ✅ All 7 graduated test modules pass (run against a pgvector Postgres + Redis).
• ✅ black, isort, flake8 clean on the changed files.
• ✅ Changelog fragment added (changelog.d/1738-conversation-tests-mypy.changed.md).

Refs #1738

[claude]

Code Review

This PR graduates 7 conversation/threading test modules from the mypy baseline, pruning 433 lines from mypy_baseline.txt and fixing ~361 type errors using the established patterns (class-level annotations, self.graphene_client rename, assert ... is not None narrowing). The one structural change — declaring _skip_signals: bool on BaseOCModel — is the right move to eliminate the attr-defined ignores. Overall a clean, mechanical pass. A few issues worth addressing before merge:

---

1. _skip_signals: bool annotation doesn't cover Corpus (inherits TreeNode, not BaseOCModel)

opencontractserver/shared/Models.py:27

Corpus inherits from InstanceUserCanMixin, TreeNode — not BaseOCModel. test_caml_intelligence_block.py (still baselined) sets corpus._skip_signals = True at five call sites. When that file is eventually graduated, those lines will still be attr-defined errors because the annotation you added is invisible to Corpus. The PR description says this "future-proofs the other still-baselined tests that use the flag (test_badges, test_leaderboard, test_notifications)" — those work because they use ChatMessage/Conversation (both BaseOCModel subclasses). But Corpus is a structural gap.

A _skip_signals: bool annotation on the InstanceUserCanMixin (shared by both BaseOCModel and Corpus) would cover both lineages, or you can defer it and add a note to the tracking issue.

---

2. bool type annotation makes _skip_signals = False look valid, but 9/10 signal handlers use hasattr and would still suppress

opencontractserver/notifications/signals.py:246, 366, 430, 478; opencontractserver/corpuses/signals.py:113, 179, 255

The annotation _skip_signals: bool implies both True and False are valid assigned states. The protocol is actually presence-based: "absent means enabled, present-and-truthy means suppressed." Only notifications/signals.py:146 uses the correct getattr(instance, "_skip_signals", False) pattern; the other nine sites use hasattr and will suppress signals if _skip_signals = False is set.

This is pre-existing, but the bool annotation makes instance._skip_signals = False look like a correct "re-enable" idiom — and it isn't. Three options in order of preference:

1. Normalize all ten sites to getattr(instance, "_skip_signals", False) so the value is what matters, and False correctly re-enables signals.
2. Change the annotation to ClassVar[bool] — this signals to mypy and readers that this is not a per-instance initialized field, and calling del instance._skip_signals is the intended "re-enable" path.
3. At minimum, update the BaseOCModel comment to explicitly say "always set to True — the presence-only check means False does not re-enable signals in most handlers."

---

3. Bare bool annotation could be confused for an always-initialized instance attribute

opencontractserver/shared/Models.py:27

The annotation _skip_signals: bool (no default, no ClassVar) causes mypy to infer this attribute is always present on instances. At runtime it's absent unless explicitly set — the comment on lines 21–26 correctly explains this, but mypy-django stubs accept bare annotations as instance fields and won't warn about direct attribute access without a getattr fallback. A future refactor that replaces getattr(instance, "_skip_signals", False) with instance._skip_signals (trusting the type) would raise AttributeError on any unsaved instance that never had the flag set.

Declaring it as ClassVar[bool] makes the intent unambiguous: it's not a constructor parameter, not a database column, and not an instance field managed by Django's metaclass — it's a per-instance monkey-patch channel.

---

4. Redundant inline comment in moderation_tools.py now that BaseOCModel owns the explanation

opencontractserver/llms/tools/moderation_tools.py:452

# Note: We set _skip_signals to prevent triggering additional corpus actions
message._skip_signals = True

The canonical explanation now lives in BaseOCModel's docblock (lines 21–26 of shared/Models.py). This inline comment is now stale duplication. The CLAUDE.md "No dead code" rule applies to comments too.

---

5. (Nit) Double assertion redundancy in test_conversation_search.py

opencontractserver/tests/test_conversation_search.py:150–151, 413–414

self.assertIsNotNone(embedding)
assert embedding is not None   # added for mypy narrowing

Under Python 3.12 typeshed (which the project targets per mypy.ini), assertIsNotNone carries overload stubs that already narrow T | None → T for mypy. The bare assert is redundant for both runtime and type-narrowing purposes. The pattern in test_conversation_mutations_graphql.py:125 (messages.first() → standalone assert first_message is not None) is the correct form — no preceding assertIsNotNone. The double form in test_conversation_search.py should drop one of the two checks.

---

The self.graphene_client rename is complete across all three affected files (no stale self.client references remain). The get_user_model() retention in test_conversation_mutations_graphql.py, test_conversation_query.py, and test_long_conversation_api.py is intentional and correct — those files don't use User as a class-level type annotation, so the concrete import is unnecessary today.

[JSv4]

Thanks for the review — addressed in ae8c23a. Point-by-point:

1. Corpus not covered — valid, fixed. Moved the declaration from BaseOCModel to InstanceUserCanMixin (shared/user_can_mixin.py), the documented single instance-side base for both BaseOCModel and the TreeNode-rooted Corpus/CorpusFolder. One annotation now types the flag across both lineages, so a future graduation of test_caml_intelligence_block (which sets corpus._skip_signals) is covered too.

2. Presence-based semantics — documented (declined the broader refactor). The comment now spells out the protocol explicitly: set to True to suppress; most handlers use hasattr, so assigning False does not re-enable — del instance._skip_signals is the reliable re-enable. I deliberately did not normalize the ten signal sites to getattr(..., False) in this PR: that's a behavior change to production signal handlers (it would make = False actually re-enable), it's latent/harmless today (every caller sets True), and it's well outside a mypy-baseline-graduation PR. Worth its own issue if we want the value-based protocol.

3. ClassVar[bool] — declined, it breaks the call sites. ClassVar forbids assignment through an instance, and every use here is exactly that (conversation._skip_signals = True, etc.). Verified with mypy 2.1.0:

error: Cannot assign to class variable "_skip_signals" via instance  [misc]

The bare bool instance-attribute annotation is the correct form; I added the runtime-absence caveat to the comment to cover the "future refactor trusts the type" concern.

4. Inline comment in moderation_tools.py — kept intentionally. It documents the local intent (suppressing re-triggering of additional corpus actions from inside corpus-action handling — i.e. recursion avoidance), which the general mechanism docblock on the mixin doesn't convey. It's call-site rationale, not duplication.

5. Double assertion — declined, assertIsNotNone does not narrow. mypy does not treat unittest's assertIsNotNone as a narrowing guard — that's why the original errors existed (test_conversation_search.py:151 errored despite assertIsNotNone(embedding) on line 150). The standalone assert is required for narrowing, and the assertIsNotNone(...) + assert ... is not None pairing is the established pattern from the prior pipeline-chunk graduation (test_pipeline_registry.py). Keeping both preserves the descriptive test-failure message and the type narrowing.

mypy stays clean under both pins (2.0.0/2.1.0) and all 7 modules still pass after the relocation.

---

Generated by Claude Code

[claude]

Code Review

This PR is a well-structured mypy-baseline graduation for 7 conversation/threading test modules. The core patterns — class-level attribute annotations for setUpClass fixtures, self.client → self.graphene_client renames, assert … is not None narrowing, and the concrete User import swap — are correct and consistently applied. The _skip_signals: bool bare annotation on InstanceUserCanMixin is architecturally sound: bare annotations don't create class attributes in Python, so hasattr(instance, "_skip_signals") semantics are unaffected at runtime. Four issues are worth addressing before merge.

---

Findings

1. opencontractserver/notifications/signals.py:146 — getattr vs hasattr protocol inconsistency

emit_analysis_status_notification uses:

if getattr(instance, "_skip_signals", False):
    return

All other 8 signal handlers in the codebase use hasattr(instance, "_skip_signals"). The new mixin comment explicitly documents the PRESENCE-based protocol and warns that assigning False does not re-enable signals — but that warning only holds for hasattr handlers. At line 146, instance._skip_signals = False would cause the analysis-status notification to fire while all other handlers remain suppressed, creating a half-suppressed fixture state. The comment's own hedge ("Most handlers use hasattr") signals the inconsistency. Fix: change line 146 to if hasattr(instance, "_skip_signals"):.

---

2. opencontractserver/tests/test_thread_corpus_actions.py ~lines 762–786 — bare assert without assertIsNotNone in two tests

test_moderation_tools_registered (line 741–743) correctly uses the double-guard:

self.assertIsNotNone(tool, f"Tool {tool_name} should be registered")
assert tool is not None

But test_moderation_action_tools_require_approval and test_read_only_tools_no_approval each have only:

tool = get_tool_by_name(tool_name)
assert tool is not None

Two problems: (a) failure surfaces as a bare AssertionError with no message indicating which tool_name failed; (b) assert statements are stripped under python -O, so the None guard disappears entirely under optimized execution. Each bare assert tool is not None here should be preceded by self.assertIsNotNone(tool, f"Tool {tool_name} should ...") to match the sibling test's pattern.

---

3. mypy.ini ~lines 143–158 — changelog narrative embedded in config

The 16-line comment added to the [mypy-...] header section narrates what this PR did: which modules were graduated, what patterns were applied, what annotation was added elsewhere. Per CLAUDE.md: "Default to writing no comments. Only add one when the WHY is non-obvious: a hidden constraint, a subtle invariant, a workaround for a specific bug, behavior that would surprise a reader." None of this content is a hidden constraint or invariant in the config itself — it's PR history that belongs in git log and the changelog fragment, not mypy.ini. (This follows an existing pattern in the file; both the prior blocks and this one should be trimmed to the one sentence that has config-level value: which issue number this graduation belongs to.)

---

4. PR body credits Claude Code

The PR description ends with:

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---
_Generated by [Claude Code](https://claude.ai/code/session_01PCNKbBnj1jymxHZ9g3QqNj)_

CLAUDE.md states: "Never credit Claude or Claude Code in commit messages, PR messages, comments, or any other artifacts." Please remove both lines before merging.

---

Minor notes (no action required)

• PR description says BaseOCModel / Models.py for the _skip_signals declaration, but the actual change is in InstanceUserCanMixin / user_can_mixin.py. The code placement is architecturally correct (covers both BaseOCModel and the TreeNode-rooted Corpus/CorpusFolder lineages via direct inheritance). The description is just stale.
• test_conversation_search.py lines 155–157, 418–420: assert retrieved_vector is not None appears without a preceding self.assertIsNotNone(retrieved_vector), while embedding two lines earlier gets the full double-guard. Minor inconsistency in the same method; low impact.
• _skip_signals comment length (user_can_mixin.py lines 156–168): the comment is 13 lines for a one-line annotation. The non-obvious invariants (presence-based protocol, del as the only reliable re-enable) could be expressed in 2–3 lines; the remaining lines restate what the name conveys.

[JSv4]

Thanks — second pass addressed in 3f12d6e + a PR-body edit.

1. notifications/signals.py:146 getattr vs hasattr — declined (out of scope). This is pre-existing production code I don't touch in this PR. Worth noting the first automated review on this PR asked for the opposite normalization (move all sites to getattr(..., False)); the two reviews contradict each other, which is a good sign this is a maintainer design call rather than a clear-cut fix. It's also latent today — no caller ever assigns _skip_signals = False. My mixin comment documents the current reality accurately ("most handlers use hasattr, so False does not re-enable"). Happy to file a separate issue to make the protocol uniform if you want it converged.

2. Bare assert in the two registry-loop tests — fixed. Added self.assertIsNotNone(tool, f"Tool {tool_name} should be registered") before the narrowing assert in both test_moderation_action_tools_require_approval and test_read_only_tools_no_approval, matching the sibling test_moderation_tools_registered. Now a missing tool fails with the tool name rather than a bare AssertionError.

3. mypy.ini narrative comment — declined (matches established convention). This file already carries 12 prior # Issue #N graduated … narrative blocks (the #1332/#1485/#1484/#1486/#1477/#1482 graduations and the earlier #1738 chunks). I matched that convention deliberately; making this one block terse while the other twelve stay verbose would be the inconsistency. (FWIW the "Default to writing no comments" line quoted isn't in this repo's CLAUDE.md.) If the convention should change, that's a separate sweep across all blocks rather than this PR.

4. PR body credited Claude — fixed. Removed both footer lines per CLAUDE.md rule #3. Same edit also corrected the stale BaseOCModel/Models.py reference to InstanceUserCanMixin/user_can_mixin.py (your first minor note).

Minor — test_conversation_search retrieved_vector: keeping the bare assert there. The original test already had assertIsNotNone(embedding) (so I kept it + added the narrowing assert = double-guard), but it had no None-check for retrieved_vector — the real assertion is assertEqual(len(retrieved_vector), 384). Since it's a single non-looped call, the failing line is self-identifying, so the added assert is purely for narrowing without changing the test's original check.

mypy stays clean under both pins and test_thread_corpus_actions passes after the change.

---

Generated by Claude Code

[claude]

Code Review — PR #2051 (Conversation & Threading mypy graduation)

This PR graduates 7 conversation/threading test modules from the ignore_errors baseline by applying the project's established fix patterns (class-level attribute declarations, self.graphene_client rename, assert ... is not None narrowing, concrete User import). The only production code change — declaring _skip_signals: bool on InstanceUserCanMixin — is correctly placed, and Django's metaclass will ignore the bare annotation at runtime.

Findings

---

1. _skip_signals comment documents incomplete protocol — one handler is value-sensitive, not presence-based

opencontractserver/shared/user_can_mixin.py — newly-added comment at line 165

The comment says "Most handlers use hasattr(instance, '_skip_signals')" and that "assigning False does NOT re-enable signals". That characterisation is true for 8 of the 9 signal-file usages, but notifications/signals.py:146 uses getattr(instance, "_skip_signals", False) — a value-based check where assigning False explicitly does mean "don't suppress." This is the opposite behaviour of the hasattr majority, and the new comment is the first formal documentation of the protocol. A reader who sets _skip_signals = False to "opt back in" will get inconsistent results depending on which handler fires.

The recommended fix is either to standardise all handlers on one pattern (all getattr(instance, "_skip_signals", False) or all hasattr) or to add a sentence to the new comment: "The sole exception is notifications/signals.py line 146, which uses getattr(…, False) and IS value-sensitive."

The del instance._skip_signals guidance is correct for both patterns, so the advice to callers is safe — this is a documentation clarity gap, not a behavioural regression.

---

2. PR body misidentifies the location of the _skip_signals annotation

The PR body states:

Declared once as _skip_signals: bool on BaseOCModel (opencontractserver/shared/Models.py)

The actual change is on InstanceUserCanMixin in opencontractserver/shared/user_can_mixin.py. BaseOCModel inherits the mixin transitively, so the runtime effect is the same and the annotation is correctly placed — but the PR description (and the mypy.ini prose comment added at line 156) names the wrong class and file. The changelog fragment already has the correct attribution (InstanceUserCanMixin / user_can_mixin.py). Worth fixing in the PR body or mypy.ini comment to avoid confusing future git blame readers.

---

3. Redundant assert x is not None after self.assertIsNotNone(x) — mypy narrowing, but noisy

In several places the PR emits:

self.assertIsNotNone(foo, "descriptive message")
assert foo is not None          # ← purely for mypy
self.assertEqual(foo.attr, ...)

The runtime guard is fully handled by assertIsNotNone; the bare assert adds no new failure information and is only there to narrow the type for mypy. This is a legitimate pattern (mypy can't infer narrowing from assertIsNotNone), but the PR applies it inconsistently: some call sites use both (e.g. test_long_conversation_api.py:163–164), while others use only assert ... is not None without the preceding assertIsNotNone (e.g. test_conversation_search.py retrieved_vector checks). Neither form is wrong, but picking one consistently would reduce the "why two checks?" puzzle for the next reader. An alternative that satisfies both runtime quality and mypy narrowing in one line is assert foo is not None, "descriptive message" — no assertIsNotNone needed.

---

4. (Good catch) Latent TypeError in two tool-registry test loops — now correctly guarded

opencontractserver/tests/test_thread_corpus_actions.py lines ~762–791

Before this PR, test_moderation_action_tools_require_approval and test_read_only_tools_no_approval subscripted the return value of get_tool_by_name() directly (tool["requiresApproval"]) without checking for None. If any tool name was absent from the registry, the test would crash with a confusing TypeError: 'NoneType' object is not subscriptable instead of a clear assertion failure. The PR adds self.assertIsNotNone(tool, f"Tool {tool_name} should be registered") before the subscript, which is the right fix. Worth noting as a pre-existing latent bug that the mypy work surfaced.

---

Summary

The core change (_skip_signals: bool on InstanceUserCanMixin) is correct and safe — the bare annotation is invisible to hasattr/Django's metaclass and removes a legitimate # type: ignore. The test fixes are mechanical and accurate. The two issues worth acting on before merging are #1 (incomplete protocol documentation for the getattr handler) and #2 (PR body / mypy.ini prose naming the wrong file and class).

[JSv4]

Thanks — addressed in bfea061.

1. Incomplete protocol doc — fixed. The _skip_signals comment now names the exception explicitly: notifications/signals.py's emit_analysis_status_notification reads getattr(instance, "_skip_signals", False) and so is value-sensitive, unlike the eight hasattr handlers. (Took the documentation option rather than standardizing the handlers, since that's a production behavior change out of scope here — and an earlier review on this PR asked for the opposite standardization direction.)

2. PR body / mypy.ini name the wrong class — already fixed (stale). Both were corrected in ae8c23a + the PR-body edit last round; they currently read InstanceUserCanMixin (shared/user_can_mixin.py). This review looks to have run against an earlier commit. No further change needed.

3. Inconsistent assertIsNotNone + assert — keeping the current rule (declined). The application is principled, not arbitrary: where the original test already had a descriptive assertIsNotNone (or the call is in a loop where the message aids diagnosis), I keep it and add the bare assert for narrowing; where there was no prior None-check, the real assertion is the following line (len(...), subscript) and the bare assert is purely for mypy. I'd avoid the suggested assert x is not None, "msg" one-liner specifically because assert is stripped under python -O — that's the exact -O fragility the previous review flagged, and assertIsNotNone (not stripped) is the more robust runtime guard. Converting the existing assertIsNotNone calls would also rewrite pre-existing test assertions. The double-guard matches the established pipeline-chunk graduation pattern.

4. Latent TypeError catch — thanks, noted. Agreed it's a pre-existing latent bug the mypy work surfaced; the assertIsNotNone(tool, …) guard (now in both loops, 3f12d6e) turns it into a clear assertion failure.

mypy stays clean under both pins; the change is documentation-only.

---

Generated by Claude Code

[claude]

Code Review

This PR is a clean, incremental step in the #1738 mypy-baseline drain for the conversation/threading domain. It applies the established patterns (class-level attribute annotations, self.client → self.graphene_client rename, assert x is not None narrowing, get_user_model() alias → concrete import) consistently across 7 test files and removes a now-unnecessary # type: ignore. Two items worth discussing:

---

1. _skip_signals protocol asymmetry — documented but not resolved

opencontractserver/shared/user_can_mixin.py:172

The PR correctly declares _skip_signals: bool as a bare annotation (no assignment, so hasattr correctly returns False on all pristine instances) and adds a thorough comment warning about the asymmetry. But the asymmetry itself remains in place:

• 9 handlers (notifications/signals.py:246,366,430,478; corpuses/signals.py:113,179,255; badges/signals.py:38,85) use hasattr(instance, "_skip_signals") — PRESENCE-based.
• 1 handler (notifications/signals.py:146, emit_analysis_status_notification) uses getattr(instance, "_skip_signals", False) — VALUE-based.

Assigning instance._skip_signals = False (a natural "re-enable" attempt given the bool type annotation) would suppress all 9 presence-based handlers silently but leave emit_analysis_status_notification firing. The only reliable re-enable (del instance._skip_signals) is documented in the comment, but at the call site — which may be deep in a test fixture far from the mixin doc — it's easy to miss.

The complete fix (not required to land this PR, but worth tracking) would be to standardize all handlers to getattr(instance, "_skip_signals", False) so the bool annotation aligns with the actual semantics. That would make _skip_signals = False a safe no-op rather than a footgun.

---

2. Missing assertIsNotNone guards added in tool loops — positive, not just narrowing

opencontractserver/tests/test_thread_corpus_actions.py:765,784

The test_moderation_action_tools_require_approval and test_read_only_tools_no_approval loops were previously:

tool = get_tool_by_name(tool_name)
# No None check
self.assertTrue(tool["requiresApproval"], ...)   # ← TypeError crash if tool is None

The PR adds a proper self.assertIsNotNone(tool, ...) guard before the subscript, converting a confusing TypeError: 'NoneType' object is not subscriptable into a clear diagnostic test failure. Good catch — this is a genuine correctness improvement beyond just mypy narrowing.

---

3. Dual assertions are redundant (established convention, minor)

Several spots pair self.assertIsNotNone(x, msg) immediately with assert x is not None. The assertIsNotNone already raises with msg on failure, so the bare assert only serves mypy narrowing. This pattern is already in test_corpus_fork_round_trip.py and other graduated files so it's consistent with project convention — just noting that cast(Type, x) after the single assertIsNotNone is a more explicit idiom that other parts of the test suite also use.

---

Overall this is solid work — clean, mechanical, and well-verified. Finding #2 (the tool-loop guards) is a small positive beyond the stated goal. Finding #1 is the only architectural concern worth a follow-up issue if one doesn't already exist.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!