From 03887355935643fea375f18c3341a8f07e6a236c Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 11 Jun 2026 04:59:53 +0200 Subject: [PATCH 1/4] fix(vector): URL-encode DAV paths and unwrap TaskGroup exceptions Two ingest-robustness fixes from card 309 (OHR-Bench smoke-test triage). WebDAV paths flowed through the client already URL-decoded (unquote on the PROPFIND/REPORT , or raw MCP-tool input), so a '#' reached httpx as a URL fragment and silently truncated the request -> spurious 404 on otherwise valid files (e.g. law filenames with '#', commas, double/trailing spaces). Route every caller-path builder through a new _webdav_path helper that percent-encodes the path once (preserving separators); the MOVE/COPY Destination header is encoded too. Vector-sync runs inside anyio task groups, so a child-task failure surfaced as a BaseExceptionGroup whose str() is the useless "unhandled errors in a TaskGroup (N sub-exception)" -- hiding the real ConnectError operators need. Add format_exception_group to flatten the group to its leaf exceptions and use it at the broad catch/log sites in processor.py and oauth_sync.py. Refs: Deck board 12 card 309 (AC #4 filename handling, AC #2 observability). Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/client/webdav.py | 67 ++++++++++++------- nextcloud_mcp_server/vector/_errors.py | 36 +++++++++++ nextcloud_mcp_server/vector/oauth_sync.py | 16 +++-- nextcloud_mcp_server/vector/processor.py | 7 +- tests/unit/client/test_webdav.py | 78 +++++++++++++++++++++++ tests/unit/vector/test_errors.py | 41 ++++++++++++ 6 files changed, 216 insertions(+), 29 deletions(-) create mode 100644 nextcloud_mcp_server/vector/_errors.py create mode 100644 tests/unit/vector/test_errors.py diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index bceafacb..78ce64b2 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,37 @@ 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. + """ + 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 +65,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 +150,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 +170,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 +235,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 +279,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 +379,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 +406,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 +437,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 +495,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 +577,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 +1701,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..903be308 --- /dev/null +++ b/nextcloud_mcp_server/vector/_errors.py @@ -0,0 +1,36 @@ +"""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. +""" + +from __future__ import annotations + + +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. + """ + leaves = _flatten(exc) + if len(leaves) == 1 and leaves[0] is exc: + return repr(exc) + return f"{len(leaves)} sub-exception(s): " + "; ".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..6ce6b956 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, @@ -347,12 +348,15 @@ async def multi_user_processor_task( 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..3d48be6f 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, @@ -269,7 +270,7 @@ async def processor_task( worker_id, doc_task.doc_type, doc_task.doc_id, - e, + format_exception_group(e), exc_info=True, ) # Continue to next document (no task_done() needed with streams) @@ -443,7 +444,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 +461,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..396ced79 100644 --- a/tests/unit/client/test_webdav.py +++ b/tests/unit/client/test_webdav.py @@ -510,3 +510,81 @@ 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 +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 diff --git a/tests/unit/vector/test_errors.py b/tests/unit/vector/test_errors.py new file mode 100644 index 00000000..3336e03a --- /dev/null +++ b/tests/unit/vector/test_errors.py @@ -0,0 +1,41 @@ +"""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 "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-exception(s)" in formatted From a188e9fced9daf31055468032f411900b5406eec Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 11 Jun 2026 05:31:56 +0200 Subject: [PATCH 2/4] fix(vector): guard unbound doc_task + address review nits (#891) Round-1 review on PR #891: - Guard processor_task's broad except handler against an unbound doc_task (mirrors multi_user_processor_task): initialise doc_task=None before the loop and branch the error log. Fixes a latent NameError if receive() raises a non-TimeoutError/EndOfStream before the first document binds. Regression test added. - Drop the unnecessary `from __future__ import annotations` in vector/_errors.py and express format_exception_group's non-group fast path as an explicit isinstance check. - Add a copy_resource Destination-header encoding test (analogue to MOVE); strengthen the ExceptionGroup test to assert the full leaf repr survives. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/vector/_errors.py | 6 +-- nextcloud_mcp_server/vector/processor.py | 29 ++++++++++---- tests/unit/client/test_webdav.py | 20 ++++++++++ tests/unit/vector/test_errors.py | 3 ++ tests/unit/vector/test_processor_task.py | 50 ++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 12 deletions(-) create mode 100644 tests/unit/vector/test_processor_task.py diff --git a/nextcloud_mcp_server/vector/_errors.py b/nextcloud_mcp_server/vector/_errors.py index 903be308..6e526317 100644 --- a/nextcloud_mcp_server/vector/_errors.py +++ b/nextcloud_mcp_server/vector/_errors.py @@ -9,8 +9,6 @@ exceptions so log lines name the actual cause; pair it with ``exc_info=True`` to keep the full traceback. """ -from __future__ import annotations - def format_exception_group(exc: BaseException) -> str: """Return a concise, leaf-naming string for ``exc``. @@ -20,9 +18,9 @@ def format_exception_group(exc: BaseException) -> str: result is meant for the human-readable portion of a log message, not for parsing. """ - leaves = _flatten(exc) - if len(leaves) == 1 and leaves[0] is exc: + if not isinstance(exc, BaseExceptionGroup): return repr(exc) + leaves = _flatten(exc) return f"{len(leaves)} sub-exception(s): " + "; ".join(repr(e) for e in leaves) diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 3d48be6f..72172146 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -236,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) @@ -265,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, - format_exception_group(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) diff --git a/tests/unit/client/test_webdav.py b/tests/unit/client/test_webdav.py index 396ced79..b1268f2f 100644 --- a/tests/unit/client/test_webdav.py +++ b/tests/unit/client/test_webdav.py @@ -588,3 +588,23 @@ async def test_move_resource_encodes_destination_header(mocker): 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 index 3336e03a..f3f7f0e4 100644 --- a/tests/unit/vector/test_errors.py +++ b/tests/unit/vector/test_errors.py @@ -22,6 +22,9 @@ def test_format_exception_group_names_leaf_cause(): 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 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) From 7b274cd8e224929e4f0fd03fafe233a1ad44d414 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 11 Jun 2026 06:13:55 +0200 Subject: [PATCH 3/4] test(webdav): direct _webdav_path test + double-encode precondition (#891 r2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review on PR #891 (non-blocking): - Add a parametrised test_webdav_path_encoding covering empty path, leading-slash stripping, '#'/comma/space, and a non-ASCII name — the single source of truth for every caller-path builder's encoding, so write_file / delete_resource / create_directory / attachments are covered transitively. - Document the decoded-input precondition on _webdav_path (encode-exactly-once; passing an already-encoded path would double-encode). Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/client/webdav.py | 5 +++++ tests/unit/client/test_webdav.py | 23 +++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index 78ce64b2..19b654d4 100644 --- a/nextcloud_mcp_server/client/webdav.py +++ b/nextcloud_mcp_server/client/webdav.py @@ -54,6 +54,11 @@ class WebDAVClient(BaseNextcloudClient): 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('/'))}" diff --git a/tests/unit/client/test_webdav.py b/tests/unit/client/test_webdav.py index b1268f2f..cf8a3871 100644 --- a/tests/unit/client/test_webdav.py +++ b/tests/unit/client/test_webdav.py @@ -517,6 +517,29 @@ def _request_url(mock_http_client) -> str: 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 async def test_read_file_encodes_special_chars(mocker): """read_file must percent-encode '#', commas, and spaces in the path (card 309). From 801bf108faa406b87bfb9a1e9cf3724b47299400 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 11 Jun 2026 06:21:36 +0200 Subject: [PATCH 4/4] test(webdav): pin encode-once contract + nit cleanups (#891 r3) Round-3 review on PR #891 (no blockers): - Add test_encode_dav_path_encodes_exactly_once pinning the documented decoded-input precondition ("already%20encoded.pdf" -> "already%2520..."). - format_exception_group: proper singular/plural ("1 sub-exception" vs "N sub-exceptions") instead of "(s)". - oauth_sync: use `if doc_task is not None:` to match processor_task's guard. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/vector/_errors.py | 3 ++- nextcloud_mcp_server/vector/oauth_sync.py | 4 ++-- tests/unit/client/test_webdav.py | 9 +++++++++ tests/unit/vector/test_errors.py | 2 +- 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/nextcloud_mcp_server/vector/_errors.py b/nextcloud_mcp_server/vector/_errors.py index 6e526317..f2cddc79 100644 --- a/nextcloud_mcp_server/vector/_errors.py +++ b/nextcloud_mcp_server/vector/_errors.py @@ -21,7 +21,8 @@ def format_exception_group(exc: BaseException) -> str: if not isinstance(exc, BaseExceptionGroup): return repr(exc) leaves = _flatten(exc) - return f"{len(leaves)} sub-exception(s): " + "; ".join(repr(e) for e in leaves) + 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]: diff --git a/nextcloud_mcp_server/vector/oauth_sync.py b/nextcloud_mcp_server/vector/oauth_sync.py index 6ce6b956..6cf44564 100644 --- a/nextcloud_mcp_server/vector/oauth_sync.py +++ b/nextcloud_mcp_server/vector/oauth_sync.py @@ -332,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, @@ -342,7 +342,7 @@ 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, diff --git a/tests/unit/client/test_webdav.py b/tests/unit/client/test_webdav.py index cf8a3871..ffa7c6f2 100644 --- a/tests/unit/client/test_webdav.py +++ b/tests/unit/client/test_webdav.py @@ -540,6 +540,15 @@ def test_webdav_path_encoding(path, expected): 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). diff --git a/tests/unit/vector/test_errors.py b/tests/unit/vector/test_errors.py index f3f7f0e4..8c5dfe22 100644 --- a/tests/unit/vector/test_errors.py +++ b/tests/unit/vector/test_errors.py @@ -41,4 +41,4 @@ def test_format_nested_exception_group_flattens_all_leaves(): assert "ValueError" in formatted assert "ConnectError" in formatted assert "RuntimeError" in formatted - assert "3 sub-exception(s)" in formatted + assert "3 sub-exceptions" in formatted