From e5ad625a6654c9a080d43a8ce566a4bbd114ca9b Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 25 Mar 2026 08:07:37 +0100 Subject: [PATCH] fix: address PR review feedback for Collectives support - Validate OCS envelope status before unwrapping data (raise OCSError on statuscode >= 400) - Fix test data: filePath should be "" for root-level pages, not filename - Catch specific exceptions (HTTPStatusError, OSError) instead of bare Exception in WebDAV content fetch, include error in log message - Return updated resource data from update_collective, move_page, and set_page_emoji instead of discarding API responses - Fix create_page docstring to mention collectivePath/filePath/fileName - Remove unused additional_headers parameter from _get_ocs_headers - Add unit test for OCS error status validation Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/client/collectives.py | 31 ++++++++++++----- nextcloud_mcp_server/server/collectives.py | 34 +++++++++++-------- .../collectives/test_collectives_api.py | 26 ++++++++++++-- 3 files changed, 66 insertions(+), 25 deletions(-) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index 2366cbaf..cb23ebe4 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -1,33 +1,46 @@ """Client for Nextcloud Collectives app API (OCS).""" +import logging from typing import Any from nextcloud_mcp_server.client.base import BaseNextcloudClient +logger = logging.getLogger(__name__) + API_BASE = "/ocs/v2.php/apps/collectives/api/v1.0" +class OCSError(Exception): + """Error returned in the OCS response envelope.""" + + def __init__(self, status_code: int, message: str): + self.status_code = status_code + self.message = message + super().__init__(f"OCS error {status_code}: {message}") + + class CollectivesClient(BaseNextcloudClient): """Client for Nextcloud Collectives app operations.""" app_name = "collectives" - def _get_ocs_headers( - self, additional_headers: dict[str, str] | None = None - ) -> dict[str, str]: + def _get_ocs_headers(self) -> dict[str, str]: """Get standard headers required for OCS API calls.""" - headers = { + return { "OCS-APIRequest": "true", "Content-Type": "application/json", "Accept": "application/json", } - if additional_headers: - headers.update(additional_headers) - return headers def _unwrap_ocs(self, response_json: dict[str, Any]) -> Any: - """Unwrap OCS envelope, returning the data payload.""" - return response_json["ocs"]["data"] + """Unwrap OCS envelope, validating the status before returning data.""" + ocs = response_json["ocs"] + meta = ocs.get("meta", {}) + status_code = meta.get("statuscode", 200) + if status_code >= 400: + message = meta.get("message", "OCS error") + raise OCSError(status_code, message) + return ocs["data"] # Collectives diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index 0c40b49a..cdea6a33 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -2,6 +2,7 @@ import logging +from httpx import HTTPStatusError from mcp.server.fastmcp import Context, FastMCP from mcp.types import ToolAnnotations @@ -80,8 +81,8 @@ def configure_collectives_tools(mcp: FastMCP): """Get a page's metadata and markdown content from a Nextcloud Collective. Content is fetched via WebDAV using the page's file path. To update - page content, use the nc_webdav_write_file tool with the path from - the page's collectivePath/filePath fields. + page content, use the nc_webdav_write_file tool with the path + collectivePath/filePath/fileName (omit filePath for root-level pages). Args: collective_id: ID of the collective @@ -104,10 +105,11 @@ def configure_collectives_tools(mcp: FastMCP): try: file_bytes, _ = await client.webdav.read_file(webdav_path) content = file_bytes.decode("utf-8") - except Exception: + except (HTTPStatusError, OSError) as e: logger.warning( - "Failed to read page content via WebDAV: %s", + "Failed to read page content via WebDAV: %s: %s", webdav_path, + e, ) return GetPageResponse(page=page, content=content) @@ -217,11 +219,12 @@ def configure_collectives_tools(mcp: FastMCP): emoji: New emoji for the collective """ client = await get_client(ctx) - await client.collectives.update_collective(collective_id, emoji) + raw = await client.collectives.update_collective(collective_id, emoji) + collective = Collective(**raw) return CollectiveOperationResponse( - collective_id=collective_id, + collective_id=collective.id, status_code=200, - message="Collective updated", + message=f"Collective updated (emoji: {collective.emoji})", ) @mcp.tool( @@ -236,7 +239,8 @@ def configure_collectives_tools(mcp: FastMCP): """Create a new page in a Nextcloud Collective. Pages are created as empty markdown files. Use nc_webdav_write_file - with the page's collectivePath/filePath to add content after creation. + with the path collectivePath/filePath/fileName to add content after + creation (omit filePath for root-level pages). Args: collective_id: ID of the collective @@ -279,15 +283,16 @@ def configure_collectives_tools(mcp: FastMCP): copy: If true, copy instead of move """ client = await get_client(ctx) - await client.collectives.move_page( + raw = await client.collectives.move_page( collective_id, page_id, parent_id, title, index, copy ) + page = PageInfo(**raw) action = "copied" if copy else "moved" return PageOperationResponse( - page_id=page_id, + page_id=page.id, collective_id=collective_id, status_code=200, - message=f"Page {action}", + message=f"Page {action} (title: {page.title}, parent: {page.parentId})", ) @mcp.tool( @@ -360,12 +365,13 @@ def configure_collectives_tools(mcp: FastMCP): emoji: Emoji to set, or null to clear """ client = await get_client(ctx) - await client.collectives.set_page_emoji(collective_id, page_id, emoji) + raw = await client.collectives.set_page_emoji(collective_id, page_id, emoji) + page = PageInfo(**raw) return PageOperationResponse( - page_id=page_id, + page_id=page.id, collective_id=collective_id, status_code=200, - message="Page emoji updated", + message=f"Page emoji updated (emoji: {page.emoji})", ) @mcp.tool( diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py index 56adea5d..5bdc3ca4 100644 --- a/tests/client/collectives/test_collectives_api.py +++ b/tests/client/collectives/test_collectives_api.py @@ -3,7 +3,7 @@ import httpx import pytest -from nextcloud_mcp_server.client.collectives import CollectivesClient +from nextcloud_mcp_server.client.collectives import CollectivesClient, OCSError from tests.client.conftest import create_mock_response pytestmark = pytest.mark.unit @@ -46,7 +46,7 @@ def _sample_page( "title": title, "emoji": None, "fileName": f"{title}.md", - "filePath": f"{title}.md", + "filePath": "", "collectivePath": collective_path, "parentId": parent_id, "timestamp": 1700000000, @@ -327,6 +327,28 @@ async def test_restore_page(mocker): # --- Error Handling --- +async def test_ocs_error_status_raises(mocker): + """Test that OCS envelope with error statuscode raises OCSError.""" + mock_response = create_mock_response( + status_code=200, + json_data={ + "ocs": { + "meta": { + "status": "failure", + "statuscode": 403, + "message": "Not permitted", + }, + "data": {}, + } + }, + ) + mocker.patch.object(CollectivesClient, "_make_request", return_value=mock_response) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + with pytest.raises(OCSError, match="Not permitted"): + await client.get_collectives() + + async def test_get_collectives_403(mocker): """Test 403 response raises HTTPStatusError.""" mock_response = create_mock_response(