Address PR #1380 review: model_override allowlist + doc clarifications

- Add a runtime allowlist guard for ``model_override`` in
  ``doc_extract_query_task``. When the optional Django setting
  ``BENCHMARK_ALLOWED_MODEL_OVERRIDES`` is unset (default), no
  enforcement runs — preserves operator-only workflows while giving
  operators a no-code-change path to lock down this surface if the task
  is ever exposed to untrusted input. Rejected overrides mark the
  Datacell as failed with a clear stacktrace and re-raise so celery
  workers log the violation.
- Update the merge-from-main fix in ``test_tool_approval_gate``: the
  earlier refactor of ``_get_function_tools`` to use the public
  ``agent.toolsets`` API broke the test mock that exposed the old
  private ``_function_tools`` attribute. Mock now exposes a real
  ``FunctionToolset`` via ``inst.toolsets`` so all four approval-flow
  tests pass on this branch.
- Document the failure-mode convention on ``Datacell.stacktrace`` (the
  field name implies unstructured exception text but we also persist
  the structured ``failure_mode=`` lines that ``_classify_none_result``
  produces — operators ``grep failure_mode=`` to triage).
- Document reranker cache invalidation semantics in
  ``get_default_reranker_instance`` (DB-write busts the cache; in-memory
  test patches don't — set ``STRICT_RERANKER`` or call
  ``invalidate_reranker_cache`` explicitly).
- Document TLS verification posture on ``MicroserviceReranker`` (relies
  on system trust store, no per-instance opt-out).
- Document one-shot semantics on the 0037 migration so operators don't
  expect re-running ``migrate`` to re-seed an already-set value.
- Add ``ModelOverrideAllowlistTests`` covering the unknown-model
  rejection path end-to-end.

Author: JSv4

opencontractserver/documents/migrations/0037_add_default_reranker_to_pipeline_settings.py

@@ -7,6 +7,12 @@ def seed_default_reranker(apps, schema_editor):
 
     Intentionally a no-op when ``DEFAULT_RERANKER`` is not defined, so
     existing deployments keep reranking disabled until an operator opts in.
+
+    One-shot semantics: re-running ``migrate`` after a value has already been
+    persisted will NOT re-seed it (the existing value is preserved by the
+    ``not instance.default_reranker`` guard). Operators changing rerankers
+    should update via the admin / pipeline settings UI, not by re-running
+    this migration.
     """
     PipelineSettings = apps.get_model("documents", "PipelineSettings")
     initial = getattr(django_settings, "DEFAULT_RERANKER", "")

opencontractserver/pipeline/rerankers/microservice_reranker.py

@@ -56,7 +56,12 @@
 
 
 class MicroserviceReranker(BaseReranker):
-    """Reranker that delegates to an external HTTP microservice."""
+    """Reranker that delegates to an external HTTP microservice.
+
+    TLS: HTTPS endpoints are verified via the system trust store
+    (``requests`` defaults to ``verify=True``). Self-signed/internal CAs
+    must be trusted at the OS level — there is no per-instance opt-out.
+    """
 
     title = "Microservice Reranker"
     description = (

opencontractserver/pipeline/utils.py

@@ -613,6 +613,12 @@ def get_default_reranker_instance(
     in one worker must not pin that worker to degraded behaviour while
     siblings continue reranking successfully. See the module-level comment
     for the rationale.
+
+    Cache invalidation: the cache key includes ``PipelineSettings.modified``,
+    so DB writes bust it process-wide. Tests that patch settings purely
+    in-memory will hit stale instances — set ``STRICT_RERANKER`` (which
+    bypasses the cache fast-path) or call :func:`invalidate_reranker_cache`
+    explicitly if you need a fresh instance from a fixture.
     """
     from django.conf import settings as django_settings
 

opencontractserver/tasks/data_extract_tasks.py

@@ -106,18 +106,22 @@ async def doc_extract_query_task(
             default extraction model. Used by the benchmark runner to sweep
             models without touching production defaults.
 
-            Trust assumption: this string is passed straight to the agent
-            factory and ultimately to the model registry.  Current call
+            Trust boundary: this string is passed straight to the agent
+            factory and ultimately to the model registry. Current call
             sites (CLI ``run_benchmark`` command, internal benchmark
-            runner) are operator-controlled.  If this task is ever
-            exposed to user-controlled input (webhook, public API), gate
-            it behind an allowlist of approved model identifiers — an
-            arbitrary string here can redirect extraction traffic to an
+            runner) are operator-controlled. The optional Django setting
+            ``BENCHMARK_ALLOWED_MODEL_OVERRIDES`` (iterable of allowed
+            identifiers) gates this parameter at runtime — by default it is
+            unset, meaning no enforcement (operator-only path). Operators
+            exposing this task to user-controlled input (webhook, public
+            API) must set the allowlist to lock down the surface so an
+            arbitrary string cannot redirect extraction traffic to an
             unintended model endpoint.
     """
     import traceback
     from typing import get_origin
 
+    from django.conf import settings
     from django.utils import timezone
     from pydantic import BaseModel
     from pydantic_ai import capture_run_messages
@@ -156,7 +160,15 @@ def sync_mark_completed(dc, data_dict, llm_log=None):
 
     @sync_to_async
     def sync_mark_failed(dc, exc, tb, llm_log=None):
-        """Mark datacell as failed with error and optional LLM log."""
+        """Mark datacell as failed with error and optional LLM log.
+
+        Convention: ``Datacell.stacktrace`` is the only persisted text field
+        for failure context, so we use it for both real exception
+        tracebacks AND the structured ``failure_mode=`` lines that
+        ``_classify_none_result`` produces for None outcomes. Operators
+        ``grep failure_mode=`` to separate legitimate "data not present"
+        outcomes from pipeline bugs.
+        """
         dc.stacktrace = f"Error: {exc}\n\nTraceback:\n{tb}"
         dc.failed = timezone.now()
         if llm_log:
@@ -188,6 +200,19 @@ def sync_get_corpus_id(document):
         await sync_mark_started(datacell)
         logger.info(f"Marked datacell {cell_id} as started")
 
+        # Optional allowlist guard for ``model_override``. When
+        # ``BENCHMARK_ALLOWED_MODEL_OVERRIDES`` is unset (default), no
+        # enforcement runs — preserves operator-only workflows while
+        # giving operators a no-code-change path to lock down this
+        # surface if the task is ever exposed to untrusted input.
+        if model_override is not None:
+            allowed = getattr(settings, "BENCHMARK_ALLOWED_MODEL_OVERRIDES", None)
+            if allowed is not None and model_override not in allowed:
+                raise ValueError(
+                    f"model_override {model_override!r} is not in "
+                    f"BENCHMARK_ALLOWED_MODEL_OVERRIDES"
+                )
+
         document = datacell.document
         column = datacell.column
         logger.info(f"Document: {document.id}, Column: {column.name}")

opencontractserver/tests/test_data_extract_helpers.py

@@ -323,5 +323,65 @@ def test_score_count_mismatch_pads_with_neg_inf(self) -> None:
         self.assertEqual(results[1].score, float("-inf"))
 
 
+class ModelOverrideAllowlistTests(TestCase):
+    """``BENCHMARK_ALLOWED_MODEL_OVERRIDES`` guard fires before any Datacell
+    work runs, so a rejected override marks the cell as failed without
+    touching the agent runtime.
+    """
+
+    def setUp(self) -> None:
+        self.user = User.objects.create_user(
+            username="allowlist_user", password="testpass"
+        )
+        corpus = Corpus.objects.create(title="Allowlist Corpus", creator=self.user)
+        document = Document.objects.create(
+            title="Allowlist Doc", creator=self.user, file_type="text/plain"
+        )
+        corpus.add_document(document=document, user=self.user)
+        fieldset = Fieldset.objects.create(name="fs", creator=self.user)
+        column = Column.objects.create(
+            fieldset=fieldset,
+            name="col",
+            query="anything",
+            output_type="str",
+            creator=self.user,
+        )
+        extract = Extract.objects.create(
+            corpus=corpus, fieldset=fieldset, name="ex", creator=self.user
+        )
+        self.cell = Datacell.objects.create(
+            extract=extract,
+            column=column,
+            document=document,
+            data_definition="x",
+            creator=self.user,
+        )
+
+    def test_unknown_model_override_marks_cell_failed(self) -> None:
+        from django.test import override_settings
+
+        from opencontractserver.tasks.data_extract_tasks import (
+            doc_extract_query_task,
+        )
+
+        with override_settings(
+            BENCHMARK_ALLOWED_MODEL_OVERRIDES=["openai:gpt-4o-mini"]
+        ):
+            # The task re-raises after marking the cell failed; the
+            # operator-facing celery worker logs the error and the cell
+            # carries the explanation in its stacktrace for ops review.
+            with self.assertRaises(ValueError):
+                doc_extract_query_task.si(
+                    self.cell.id, model_override="anthropic:not-allowed"
+                ).apply().get()
+
+        self.cell.refresh_from_db()
+        self.assertIsNotNone(self.cell.failed)
+        self.assertIn(
+            "BENCHMARK_ALLOWED_MODEL_OVERRIDES",
+            self.cell.stacktrace or "",
+        )
+
+
 # Suppress unused-import warning for the SimpleNamespace shim used elsewhere
 _ = SimpleNamespace

opencontractserver/tests/test_tool_approval_gate.py

@@ -199,14 +199,22 @@ async def _run_side_effect(*_a, **_kw):
         inst.run = AsyncMock(side_effect=_run_side_effect)
         inst.iter = MagicMock(return_value=_IterCtx())
 
-        # Provide registry entry so resume_with_approval can execute tool
+        # Provide registry entry so resume_with_approval can execute tool.
+        # ``_get_function_tools`` now reads from the public ``agent.toolsets``
+        # API and only consumes ``FunctionToolset`` instances, so the mock
+        # exposes a real FunctionToolset whose ``tools`` dict carries the
+        # stub functions keyed by name.
+        from pydantic_ai.toolsets import FunctionToolset
+
         async def _approved_tool(ctx, x: int):  # noqa: D401 – minimal stub
             return x * 2
 
-        inst._function_tools = {
+        toolset = FunctionToolset()
+        toolset.tools = {
             "approved_tool": types.SimpleNamespace(function=_approved_tool),
             "second_gate_tool": types.SimpleNamespace(function=_approved_tool),
         }
+        inst.toolsets = [toolset]
         mock_cls.return_value = inst
 
     # ------------------------------------------------------------------