Merge pull request #2047 from Open-Source-Legal/claude/gifted-darwin-4e01ww

Address post-merge review of the bulk-trash primitive (#2039)

Author: JSv4

changelog.d/2039-review-followups.changed.md

@@ -0,0 +1,28 @@
+- **Post-merge review follow-ups for the bulk-trash primitive (#2039, follow-up
+  to #2030).** Documentation + test polish, no behavior change. This work landed
+  after the overlapping #2046 (review #2036) merged, so the
+  `bulk_soft_delete_documents` parts are reconciled with it: in
+  `opencontractserver/corpuses/services/lifecycle.py` the *memory* follow-up
+  references (the docstring "Scaling caveat" and the `PERF/MEMORY` comment) are
+  retargeted from the now-closed #1951 to the open #2045 — completing the #2039
+  point that the `(#1951)` forward-reference dangles once #1951 is closed (#2046
+  added the caveat but kept pointing at the closed #1951). The separate #2039
+  nit about `trashed_doc_ids` being read outside its `with` block is now moot:
+  #2046 moved the audit log into `transaction.on_commit` *inside* the block, so
+  there is no outside-block reference left to guard and no pre-initialisation is
+  needed. Both the module and class docstrings in
+  `opencontractserver/corpuses/services/paths.py` are corrected to describe the
+  mixed convention accurately — they previously claimed *all* methods were
+  underscore-prefixed, which the public `disambiguate_path` /
+  `reconcile_paths_after_folder_change` contradict — and record that the
+  underscore helpers (e.g. `_dispatch_document_path_created_signals`) are
+  deliberately shared across sibling services, while public `disambiguate_path`
+  is also called from the model layer (`Corpus.add_document`) and
+  `documents.versioning`. `mypy.ini`'s `python_version` comment is trimmed from
+  12 lines to 6 — dropping prose while keeping the numpy detail and the
+  dual-usage note (pre-commit hook + standalone CI `Run mypy` step) inline,
+  rather than cross-referencing the `2030-mypy-py312` changelog fragment (which
+  `collate_changelog.py --apply` deletes at release).
+  `test_query_count_independent_of_document_count` gains `msg=` strings on both
+  its query-count floor and the O(1) equality assertions so a future failure is
+  self-documenting.

mypy.ini

@@ -10,16 +10,12 @@
 # ---------------------------------------------------------------------------
 
 [mypy]
-# Must track the runtime Python the project actually ships
-# (``compose/*/django/Dockerfile`` -> ``ARG PYTHON_VERSION=3.12.*``). It was
-# stale at 3.11, which made mypy reject PEP 695 ``type`` statements that newer
-# dependency stubs now emit -- e.g. numpy 2.5.0's ``numpy/__init__.pyi`` ships
-# a ``type`` alias that errors with "Type statement is only supported in
-# Python 3.12 and greater" under a sub-3.12 target, aborting the whole run
-# ("errors prevented further checking"). numpy is an unpinned transitive dep,
-# so a fresh CI env resolves it to latest and any PR's linter breaks. Keeping
-# this in sync with the Dockerfile is the durable fix (it also covers the
-# standalone ``Run mypy`` CI step, not just the pre-commit hook).
+# Must match ARG PYTHON_VERSION in compose/*/django/Dockerfile (3.12.*); it
+# governs both the pre-commit ``mypy`` hook and the standalone ``Run mypy`` CI
+# step (.github/workflows/backend.yml). A stale sub-3.12 target makes mypy
+# reject PEP 695 ``type`` statements emitted by newer (unpinned) dependency
+# stubs (e.g. numpy >= 2.5.0's ``__init__.pyi``), aborting the whole run with
+# "errors prevented further checking".
 python_version = 3.12
 check_untyped_defs = True
 ignore_missing_imports = True

opencontractserver/corpuses/services/lifecycle.py

@@ -467,7 +467,8 @@ def bulk_soft_delete_documents(
         fully into Python lists, so there is **no built-in document-count
         ceiling**. That is fine for realistic corpus sizes, but chunk/iterate
         the doc set here before exposing an unbounded ``empty_corpus`` / folder
-        cascade-delete to arbitrarily large corpora (memory follow-up to #1951).
+        cascade-delete to arbitrarily large corpora (memory follow-up tracked
+        in #2045; #1951 was the now-closed query-count fix).
 
         NOTE: This is an internal primitive that performs **no permission
         check** — callers (``empty_corpus``, folder cascade-delete) must already
@@ -515,7 +516,7 @@ def bulk_soft_delete_documents(
             # PERF/MEMORY: this ``list()`` (plus the caller's ``document_ids``
             # snapshot) is the unbounded allocation point behind the docstring
             # "Scaling caveat" — chunk/iterate here to add a document-count
-            # ceiling for the empty_corpus / cascade-delete callers (#1951).
+            # ceiling for the empty_corpus / cascade-delete callers (#2045).
             active_paths = list(
                 DocumentPath.objects.select_for_update(of=("self",))
                 .select_related("document")

opencontractserver/corpuses/services/paths.py

@@ -1,11 +1,12 @@
-"""DocumentPath disambiguation internals for the corpus service layer.
+"""DocumentPath path-manipulation + signal-replay internals for the corpus service layer.
 
-``CorpusPathService`` holds the low-level :class:`DocumentPath` path
-manipulation helpers used by the folder write operations when documents are
-moved between folders or displaced by a folder deletion. Every method here is
-an internal helper (underscore-prefixed): the path layer performs NO
-permission checks — callers gate corpus permissions *before* reaching these
-helpers.
+``CorpusPathService`` holds the low-level :class:`DocumentPath` helpers used
+across the corpus/document layers when documents are moved between folders,
+displaced by a folder deletion, soft-deleted, or restored. The naming
+convention is mixed (a couple of methods are public, the rest are
+underscore-prefixed package-internal helpers) — see the class docstring for
+the distinction. No method here performs permission checks — callers gate
+corpus permissions *before* reaching these helpers.
 
 Split out of the former ``corpus_objs_service.py`` monolith — see
 ``docs/refactor_plans/2026-05-21-service-layer-phase2-corpus-services-plan.md``
@@ -37,15 +38,25 @@
 
 
 class CorpusPathService(BaseService):
-    """Low-level :class:`DocumentPath` disambiguation helpers.
-
-    All methods are internal helpers (underscore-prefixed) shared by the
-    folder write operations in
-    :class:`~opencontractserver.corpuses.services.folders.FolderCRUDService`
+    """Low-level :class:`DocumentPath` path-manipulation + signal-replay helpers.
+
+    Shared across the corpus/document layers; they perform NO permission
+    checks — the calling code gates corpus permissions first.
+
+    The naming convention is mixed. ``disambiguate_path`` and
+    ``reconcile_paths_after_folder_change`` are **public**: ``disambiguate_path``
+    is called throughout the corpus service layer and beyond it — e.g.
+    ``Corpus.add_document`` in the model layer and
+    ``documents.versioning.restore_document``. The remaining methods are
+    underscore-prefixed **package-internal** helpers — NOT class-private.
+    ``_dispatch_document_path_created_signals`` in particular is deliberately
+    invoked from the bulk write paths in sibling services
+    (:class:`~opencontractserver.corpuses.services.folders.FolderCRUDService`,
+    :class:`~opencontractserver.corpuses.services.folder_documents.FolderDocumentService`,
     and
-    :class:`~opencontractserver.corpuses.services.folder_documents.FolderDocumentService`.
-    They perform NO permission checks — the calling service is responsible
-    for gating corpus permissions first.
+    :class:`~opencontractserver.corpuses.services.lifecycle.DocumentLifecycleService`)
+    after each ``bulk_create`` of path rows, so a cross-service caller reaching
+    it is expected, not a leak.
     """
 
     @staticmethod

opencontractserver/tests/test_corpus_objs_service.py

@@ -1190,8 +1190,16 @@ def test_query_count_independent_of_document_count(self):
         # Floor so the equality below isn't vacuously satisfied by 0 == 0 (a
         # regression that silently skips all work): real trashing always runs at
         # least the SELECT-FOR-UPDATE + is_current UPDATE + bulk_create.
-        self.assertGreaterEqual(len(small_ctx), 3)
-        self.assertEqual(len(small_ctx), len(large_ctx))
+        self.assertGreaterEqual(
+            len(small_ctx),
+            3,
+            "Expected at least SELECT FOR UPDATE + is_current UPDATE + bulk_create",
+        )
+        self.assertEqual(
+            len(small_ctx),
+            len(large_ctx),
+            "Query count must not vary with document count (the O(1) guarantee)",
+        )
 
         # The ``is_public`` revocation branch is skipped for private corpora
         # above, so prove it is ALSO O(1) in the document count: the
@@ -1215,7 +1223,11 @@ def test_query_count_independent_of_document_count(self):
                 large_pub, large_pub_ids, self.owner
             )
 
-        self.assertEqual(len(small_pub_ctx), len(large_pub_ctx))
+        self.assertEqual(
+            len(small_pub_ctx),
+            len(large_pub_ctx),
+            "is_public revocation query count must not vary with document count",
+        )
         # The public path adds the (constant) is_public revocation cost on top
         # of the private path. Assert only that it is strictly greater — the
         # cross-size equality above is the real O(1) guarantee; a hardcoded delta