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

[JSv4]

Closes #1943.

Addresses the PR #1940 code-review follow-up. Surgical changes scoped to the corpus file-management agent tools and the shared sanitiser.

Item-by-item

✅ Item 1 — extra DB read + status race in rename_document (fixed)

FolderDocumentService.rename_document (opencontractserver/corpuses/services/folder_documents.py) now returns a (success, error, new_path, changed) 4-tuple. The new changed boolean is True only for a real rename and False for a no-op (sanitised name already matched the current filename) or any failure.

The tool (opencontractserver/llms/tools/core_tools/documents.py) previously snapshotted the pre-rename DocumentPath.path and diffed it against 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 status. (Data integrity was never at risk — the service's select_for_update guarantees that; only the tool-layer status string could drift.)

✅ Item 3 — delete_document WRITE‑vs‑DELETE gate (description update — the issue's stated minimum)

The delete_document description (tool_registry.py) 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 (defense in depth). Tightening this to a dedicated requires_delete_permission flag (so the tool is filtered out before such a user ever sees it) is left as the tracked near-term follow-up the issue itself proposed.

✅ Item 4 — repeated user-guard (fixed, DRY)

Added require_user(user_id, caller) to core_tools/_helpers.py (companion to the existing get_user_or_none). It collapses the 5‑line "no user injected / id not found → PermissionError" block that rename_document and delete_document each repeated, preserving the exact same per-caller error messages. Read-only tools keep using get_user_or_none (which tolerates anonymous access).

✅ Item 5 — direct unit test for sanitize_corpus_filename (added)

New TestSanitizeCorpusFilename in opencontractserver/tests/test_shared_utils.py covering: empty/None → fallback, custom fallback, all‑special‑chars → underscores, path‑separator collapse (no directory traversal), runs not collapsed, leading‑dot preservation, and truncation at MAX_FILENAME_LENGTH (incl. truncate‑before‑sanitise).

⚠️ Item 2 — intentionally not actioned (with rationale)

The review suggested making CorpusDocumentService._check_document_in_corpus public. I deliberately kept it private: it was renamed from public → underscore‑prefixed in PR #1685 (documented in CHANGELOG.md) specifically to signal its "NO PERMISSION CHECK" contract. Its cross-service callers (lifecycle.py, folder_documents.py) all live inside the corpuses.services package and gate corpus READ upstream, so the leading underscore (service-internal, footgun-out-of-autocomplete) is the correct state. Reversing it would undo a security-conscious decision. The issue's own summary lists item 2 as a non-blocking follow-up.

Verification

• black, flake8, isort (profile=black) clean on all changed files.
• sanitize_corpus_filename assertions validated standalone (all 11 pass).
• Existing test_corpus_file_tools.py traced against the new 4‑tuple / require_user paths — all assertions remain consistent (status, path, and PermissionError/ValueError messages unchanged).

Note: the full Django test suite could not be executed in this environment — the test image build needs PyPI access, which the session's network policy blocks. Please run test_shared_utils.py and test_corpus_file_tools.py in CI/locally before merge.

Changelog fragments: changelog.d/1943-corpus-file-tools-followup.{fixed,changed}.md.

---

Generated by Claude Code

[claude]

Code Review

Overview

This PR delivers surgical follow-up fixes from the PR #1940 review across four areas: eliminating an extra DB read + race condition in rename_document, DRY-ing up a repeated user-guard pattern, improving the delete_document tool description, and adding direct unit tests for sanitize_corpus_filename. The PR description is detailed and the rationale for not actioning item 2 is well-documented.

---

Positives

• 4-tuple return is the right fix. The previous snapshot-vs-service pattern was a real race: concurrent renames inside the select_for_update window could make the tool emit a misleading "renamed" status for what was actually a no-op, and vice versa. Pushing the changed flag into the service is the correct layer separation.
• require_user is a clean, minimal helper. Its docstring clearly articulates the two distinct failure modes and why read-only tools keep using get_user_or_none. The implementation is 5 lines with no branching surprises.
• Test coverage is solid. TestSanitizeCorpusFilename covers the documented invariants — fallback-on-empty, special-chars-to-underscore, no-run-collapse, no-directory-traversal, leading-dot, truncation ordering. The existing test_corpus_file_tools.py already exercises the user_id=None and stale-id paths through require_user transitively.
• Item 2 decision is correct. Keeping _check_document_in_corpus private preserves the deliberate security signal from PR Refactor: Split DocumentService and CorpusObjsService for cleaner architecture #1685.
• DocumentPath import not orphaned. After removing the pre-rename snapshot query, DocumentPath is still referenced at line 122 of documents.py, so the import is clean.

---

Issues / Nits

Minor — sanitize_corpus_filename type hint accepts None but doesn't declare it

The function signature is sanitize_corpus_filename(name: str, ...) but test_none_input_uses_fallback calls it with None and the implementation handles it via (name or ""). This is a type-hint lie — callers reading the signature don't know None is safe, and static analysers will flag the test call. One-line fix:

def sanitize_corpus_filename(name: str | None, *, fallback: str = "untitled") -> str:

Nit — require_user "user not found" message omits the caller name

The first error includes the caller ("{caller} requires an authenticated user.") but the second doesn't ("User {user_id} not found."). Since caller is already threaded through, making the second consistent ("{caller}: User {user_id} not found.") would help triage agent logs. The PR description explicitly calls out message preservation as intentional, so this is purely advisory.

Nit — changed=False is ambiguous at the service boundary

changed is False for both "a no-op" and "any failure" — callers must check success first to distinguish. The tool does this correctly, and the docstring warns about it. Worth flagging for anyone adding a new caller directly against the service.

---

What's Not in Scope (acknowledged)

• Item 3 (requires_delete_permission flag): Correctly tracked as near-term follow-up. The improved LLM description is a reasonable interim mitigation against retry-loop behavior.
• Other write tools using get_user_or_none directly (e.g. move_document): Candidates for the same require_user migration in a follow-up.

---

Verdict

Approve with minor nits. The core fixes are correct and well-reasoned. The one substantive nit — updating sanitize_corpus_filename to str | None given the new test explicitly calls it with None — is worth a quick fix before merge. The other nits are cosmetic and advisory.