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) <noreply@anthropic.com>

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
 

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(

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(