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 <d:href>, 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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
457c115ef4
commit
0388735593
@@ -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
|
||||
``<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.
|
||||
"""
|
||||
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 = """<?xml version="1.0"?>
|
||||
<d:propfind xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">
|
||||
|
||||
@@ -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]
|
||||
@@ -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:
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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 <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
|
||||
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
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user