diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index bceafacb..19b654d4 100644 --- a/nextcloud_mcp_server/client/webdav.py +++ b/nextcloud_mcp_server/client/webdav.py @@ -5,7 +5,7 @@ import mimetypes import xml.etree.ElementTree as ET from email.utils import parsedate_to_datetime from typing import Any, Dict, List, Optional, Tuple -from urllib.parse import unquote +from urllib.parse import quote, unquote from xml.sax.saxutils import escape as xml_escape from httpx import HTTPStatusError @@ -26,11 +26,42 @@ WEBDAV_SEARCH_PAGE_SIZE = 500 WEBDAV_SEARCH_MAX_RESULTS = 50000 +def _encode_dav_path(path: str) -> str: + """Percent-encode a *decoded* DAV path for use in a request URL/header. + + Paths flow through this client already URL-decoded (e.g. ``unquote`` on the + ```` of a PROPFIND/REPORT response, or raw user-supplied paths from + MCP tools), so characters like ``#``, ``,`` and spaces reach httpx verbatim. + An unencoded ``#`` is parsed as a URL fragment and silently truncates the + request path → spurious 404 on otherwise-valid files (issue: OHR-Bench + ingest, card 309). ``quote`` with ``safe="/"`` encodes the unsafe characters + while preserving the path separators; ASCII-only paths are unchanged. + + Encode exactly once: the input is decoded, so a literal ``%`` becomes + ``%25`` (correct) rather than being mistaken for an existing escape. + """ + return quote(path, safe="/") + + class WebDAVClient(BaseNextcloudClient): """Client for Nextcloud WebDAV operations.""" app_name = "webdav" + def _webdav_path(self, path: str) -> str: + """Build the request path for ``path`` under the user's DAV root. + + Percent-encodes the caller-supplied portion (see ``_encode_dav_path``) + so names with ``#``, commas, or spaces don't truncate/404; the base + ``/remote.php/dav/files/`` segment is left as-is. + + Precondition: ``path`` is a **decoded** path (the convention everywhere + in this client — PROPFIND/REPORT hrefs are ``unquote``d before storage, + and MCP-tool inputs are raw). It is encoded exactly once, so passing an + already-encoded path would double-encode it (``%20`` → ``%2520``). + """ + return f"{self._get_webdav_base_path()}/{_encode_dav_path(path.lstrip('/'))}" + async def delete_resource(self, path: str) -> Dict[str, Any]: """Delete a resource (file or directory) via WebDAV DELETE.""" # Ensure path ends with a slash if it's a directory @@ -39,7 +70,7 @@ class WebDAVClient(BaseNextcloudClient): else: path_with_slash = path - webdav_path = f"{self._get_webdav_base_path()}/{path_with_slash.lstrip('/')}" + webdav_path = self._webdav_path(path_with_slash) logger.debug("Deleting WebDAV resource: %s", webdav_path) headers = {"OCS-APIRequest": "true"} @@ -124,15 +155,15 @@ class WebDAVClient(BaseNextcloudClient): mime_type: Optional[str] = None, ) -> Dict[str, Any]: """Add/Update an attachment to a note via WebDAV PUT.""" - # Construct paths based on provided category - webdav_base = self._get_webdav_base_path() + # Construct paths based on provided category. Encode via _webdav_path so + # categories/filenames with '#', commas or spaces don't truncate/404. category_path_part = f"{category}/" if category else "" attachment_dir_segment = f".attachments.{note_id}" parent_dir_webdav_rel_path = ( f"Notes/{category_path_part}{attachment_dir_segment}" ) - parent_dir_path = f"{webdav_base}/{parent_dir_webdav_rel_path}" - attachment_path = f"{parent_dir_path}/{filename}" + parent_dir_path = self._webdav_path(parent_dir_webdav_rel_path) + attachment_path = self._webdav_path(f"{parent_dir_webdav_rel_path}/{filename}") logger.debug("Uploading attachment '%s' for note %s", filename, note_id) @@ -144,7 +175,7 @@ class WebDAVClient(BaseNextcloudClient): headers = {"Content-Type": mime_type, "OCS-APIRequest": "true"} try: # First check if we can access WebDAV at all - notes_dir_path = f"{webdav_base}/Notes" + notes_dir_path = self._webdav_path("Notes") propfind_headers = {"Depth": "0", "OCS-APIRequest": "true"} notes_dir_response = await self._make_request( "PROPFIND", notes_dir_path, headers=propfind_headers @@ -209,10 +240,11 @@ class WebDAVClient(BaseNextcloudClient): self, note_id: int, filename: str, category: Optional[str] = None ) -> Tuple[bytes, str]: """Fetch a specific attachment from a note via WebDAV GET.""" - webdav_base = self._get_webdav_base_path() category_path_part = f"{category}/" if category else "" attachment_dir_segment = f".attachments.{note_id}" - attachment_path = f"{webdav_base}/Notes/{category_path_part}{attachment_dir_segment}/{filename}" + attachment_path = self._webdav_path( + f"Notes/{category_path_part}{attachment_dir_segment}/{filename}" + ) logger.debug("Fetching attachment '%s' for note %s", filename, note_id) @@ -252,7 +284,7 @@ class WebDAVClient(BaseNextcloudClient): async def list_directory(self, path: str = "") -> List[Dict[str, Any]]: """List files and directories in the specified path via WebDAV PROPFIND.""" - webdav_path = f"{self._get_webdav_base_path()}/{path.lstrip('/')}" + webdav_path = self._webdav_path(path) if not webdav_path.endswith("/"): webdav_path += "/" @@ -352,7 +384,7 @@ class WebDAVClient(BaseNextcloudClient): async def read_file(self, path: str) -> Tuple[bytes, str]: """Read a file's content via WebDAV GET.""" - webdav_path = f"{self._get_webdav_base_path()}/{path.lstrip('/')}" + webdav_path = self._webdav_path(path) logger.debug("Reading file: %s", path) @@ -379,7 +411,7 @@ class WebDAVClient(BaseNextcloudClient): self, path: str, content: bytes, content_type: Optional[str] = None ) -> Dict[str, Any]: """Write content to a file via WebDAV PUT.""" - webdav_path = f"{self._get_webdav_base_path()}/{path.lstrip('/')}" + webdav_path = self._webdav_path(path) logger.debug("Writing file: %s", path) @@ -410,7 +442,7 @@ class WebDAVClient(BaseNextcloudClient): self, path: str, recursive: bool = False ) -> Dict[str, Any]: """Create a directory via WebDAV MKCOL.""" - webdav_path = f"{self._get_webdav_base_path()}/{path.lstrip('/')}" + webdav_path = self._webdav_path(path) if not webdav_path.endswith("/"): webdav_path += "/" @@ -468,10 +500,8 @@ class WebDAVClient(BaseNextcloudClient): Returns: Dict with status_code and optional message """ - source_webdav_path = f"{self._get_webdav_base_path()}/{source_path.lstrip('/')}" - destination_webdav_path = ( - f"{self._get_webdav_base_path()}/{destination_path.lstrip('/')}" - ) + source_webdav_path = self._webdav_path(source_path) + destination_webdav_path = self._webdav_path(destination_path) # Ensure paths have consistent trailing slashes for directories if source_path.endswith("/") and not destination_path.endswith("/"): @@ -552,10 +582,8 @@ class WebDAVClient(BaseNextcloudClient): Returns: Dict with status_code and optional message """ - source_webdav_path = f"{self._get_webdav_base_path()}/{source_path.lstrip('/')}" - destination_webdav_path = ( - f"{self._get_webdav_base_path()}/{destination_path.lstrip('/')}" - ) + source_webdav_path = self._webdav_path(source_path) + destination_webdav_path = self._webdav_path(destination_path) # Ensure paths have consistent trailing slashes for directories if source_path.endswith("/") and not destination_path.endswith("/"): @@ -1678,7 +1706,7 @@ class WebDAVClient(BaseNextcloudClient): distinguish a definitive absence (HTTP 404) from a brittle response (None). """ - webdav_path = f"{self._get_webdav_base_path()}/{path.lstrip('/')}" + webdav_path = self._webdav_path(path) propfind_body = """ diff --git a/nextcloud_mcp_server/vector/_errors.py b/nextcloud_mcp_server/vector/_errors.py new file mode 100644 index 00000000..f2cddc79 --- /dev/null +++ b/nextcloud_mcp_server/vector/_errors.py @@ -0,0 +1,35 @@ +"""Error-formatting helpers for the vector-sync pipeline. + +Vector-sync work runs inside anyio task groups, so a failure in a child task +surfaces as a ``BaseExceptionGroup`` whose default ``str()`` is the useless +``"unhandled errors in a TaskGroup (N sub-exception)"`` -- it hides the real +``ConnectError`` / ``APIConnectionError`` that operators need to triage embed +drops (card 309). ``format_exception_group`` flattens the group to the leaf +exceptions so log lines name the actual cause; pair it with ``exc_info=True`` to +keep the full traceback. +""" + + +def format_exception_group(exc: BaseException) -> str: + """Return a concise, leaf-naming string for ``exc``. + + For a (possibly nested) ``BaseExceptionGroup`` this joins the ``repr`` of + each leaf exception; for an ordinary exception it returns its ``repr``. The + result is meant for the human-readable portion of a log message, not for + parsing. + """ + if not isinstance(exc, BaseExceptionGroup): + return repr(exc) + leaves = _flatten(exc) + noun = "sub-exception" if len(leaves) == 1 else "sub-exceptions" + return f"{len(leaves)} {noun}: " + "; ".join(repr(e) for e in leaves) + + +def _flatten(exc: BaseException) -> list[BaseException]: + """Depth-first list of the leaf exceptions within ``exc``.""" + if isinstance(exc, BaseExceptionGroup): + leaves: list[BaseException] = [] + for sub in exc.exceptions: + leaves.extend(_flatten(sub)) + return leaves + return [exc] diff --git a/nextcloud_mcp_server/vector/oauth_sync.py b/nextcloud_mcp_server/vector/oauth_sync.py index eb4f1bb0..6cf44564 100644 --- a/nextcloud_mcp_server/vector/oauth_sync.py +++ b/nextcloud_mcp_server/vector/oauth_sync.py @@ -30,6 +30,7 @@ from httpx import BasicAuth, HTTPStatusError from nextcloud_mcp_server.auth.storage import RefreshTokenStorage from nextcloud_mcp_server.client import NextcloudClient from nextcloud_mcp_server.config import get_settings +from nextcloud_mcp_server.vector._errors import format_exception_group from nextcloud_mcp_server.vector.processor import process_document from nextcloud_mcp_server.vector.queue.ports import TaskProducer from nextcloud_mcp_server.vector.scanner import DocumentTask, scan_user_documents @@ -257,7 +258,7 @@ async def user_scanner_task( logger.error( "[BasicAuth] Scanner error for %s: %s (%s/%s)", user_id, - e, + format_exception_group(e), consecutive_errors, max_consecutive_errors, exc_info=True, @@ -331,7 +332,7 @@ async def multi_user_processor_task( break except NotProvisionedError: - if doc_task: + if doc_task is not None: logger.warning( "[BasicAuth] User %s not provisioned, skipping %s_%s", doc_task.user_id, @@ -341,18 +342,21 @@ async def multi_user_processor_task( continue except Exception as e: - if doc_task: + if doc_task is not None: logger.error( "[BasicAuth] Processor %s error processing %s_%s: %s", worker_id, doc_task.doc_type, doc_task.doc_id, - e, + format_exception_group(e), exc_info=True, ) else: logger.error( - "[BasicAuth] Processor %s error: %s", worker_id, e, exc_info=True + "[BasicAuth] Processor %s error: %s", + worker_id, + format_exception_group(e), + exc_info=True, ) finally: @@ -481,7 +485,11 @@ async def user_manager_task( logger.info("[BasicAuth] Stopped %s scanner(s)", len(revoked_users)) except Exception as e: - logger.error("[BasicAuth] User manager error: %s", e, exc_info=True) + logger.error( + "[BasicAuth] User manager error: %s", + format_exception_group(e), + exc_info=True, + ) # Sleep until next poll try: diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 38c04b33..72172146 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -31,6 +31,7 @@ from nextcloud_mcp_server.observability.tracing import trace_operation from nextcloud_mcp_server.search.pdf_highlighter import PDFHighlighter from nextcloud_mcp_server.usage import UsageEventStore from nextcloud_mcp_server.vector import payload_keys +from nextcloud_mcp_server.vector._errors import format_exception_group from nextcloud_mcp_server.vector.document_chunker import ( DocumentChunker, PageAwareChunker, @@ -235,6 +236,11 @@ async def processor_task( # Signal that the task has started and is ready task_status.started() + # Initialised before the loop so the broad except handler below can't hit an + # unbound name if receive() itself raises a non-TimeoutError/EndOfStream + # exception on the very first iteration (mirrors multi_user_processor_task). + doc_task: DocumentTask | None = None + while not shutdown_event.is_set(): try: # Get document with timeout (allows checking shutdown) @@ -264,14 +270,22 @@ async def processor_task( break except Exception as e: - logger.error( - "Processor %s error processing %s_%s: %s", - worker_id, - doc_task.doc_type, - doc_task.doc_id, - e, - exc_info=True, - ) + if doc_task is not None: + logger.error( + "Processor %s error processing %s_%s: %s", + worker_id, + doc_task.doc_type, + doc_task.doc_id, + format_exception_group(e), + exc_info=True, + ) + else: + logger.error( + "Processor %s error: %s", + worker_id, + format_exception_group(e), + exc_info=True, + ) # Continue to next document (no task_done() needed with streams) logger.info("Processor %s stopped", worker_id) @@ -443,7 +457,7 @@ async def process_document( max_retries, doc_task.doc_type, doc_task.doc_id, - e, + format_exception_group(e), extra={ "doc_id": doc_task.doc_id, "doc_type": doc_task.doc_type, @@ -460,7 +474,7 @@ async def process_document( doc_task.doc_type, doc_task.doc_id, max_retries, - e, + format_exception_group(e), extra={ "doc_id": doc_task.doc_id, "doc_type": doc_task.doc_type, diff --git a/tests/unit/client/test_webdav.py b/tests/unit/client/test_webdav.py index c02144e0..ffa7c6f2 100644 --- a/tests/unit/client/test_webdav.py +++ b/tests/unit/client/test_webdav.py @@ -510,3 +510,133 @@ def test_parse_search_response_decodes_non_ascii_paths(mocker): assert results[0]["href"] == "/remote.php/dav/files/testuser/学生邮箱/report.pdf" # name comes from , which is not URL-encoded; sanity-check it. assert results[0]["name"] == "report.pdf" + + +def _request_url(mock_http_client) -> str: + """Positional URL passed to the underlying httpx ``request`` call.""" + return mock_http_client.request.call_args[0][1] + + +@pytest.mark.unit +@pytest.mark.parametrize( + "path, expected", + [ + ("", "/remote.php/dav/files/testuser/"), + ("/Documents/notes.txt", "/remote.php/dav/files/testuser/Documents/notes.txt"), + ("Documents/notes.txt", "/remote.php/dav/files/testuser/Documents/notes.txt"), + ("a/b #1.pdf", "/remote.php/dav/files/testuser/a/b%20%231.pdf"), + ("law/x, y z.pdf", "/remote.php/dav/files/testuser/law/x%2C%20y%20%20z.pdf"), + ( + "学生邮箱/r.pdf", + "/remote.php/dav/files/testuser/%E5%AD%A6%E7%94%9F%E9%82%AE%E7%AE%B1/r.pdf", + ), + ], +) +def test_webdav_path_encoding(path, expected): + """_webdav_path encodes the decoded caller path once, preserving '/', and + strips a leading slash. Every caller-path builder routes through this, so + it is the single source of truth for their encoding.""" + client = WebDAVClient(AsyncMock(), "testuser") + assert client._webdav_path(path) == expected + + +@pytest.mark.unit +def test_encode_dav_path_encodes_exactly_once(): + """Pins the decoded-input precondition: a literal '%' becomes '%25', so an + already-encoded path passed in error would double-encode (caught here).""" + from nextcloud_mcp_server.client.webdav import _encode_dav_path + + assert _encode_dav_path("already%20encoded.pdf") == "already%2520encoded.pdf" + + +@pytest.mark.unit +async def test_read_file_encodes_special_chars(mocker): + """read_file must percent-encode '#', commas, and spaces in the path (card 309). + + Paths arrive already URL-decoded from PROPFIND/REPORT, so an unencoded '#' + reaches httpx as a URL fragment and silently truncates the request → 404 on + valid files (e.g. OHR-Bench law filenames). The outgoing request path must be + percent-encoded. + """ + mock_http_client = AsyncMock() + client = WebDAVClient(mock_http_client, "testuser") + + mock_response = AsyncMock() + mock_response.content = b"%PDF-1.4 data" + mock_response.headers = {"content-type": "application/pdf"} + mock_response.raise_for_status = mocker.Mock() + mock_http_client.request = AsyncMock(return_value=mock_response) + + # Name with a '#', a comma, a double space and a trailing space before ".pdf". + await client.read_file("law/ADMA BioManufacturing, LLC - Amendment #2 .pdf") + + url = _request_url(mock_http_client) + assert url.startswith("/remote.php/dav/files/testuser/") + # The hazardous characters are encoded; path separators are preserved. + assert "%23" in url # '#' + assert "%2C" in url # ',' + assert "%20" in url # space + assert "#" not in url + assert ", " not in url + assert "/law/" in url + + +@pytest.mark.unit +async def test_read_file_ascii_path_unchanged(mocker): + """A plain ASCII path must pass through unchanged (no spurious encoding).""" + mock_http_client = AsyncMock() + client = WebDAVClient(mock_http_client, "testuser") + + mock_response = AsyncMock() + mock_response.content = b"data" + mock_response.headers = {"content-type": "text/plain"} + mock_response.raise_for_status = mocker.Mock() + mock_http_client.request = AsyncMock(return_value=mock_response) + + await client.read_file("Documents/notes.txt") + + assert ( + _request_url(mock_http_client) + == "/remote.php/dav/files/testuser/Documents/notes.txt" + ) + + +@pytest.mark.unit +async def test_move_resource_encodes_destination_header(mocker): + """The MOVE Destination header must be percent-encoded too (card 309).""" + mock_http_client = AsyncMock() + client = WebDAVClient(mock_http_client, "testuser") + + mock_response = AsyncMock() + mock_response.status_code = 201 + mock_response.raise_for_status = mocker.Mock() + mock_http_client.request = AsyncMock(return_value=mock_response) + + await client.move_resource("a/old.pdf", "b/new #1.pdf") + + call = mock_http_client.request.call_args + # Source is the request path; destination is the header. + assert call[0][1] == "/remote.php/dav/files/testuser/a/old.pdf" + destination = call.kwargs["headers"]["Destination"] + assert "%23" in destination + assert "#" not in destination + + +@pytest.mark.unit +async def test_copy_resource_encodes_destination_header(mocker): + """The COPY Destination header must be percent-encoded too (card 309).""" + mock_http_client = AsyncMock() + client = WebDAVClient(mock_http_client, "testuser") + + mock_response = AsyncMock() + mock_response.status_code = 201 + mock_response.raise_for_status = mocker.Mock() + mock_http_client.request = AsyncMock(return_value=mock_response) + + await client.copy_resource("a/old.pdf", "b/new #1.pdf") + + call = mock_http_client.request.call_args + assert call[0][1] == "/remote.php/dav/files/testuser/a/old.pdf" + destination = call.kwargs["headers"]["Destination"] + assert "%23" in destination + assert "#" not in destination diff --git a/tests/unit/vector/test_errors.py b/tests/unit/vector/test_errors.py new file mode 100644 index 00000000..8c5dfe22 --- /dev/null +++ b/tests/unit/vector/test_errors.py @@ -0,0 +1,44 @@ +"""Unit tests for vector-sync error formatting (card 309).""" + +import httpx +import pytest + +from nextcloud_mcp_server.vector._errors import format_exception_group + + +@pytest.mark.unit +def test_format_plain_exception_returns_repr(): + exc = httpx.ConnectError("Connection error") + assert format_exception_group(exc) == repr(exc) + + +@pytest.mark.unit +def test_format_exception_group_names_leaf_cause(): + """A single-child group must surface the real ConnectError, not the group's + useless 'unhandled errors in a TaskGroup' default message.""" + leaf = httpx.ConnectError("Connection error") + group = BaseExceptionGroup("unhandled errors in a TaskGroup", [leaf]) + + formatted = format_exception_group(group) + + assert "ConnectError" in formatted + # Assert the full leaf repr survives, not just the type name -- guards a + # future format change that kept the type but dropped the message. + assert repr(leaf) in formatted + assert "unhandled errors in a TaskGroup" not in formatted + assert "1 sub-exception" in formatted + + +@pytest.mark.unit +def test_format_nested_exception_group_flattens_all_leaves(): + inner = BaseExceptionGroup( + "inner", [ValueError("bad value"), httpx.ConnectError("conn")] + ) + outer = BaseExceptionGroup("outer", [inner, RuntimeError("boom")]) + + formatted = format_exception_group(outer) + + assert "ValueError" in formatted + assert "ConnectError" in formatted + assert "RuntimeError" in formatted + assert "3 sub-exceptions" in formatted diff --git a/tests/unit/vector/test_processor_task.py b/tests/unit/vector/test_processor_task.py new file mode 100644 index 00000000..8276a073 --- /dev/null +++ b/tests/unit/vector/test_processor_task.py @@ -0,0 +1,50 @@ +"""Regression test for processor_task's exception handler (card 309 / PR #891). + +If ``receive_stream.receive()`` raises something other than +``TimeoutError``/``EndOfStream`` before any document is bound, the broad +``except`` handler must not crash on an unbound ``doc_task`` name. +""" + +from unittest.mock import MagicMock + +import anyio +import pytest + +from nextcloud_mcp_server.vector.processor import processor_task + + +class _ReceiveBoomThenEnd: + """First receive() raises a non-Timeout error (no doc_task bound yet); the + second ends the stream so the loop exits.""" + + def __init__(self, shutdown: anyio.Event): + self._calls = 0 + self._shutdown = shutdown + + async def receive(self): + self._calls += 1 + if self._calls == 1: + raise RuntimeError("transport blew up before any document") + self._shutdown.set() + raise anyio.EndOfStream + + def statistics(self): # pragma: no cover - not reached on the error path + return MagicMock(current_buffer_used=0) + + +@pytest.mark.unit +async def test_processor_task_receive_error_does_not_raise_unbound(caplog): + shutdown = anyio.Event() + stream = _ReceiveBoomThenEnd(shutdown) + + # Must complete without a NameError leaking out of the except handler. + with caplog.at_level("ERROR", logger="nextcloud_mcp_server.vector.processor"): + await processor_task( + worker_id=0, + receive_stream=stream, # type: ignore[arg-type] + shutdown_event=shutdown, + nc_client=MagicMock(), + user_id="alice", + ) + + assert any("RuntimeError" in rec.message for rec in caplog.records)