Type-check opencontractserver.llms modules (issue #1484)

[JSv4]

Summary

Graduate the remaining opencontractserver.llms.* modules (agents, api/client, vector_stores) out of the mypy baseline by fixing type errors and adding proper type annotations. This resolves issue #1484 and completes the typing work started in #1468.

Key Changes

Core Agent Framework (core_agents.py)

• Fixed CorpusAgentContext.documents to use field(default_factory=list) instead of Optional[list[Document]]
• Updated initialize() to safely handle async document loading with proper type narrowing
• Changed stream() protocol method signature from async def to def (returns async generator, not coroutine)
• Fixed resume_with_approval() to return AsyncGenerator[UnifiedStreamEvent, None] consistently
• Added Optional type hints to sources and metadata parameters across message methods
• Fixed _stream_raw() abstract method with unreachable yield to satisfy type checker
• Improved type safety in _normalise_source() with explicit string conversion

PydanticAI Agent Implementation (pydantic_ai_agents.py)

• Refactored _stream_core() signature to accept **kwargs: Any for compatibility with mixin contract
• Extracted typed kwargs (force_llm_id, force_user_msg_id, etc.) from **kwargs with proper defaults
• Added explicit type annotation for effective_history variable
• Removed unnecessary # type: ignore comments where types are now clear
• Added null-safety checks before calling _finalise_llm_message() and complete_message()
• Fixed resume_with_approval() to safely handle MessageState enum migration with fallback
• Added type annotation for _vs_kwargs dictionary
• Improved tool result handling with explicit type narrowing

Agent Factory (agent_factory.py)

• Added Any import for proper type annotations
• Fixed type hints in factory methods

API Module (api.py)

• Changed ToolType from Union[str, CoreTool, callable] to Union[str, CoreTool, Callable[..., Any]]
• Extracted framework resolution logic into _resolve_framework() helper
• Added explicit type annotations for resolved tools lists
• Fixed from_function() parameter type from callable to Callable[..., Any]
• Improved type safety in get_structured_response_and_sources_from_document()

Vector Store Modules

• core_vector_stores.py: Fixed embedder initialization with proper null checks and assertions; updated return type to AnnotationQuerySet
• core_conversation_vector_stores.py: Added ChatMessageQuerySet import; improved embedder resolution with conditional logic
• pydantic_ai_vector_stores.py: Added Callable import for proper type hints

Timeline & Mixin Support

• timeline_stream_mixin.py: Fixed _stream_core() return type to AsyncGenerator[UnifiedStreamEvent, None] with unreachable yield
• timeline_utils.py: Added cast import and TimelineEntry import for type safety

Configuration

• Removed 9 ignore_errors = True directives from mypy.ini for llms modules
• Added explanatory comment documenting the graduation of these modules

Implementation Details

• Used sync_to_async(lambda: ...)() pattern for safe queryset execution in async contexts
• Added # nosec B101 comments for intentional assertions used as type guards
• Preserved backward compatibility by accepting **kwargs in public methods while extracting typed parameters internally
• Used cast() for type narrowing where runtime behavior is guaranteed but type checker needs help
• Added null-safety checks before database operations that require valid IDs

https://claude.ai/code/session_016L1wbQaPdTKtpuJnxErihF

[claude]

Code Review: PR #1543 — Type-check opencontractserver.llms modules

Overview

This PR graduates 9 modules from the mypy ignore_errors = True baseline, fixing ~125 type errors across opencontractserver.llms.*. The work is valuable beyond pure type-hygiene: the typing pass uncovered several real runtime bugs that are fixed here.

---

Real bugs fixed — worth highlighting

These are not just type annotations; they are correctness fixes:

• dataclasses.asdict() on a class instead of an instance (pydantic_ai_agents.py): Previously dataclasses.is_dataclass(usage) was truthy for both instances and class objects; the new guard and not isinstance(usage, type) prevents asdict() from being called on a bare class, which would crash at runtime.

• Null message IDs passed to pause_for_approval() / _finalise_llm_message() / complete_message(): The null-safety guards (if llm_msg_id is not None:) prevent a TypeError in persistence-disabled flows.

• String comparison for event type dispatch replaced with isinstance (pydantic_ai_agents.py): getattr(ev, "type", "") == "sources" was fragile and would silently break if the attribute name changed. isinstance(ev, SourceEvent) is both more correct and type-safe.

• New permission pre-checks in tool closures (add_document_note_tool, add_exact_string_annotations_tool, duplicate_annotations_tool): Raises PermissionError before attempting the DB write when user_id is None or corpus is None. Good security tightening.

---

Positive patterns used well

• sync_to_async(lambda: list(qs))() everywhere instead of sync_to_async(list)(qs) — the lambda form correctly wraps queryset evaluation inside the thread-pool executor.
• cast(AnnotationQuerySet, queryset) / cast(ChatMessageQuerySet, queryset) — minimal surface casts to expose the custom manager's search_by_embedding method; the casts are safe because the managers actually return these custom queryset types.
• _resolve_framework() helper extraction — the three copy-pasted framework-resolution blocks in api.py are collapsed into one function. Clean DRY improvement.
• nosec B101 on intentional assertions — accurate: these are type-narrowing invariants, not security-relevant assertions.
• Changelog entry is detailed and accurate.

---

Issues and suggestions

Medium — CorpusAgentContext.documents empty-vs-uninitialized ambiguity

Before: documents: Optional[list[Document]] = None / if self.documents is None:
After: documents: list[Document] = field(default_factory=list) / if not self.documents:

For a corpus with zero active documents, initialize() will re-query the database on every call because an empty list is falsy. You can no longer distinguish "not yet loaded" from "loaded and empty". A sentinel _initialized: bool = field(default=False, init=False) would be cleaner. Worth a comment at minimum.

Minor — import inside unreachable dead code

In core_agents.py _stream_raw(), the from typing import cast as _cast inside unreachable code is unusual and may confuse linters. Consider the conventional if False: idiom which avoids the unusual local import and is immediately recognizable as a type-narrowing stub:

raise NotImplementedError
if False:  # pragma: no cover
    yield cast(UnifiedStreamEvent, None)

Minor — vector_store: Optional[Any] is a type regression

pydantic_ai_tools.py loosens Optional[CoreAnnotationVectorStore] to Optional[Any]. The runtime reason is valid (two concrete store types share the field), but it removes all type-checking on this attribute. A VectorStoreProtocol defining search / async_search methods would be the tightest fix. Acceptable as a follow-up.

Minor — _stream_core(**kwargs) loses compile-time parameter checking

The **kwargs: Any + .pop() pattern satisfies the mixin contract but loses mypy verification on call-site arguments (force_llm_id, etc.). A TypedDict for the kwargs would recover that. Low priority but worth noting.

Minor — silent empty tool_name on malformed approval data

tool_name = str(pending.get("name") or "") silently produces "" when the field is absent. A warning log would help diagnose malformed persistence state.

---

Test coverage gap

The PR fixes real runtime behavior (the asdict guard, null-ID guards, permission pre-checks in tools) but adds no new tests for them. The _usage_to_dict / asdict guard would benefit from a unit test that passes a dataclass class object (not an instance) to confirm the old crash no longer occurs. Non-blocking since these are improvements over the pre-existing untested baseline, but should be tracked.

---

Summary

Solid, careful typing work that caught real bugs along the way. The dataclasses.asdict fix and null-ID guards are the most impactful; the permission pre-checks are a nice security improvement. None of the raised issues block merging. Recommend addressing the unreachable-import style and the silent empty tool_name before merge; the rest can be tracked as follow-ups.

[codecov]

Codecov Report

❌ Patch coverage is 74.23313% with 42 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...encontractserver/llms/agents/pydantic_ai_agents.py	68.08%	15 Missing ⚠️
opencontractserver/llms/agents/core_agents.py	62.50%	12 Missing ⚠️
...s/vector_stores/core_conversation_vector_stores.py	73.91%	6 Missing ⚠️
...actserver/llms/vector_stores/core_vector_stores.py	80.00%	4 Missing ⚠️
...ontractserver/llms/agents/timeline_stream_mixin.py	40.00%	3 Missing ⚠️
opencontractserver/llms/client.py	0.00%	1 Missing ⚠️
opencontractserver/tasks/agent_tasks.py	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[claude]

Code Review: Type-check opencontractserver.llms modules (#1484)

This PR graduates the remaining opencontractserver.llms.* modules from the mypy baseline, fixing ~125 type errors across agents, API, client, and vector store modules. The work is well-scoped and the changes are clearly motivated. A few items worth discussing before merging:

---

Behavioral Changes Hidden in a Typing PR

Several changes go beyond annotation-only fixes and alter runtime behavior. These are worth calling out explicitly.

CorpusAgentContext.documents field change (core_agents.py:317)

# Before
documents: Optional[list[Document]] = None

# After
documents: list[Document] = field(default_factory=list)

The initialize() method now uses if not self.documents: instead of if self.documents is None:. This means a context pre-loaded with an intentionally empty document list will trigger a DB fetch on initialize(), changing semantics. If any caller deliberately passes documents=[] to signal "this corpus has no documents," it will be silently overwritten.

_normalise_source() falsy-check change (core_agents.py:904-508)

# Before (None-safe default)
content=raw.get("content", raw.get("text", ""))

# After (falsy-check, treats empty string "" as missing)
content = raw.get("content") or raw.get("text") or ""

The new version will fall through to text if content exists but is an empty string "". The old version would have kept the empty string. Minor, but worth confirming this is the intended behavior.

_resolve_framework() now raises TypeError (api.py:1232)
Previously, unresolvable framework values were silently passed downstream and would fail at the factory level. Now they raise at the API boundary. This is strictly better, but it's a new exception type callers might not expect.

---

Protocol Contract Changes

CoreAgent.stream() signature change (core_agents.py:344)

# Before
async def stream(self, message: str, **kwargs) -> AsyncGenerator[...]

# After
def stream(self, message: str, **kwargs: Any) -> AsyncGenerator[...]

Changing the Protocol method from async def to def is correct for returning an async generator (the generator itself is the async iterable, not the coroutine result). However, any caller that currently does await agent.stream(...) will silently stop awaiting after this change — the object returned by a sync method returning AsyncGenerator is just the generator, not a coroutine. Existing callers should be audited to confirm none do await agent.stream(...).

resume_with_approval() return type narrowed (core_agents.py:387-394)
The Protocol return type dropped UnifiedChatResponse from the union. If any caller relies on the UnifiedChatResponse path, they'll now get type errors (or runtime surprises). Verify no callers branch on isinstance(result, UnifiedChatResponse).

---

**kwargs Erosion in _stream_core()

The signature change from explicit parameters to **kwargs + manual extraction (pydantic_ai_agents.py:701-715) keeps the mixin contract happy but removes the key benefit of typed parameters — IDE completeness and call-site type checking. If this method is called from many places, consider whether a TypedDict or Protocol for the kwargs would preserve some safety without requiring the explicit signature.

---

Optional[Any] in PydanticAIDependencies

# pydantic_ai_tools.py:274
vector_store: Optional[Any] = Field(...)

The comment explains the reasoning (two different concrete types both work at runtime), but this erases all type safety for a field that the agent directly calls methods on. A small Protocol with just the search methods would restore meaningful type checking here without coupling to either concrete class:

class VectorStoreProtocol(Protocol):
    async def async_search(self, query: VectorSearchQuery) -> list[VectorSearchResult]: ...
    def search(self, query: VectorSearchQuery) -> list[VectorSearchResult]: ...

---

Positive Changes Worth Noting

• Real bug fix: sync_to_async(lambda: list(queryset))() vs the old sync_to_async(list)(queryset) — the lambda form correctly defers queryset evaluation to the thread pool. The old form evaluated the queryset in the calling async context before handing off.
• Real bug fix: dataclasses.is_dataclass(usage) and not isinstance(usage, type) prevents asdict() being called on a class rather than an instance.
• DRY improvement: _resolve_framework() cleanly extracts the repeated framework resolution pattern across three callers.
• Security improvement: The new explicit user_id is None and corpus is None guards in add_document_note_tool, duplicate_annotations_tool, and add_exact_string_annotations_tool add meaningful pre-flight validation.
• isinstance over getattr(ev, "type", "") comparisons in ask_document event handling (pydantic_ai_agents.py:2936-2950) is strictly better — safe, readable, and not fragile to type attribute changes.

---

Test Coverage

No tests accompany the behavioral changes noted above. Specifically, the new PermissionError/ValueError guards in tool closures, the _resolve_framework() TypeError path, and the documents=[] initialize behavior would each benefit from a test. Given the CLAUDE.md guidance to ensure all affected tests pass, it would be worth confirming the existing agent test suite still passes without changes.

---

Minor

• The # nosec B101 comments on assertions are appropriate and correctly annotated.
• The if False: yield cast(UnifiedStreamEvent, None) pattern in abstract methods is unconventional but valid. A short comment explaining the pattern to future readers (beyond what's already in _stream_raw) would help, since it looks like dead code on first scan.
• The inspected: Any = part workaround in _event_to_text_and_meta is a reasonable way to tame mypy's union narrowing complaints, though it obscures the type at the dispatch site.

---

Overall this is solid typing work. The main asks are: (1) confirm the documents=[] semantic change is intentional, (2) audit callers of stream() for any await agent.stream() usage, and (3) consider adding tests for the new runtime guards.

[claude]

Code Review: Type-check opencontractserver.llms modules (#1543)

This is a well-scoped, systematic typing PR that cleans up ~125 mypy errors across 9 modules and graduates them out of the baseline. Overall the approach is sound and the changes are correct. A few items worth discussing:

---

Positive Changes Worth Calling Out

• sync_to_async pattern fix — The change from sync_to_async(list)(queryset) to sync_to_async(lambda: list(queryset))() is an actual correctness fix, not just cosmetic. The old form passes the queryset as an argument into a sync-wrapped list() call, meaning queryset attribute access (lazy evaluation, Django's _result_cache) happens in the wrong thread context. The lambda-capture form evaluates everything inside the thread pool worker. The changelog entry should call this out as a behavior fix, not just a typing change.

• isinstance event checks — Replacing getattr(ev, "type", "") == "thought" with isinstance(ev, ThoughtEvent) throughout pydantic_ai_agents.py is strictly better: it's type-safe, avoids attribute access on Any, and doesn't silently pass when an event happens to carry a matching string literal for unrelated reasons.

• dataclasses.asdict guard — dataclasses.is_dataclass(usage) and not isinstance(usage, type) correctly excludes class objects (which is_dataclass returns True for). This is a real bug fix.

• _normalise_source improvement — Using str(content) after or-chain fallback avoids passing None as SourceNode.content.

• Permission guards before annotation tools — Adding if config.user_id is None: raise PermissionError(...) before aadd_document_note, aduplicate_annotations_with_label, and aadd_annotations_from_exact_strings closes a real security gap.

---

Issues / Concerns

1. _stream_core(**kwargs: Any) loses static call-site type safety

Changing from explicit keyword parameters to **kwargs: Any with internal kwargs.pop() satisfies the mixin's Liskov requirement, but it silently accepts any kwargs (including typos) without error and makes IDE assistance useless for callers. The hidden contract should at least be documented in the docstring with a Parameters section. Consider whether a TypedDict overload would be practical here.

2. CorpusAgentContext.documents = field(default_factory=list) edge case

The initialize() check changed from if self.documents is None: to if not self.documents:. This means a caller who pre-populates documents=[] (empty list, intentionally) will trigger a DB fetch when initialize() is called. The old behaviour was explicit: only fetch if None. This is an unlikely edge case but the semantic change is invisible in the PR summary. Adding a comment on initialize() noting this behaviour would prevent future confusion.

3. resume_with_approval return-type narrowing is a breaking API change

The Protocol's resume_with_approval was changed from Union[UnifiedChatResponse, AsyncGenerator[...]] to AsyncGenerator[...] only. Any external code holding a typed reference to a CoreAgent Protocol and calling resume_with_approval expecting a possible UnifiedChatResponse will now get a type error — or worse, silently do the wrong thing at runtime if they await the result expecting a response object. Confirm no callers branch on the old union type before merging.

4. New # type: ignore[assignment] introduced

# pydantic_ai_agents.py
async for event in tool_stream:  # type: ignore[assignment]

This PR removes many # type: ignore comments but adds one new one. The suppression should be replaced with a proper fix (e.g., an explicit cast) with a comment explaining why the cast is safe — otherwise it's just moving the problem.

5. vector_store: Optional[Any] in PydanticAIDependencies

Typing vector_store as Optional[Any] erases all type checking for this field. The comment explains the runtime reason correctly, but a structural Protocol (e.g., class VectorStoreProtocol(Protocol): def search(...) -> ...: ...) would preserve safety while handling both concrete types. The project already has VectorStoreProtocol in opencontractserver/types/protocols.py — consider using or extending that.

6. Duplicated __init__ embedder-resolution block

The sync and async variants of the conversation vector store class now both contain an identical ≈30-line if embedder_path is not None / else assert corpus_id is not None block. This was pre-existing duplication but this PR added significant content to both copies identically. A shared _resolve_embedder(corpus_id, embedder_path) helper would follow the DRY principle stated in CLAUDE.md and avoid the two copies drifting apart.

---

Missing Test Coverage

The following are behaviour changes (not just typing changes) with no accompanying tests:

• llm_msg_id is not None guard in _stream_core — the "no persistence" code path was previously incorrect; a test for stream() with enable_message_persistence=False would confirm the guard works.
• Permission checks in annotation tools (add_document_note_tool, duplicate_annotations_tool, add_exact_string_annotations_tool) — a simple test asserting PermissionError is raised when user_id=None would protect these.
• The _resolve_framework helper — a TypeError is now raised for unrecognised framework values instead of propagating silently; this path is untested.

---

Minor Nits

• The if False: yield cast(UnifiedStreamEvent, None) pattern in _stream_raw is valid but the comment in the if False: block says "Unreachable" which is slightly misleading — the real purpose is to syntactically turn the method into a generator function. A one-line comment at the method level (# unreachable yield makes this an async generator at the type level) would be clearer than inline documentation inside a dead branch.
• effective_history: Optional[list[Any]] declaration before the if/else block is fine but the variable is never None by the time it leaves the conditional — the annotation slightly overstates what's possible downstream.

---

Summary

The typing work is thorough and correct. The real behaviour fixes (sync_to_async, permission checks, isinstance event dispatch, dataclasses guard) are genuinely valuable. The main asks before merging:

1. Confirm or fix the resume_with_approval union-type break for existing callers.
2. Replace the newly-added # type: ignore[assignment] with a cast + explanation, or fix the underlying type.
3. Consider VectorStoreProtocol instead of Optional[Any] for PydanticAIDependencies.vector_store.
4. Add tests for the behaviour changes listed above (or open a follow-up issue).

[claude]

Code Review — PR #1543: Type-check opencontractserver.llms modules

Overall: This is solid, well-scoped work. Graduating 9 modules from the mypy baseline clears ~125 real type errors and includes several genuine runtime correctness fixes alongside the annotation work. A few things worth addressing before merge.

---

What the PR Does Well

• sync_to_async(list)(queryset) → sync_to_async(lambda: list(queryset))() — This is a real bug fix, not just a typing change. The original form evaluates the queryset in the calling async context before the thread pool sees it, which can raise SynchronousOnlyOperation. The lambda defers evaluation correctly.

• _resolve_framework() helper — Good DRY refactoring. Three identical framework-resolution blocks collapsed into one with a TypeError escape valve for unrecognized types.

• CorpusAgentContext.documents change — Changing from Optional[list[Document]] to list[Document] = field(default_factory=list) is the right semantic call; None was never a meaningful steady state.

• isinstance(ev, SourceEvent/ThoughtEvent/FinalEvent) over getattr(ev, "type", "") — Correct improvement; string-based type dispatch is fragile.

• dataclasses.is_dataclass(usage) and not isinstance(usage, type) — Correctly guards against asdict() receiving a class object rather than an instance.

• User-ID guards on annotation tools — PermissionError raised before any DB write when config.user_id is None is a good defensive addition.

---

Issues Worth Addressing

1. assert used for runtime security invariants (medium)

Several new assert statements guard security/correctness invariants:

assert self.user_id is not None  # nosec B101 — factory invariant

assert is stripped by Python's -O optimisation flag. If this code ever runs in an optimised context (some container images enable it), the guard silently vanishes and creator_id=None is passed to the ORM, producing a confusing DB error rather than a clean early failure.

Suggestion: Replace with explicit if/raise:

if self.user_id is None:
    raise RuntimeError("factory invariant violated: user_id must be set for persistent messages")

The three nosec B101 sites in core_agents.py and the two in core_conversation_vector_stores.py / core_vector_stores.py all fall into this category.

2. vector_store: Optional[Any] widens type to Any (low-medium)

In pydantic_ai_tools.py:

# Before
vector_store: Optional[CoreAnnotationVectorStore] = Field(...)
# After
vector_store: Optional[Any] = Field(...)

The comment explains why (CoreAnnotationVectorStore vs. its PydanticAI wrapper), but Any silently disables all type checking on this field for callers. A Protocol or Union[CoreAnnotationVectorStore, PydanticAIAnnotationVectorStore] would preserve meaningful coverage. If a Protocol is too much work now, at least a # type: ignore[assignment] at the injection site keeps the field typed and confines the suppression.

3. cast("list[TimelineEntry]", self._timeline) is unchecked (low)

_timeline is accumulated as list[dict[str, Any]] throughout TimelineBuilder.add(). Casting it to list[TimelineEntry] satisfies mypy but does not guarantee the dictionaries actually conform to the TypedDict. If a caller later adds a stricter consumer that expects real TimelineEntry keys, the cast silently hides the mismatch. Consider either narrowing the accumulation type or keeping it as list[dict[str, Any]] and updating the property's return type to match.

4. Semantic change in CorpusAgentContext.initialize() (low)

# Before
if self.documents is None:
# After
if not self.documents:

An explicitly-passed empty list now triggers a DB reload. This could surprise callers who pass documents=[] to signal "no documents; skip loading". If the intent is truly "reload when empty", the condition is fine — but worth a note in the docstring or constructor signature.

5. _resolve_framework default setting name for vector stores (low)

In api.py, VectorStoreAPI.create() calls:

resolved_framework = _resolve_framework(framework)  # default: LLMS_DOCUMENT_AGENT_FRAMEWORK

A vector store factory reading LLMS_DOCUMENT_AGENT_FRAMEWORK looks like an unintentional leak from the document-agent path. Should this be a LLMS_VECTOR_STORE_FRAMEWORK setting, or at least documented as intentional?

---

Test Coverage

The new test_llms_typing_behavior_guards.py is a good addition. A few observations:

• test_aadd_document_note_is_not_called_when_user_id_is_none and the two sibling tests only assert callable(paa.aadd_document_note) — they don't test the guard itself. The comment acknowledges this limitation ("can't construct it without a full Agent"), which is fair, but the tests read as guard tests and aren't. Consider either removing them and relying on the source-inspection test, or renaming them to make the scope clear (e.g., test_aadd_document_note_is_importable).

• test_permission_error_message_format uses inspect.getsource() to count occurrences of a string in source code. This is explicitly acknowledged as brittle and intentional — that's fine as long as the team understands it's a lint-via-test rather than a functional test.

• TestSyncToAsyncListEvaluation is clever: it greps source files to prevent the bug regressing. No objection, just note that it reads files by relative path from the CWD, which works in the Docker test environment but would fail if tests are ever run from a different working directory. Using Path(__file__).parent.parent.parent / "opencontractserver/..." makes it path-safe.

---

Minor Style Notes

• A few new comments in pydantic_ai_agents.py describe what the code does rather than why (CLAUDE.md asks to omit those). The _stream_core(**kwargs) block is borderline — the explanation of the mixin constraint is genuinely non-obvious.
• The # ``[assignment]`` ignore multi-line comment block at the async for event in tool_stream site is a good write-up; no issue there.

---

Summary

The core work is correct and the runtime fixes (sync_to_async lambda, is_dataclass guard, isinstance dispatch) are worth merging on their own. The main asks before landing:

1. Convert assert self.user_id is not None guards to explicit if/raise (the -O risk is real).
2. Reconsider Optional[Any] for vector_store — at minimum document the decision clearly.
3. Clarify or fix the _resolve_framework default setting name for vector stores.

[claude]

Code Review: Type-check opencontractserver.llms modules (Issue #1484)

Good work graduating 9 modules out of the mypy ignore_errors = True baseline. Several changes fix real runtime bugs alongside the type annotations. Below is a prioritized breakdown.

---

Real Bug Fixes (Positive)

These changes correct actual runtime issues, not just type-checker noise:

• sync_to_async(lambda: list(qs))() — the old sync_to_async(list)(qs) evaluated the queryset on the calling thread before handing work to the pool. The lambda-wrap fixes the actual async-safety issue across core_vector_stores.py, core_conversation_vector_stores.py, and core_agents.py.

• dataclasses.asdict guard — dataclasses.is_dataclass returns True for both instances and class objects. Adding and not isinstance(usage, type) in _usage_to_dict prevents a TypeError when a dataclass class (not instance) is accidentally passed.

• isinstance(ev, SourceEvent/ThoughtEvent/FinalEvent) over getattr(ev, "type", "") == "..." — the old form silently matched any event carrying a .type attribute with the matching string. The new form is correct and less fragile.

• doc_pk = self.document_id lambda closure capture — avoids stale-capture bugs in the sync_to_async lambda in _build_base_queryset.

• getattr(agent, "agent_deps", None) — the original getattr(agent.agent_deps, ...) would raise AttributeError if agent_deps didn't exist; the two-step approach is safer.

• user_id is None pre-checks before DB writes — good defensive addition in create_placeholder_message, store_user_message, store_llm_message, and the tool closures. Using raise RuntimeError instead of assert (to survive python -O) is the right call.

---

Concerns

1. _stream_core(**kwargs) erases the public contract (medium)

# Before
async def _stream_core(self, message: str, *, force_llm_id: int | None = None, ...) -> ...:

# After
async def _stream_core(self, message: str, **kwargs: Any) -> ...:
    force_llm_id: Optional[int] = kwargs.pop("force_llm_id", None)

This moves from a typed, IDE-navigable signature to a stringly-typed interface. A misspelled kwarg (e.g. force_llm_idd=123) will be silently ignored — kwargs.pop returns None, the message ID is lost, and there's no error. The PR description explains why (mixin contract compatibility), but the approach creates a maintenance trap.

A safer alternative is a typed TypedDict or dataclass for the kwargs, or a **Unpack[_StreamCoreKwargs] annotation (PEP 692, available via typing_extensions). Worth a follow-up issue.

2. Unreachable yield cast(UnifiedStreamEvent, None) in Protocol stub (low)

async def _stream_core(self, ...) -> AsyncGenerator[UnifiedStreamEvent, None]:
    raise NotImplementedError("_stream_core() must be implemented by adapter")
    yield cast(UnifiedStreamEvent, None)  # unreachable

cast(UnifiedStreamEvent, None) would yield None at runtime if the raise were ever removed — a silent misbehavior. A bare yield # type: ignore[misc] is the conventional idiom for this pattern, or verify whether the current mypy version accepts raise NotImplementedError alone in an AsyncGenerator-returning method (recent versions do).

3. cast() in TimelineBuilder.timeline is an unsound contract (low)

@property
def timeline(self) -> list[TimelineEntry]:
    return cast("list[TimelineEntry]", self._timeline)

The comment acknowledges _timeline is list[dict[str, Any]] — the cast is unsound. Callers that trust the narrower return type will get KeyError at runtime for malformed entries. Consider keeping the return type as list[dict[str, Any]] and using TypeAlias at call sites that need the narrower hint — that would be an honest contract rather than silencing the type checker at the boundary.

4. int(self.corpus_id) cast in queryset filter (low)

queryset = queryset.filter(conversation__chat_with_corpus_id=int(self.corpus_id))

If corpus_id requires an int() cast at filter time, its type annotation should reflect Union[int, str] or the constructor should coerce it on entry. A silent runtime cast at point of use suggests the declared type doesn't match reality.

5. Logically dead cid is None guards (low)

elif conversation_id or config.conversation_id:
    cid = conversation_id or config.conversation_id
    if cid is None:  # can never be True here
        raise RuntimeError("internal invariant violated: ...")

The guard exists purely for mypy type narrowing — raise RuntimeError implies a recoverable runtime failure, which this isn't. assert cid is not None, "..." (with # nosec B101) communicates invariant intent more idiomatically.

6. Duplicate embedder-resolution logic (low)

The __init__ methods of the two concrete conversation vector store classes in core_conversation_vector_stores.py contain nearly identical ~40-line if embedder_path is not None / else blocks after this PR. A shared _resolve_embedder_path(corpus_id, embedder_path) helper would eliminate the duplication.

---

Test File Observations

The new test_llms_typing_behavior_guards.py pins _resolve_framework and CorpusAgentContext.documents behavior cleanly. A few notes:

• Source-grep tests (TestSyncToAsyncListEvaluation, TestDataclassesAsdictUsageGuard, TestIsinstanceEventDispatchKeepsTypeSafety, TestStreamCorePersistenceGuard) are brittle: they break on formatting changes, pass if the pattern appears only in a comment, and don't test runtime behavior. These are better as CI grep shell checks or mypy rules than as TestCase assertions.

• TestPermissionGuardsOnAnnotationTools — the three _is_importable tests only verify callable(fn), which is trivially true for any imported function. The guard itself is never exercised. Consider replacing them with a test that constructs a minimal AgentConfig(user_id=None) and asserts the PermissionError is raised from the inner closure.

• Multi-paragraph docstrings throughout the test file conflict with CLAUDE.md ("Never write multi-paragraph docstrings or multi-line comment blocks — one short line max"). The rationale is valuable but belongs in the PR description, not inline docstrings.

---

Nits

• messages = loaded_messages or config.loaded_messages or [] — the or [] is unreachable since the branch is only entered when the preceding if loaded_messages or config.loaded_messages is truthy. Harmless but misleading.
• The _resolve_framework extraction is clean and all three call sites benefit. Good DRY improvement.
• _AgentVectorStoreProto Protocol with @runtime_checkable is the right tool for breaking the circular import without widening to Any.

---

Summary

The real bug fixes (sync_to_async lambda, dataclasses.asdict guard, isinstance event dispatch) make this PR worthwhile independent of the typing graduation. The main concern worth a follow-up issue is the **kwargs change to _stream_core — it trades type safety for mixin compatibility in a way that silently swallows misspelled arguments. Everything else is low-priority cleanup.

[claude]

Code Review — PR #1543 (Type-check opencontractserver.llms modules)

Overall this is a well-structured typing graduation. The changes are clearly scoped to the llms.* modules, the mypy.ini removals map 1:1 to the modules touched, and the changelog entry is detailed enough to be useful. Several changes also fix latent runtime bugs, which is a nice bonus.

---

Genuine bug fixes worth calling out explicitly

dataclasses.is_dataclass(usage) and not isinstance(usage, type) (pydantic_ai_agents.py)

This is a real correctness fix. dataclasses.is_dataclass() returns True for both dataclass classes and instances, but dataclasses.asdict() only accepts instances and raises TypeError on classes. The added isinstance(usage, type) guard is the idiomatic fix.

if llm_msg_id is not None: guards before persistence calls

Adding explicit null-guards before _finalise_llm_message() and complete_message() is correct. Ephemeral (non-persisted) agents legitimately produce llm_msg_id = None, and the previous code would TypeError at the function boundary.

isinstance(ev, SourceEvent/ThoughtEvent/FinalEvent) replacing getattr(ev, "type", "") == "..."

Switching to isinstance checks is strictly better — no magic string, not susceptible to objects that happen to have a .type attribute set to the right string for unrelated reasons.

getattr(agent, "agent_deps", None) in api.py

Guards against AttributeError if agent_deps is absent on the agent object. Correct defensive change.

---

Concerns

1. Semantic change in CorpusAgentContext.initialize() (medium)

# Before
documents: Optional[list[Document]] = None
if self.documents is None:
    self.documents = await sync_to_async(list)(self.corpus.get_documents())

# After
documents: list[Document] = field(default_factory=list)
if not self.documents:
    self.documents = await sync_to_async(lambda: list(self.corpus.get_documents()))()

The intent is to replace None as the "not loaded" sentinel with []. However if not self.documents: conflates two distinct states: never loaded ([]) and loaded but empty ([]). If a corpus genuinely has zero active documents, calling initialize() again will re-query the database unnecessarily and overwrite the already-correct empty list.

The PR comment in the docstring acknowledges this ("Callers that want to explicitly state 'no documents — skip loading' should not invoke initialize()"), but that's a new caller-responsibility that didn't exist before. Consider using a sentinel — either keep Optional[list[Document]] and check is None, or use a private _initialized: bool flag. The typing improvement doesn't require conflating these two states.

2. _stream_core(**kwargs: Any) erases caller-side type safety (minor)

# Before
async def _stream_core(
    self,
    message: str,
    *,
    force_llm_id: int | None = None,
    force_user_msg_id: int | None = None,
    ...

# After
async def _stream_core(self, message: str, **kwargs: Any) -> ...:
    force_llm_id: Optional[int] = kwargs.pop("force_llm_id", None)
    ...

The PR correctly identifies that the mixin contract requires a compatible signature. However, the tradeoff is that callers now pass these parameters as untyped kwargs — a typo like forcellm_id=123 becomes a silent no-op instead of a type error. Since these are internal methods, not public API, it's an acceptable workaround, but the comment in _stream_core's docstring should explicitly name all accepted kwargs so future readers know the implicit contract.

3. Permission guard tests are mostly importability checks, not behavioral tests

def test_aadd_document_note_is_importable(self) -> None:
    from opencontractserver.llms.agents import pydantic_ai_agents as paa
    self.assertTrue(callable(paa.aadd_document_note))

The test file acknowledges this limitation ("We can't easily invoke the inner closure from outside"). These tests verify the function names are importable but don't verify that PermissionError is actually raised when user_id is None. The source-inspection test (source.count("requires an authenticated user")) is brittle by the PR's own admission.

Two untested guards worth adding behavioral tests for:

• The context.corpus is None → ValueError path in add_document_note_tool and add_exact_string_annotations_tool (neither is covered by the count test).
• The resume_with_approval path when tool_name is empty after stripping.

4. Unreachable yield cast(UnifiedStreamEvent, None) in timeline_stream_mixin.py

async def _stream_core(self, ...) -> AsyncGenerator[UnifiedStreamEvent]:
    raise NotImplementedError("_stream_core() must be implemented by adapter")
    # The unreachable yield turns this into an async generator...
    yield cast(UnifiedStreamEvent, None)

This is a known Python trick to make a function an async generator. It works, but cast(UnifiedStreamEvent, None) yields None at runtime if somehow the raise is bypassed. A safer alternative is yield # type: ignore[misc] or better: declare the base as an abstract class with @abc.abstractmethod rather than a Protocol stub. Minor, but the cast(X, None) pattern is a lie to the type checker that could bite a future consumer of the yielded value.

5. _resolve_framework uses same default setting_name doc but different for corpus

The default setting_name="LLMS_DOCUMENT_AGENT_FRAMEWORK" is fine since the call-sites pass the correct setting for corpus agents. Just make sure the default isn't accidentally used in a corpus context without the override — the test covers None resolution but doesn't exercise the corpus-specific setting name path.

---

Nits / style

• The inspected: Any = part alias in _event_to_text_and_meta is functional but adds visual noise. An alternative is # type: ignore[union-attr] on the offending lines, which is narrower and self-documenting.
• Several new RuntimeError messages are verbose multi-sentence strings. The message should be one short clause; the explanation belongs in the surrounding comment (which is already there).
• The new _AgentVectorStoreProto Protocol in pydantic_ai_tools.py is a clean solution to the circular import. @runtime_checkable is correctly applied since callers likely won't isinstance-check against it, but it's harmless.

---

Summary

The PR achieves its goal — 9 modules graduate from ignore_errors = True and ~125 errors are resolved. The runtime changes (null guards, permission checks, isinstance dispatch) are improvements. The main concern is the CorpusAgentContext.initialize() semantic change that conflates "not yet loaded" with "legitimately empty"; that deserves a follow-up or a clarification comment in the code. The test file is a good starting point but the behavioral coverage for the permission guards is thin.

[claude]

Code Review: PR #1543 — Type-check opencontractserver.llms modules

Overall this is solid, well-scoped work. Graduating 9 modules out of mypy's ignore_errors baseline with ~125 fixes is meaningful progress. The sync_to_async(lambda: list(qs))() correction is a genuine runtime improvement, and the cast(UnifiedStreamEvent, None) unreachable-yield trick for abstract async generators is the right pattern. The review below focuses on a functional regression, a few behavioral changes that need attention, and some type-safety trade-offs introduced during the refactor.

---

🔴 Bug — add_document_note_tool breaks standalone document agents

File: opencontractserver/llms/agents/pydantic_ai_agents.py ~line 2368

if context.corpus is None:
    raise ValueError(
        "add_document_note requires the agent to be scoped to a corpus"
    )

The underlying aadd_document_note accepts corpus_id: int | None = None — corpus is optional. This guard breaks agents that are scoped to a document without a corpus. The guard was added purely to satisfy the type checker (context.corpus.id fails when corpus is None). The fix is to pass corpus_id=context.corpus.id if context.corpus else None and remove the guard.

Contrast with the guard in add_exact_string_annotations_tool — that one is correct because aadd_annotations_from_exact_strings takes corpus_id: int (non-optional). Both guards look parallel in the source, but only one is semantically justified. No test covers the corpus is None branch here.

---

🟡 Behavioral Change — Undocumented LLMS_VECTOR_STORE_FRAMEWORK setting

File: opencontractserver/llms/api.py ~line 659

The VectorStoreAPI.create() method now reads from a new LLMS_VECTOR_STORE_FRAMEWORK setting instead of falling through to LLMS_DOCUMENT_AGENT_FRAMEWORK. The default is still PYDANTIC_AI, so green-field deployments are fine. But operators who explicitly set LLMS_DOCUMENT_AGENT_FRAMEWORK expecting it to govern both agents and vector stores will find vector store framework selection silently decoupled. This should be called out in the settings docs / release notes.

---

🟡 Behavioral Change — Double-fetch for empty corpuses after documents field change

File: opencontractserver/llms/agents/core_agents.py ~line 317 and 361

# Before
documents: Optional[list[Document]] = None
# ...
if self.documents is None:   # short-circuits after first load

# After
documents: list[Document] = field(default_factory=list)
# ...
if not self.documents:       # always True for empty corpus → always re-fetches

When CoreCorpusAgentFactory.create_context pre-populates documents=[] (empty corpus) then calls context.initialize(), the new guard triggers a redundant DB fetch. Not a crash, but wasteful and semantically surprising — the previous guard correctly distinguished "not yet loaded" from "loaded and empty". One way to preserve the intent without Optional is a separate _documents_loaded: bool = False sentinel field.

---

🟡 Type Safety Regression — **kwargs: Any in _stream_core

File: opencontractserver/llms/agents/pydantic_ai_agents.py ~line 619

Replacing explicit typed parameters with **kwargs: Any + kwargs.pop(...) makes callers lose static type checking on force_llm_id, force_user_msg_id, etc. After each pop, unknown keys are silently ignored — a misspelled kwarg name produces no error at the call site or at runtime. Consider at least adding assert not kwargs, f"Unexpected kwargs: {list(kwargs)}" after all pops to catch caller mistakes at dev time (with # nosec B101 since it's intentional).

---

🟡 Type Asymmetry — corpus_id int-cast missing in CoreConversationVectorStore

File: opencontractserver/llms/vector_stores/core_conversation_vector_stores.py ~lines 210, 280

CoreChatMessageVectorStore.search applies int(self.corpus_id) in its ORM filter. CoreConversationVectorStore.search (same file) does not, despite having the same corpus_id: Union[str, int, None] signature. This asymmetry means the latter may still generate a mypy warning or behave unexpectedly when corpus_id is passed as a string.

---

🔵 Minor — Redundant list() wrap in _resolve_tools

File: opencontractserver/llms/api.py ~line 118

resolved_tools: Optional[list[Union[CoreTool, Callable[..., Any], str]]] = (
    list(_resolve_tools(tools)) if tools else None
)

_resolve_tools already returns a list[CoreTool], so the list(...) call is a no-op copy. The Callable and str union members in the type annotation are also spurious leftovers from the old ToolType union — the resolver normalises everything to CoreTool. Simplify to Optional[list[CoreTool]] and drop the redundant wrap.

---

🔵 Pre-existing Issue — messages variable assigned but never consumed

File: opencontractserver/llms/agents/core_agents.py ~lines 1240, 1314

messages = loaded_messages or config.loaded_messages or []
logger.debug(f"Loaded {len(messages)} existing messages ...")
# `messages` never used again

The or [] addition (to fix a type error) highlights a variable that's computed, logged, and discarded — a pre-existing dead-code pattern. CLAUDE.md calls for removing dead code. Out of scope for a typing PR but worth a follow-up ticket.

---

✅ Things done well

• sync_to_async(lambda: list(queryset))() is the correct fix vs. the old sync_to_async(list)(queryset) — the lambda correctly defers queryset evaluation into the thread pool.
• if False: yield cast(UnifiedStreamEvent, None) unreachable-yield pattern correctly makes abstract _stream_raw compile as an async generator. The explanatory comment is justified ("explains the WHY — a mypy limitation workaround"), consistent with CLAUDE.md.
• All cast() usages are safe: cast(UnifiedStreamEvent, None) in unreachable branches, cast(AnnotationQuerySet/ChatMessageQuerySet, qs) on correctly-typed managers.
• mypy.ini cleanup is clean and well-commented.
• CHANGELOG entry is thorough.
• New test file test_llms_typing_behavior_guards.py is a good addition, though the add_document_note_tool guard test should be extended to cover the corpus is None path.

---

Summary

Severity	Issue
🔴 Bug	add_document_note_tool raises for valid standalone-document agents
🟡 Behavioral	Undocumented LLMS_VECTOR_STORE_FRAMEWORK setting split from LLMS_DOCUMENT_AGENT_FRAMEWORK
🟡 Behavioral	Empty-corpus double-fetch after Optional[list] → list field change
🟡 Type safety	**kwargs: Any in _stream_core silently swallows unknown keys
🟡 Type asymmetry	int(corpus_id) cast missing in CoreConversationVectorStore.search
🔵 Minor	Redundant list() wrap + spurious union members in _resolve_tools caller
🔵 Pre-existing	Dead messages variable — follow-up ticket recommended

The functional regression in add_document_note_tool should be fixed before merge. The remaining items are lower-priority but worth addressing.

[claude]

Code Review: PR #1543 — Type-check opencontractserver.llms modules

Overview

This PR graduates 9 opencontractserver.llms.* modules out of the mypy baseline by fixing ~125 type errors across agents, API/client, and vector stores. Importantly, the type fixes also surface and fix several genuine runtime bugs — this is a high-quality typing PR that goes beyond mechanical annotation. Two new test files are added to pin the behavioral contracts.

---

Strengths

• Substantive fixes alongside type annotations. The sync_to_async(lambda: list(...))() change (vs. the old sync_to_async(list)(...)) is a real bug fix: the queryset was being evaluated outside the thread-pool worker. The test in TestSyncToAsyncListEvaluation is a smart regression pin.
• RuntimeError instead of assert for invariant guards. The user_id is None checks in create_placeholder_message, store_user_message, and store_llm_message correctly use explicit raise RuntimeError so the guards survive python -O. The inline comment explaining the rationale is exactly the right kind of non-obvious comment.
• isinstance(ev, SourceEvent) over getattr(ev, "type", "") == "sources". Safer dispatch with real type-narrowing benefit.
• _resolve_framework() extraction cleanly deduplicates framework resolution across for_document and for_corpus, and the new TypeError on unrecognised values is a meaningful contract tightening.
• AnnotationItem TypedDict replaces raw dict for aadd_annotations_from_exact_strings — correct use of the typed layer.
• dataclasses.asdict guard. Adding not isinstance(usage, type) before calling asdict(usage) is the correct fix for the class-vs-instance confusion.
• Pruned # type: ignore backlog. The single remaining [assignment] on the tool-stream loop is well-explained and justified.

---

Issues and Concerns

1. Protocol signature break: stream() and resume_with_approval() changed from async def to def

The CoreAgent Protocol methods stream() and resume_with_approval() were async def and are now def. This is semantically correct (both return an AsyncGenerator — callers iterate with async for, not await), but any existing call-site that does await agent.stream(...) will break silently at runtime (returns a generator, not a coroutine). The corresponding implementations in CoreAgentBase.stream() and the PydanticAI agents are still async def, which is fine since those are real async def methods that yield.

Recommendation: Grep across all callers of .stream( and .resume_with_approval( — especially in config/websocket/consumers/unified_agent_conversation.py — to verify none are preceded by await. That consumer file is still in the mypy baseline (ignore_errors = True), so mypy would not have caught a mismatch there.

2. Empty-list semantics for CorpusAgentContext.documents — potential repeated-reload edge case

Before: Optional[list[Document]] = None → reload trigger was is None
After: list[Document] = field(default_factory=list) → trigger is not self.documents

The docstring explains the intent, but there is a subtle edge case: a corpus with zero documents will trigger a reload on every initialize() call because an empty list is falsy. In the current create_context path this is harmless (documents are loaded before construction, so the list is non-empty and initialize() is a no-op), but if any caller constructs a CorpusAgentContext with the default empty list and then calls initialize() on a genuinely empty corpus, it will hit the DB on every invocation.

The factory also now loads documents before construction AND calls initialize() after — the no-op is correct but the double-intent is subtle. A brief comment at the create_context call site (# initialize() is a no-op here since documents were pre-loaded) would clarify.

3. timeline_stream_mixin.py: event.llm_message_id or 0 passes 0 as a message ID

await finalise(
    event.llm_message_id or 0,   # pk=0 almost certainly doesn't exist
    ...
)

This predates the PR, but the surrounding code is being refactored here and it's a good opportunity to fix it. The correct guard is:

if event.llm_message_id is not None:
    await finalise(event.llm_message_id, ...)

This is consistent with the pattern correctly applied in _stream_core (also in this PR) where if llm_msg_id is not None: guards the same call.

4. Test quality: import-callability checks and source-text inspection are fragile

In TestPermissionGuardsOnAnnotationTools, three tests only verify that a function is importable and callable. The docstring honestly acknowledges this. The test_permission_error_message_format test reads the source file and counts occurrences of a string literal — brittle to refactoring (rename the message → test fails; extract the guard into a helper → count is wrong).

A more robust approach: extract the user_id is None guard logic into a shared helper function that can be unit-tested directly, rather than coupling tests to source text.

Similarly, TestSyncToAsyncListEvaluation.test_no_naked_sync_to_async_list_callsites reads module files directly to grep for a pattern. This is fragile to whitespace/reformatting and scans only the four listed modules (a new module with the same bug wouldn't be caught). A linting rule (e.g., a custom flake8 check) would be more reliable.

5. Unreachable RuntimeError for conversation-id narrowing

elif conversation_id or config.conversation_id:
    cid = conversation_id or config.conversation_id
    if cid is None:          # truly unreachable given the elif condition above
        raise RuntimeError(...)

This branch cannot fire — the elif guarantees at least one is truthy. The RuntimeError message ("factory invariant violated") is misleading since no invariant is actually at risk. Consider assert cid is not None # nosec B101 with a short comment, consistent with the type-narrowing-only assertion pattern used elsewhere in this codebase.

---

Minor Nits

• resolved: Any = framework in _resolve_framework widens immediately to Any, which prevents mypy from following the isinstance chain. Non-blocking since the return annotation is correct, but keeping the typed form through the chain would be cleaner.
• _resolve_framework is correctly prefixed _ (private), but is imported directly in tests. Fine for now.

---

Summary

This is a well-executed typing graduation PR that fixes real bugs alongside mechanical annotation. The sync_to_async(lambda: ...) fix, the user_id invariant guards, and the isinstance event dispatch are all substantive improvements worth having.

Main actionable items before merge:

1. Verify no await agent.stream(...) callers exist, especially in unified_agent_conversation.py (still mypy-ignored). This is the only potential silent breakage introduced.
2. Fix event.llm_message_id or 0 in timeline_stream_mixin.py to use if event.llm_message_id is not None: instead.
3. Add a comment in create_context explaining why initialize() is a no-op there (pre-loaded documents).

Overall: approvable with the stream/await call-site check done.

[claude]

Code Review: PR #1543 — Type-check opencontractserver.llms modules

Overall: This is a solid typing graduation PR. The core goal (removing 9 ignore_errors = True blocks from mypy.ini) is achieved cleanly, and several legitimate runtime bugs were fixed in the process. The new test files are a welcome addition. Comments below are organized from most to least important.

---

Issues

1. CorpusAgentContext.documents — empty-list ambiguity (core_agents.py:338)

The switch from Optional[list[Document]] = None to list[Document] = field(default_factory=list) conflates two distinct states:

• Empty list = "not yet loaded" (triggers corpus fetch in initialize())
• Empty list = "corpus has no documents" (after loading, should be a no-op)

A corpus can legitimately have zero documents. After initialize() runs on such a corpus it populates documents with [], but a subsequent initialize() call (or any other caller using not self.documents as the load-trigger) will incorrectly re-fetch. The docstring acknowledges this but labels it a "hypothetical" concern — it isn't, it's a real case.

The minimal fix is a sentinel field:

_initialized: bool = field(default=False, init=False)

async def initialize(self):
    if not self._initialized:
        self.documents = await sync_to_async(lambda: list(self.corpus.get_documents()))()
        self._initialized = True

Alternatively, keep Optional with None as the unambiguous sentinel. The typing issue that drove this change (no_implicit_optional) is trivially satisfied by just adding Optional[list[Document]] = None explicitly.

2. create_context() calls initialize() after pre-loading (core_agents.py:1101–1137)

documents: list[Document] = await sync_to_async(lambda: list(corpus_obj.get_documents()))()
context = CorpusAgentContext(corpus=corpus_obj, config=config, documents=documents)
await context.initialize()  # No-op: documents is already non-empty

The initialize() call is now dead code (it will short-circuit every time). If the empty-list ambiguity above is fixed, this becomes the only caller that reliably passes documents — but it still calls initialize() unnecessarily. Either remove the trailing await context.initialize() here, or drop the pre-load and let initialize() own the fetch.

3. Permission guards on tool closures are untestable as written (test_llms_typing_behavior_guards.py:130–200)

The tests for add_document_note_tool, duplicate_annotations_tool, and add_exact_string_annotations_tool verify importability and source-code string counts, not runtime behavior:

# Source-grep as a test — brittle and not a behaviour assertion
guard_count = source.count("requires an authenticated user")
self.assertGreaterEqual(guard_count, 3, ...)

These guards are security-relevant checks (preventing writes without an authenticated user). A source scan won't catch a refactor that moves or renames the guard. The test comment acknowledges the closures can't be constructed without a full agent, but that constraint is worth pushing back on: the guard logic (if config.user_id is None: raise PermissionError) could trivially be extracted into a helper or tested by constructing a minimal mock context and calling the tool factory. That would give a real behaviour assertion.

4. _stream_core(**kwargs) swallows the explicit signature (pydantic_ai_agents.py:618)

async def _stream_core(self, message: str, **kwargs: Any) -> AsyncGenerator[...]:
    force_llm_id: Optional[int] = kwargs.pop("force_llm_id", None)
    force_user_msg_id: Optional[int] = kwargs.pop("force_user_msg_id", None)
    ...
    if kwargs:
        raise TypeError(f"_stream_core got unexpected keyword arguments: {sorted(kwargs)}")

The raise TypeError guard is good, but this is essentially a hand-rolled TypedDict without the type safety. The mixin contract that forced this signature relaxation (TimelineStreamMixin._stream_core) could accept a TypedDict or Protocol typed kwargs instead of **Any. The current approach pushes type errors from call-time to runtime for internal callers.

Not a blocker, but worth a follow-up ticket.

---

Correctness Improvements (positive)

These changes fix real bugs and are good to land:

• sync_to_async(lambda: list(queryset))() — The original sync_to_async(list)(queryset) evaluated the queryset in the calling context before handing the result to the worker thread. The lambda form correctly defers evaluation. This appears in core_vector_stores.py:103, core_conversation_vector_stores.py:57, and core_agents.py:342 — all correct.

• Lambda closure capture (core_vector_stores.py:387): doc_pk = self.document_id before the lambda correctly captures the value at construction time rather than late-binding self.

• dataclasses.is_dataclass(usage) and not isinstance(usage, type) (pydantic_ai_agents.py:3101): Correctly restricts dataclasses.asdict() to instances only — is_dataclass() returns True for both the class and its instances.

• isinstance(ev, SourceEvent/ThoughtEvent/FinalEvent) replacing getattr(ev, "type", "") == "...": More robust and type-safe.

• agent_tasks.py:202: async for event in agent.stream(...) dropping the redundant await correctly matches the new non-coroutine Protocol definition.

• Null guards on llm_msg_id: The if llm_msg_id is not None: guards before _finalise_llm_message and complete_message prevent silent DB errors when persistence is disabled.

---

Minor Notes

• _resolve_framework() new setting LLMS_VECTOR_STORE_FRAMEWORK: Clean extraction of the repeated framework-resolution pattern. Worth documenting the new setting in the README or settings reference so operators know it exists.

• Unreachable yield for async generator typing (core_agents.py:673–679): The if False: yield cast(...) pattern is the standard idiom for this mypy constraint. The comment is clear; no change needed.

• _AgentVectorStoreProto Protocol (pydantic_ai_tools.py): Clean solution to the circular import. The k: int = ... default in the Protocol method is valid Python typing syntax.

• docs/typing/mypy_baseline.txt diff not reviewed here — if it shows a net reduction in suppressed errors across the graduated modules, that's exactly what's expected.

---

Summary

Approve with requests: The CorpusAgentContext.documents empty-list ambiguity (#1) and the redundant initialize() call in create_context() (#2) are the most consequential issues — they interact and could cause silent double-fetches on corpuses with zero documents. The permission guard tests (#3) are a follow-up-worthy gap. Everything else is clean and the thread-safety and null-safety fixes are genuinely valuable.

[claude]

Code Review: Type-check opencontractserver.llms modules (#1543)

This is a solid typing graduation PR. Removing 9 ignore_errors = True blocks is real progress, and several changes here are genuine bug fixes rather than pure annotation work. Overall the approach is sound — a few items worth addressing before merge.

---

Real Bug Fixes (worth calling out explicitly)

sync_to_async(list)(queryset) → sync_to_async(lambda: list(queryset))()
The original form evaluates the queryset iterator before entering the worker thread — the queryset is built in the async event loop where Django ORM calls raise SynchronousOnlyOperation. The lambda form defers evaluation to inside the thread pool. This is a genuine correctness fix in core_vector_stores.py, core_conversation_vector_stores.py, and core_agents.py. The regression test in test_no_naked_sync_to_async_list_callsites is a good belt-and-suspenders guard.

await agent.stream() → agent.stream() in agent_tasks.py
Since stream() is now a sync function returning AsyncGenerator (not a coroutine returning one), the await was consuming the generator object itself and then iterating over None. Correct fix.

isinstance(ev, SourceEvent/ThoughtEvent/FinalEvent) over getattr(ev, "type", "")
More robust — getattr would silently pass if an event object happened to lack a type attribute or renamed it.

doc_pk = self.document_id before lambda in _build_base_queryset
Correct closure capture fix. Without the local binding, the lambda captures self and mypy (and linters) can struggle to narrow self.document_id. The local also avoids late-binding surprises if self.document_id were mutable (it's not here, but good habit).

---

Issues

1. CorpusAgentContext.initialize() — semantic ambiguity with documents=[] (Medium)

# Before: if self.documents is None:
# After:  if not self.documents:

A caller that constructs CorpusAgentContext(corpus=..., config=..., documents=[]) intending to say "this agent needs no documents" will now silently trigger a corpus DB fetch. The docstring calls this out but marks it as a future concern ("callers that want to explicitly state 'no documents — skip loading' should not invoke initialize()"). This is a latent footgun — the pre-typing API made the intent explicit (None = unloaded, [] = intentionally empty); the new API collapses both into the same falsy branch.

Consider a sentinel or an explicit _documents_loaded: bool flag if the "intentionally empty" use case ever materialises. At minimum, a comment in the dataclass docstring would help future callers.

2. _stream_core(**kwargs) weakens the call-site API (Medium)

# Before:
async def _stream_core(self, message: str, *, force_llm_id: int | None = None, ...)

# After:
async def _stream_core(self, message: str, **kwargs: Any)

The explicit keyword parameters were IDE-discoverable and caught typos at type-check time. The **kwargs approach with runtime extraction means _stream_core(msg, force_llm_idd=123) fails at runtime with TypeError rather than at mypy time. The if kwargs: raise TypeError(...) guard is a good safety net but it only fires at runtime.

The PR comment explains this is needed for mixin compatibility. If the mixin's _stream_core signature must be **kwargs, consider at least documenting the accepted keys in the docstring (they are listed in the body, but not in the signature).

3. New LLMS_VECTOR_STORE_FRAMEWORK setting is undocumented (Low)

_resolve_framework(..., setting_name="LLMS_VECTOR_STORE_FRAMEWORK") introduces a new Django setting but neither the CHANGELOG entry, README, nor any settings documentation mentions it. An operator who reads the code can find it, but discovery is poor. The existing LLMS_DOCUMENT_AGENT_FRAMEWORK and LLMS_CORPUS_AGENT_FRAMEWORK settings also appear undocumented, but this PR adds a third one.

4. cast in TimelineBuilder.timeline is a runtime lie (Low)

self._timeline: list[dict[str, Any]] = []

@property
def timeline(self) -> list[TimelineEntry]:
    return cast("list[TimelineEntry]", self._timeline)

add() accepts Any and appends raw dicts — there is no validation that the dict actually matches the TimelineEntry TypedDict shape. The cast silences mypy at the call site but doesn't prevent e.g. a FinalEvent with a missing type key from producing a malformed TimelineEntry at runtime. The comment says "add() is the gatekeeper" but add() doesn't validate structure. This is fine as a pragmatic tradeoff, but consider a comment clarifying that TimelineEntry is a documentation hint rather than a runtime contract.

5. Permission guard tests don't exercise the guard path (Low)

TestPermissionGuardsOnAnnotationTools verifies the functions are importable and that the error string appears in source. It does not call the tool closure with user_id=None to verify the PermissionError actually raises. The PR author acknowledges the closures are hard to reach without a full agent.

A narrower test would be possible: extract the user_id is None guard into a small helper (e.g. _assert_authenticated(config)) and test that helper directly. This would decouple the permission contract from the full agent construction. Not required, but would make the guards feel properly pinned rather than linted.

Similarly, the context.corpus is None guard added to add_exact_string_annotations_tool is not counted in the assertGreaterEqual(guard_count, 3) check (that check is for "requires an authenticated user", not the corpus guard message). Consider adding a matching assertion for "requires the agent to be scoped to a corpus".

6. Constructor invariant not enforced at the entry point (Low)

In core_vector_stores.py:

# The constructor invariant above guarantees that at least one of
# ``corpus_id`` / ``embedder_path`` is provided

But the actual validation happens downstream (inside the else: if corpus_id is None: raise RuntimeError(...) branch). If a future refactor changes the constructor guard (wherever "above" refers to), the error from the downstream check will be confusing. A single guard at the top of __init__ (e.g., if corpus_id is None and embedder_path is None: raise ValueError(...)) would be clearer.

---

Correct design decisions worth keeping

• if False: yield cast(UnifiedStreamEvent, None) — the right idiomatic way to declare an abstract async generator without marking it @abstractmethod (which would complicate multiple inheritance with mixins). The placement after raise NotImplementedError means the if False block is unreachable at runtime but visible to mypy's control-flow analysis.
• _resolve_framework() helper — eliminates three identical framework-resolution blocks. The TypeError on unrecognised object types is a good fail-closed addition.
• getattr(agent, "agent_deps", None) double-getattr — safer than the previous getattr(agent.agent_deps, ...) which would raise AttributeError if the attribute didn't exist.
• resume_with_approval return type narrowed to AsyncGenerator — makes the API consistent; callers always async for regardless of approval outcome.
• AnnotationQuerySet return type on _build_base_queryset — correct; QuerySet[Annotation] doesn't expose .search_by_embedding() so mypy was right to reject the old annotation.

---

Minor nits

• test_llms_typing_coverage.py imports dataclasses but doesn't appear to use it directly (it's used transitively via CorpusAgentContext which is a @dataclass). Harmless but can be removed.
• The effective_history: Optional[list[Any]] annotation in _stream_core before the if block is correct, but the variable is assigned in both branches, so mypy would infer the type without the annotation. Worth keeping for readability, just noting it's not strictly needed.
• The long inline comment on the type: ignore[assignment] for async for event in tool_stream is well-justified. Keeping the narrow ignore is the right call.

[claude]

Code Review: PR #1543 — Type-check opencontractserver.llms modules

Overall: This is a well-executed typing graduation. The ~125 type errors are fixed correctly, the behavioral side-effects are documented, and the test file covers the key new runtime contracts. A few things worth discussing before merge.

---

What's Done Well

• _resolve_framework() refactor — Extracting the duplicated framework-resolution logic from three call sites into a single helper is a genuine DRY improvement. The new LLMS_VECTOR_STORE_FRAMEWORK setting is a clean addition that doesn't break existing deployments.

• isinstance() over getattr(ev, "type", "") == "..." string comparisons — The switch in pydantic_ai_agents.py for dispatching SourceEvent, ThoughtEvent, and FinalEvent is more robust and the right approach.

• dataclasses.is_dataclass(usage) and not isinstance(usage, type) — This is a real correctness fix, not just a typing fix. dataclasses.is_dataclass returns True for the class itself, so the original code would have called dataclasses.asdict(SomeClass) if a class object were passed, which would raise. The guard is correct.

• _AgentVectorStoreProto Protocol — Breaking the circular import between pydantic_ai_tools and pydantic_ai_vector_stores via a structural Protocol is the right architectural call. It keeps callers type-safe without forcing the concrete import.

• PermissionError guards before annotation mutations — The if config.user_id is None: raise PermissionError(...) guards before aadd_document_note, aduplicate_annotations_with_label, and aadd_annotations_from_exact_strings are a real security improvement per the CLAUDE.md IDOR prevention patterns.

• if self.user_id is None: raise RuntimeError(...) over assert — The comment explaining why assert is avoided (survives python -O) is precisely the right reasoning, and using an explicit raise is the correct approach for invariant enforcement.

• sync_to_async(lambda: list(queryset))() — The change from sync_to_async(list)(queryset) is important. sync_to_async(list) calls list() in a thread, but the queryset has already been partially set up in the async context. The lambda form closes over the queryset reference correctly.

---

Issues Worth Discussing

1. Semantic change in CorpusAgentContext.initialize() — empty-list edge case

# Before
if self.documents is None:
# After
if not self.documents:

The docstring correctly documents that callers "that want to explicitly state 'no documents — skip loading' should not invoke initialize()". But in create_context(), pre-loaded documents are passed via the constructor AND initialize() is still called afterward. This means the new behavior is: if documents=[] is passed explicitly (e.g., a corpus with no documents), initialize() will trigger a DB fetch that will also return [] — a wasted async DB round-trip. Not a bug, but if this pattern is expected, a docstring note or early-return guard in create_context() would clarify intent.

2. Test coverage gap for the annotation tool closures

TestPermissionGuardsOnAnnotationTools acknowledges that the guards live inside closures that can't be invoked without a full PydanticAI agent stack, so it uses source-grep to verify the error strings are present. That's a reasonable tradeoff, but the three "is importable" tests read as placeholders for real behavior tests. If a full agent mock is ever added to the test suite, these should be upgraded to actually call the closures with user_id=None and assert PermissionError.

3. int(self.corpus_id) coercions

In core_conversation_vector_stores.py:

queryset = queryset.filter(chat_with_corpus_id=int(self.corpus_id))

This correctly narrows str | int for the ORM lookup, but will throw ValueError if corpus_id is a non-integer string (e.g., a UUID). The existing code paths only reach this branch with numeric PKs, so this is fine in practice — but a type alias or earlier coercion to int in the constructor would be cleaner.

4. Minor: resolved_tools cast comment is verbose

# ``_resolve_tools`` returns ``list[CoreTool]``; the factory parameter
# is the wider ``list[Union[CoreTool, Callable, str]]`` so it can also
# accept un-resolved tool spec lists from other callers. ``list`` is
# invariant in the type system, so we cast through the wider alias.
resolved_tools: Optional[list[Union[CoreTool, Callable[..., Any], str]]] = (
    cast(...)
    if tools
    else None
)

This appears twice identically in api.py (for_document and for_corpus). The comment and cast could be extracted into a helper (e.g., _resolve_and_cast_tools(tools)) to keep it DRY, consistent with the project's DRY guideline and the _resolve_framework refactor pattern already in this PR.

5. CoreAgent.stream Protocol signature change

# Before
async def stream(self, message: str, **kwargs) -> AsyncGenerator[UnifiedStreamEvent, None]:
# After
def stream(self, message: str, **kwargs: Any) -> AsyncGenerator[UnifiedStreamEvent, None]:

Changing the Protocol method from async def to def is semantically correct — a method that returns an AsyncGenerator shouldn't itself be a coroutine — but this is a breaking protocol change for any external implementations of CoreAgent. The change is correct and the downstream fix in agent_tasks.py (removing the spurious await) confirms it was wrong before. Just worth noting for any third-party integrators.

---

No Security Issues Found

The new guards (PermissionError before annotation mutations, explicit RuntimeError for user_id invariants) improve the security posture. No new attack surfaces introduced.

---

Summary

Approve with minor suggestions. The type errors are fixed correctly, the behavioral changes are documented, and the runtime guards are an improvement. The main actionable items are:

1. Consider whether the for_document/for_corpus cast duplication in api.py warrants a helper (low priority).
2. Track the annotation tool closure tests as candidates for upgrade when a lighter agent mock becomes available.

The if not self.documents: semantic change and the int() corpus_id coercions are acceptable as-is given the documented invariants.