Corpus file-tool review polish (PR #1940 follow-up)

changelog.d/1943-corpus-file-tools-followup.changed.md

@@ -0,0 +1,29 @@
+- **Corpus file-tool review polish (PR #1940 follow-up, issue #1943 items 3–5).**
+  - **`require_user` helper (`opencontractserver/llms/tools/core_tools/_helpers.py`).**
+    The 5-line "no user injected / id not found → `PermissionError`" guard that
+    `rename_document` and `delete_document` each repeated is now a single
+    `require_user(user_id, caller)` helper (companion to the existing
+    `get_user_or_none`), preserving the same per-caller error messages.
+    Read-only tools keep using `get_user_or_none` (which tolerates anonymous
+    access) (item 4).
+  - **`delete_document` tool description (`opencontractserver/llms/tools/tool_registry.py`).**
+    The description now spells out that deletion needs the corpus DELETE tier —
+    *higher* than the write permission `rename_document`/`move_document` use —
+    and instructs the LLM to report a `permission denied` failure rather than
+    retry-loop on it. A WRITE-but-not-DELETE user still passes the coarse
+    framework `requires_write_permission` gate and is correctly rejected by
+    `DocumentLifecycleService.soft_delete_document`; tightening this to a
+    dedicated `requires_delete_permission` flag (so the tool is filtered out
+    before such a user ever sees it) remains a tracked follow-up (item 3).
+  - **Direct unit test for `sanitize_corpus_filename`**
+    (`opencontractserver/tests/test_shared_utils.py`). The canonical
+    corpus-filename sanitiser was only exercised indirectly through the rename
+    integration paths; it now has a focused test covering empty/None →
+    fallback, all-special-chars → underscores, path-separator collapse (no
+    directory traversal), no run-collapsing, leading-dot preservation, and
+    truncation at `MAX_FILENAME_LENGTH` (item 5).
+  - **Not changed (item 2):** `CorpusDocumentService._check_document_in_corpus`
+    was kept private. It was deliberately renamed from public to underscore-
+    prefixed in PR #1685 to signal its "NO PERMISSION CHECK" contract; its
+    cross-service callers all live inside `corpuses.services` and gate corpus
+    READ upstream, so the leading underscore (service-internal use) is correct.

changelog.d/1943-corpus-file-tools-followup.fixed.md

@@ -0,0 +1,13 @@
+- **`rename_document` agent tool: dropped an extra query and closed a status race (PR #1940 follow-up, issue #1943 item 1).**
+  `FolderDocumentService.rename_document`
+  (`opencontractserver/corpuses/services/folder_documents.py`) now returns a
+  4-tuple `(success, error, new_path, changed)` — the new `changed` boolean is
+  `True` only for a real rename and `False` for a no-op (sanitised name already
+  matched) or any failure. The tool
+  (`opencontractserver/llms/tools/core_tools/documents.py`) previously
+  snapshotted the pre-rename `DocumentPath.path` and compared it to the
+  service's returned path to label the response `"renamed"` vs `"unchanged"`.
+  That extra `DocumentPath` read is gone, and so is the snapshot-vs-service race
+  window where a concurrent rename could mislabel the `status` (the underlying
+  data was always safe — the service's `select_for_update` guarantees that;
+  only the tool-layer status string could drift).

opencontractserver/corpuses/services/folder_documents.py

@@ -327,7 +327,7 @@ def rename_document(
         new_name: str,
         *,
         request: Any = None,
-    ) -> tuple[bool, str, str | None]:
+    ) -> tuple[bool, str, str | None, bool]:
         """Rename a document's file within a corpus (change the filename only).
 
         Creates a new immutable :class:`DocumentPath` history node whose
@@ -358,10 +358,14 @@ def rename_document(
             new_name: Desired new filename (last path segment only).
 
         Returns:
-            ``(success, error_message, new_path)``. ``new_path`` is the
+            ``(success, error_message, new_path, changed)``. ``new_path`` is the
             resulting path string on success (possibly disambiguated, e.g.
-            ``/Legal/Q3_Summary_1.pdf``) and ``None`` on failure. A no-op
-            rename (the name is unchanged) returns ``(True, "", <current path>)``.
+            ``/Legal/Q3_Summary_1.pdf``) and ``None`` on failure. ``changed`` is
+            ``True`` only when the filename actually changed and ``False`` for a
+            no-op (the sanitised name already matched the current filename) or
+            any failure — letting callers distinguish a real rename from a no-op
+            without re-reading the pre-rename path. A no-op rename returns
+            ``(True, "", <current path>, False)``.
 
         Validations:
             - User has corpus UPDATE permission
@@ -377,14 +381,15 @@ def rename_document(
                 False,
                 "Permission denied: You do not have write access to this corpus",
                 None,
+                False,
             )
 
         # Validate document belongs to corpus
         if not CorpusDocumentService._check_document_in_corpus(document, corpus):
-            return False, "Document does not belong to this corpus", None
+            return False, "Document does not belong to this corpus", None, False
 
         if not new_name or not new_name.strip():
-            return False, "New name must not be empty", None
+            return False, "New name must not be empty", None, False
 
         sanitized = sanitize_corpus_filename(new_name.strip())
 
@@ -407,7 +412,7 @@ def rename_document(
             )
 
             if not current:
-                return False, "No active document path found", None
+                return False, "No active document path found", None, False
 
             # Split the current path into "<directory>/" + "<filename>".
             # Stored paths always start with "/", so rpartition yields a
@@ -423,7 +428,7 @@ def rename_document(
 
             # No-op: the file already has this name.
             if new_filename == current_filename:
-                return True, "", current.path
+                return True, "", current.path, False
 
             base_path = f"{directory}{new_filename}"
 
@@ -436,7 +441,7 @@ def rename_document(
                     user=user,
                 )
             except ValueError as exc:
-                return False, str(exc), None
+                return False, str(exc), None, False
             except IntegrityError as exc:
                 logger.warning(
                     "IntegrityError renaming document %s in corpus %s after "
@@ -445,7 +450,7 @@ def rename_document(
                     corpus.id,
                     exc,
                 )
-                return False, f"{PATH_CONFLICT_MSG}, please retry: {exc}", None
+                return False, f"{PATH_CONFLICT_MSG}, please retry: {exc}", None, False
 
             logger.info(
                 "Renamed document %s in corpus %s from %r to %r by user %s",
@@ -455,7 +460,7 @@ def rename_document(
                 chosen_path,
                 user.id,
             )
-            return True, "", chosen_path
+            return True, "", chosen_path, True
 
     @classmethod
     def move_documents_to_folder(

opencontractserver/llms/tools/core_tools/_helpers.py

@@ -54,6 +54,31 @@ def get_user_or_none(user_id: int | None):
         return None
 
 
+def require_user(user_id: int | None, caller: str):
+    """Resolve ``user_id`` to a ``User`` or raise ``PermissionError``.
+
+    The write tools (e.g. ``rename_document`` / ``delete_document``) require an
+    authenticated, existing user. This collapses the two distinct guard
+    branches those tools repeated into one helper, preserving the same
+    ``caller``-specific messages for each failure mode:
+
+    * ``user_id is None`` (no user injected at all) →
+      ``"<caller> requires an authenticated user."``
+    * a stale/unknown id (no matching row) → ``"User <id> not found."``
+
+    Read-only tools that tolerate anonymous access keep using
+    :func:`get_user_or_none` instead (it returns ``None`` rather than raising).
+
+    Return type is intentionally inferred — see :func:`get_user_or_none`.
+    """
+    if user_id is None:
+        raise PermissionError(f"{caller} requires an authenticated user.")
+    user = get_user_or_none(user_id)
+    if user is None:
+        raise PermissionError(f"User {user_id} not found.")
+    return user
+
+
 def clamp_limit(limit: int | None, default: int, maximum: int) -> int:
     """Clamp a caller-supplied ``limit`` into ``[1, maximum]``.
 

opencontractserver/llms/tools/core_tools/documents.py

@@ -44,7 +44,12 @@
 from opencontractserver.corpuses.models import Corpus
 from opencontractserver.documents.models import Document, DocumentPath
 
-from ._helpers import _db_sync_to_async, clamp_limit, get_user_or_none
+from ._helpers import (
+    _db_sync_to_async,
+    clamp_limit,
+    get_user_or_none,
+    require_user,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -323,11 +328,7 @@ def rename_document(
         ValueError: If the document/corpus is not accessible or the rename
             fails.
     """
-    if user_id is None:
-        raise PermissionError("rename_document requires an authenticated user.")
-    user = get_user_or_none(user_id)
-    if user is None:
-        raise PermissionError(f"User {user_id} not found.")
+    user = require_user(user_id, "rename_document")
 
     corpus = Corpus.objects.visible_to_user(user).filter(pk=corpus_id).first()
     if corpus is None:
@@ -341,19 +342,13 @@ def rename_document(
             f"Document with id={document_id} does not exist or is not accessible."
         )
 
-    # Snapshot the pre-rename path so we can tell a real rename from a no-op
-    # (the service returns the unchanged path for both — see its docstring).
-    previous_path = (
-        DocumentPath.objects.filter(
-            document=document, corpus=corpus, is_current=True, is_deleted=False
-        )
-        .values_list("path", flat=True)
-        .first()
-    )
-
     from opencontractserver.corpuses.services import FolderDocumentService
 
-    success, error, new_path = FolderDocumentService.rename_document(
+    # The service reports whether the filename actually changed via ``changed``,
+    # so the tool no longer snapshots the pre-rename path to infer a no-op — this
+    # drops an extra query and closes the snapshot/service race that could mislabel
+    # a concurrent rename's status.
+    success, error, new_path, changed = FolderDocumentService.rename_document(
         user=user,
         document=document,
         corpus=corpus,
@@ -362,18 +357,16 @@ def rename_document(
     if not success:
         raise ValueError(f"Rename failed: {error}")
 
-    unchanged = new_path == previous_path
     return {
-        "status": "unchanged" if unchanged else "renamed",
+        "status": "renamed" if changed else "unchanged",
         "document_id": document_id,
         "corpus_id": corpus_id,
         "path": new_path,
         "message": (
-            f"Document {document_id} already named '{new_path}' in corpus "
+            f"Document {document_id} renamed to '{new_path}' in corpus {corpus_id}."
+            if changed
+            else f"Document {document_id} already named '{new_path}' in corpus "
             f"{corpus_id}; no change made."
-            if unchanged
-            else f"Document {document_id} renamed to '{new_path}' in "
-            f"corpus {corpus_id}."
         ),
     }
 
@@ -425,11 +418,7 @@ def delete_document(
         ValueError: If the document/corpus is not accessible or the delete
             fails.
     """
-    if user_id is None:
-        raise PermissionError("delete_document requires an authenticated user.")
-    user = get_user_or_none(user_id)
-    if user is None:
-        raise PermissionError(f"User {user_id} not found.")
+    user = require_user(user_id, "delete_document")
 
     corpus = Corpus.objects.visible_to_user(user).filter(pk=corpus_id).first()
     if corpus is None:

opencontractserver/llms/tools/tool_registry.py

@@ -588,7 +588,13 @@ def to_dict(self) -> dict:
             "Soft-delete a document from the current corpus (move it to the "
             "corpus trash). The document is NOT permanently erased — it keeps "
             "its history and can be restored from the corpus trash. Requires "
-            "delete permission on the corpus."
+            "the corpus DELETE permission, which is a HIGHER tier than the "
+            "write/update permission the other file tools (rename_document, "
+            "move_document) need: a user may be able to rename or move files "
+            "yet still lack delete rights. If this tool fails with a "
+            "'permission denied' error, the user cannot delete in this corpus "
+            "— report that and do NOT retry, as repeating the call will keep "
+            "failing."
         ),
         category=ToolCategory.CORPUS,
         requires_corpus=True,
@@ -598,8 +604,11 @@ def to_dict(self) -> dict:
         # the framework layer; the authoritative DELETE check lives in
         # ``DocumentLifecycleService.soft_delete_document`` (defense in depth),
         # so a WRITE-but-not-DELETE user passes this gate and is then correctly
-        # rejected by the service. If a ``requires_delete_permission`` flag is
-        # ever added, switch this entry to it.
+        # rejected by the service. The description above tells the LLM not to
+        # retry-loop on that rejection. Tightening this to a dedicated
+        # ``requires_delete_permission`` flag (so WRITE-only users never see the
+        # tool) is tracked as a near-term follow-up; switch this entry to it
+        # when that flag lands.
         requires_write_permission=True,
         parameters=(("document_id", "ID of the document to delete", True),),
     ),

opencontractserver/tests/test_shared_utils.py

@@ -9,6 +9,7 @@
 
 from django.test import TestCase, override_settings
 
+from opencontractserver.constants.document_processing import MAX_FILENAME_LENGTH
 from opencontractserver.shared.db_utils import table_has_column
 from opencontractserver.shared.defaults import (
     create_model_icon_path,
@@ -25,7 +26,10 @@
     sanitize_slug,
     validate_user_slug_or_raise,
 )
-from opencontractserver.shared.utils import calc_oc_file_path
+from opencontractserver.shared.utils import (
+    calc_oc_file_path,
+    sanitize_corpus_filename,
+)
 
 
 class TestDefaultFunctions(TestCase):
@@ -324,6 +328,65 @@ def test_filename_with_uuid(self):
         self.assertEqual(result, "uploadfiles/exports/abc-123-def.pdf")
 
 
+class TestSanitizeCorpusFilename(TestCase):
+    """Direct unit tests for shared/utils.py sanitize_corpus_filename.
+
+    This is the canonical sanitiser for the filename segment of a
+    ``DocumentPath.path`` (shared by ``Corpus.add_document``, the text-import
+    tool and ``FolderDocumentService.rename_document``). It is exercised
+    indirectly through the rename integration paths, but its invariants —
+    fallback on empty, special-chars-to-underscore, no directory traversal,
+    truncation, and leading-dot preservation — deserve an explicit test.
+    """
+
+    def test_allowed_chars_pass_through(self):
+        # Alphanumerics plus -, _ and . survive unchanged.
+        self.assertEqual(
+            sanitize_corpus_filename("Report-v2_final.pdf"), "Report-v2_final.pdf"
+        )
+
+    def test_empty_input_uses_fallback(self):
+        self.assertEqual(sanitize_corpus_filename(""), "untitled")
+
+    def test_none_input_uses_fallback(self):
+        # ``(name or "")`` coerces None before slicing, so None -> fallback too.
+        self.assertEqual(sanitize_corpus_filename(None), "untitled")
+
+    def test_custom_fallback(self):
+        self.assertEqual(sanitize_corpus_filename("", fallback="doc"), "doc")
+
+    def test_all_special_chars_become_underscores(self):
+        # Disallowed chars are replaced (never dropped), so an all-special name
+        # yields underscores, not the fallback.
+        self.assertEqual(sanitize_corpus_filename("!!!!"), "____")
+
+    def test_path_separators_collapse_to_underscore(self):
+        # Slashes (fwd and back) map to "_" so a sanitised name can never
+        # traverse directories.
+        self.assertEqual(sanitize_corpus_filename("a/b\\c"), "a_b_c")
+
+    def test_runs_are_not_collapsed(self):
+        # Each disallowed char maps to its own underscore (documented: runs are
+        # intentionally NOT collapsed to a single separator).
+        self.assertEqual(sanitize_corpus_filename("My  File"), "My__File")
+
+    def test_leading_dot_preserved(self):
+        # A leading dot is an allowed char, so dotfiles keep their dot.
+        self.assertEqual(sanitize_corpus_filename(".gitignore"), ".gitignore")
+
+    def test_truncated_to_max_length(self):
+        result = sanitize_corpus_filename("a" * (MAX_FILENAME_LENGTH + 50))
+        self.assertEqual(len(result), MAX_FILENAME_LENGTH)
+        self.assertEqual(result, "a" * MAX_FILENAME_LENGTH)
+
+    def test_truncation_happens_before_sanitisation(self):
+        # The raw input is sliced to MAX_FILENAME_LENGTH first, then each
+        # surviving char is mapped — so an over-long all-special string yields
+        # exactly MAX_FILENAME_LENGTH underscores, not more.
+        result = sanitize_corpus_filename("*" * (MAX_FILENAME_LENGTH + 10))
+        self.assertEqual(result, "_" * MAX_FILENAME_LENGTH)
+
+
 class TestVectorSearchMixinDimensionMapping(TestCase):
     """Tests for VectorSearchViaEmbeddingMixin._dimension_to_field mapping."""