Merge pull request #891 from cbcoutinho/fix/309-dav-encoding-exceptiongroup

fix(vector): URL-encode DAV paths and unwrap TaskGroup exceptions

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
+    ``<d:href>`` 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/<user>`` 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
+        # Construct paths based on provided category. Encode via _webdav_path so
-        webdav_base = self._get_webdav_base_path()
+        # 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}"
+        parent_dir_path = self._webdav_path(parent_dir_webdav_rel_path)
-        attachment_path = f"{parent_dir_path}/{filename}"
+        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('/')}"
+        source_webdav_path = self._webdav_path(source_path)
-        destination_webdav_path = (
+        destination_webdav_path = self._webdav_path(destination_path)
-            f"{self._get_webdav_base_path()}/{destination_path.lstrip('/')}"
-        )
 
         # 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('/')}"
+        source_webdav_path = self._webdav_path(source_path)
-        destination_webdav_path = (
+        destination_webdav_path = self._webdav_path(destination_path)
-            f"{self._get_webdav_base_path()}/{destination_path.lstrip('/')}"
-        )
 
         # 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 = """<?xml version="1.0"?>
 <d:propfind xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">

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]

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:

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(
+            if doc_task is not None:
-                "Processor %s error processing %s_%s: %s",
+                logger.error(
-                worker_id,
+                    "Processor %s error processing %s_%s: %s",
-                doc_task.doc_type,
+                    worker_id,
-                doc_task.doc_id,
+                    doc_task.doc_type,
-                e,
+                    doc_task.doc_id,
-                exc_info=True,
+                    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,

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 <d:displayname>, 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

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

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)