From 32f5a0fe52e6a4d1c4fd645d556ce9125dc94546 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 25 Mar 2026 07:53:50 +0100 Subject: [PATCH 01/11] feat: add Nextcloud Collectives app support (#621) Implement MCP tools for the Collectives wiki/documentation app, enabling agentic workflows for team knowledge base management. 16 tools covering collectives, pages, tags, search, and trash: - Read: list collectives, list/get pages (with WebDAV content), search, list tags, list trashed pages - Write: create/update collective, create/move/trash/restore pages, set emoji, create/assign/remove tags Includes Docker hook for app installation, OCS API client with envelope unwrapping, Pydantic models, unit tests (16), and integration tests (10). Closes #621 Co-Authored-By: Claude Opus 4.6 (1M context) --- .../10-install-collectives-app.sh | 35 ++ nextcloud_mcp_server/app.py | 2 + nextcloud_mcp_server/client/__init__.py | 2 + nextcloud_mcp_server/client/collectives.py | 233 ++++++++++ nextcloud_mcp_server/models/auth.py | 2 + nextcloud_mcp_server/models/collectives.py | 135 ++++++ nextcloud_mcp_server/server/__init__.py | 2 + nextcloud_mcp_server/server/collectives.py | 440 ++++++++++++++++++ tests/client/collectives/__init__.py | 0 .../collectives/test_collectives_api.py | 372 +++++++++++++++ tests/server/test_collectives_mcp.py | 412 ++++++++++++++++ 11 files changed, 1635 insertions(+) create mode 100755 app-hooks/post-installation/10-install-collectives-app.sh create mode 100644 nextcloud_mcp_server/client/collectives.py create mode 100644 nextcloud_mcp_server/models/collectives.py create mode 100644 nextcloud_mcp_server/server/collectives.py create mode 100644 tests/client/collectives/__init__.py create mode 100644 tests/client/collectives/test_collectives_api.py create mode 100644 tests/server/test_collectives_mcp.py diff --git a/app-hooks/post-installation/10-install-collectives-app.sh b/app-hooks/post-installation/10-install-collectives-app.sh new file mode 100755 index 00000000..7d96fab3 --- /dev/null +++ b/app-hooks/post-installation/10-install-collectives-app.sh @@ -0,0 +1,35 @@ +#!/bin/bash + +set -euox pipefail + +echo "Installing and configuring collectives app for testing..." + +# Collectives depends on Circles (teams) - ensure it's enabled +# Circles is bundled with Nextcloud, so just enable it +php /var/www/html/occ app:enable circles + +# Check if development collectives app is mounted at /opt/apps/collectives +if [ -d /opt/apps/collectives ]; then + echo "Development collectives app found at /opt/apps/collectives" + + # Remove any existing collectives app in apps (from app store or old symlink) + if [ -e /var/www/html/custom_apps/collectives ]; then + echo "Removing existing collectives in apps..." + rm -rf /var/www/html/custom_apps/collectives + fi + + # Create symlink from apps to the mounted development version + # Per Nextcloud docs: apps outside server root need symlinks in server root + echo "Creating symlink: custom_apps/collectives -> /opt/apps/collectives" + ln -sf /opt/apps/collectives /var/www/html/custom_apps/collectives + + echo "Enabling collectives app from /opt/apps (development mode via symlink)" + php /var/www/html/occ app:enable collectives +elif [ -d /var/www/html/custom_apps/collectives ]; then + echo "collectives app directory found in apps (already installed)" + php /var/www/html/occ app:enable collectives +else + echo "collectives app not found, installing from app store..." + php /var/www/html/occ app:install collectives + php /var/www/html/occ app:enable collectives +fi diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 733de031..e7f2f0c3 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -117,6 +117,7 @@ from nextcloud_mcp_server.observability.metrics import ( ) from nextcloud_mcp_server.server import ( configure_calendar_tools, + configure_collectives_tools, configure_contacts_tools, configure_cookbook_tools, configure_deck_tools, @@ -1274,6 +1275,7 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = "webdav": configure_webdav_tools, "sharing": configure_sharing_tools, "calendar": configure_calendar_tools, + "collectives": configure_collectives_tools, "contacts": configure_contacts_tools, "cookbook": configure_cookbook_tools, "deck": configure_deck_tools, diff --git a/nextcloud_mcp_server/client/__init__.py b/nextcloud_mcp_server/client/__init__.py index cd0aec4c..4b567efb 100644 --- a/nextcloud_mcp_server/client/__init__.py +++ b/nextcloud_mcp_server/client/__init__.py @@ -14,6 +14,7 @@ from httpx import ( from ..controllers.notes_search import NotesSearchController from ..http import nextcloud_httpx_transport from .calendar import CalendarClient +from .collectives import CollectivesClient from .contacts import ContactsClient from .cookbook import CookbookClient from .deck import DeckClient @@ -81,6 +82,7 @@ class NextcloudClient: ) # Uses AsyncDavClient internally self.contacts = ContactsClient(self._client, username) self.cookbook = CookbookClient(self._client, username) + self.collectives = CollectivesClient(self._client, username) self.deck = DeckClient(self._client, username) self.news = NewsClient(self._client, username) self.users = UsersClient(self._client, username) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py new file mode 100644 index 00000000..2366cbaf --- /dev/null +++ b/nextcloud_mcp_server/client/collectives.py @@ -0,0 +1,233 @@ +"""Client for Nextcloud Collectives app API (OCS).""" + +from typing import Any + +from nextcloud_mcp_server.client.base import BaseNextcloudClient + +API_BASE = "/ocs/v2.php/apps/collectives/api/v1.0" + + +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]: + """Get standard headers required for OCS API calls.""" + headers = { + "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"] + + # Collectives + + async def get_collectives(self) -> list[dict[str, Any]]: + """List all collectives the user has access to.""" + response = await self._make_request( + "GET", f"{API_BASE}/collectives", headers=self._get_ocs_headers() + ) + data = self._unwrap_ocs(response.json()) + return data["collectives"] + + async def create_collective( + self, name: str, emoji: str | None = None + ) -> dict[str, Any]: + """Create a new collective.""" + json_data: dict[str, Any] = {"name": name} + if emoji is not None: + json_data["emoji"] = emoji + response = await self._make_request( + "POST", + f"{API_BASE}/collectives", + json=json_data, + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["collective"] + + async def update_collective( + self, collective_id: int, emoji: str | None = None + ) -> dict[str, Any]: + """Update a collective (emoji).""" + json_data: dict[str, Any] = {} + if emoji is not None: + json_data["emoji"] = emoji + response = await self._make_request( + "PUT", + f"{API_BASE}/collectives/{collective_id}", + json=json_data, + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["collective"] + + # Pages + + async def get_pages(self, collective_id: int) -> list[dict[str, Any]]: + """List all pages in a collective.""" + response = await self._make_request( + "GET", + f"{API_BASE}/collectives/{collective_id}/pages", + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["pages"] + + async def get_page(self, collective_id: int, page_id: int) -> dict[str, Any]: + """Get a single page's metadata.""" + response = await self._make_request( + "GET", + f"{API_BASE}/collectives/{collective_id}/pages/{page_id}", + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["page"] + + async def create_page( + self, collective_id: int, parent_id: int, title: str + ) -> dict[str, Any]: + """Create a new page under a parent page.""" + json_data = {"title": title} + response = await self._make_request( + "POST", + f"{API_BASE}/collectives/{collective_id}/pages/{parent_id}", + json=json_data, + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["page"] + + async def move_page( + self, + collective_id: int, + page_id: int, + parent_id: int | None = None, + title: str | None = None, + index: int = 0, + copy: bool = False, + ) -> dict[str, Any]: + """Move or copy a page within a collective.""" + json_data: dict[str, Any] = {"index": index, "copy": copy} + if parent_id is not None: + json_data["parentId"] = parent_id + if title is not None: + json_data["title"] = title + response = await self._make_request( + "PUT", + f"{API_BASE}/collectives/{collective_id}/pages/{page_id}", + json=json_data, + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["page"] + + async def trash_page(self, collective_id: int, page_id: int) -> None: + """Move a page to trash (soft delete).""" + await self._make_request( + "DELETE", + f"{API_BASE}/collectives/{collective_id}/pages/{page_id}", + headers=self._get_ocs_headers(), + ) + + async def set_page_emoji( + self, collective_id: int, page_id: int, emoji: str | None + ) -> dict[str, Any]: + """Set or clear the emoji on a page.""" + json_data = {"emoji": emoji} + response = await self._make_request( + "PUT", + f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/emoji", + json=json_data, + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["page"] + + # Search + + async def search_pages( + self, collective_id: int, query: str + ) -> list[dict[str, Any]]: + """Full-text search within a collective.""" + response = await self._make_request( + "GET", + f"{API_BASE}/collectives/{collective_id}/search", + params={"searchString": query}, + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["pages"] + + # Tags + + async def get_tags(self, collective_id: int) -> list[dict[str, Any]]: + """List all tags in a collective.""" + response = await self._make_request( + "GET", + f"{API_BASE}/collectives/{collective_id}/tags", + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["tags"] + + async def create_tag( + self, collective_id: int, name: str, color: str + ) -> dict[str, Any]: + """Create a new tag in a collective.""" + json_data = {"name": name, "color": color} + response = await self._make_request( + "POST", + f"{API_BASE}/collectives/{collective_id}/tags", + json=json_data, + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["tag"] + + async def assign_tag(self, collective_id: int, page_id: int, tag_id: int) -> None: + """Assign a tag to a page.""" + await self._make_request( + "PUT", + f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/tags/{tag_id}", + headers=self._get_ocs_headers(), + ) + + async def remove_tag(self, collective_id: int, page_id: int, tag_id: int) -> None: + """Remove a tag from a page.""" + await self._make_request( + "DELETE", + f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/tags/{tag_id}", + headers=self._get_ocs_headers(), + ) + + # Trash + + async def get_trashed_pages(self, collective_id: int) -> list[dict[str, Any]]: + """List trashed pages in a collective.""" + response = await self._make_request( + "GET", + f"{API_BASE}/collectives/{collective_id}/pages/trash", + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["pages"] + + async def restore_page(self, collective_id: int, page_id: int) -> dict[str, Any]: + """Restore a page from trash.""" + response = await self._make_request( + "PATCH", + f"{API_BASE}/collectives/{collective_id}/pages/trash/{page_id}", + headers=self._get_ocs_headers(), + ) + data = self._unwrap_ocs(response.json()) + return data["page"] diff --git a/nextcloud_mcp_server/models/auth.py b/nextcloud_mcp_server/models/auth.py index f249229c..e8f1c707 100644 --- a/nextcloud_mcp_server/models/auth.py +++ b/nextcloud_mcp_server/models/auth.py @@ -74,5 +74,7 @@ ALL_SUPPORTED_SCOPES: frozenset[str] = frozenset( "sharing:write", "news:read", "news:write", + "collectives:read", + "collectives:write", } ) diff --git a/nextcloud_mcp_server/models/collectives.py b/nextcloud_mcp_server/models/collectives.py new file mode 100644 index 00000000..f6dcb180 --- /dev/null +++ b/nextcloud_mcp_server/models/collectives.py @@ -0,0 +1,135 @@ +"""Pydantic models for Nextcloud Collectives app.""" + +from pydantic import BaseModel, Field + +from .base import BaseResponse, StatusResponse + +# Domain Models + + +class Collective(BaseModel): + """A Nextcloud Collective (wiki/knowledge base).""" + + id: int = Field(description="Collective ID") + circleId: str = Field(description="Linked Circle/Team ID") + emoji: str | None = Field(default=None, description="Collective emoji") + name: str = Field(description="Collective name") + level: int = Field(description="User's membership level") + canEdit: bool = Field(description="Whether the user can edit") + canShare: bool = Field(description="Whether the user can share") + pageMode: int = Field(description="Default page mode: 0=view, 1=edit") + + +class PageInfo(BaseModel): + """A page within a Collective.""" + + id: int = Field(description="Page ID") + title: str = Field(description="Page title") + emoji: str | None = Field(default=None, description="Page emoji") + fileName: str = Field(description="Markdown file name") + filePath: str = Field(description="File path within the collective") + collectivePath: str | None = Field( + default=None, description="Collective folder path in user's files" + ) + parentId: int = Field(description="Parent page ID (0 for root)") + timestamp: int = Field(description="Last modification Unix timestamp") + size: int = Field(description="Content size in bytes") + lastUserId: str | None = Field(default=None, description="Last editor user ID") + lastUserDisplayName: str | None = Field( + default=None, description="Last editor display name" + ) + subpageOrder: list[int] = Field( + default_factory=list, description="Ordered subpage IDs" + ) + isFullWidth: bool | None = Field(default=None, description="Full-width page layout") + + +class CollectiveTag(BaseModel): + """A tag within a Collective.""" + + id: int = Field(description="Tag ID") + collectiveId: int = Field(description="Parent collective ID") + name: str = Field(description="Tag name") + color: str = Field(description="Hex color code") + + +# Response Models + + +class ListCollectivesResponse(BaseResponse): + """Response for listing collectives.""" + + collectives: list[Collective] = Field(description="List of collectives") + total: int = Field(description="Total number of collectives") + + +class CreateCollectiveResponse(BaseResponse): + """Response for creating a collective.""" + + id: int = Field(description="Created collective ID") + name: str = Field(description="Created collective name") + emoji: str | None = Field(default=None, description="Collective emoji") + + +class CollectiveOperationResponse(StatusResponse): + """Response for collective update operations.""" + + collective_id: int = Field(description="ID of the affected collective") + + +class ListPagesResponse(BaseResponse): + """Response for listing pages in a collective.""" + + pages: list[PageInfo] = Field(description="List of pages") + total: int = Field(description="Total number of pages") + collective_id: int = Field(description="Collective ID") + + +class GetPageResponse(BaseResponse): + """Response for getting a single page with content.""" + + page: PageInfo = Field(description="Page metadata") + content: str | None = Field( + default=None, + description="Page markdown content (fetched via WebDAV)", + ) + + +class CreatePageResponse(BaseResponse): + """Response for creating a page.""" + + id: int = Field(description="Created page ID") + title: str = Field(description="Created page title") + collective_id: int = Field(description="Collective ID") + parent_id: int = Field(description="Parent page ID") + + +class PageOperationResponse(StatusResponse): + """Response for page operations (update, trash, emoji, tag).""" + + page_id: int = Field(description="ID of the affected page") + collective_id: int = Field(description="Collective ID") + + +class SearchPagesResponse(BaseResponse): + """Response for full-text search within a collective.""" + + results: list[PageInfo] = Field(description="Matching pages") + total: int = Field(description="Total number of results") + query: str = Field(description="Search query") + collective_id: int = Field(description="Collective ID") + + +class ListTagsResponse(BaseResponse): + """Response for listing tags in a collective.""" + + tags: list[CollectiveTag] = Field(description="List of tags") + total: int = Field(description="Total number of tags") + + +class CreateTagResponse(BaseResponse): + """Response for creating a tag.""" + + id: int = Field(description="Created tag ID") + name: str = Field(description="Tag name") + color: str = Field(description="Tag color") diff --git a/nextcloud_mcp_server/server/__init__.py b/nextcloud_mcp_server/server/__init__.py index 0be6bbaa..c7c053a9 100644 --- a/nextcloud_mcp_server/server/__init__.py +++ b/nextcloud_mcp_server/server/__init__.py @@ -1,4 +1,5 @@ from .calendar import configure_calendar_tools +from .collectives import configure_collectives_tools from .contacts import configure_contacts_tools from .cookbook import configure_cookbook_tools from .deck import configure_deck_tools @@ -11,6 +12,7 @@ from .webdav import configure_webdav_tools __all__ = [ "configure_calendar_tools", + "configure_collectives_tools", "configure_contacts_tools", "configure_cookbook_tools", "configure_deck_tools", diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py new file mode 100644 index 00000000..0c40b49a --- /dev/null +++ b/nextcloud_mcp_server/server/collectives.py @@ -0,0 +1,440 @@ +"""MCP tool definitions for Nextcloud Collectives app.""" + +import logging + +from mcp.server.fastmcp import Context, FastMCP +from mcp.types import ToolAnnotations + +from nextcloud_mcp_server.auth import require_scopes +from nextcloud_mcp_server.context import get_client +from nextcloud_mcp_server.models.collectives import ( + Collective, + CollectiveOperationResponse, + CollectiveTag, + CreateCollectiveResponse, + CreatePageResponse, + CreateTagResponse, + GetPageResponse, + ListCollectivesResponse, + ListPagesResponse, + ListTagsResponse, + PageInfo, + PageOperationResponse, + SearchPagesResponse, +) +from nextcloud_mcp_server.observability.metrics import instrument_tool + +logger = logging.getLogger(__name__) + + +def configure_collectives_tools(mcp: FastMCP): + """Configure Nextcloud Collectives tools for the MCP server.""" + + # --- Read Tools --- + + @mcp.tool( + title="List Collectives", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("collectives:read") + @instrument_tool + async def collectives_get_collectives( + ctx: Context, + ) -> ListCollectivesResponse: + """List all Nextcloud Collectives the user has access to""" + client = await get_client(ctx) + raw_collectives = await client.collectives.get_collectives() + collectives = [Collective(**c) for c in raw_collectives] + return ListCollectivesResponse(collectives=collectives, total=len(collectives)) + + @mcp.tool( + title="List Collective Pages", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("collectives:read") + @instrument_tool + async def collectives_get_pages( + ctx: Context, collective_id: int + ) -> ListPagesResponse: + """List all pages in a Nextcloud Collective + + Args: + collective_id: ID of the collective + """ + client = await get_client(ctx) + raw_pages = await client.collectives.get_pages(collective_id) + pages = [PageInfo(**p) for p in raw_pages] + return ListPagesResponse( + pages=pages, total=len(pages), collective_id=collective_id + ) + + @mcp.tool( + title="Get Collective Page", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("collectives:read") + @instrument_tool + async def collectives_get_page( + ctx: Context, collective_id: int, page_id: int + ) -> GetPageResponse: + """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. + + Args: + collective_id: ID of the collective + page_id: ID of the page + """ + client = await get_client(ctx) + raw_page = await client.collectives.get_page(collective_id, page_id) + page = PageInfo(**raw_page) + + # Fetch content via WebDAV + # Path structure: collectivePath/filePath/fileName + # filePath is empty for root-level pages, contains subdirectory for nested pages + content = None + if page.collectivePath and page.fileName: + parts = [page.collectivePath] + if page.filePath: + parts.append(page.filePath) + parts.append(page.fileName) + webdav_path = "/".join(parts) + try: + file_bytes, _ = await client.webdav.read_file(webdav_path) + content = file_bytes.decode("utf-8") + except Exception: + logger.warning( + "Failed to read page content via WebDAV: %s", + webdav_path, + ) + + return GetPageResponse(page=page, content=content) + + @mcp.tool( + title="Search Collective Pages", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("collectives:read") + @instrument_tool + async def collectives_search_pages( + ctx: Context, collective_id: int, query: str + ) -> SearchPagesResponse: + """Full-text search within a Nextcloud Collective + + Args: + collective_id: ID of the collective + query: Search query string + """ + client = await get_client(ctx) + raw_pages = await client.collectives.search_pages(collective_id, query) + pages = [PageInfo(**p) for p in raw_pages] + return SearchPagesResponse( + results=pages, + total=len(pages), + query=query, + collective_id=collective_id, + ) + + @mcp.tool( + title="List Collective Tags", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("collectives:read") + @instrument_tool + async def collectives_get_tags( + ctx: Context, collective_id: int + ) -> ListTagsResponse: + """List all tags in a Nextcloud Collective + + Args: + collective_id: ID of the collective + """ + client = await get_client(ctx) + raw_tags = await client.collectives.get_tags(collective_id) + tags = [CollectiveTag(**t) for t in raw_tags] + return ListTagsResponse(tags=tags, total=len(tags)) + + @mcp.tool( + title="List Trashed Collective Pages", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("collectives:read") + @instrument_tool + async def collectives_get_trashed_pages( + ctx: Context, collective_id: int + ) -> ListPagesResponse: + """List trashed pages in a Nextcloud Collective + + Args: + collective_id: ID of the collective + """ + client = await get_client(ctx) + raw_pages = await client.collectives.get_trashed_pages(collective_id) + pages = [PageInfo(**p) for p in raw_pages] + return ListPagesResponse( + pages=pages, total=len(pages), collective_id=collective_id + ) + + # --- Write Tools --- + + @mcp.tool( + title="Create Collective", + annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_create_collective( + ctx: Context, name: str, emoji: str | None = None + ) -> CreateCollectiveResponse: + """Create a new Nextcloud Collective + + Args: + name: Name of the collective + emoji: Optional emoji for the collective + """ + client = await get_client(ctx) + raw = await client.collectives.create_collective(name, emoji) + collective = Collective(**raw) + return CreateCollectiveResponse( + id=collective.id, name=collective.name, emoji=collective.emoji + ) + + @mcp.tool( + title="Update Collective", + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_update_collective( + ctx: Context, collective_id: int, emoji: str | None = None + ) -> CollectiveOperationResponse: + """Update a Nextcloud Collective (emoji) + + Args: + collective_id: ID of the collective + emoji: New emoji for the collective + """ + client = await get_client(ctx) + await client.collectives.update_collective(collective_id, emoji) + return CollectiveOperationResponse( + collective_id=collective_id, + status_code=200, + message="Collective updated", + ) + + @mcp.tool( + title="Create Collective Page", + annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_create_page( + ctx: Context, collective_id: int, parent_id: int, title: str + ) -> CreatePageResponse: + """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. + + Args: + collective_id: ID of the collective + parent_id: ID of the parent page (use 0 for top-level pages) + title: Title of the new page + """ + client = await get_client(ctx) + raw = await client.collectives.create_page(collective_id, parent_id, title) + page = PageInfo(**raw) + return CreatePageResponse( + id=page.id, + title=page.title, + collective_id=collective_id, + parent_id=page.parentId, + ) + + @mcp.tool( + title="Move Collective Page", + annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_move_page( + ctx: Context, + collective_id: int, + page_id: int, + parent_id: int | None = None, + title: str | None = None, + index: int = 0, + copy: bool = False, + ) -> PageOperationResponse: + """Move or copy a page within a Nextcloud Collective + + Args: + collective_id: ID of the collective + page_id: ID of the page to move/copy + parent_id: Target parent page ID + title: New title (optional) + index: Position in subpage order (default 0) + copy: If true, copy instead of move + """ + client = await get_client(ctx) + await client.collectives.move_page( + collective_id, page_id, parent_id, title, index, copy + ) + action = "copied" if copy else "moved" + return PageOperationResponse( + page_id=page_id, + collective_id=collective_id, + status_code=200, + message=f"Page {action}", + ) + + @mcp.tool( + title="Trash Collective Page", + annotations=ToolAnnotations( + destructiveHint=True, idempotentHint=True, openWorldHint=True + ), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_trash_page( + ctx: Context, collective_id: int, page_id: int + ) -> PageOperationResponse: + """Move a page to trash in a Nextcloud Collective (soft delete) + + Args: + collective_id: ID of the collective + page_id: ID of the page to trash + """ + client = await get_client(ctx) + await client.collectives.trash_page(collective_id, page_id) + return PageOperationResponse( + page_id=page_id, + collective_id=collective_id, + status_code=200, + message="Page moved to trash", + ) + + @mcp.tool( + title="Restore Collective Page", + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_restore_page( + ctx: Context, collective_id: int, page_id: int + ) -> PageOperationResponse: + """Restore a page from trash in a Nextcloud Collective + + Args: + collective_id: ID of the collective + page_id: ID of the page to restore + """ + client = await get_client(ctx) + await client.collectives.restore_page(collective_id, page_id) + return PageOperationResponse( + page_id=page_id, + collective_id=collective_id, + status_code=200, + message="Page restored from trash", + ) + + @mcp.tool( + title="Set Collective Page Emoji", + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_set_page_emoji( + ctx: Context, + collective_id: int, + page_id: int, + emoji: str | None = None, + ) -> PageOperationResponse: + """Set or clear the emoji on a Nextcloud Collective page + + Args: + collective_id: ID of the collective + page_id: ID of the page + emoji: Emoji to set, or null to clear + """ + client = await get_client(ctx) + await client.collectives.set_page_emoji(collective_id, page_id, emoji) + return PageOperationResponse( + page_id=page_id, + collective_id=collective_id, + status_code=200, + message="Page emoji updated", + ) + + @mcp.tool( + title="Create Collective Tag", + annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_create_tag( + ctx: Context, collective_id: int, name: str, color: str + ) -> CreateTagResponse: + """Create a new tag in a Nextcloud Collective + + Args: + collective_id: ID of the collective + name: Tag name + color: Hex color code (e.g. "FF0000") + """ + client = await get_client(ctx) + raw = await client.collectives.create_tag(collective_id, name, color) + tag = CollectiveTag(**raw) + return CreateTagResponse(id=tag.id, name=tag.name, color=tag.color) + + @mcp.tool( + title="Assign Tag to Collective Page", + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_assign_tag( + ctx: Context, collective_id: int, page_id: int, tag_id: int + ) -> PageOperationResponse: + """Assign a tag to a page in a Nextcloud Collective + + Args: + collective_id: ID of the collective + page_id: ID of the page + tag_id: ID of the tag to assign + """ + client = await get_client(ctx) + await client.collectives.assign_tag(collective_id, page_id, tag_id) + return PageOperationResponse( + page_id=page_id, + collective_id=collective_id, + status_code=200, + message=f"Tag {tag_id} assigned to page", + ) + + @mcp.tool( + title="Remove Tag from Collective Page", + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_remove_tag( + ctx: Context, collective_id: int, page_id: int, tag_id: int + ) -> PageOperationResponse: + """Remove a tag from a page in a Nextcloud Collective + + Args: + collective_id: ID of the collective + page_id: ID of the page + tag_id: ID of the tag to remove + """ + client = await get_client(ctx) + await client.collectives.remove_tag(collective_id, page_id, tag_id) + return PageOperationResponse( + page_id=page_id, + collective_id=collective_id, + status_code=200, + message=f"Tag {tag_id} removed from page", + ) diff --git a/tests/client/collectives/__init__.py b/tests/client/collectives/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py new file mode 100644 index 00000000..56adea5d --- /dev/null +++ b/tests/client/collectives/test_collectives_api.py @@ -0,0 +1,372 @@ +"""Unit tests for CollectivesClient API methods.""" + +import httpx +import pytest + +from nextcloud_mcp_server.client.collectives import CollectivesClient +from tests.client.conftest import create_mock_response + +pytestmark = pytest.mark.unit + + +# --- OCS mock helpers --- + + +def _ocs_response(data: dict | list) -> httpx.Response: + """Wrap data in an OCS envelope and return as mock response.""" + return create_mock_response( + status_code=200, + json_data={"ocs": {"meta": {"status": "ok", "statuscode": 200}, "data": data}}, + ) + + +def _sample_collective( + collective_id: int = 1, name: str = "Test Wiki", emoji: str | None = None +) -> dict: + return { + "id": collective_id, + "circleId": "circle-abc", + "emoji": emoji, + "name": name, + "level": 9, + "canEdit": True, + "canShare": True, + "pageMode": 0, + } + + +def _sample_page( + page_id: int = 10, + title: str = "Test Page", + parent_id: int = 0, + collective_path: str = "Collectives/Test Wiki", +) -> dict: + return { + "id": page_id, + "title": title, + "emoji": None, + "fileName": f"{title}.md", + "filePath": f"{title}.md", + "collectivePath": collective_path, + "parentId": parent_id, + "timestamp": 1700000000, + "size": 42, + "lastUserId": "testuser", + "lastUserDisplayName": "Test User", + "subpageOrder": [], + "isFullWidth": False, + } + + +def _sample_tag( + tag_id: int = 1, name: str = "important", color: str = "FF0000" +) -> dict: + return { + "id": tag_id, + "collectiveId": 1, + "name": name, + "color": color, + } + + +# --- Collectives --- + + +async def test_get_collectives(mocker): + """Test listing collectives unwraps OCS envelope correctly.""" + mock_response = _ocs_response( + { + "collectives": [ + _sample_collective(1, "Wiki A"), + _sample_collective(2, "Wiki B"), + ] + } + ) + mocker.patch.object(CollectivesClient, "_make_request", return_value=mock_response) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.get_collectives() + + assert isinstance(result, list) + assert len(result) == 2 + assert result[0]["id"] == 1 + assert result[1]["name"] == "Wiki B" + + +async def test_create_collective(mocker): + """Test creating a collective sends name and emoji.""" + mock_response = _ocs_response( + {"collective": _sample_collective(5, "New Wiki", "๐Ÿ“š")} + ) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.create_collective("New Wiki", emoji="๐Ÿ“š") + + assert result["id"] == 5 + assert result["name"] == "New Wiki" + assert result["emoji"] == "๐Ÿ“š" + + call_args = mock_request.call_args + assert call_args[0][0] == "POST" + assert call_args[1]["json"]["name"] == "New Wiki" + assert call_args[1]["json"]["emoji"] == "๐Ÿ“š" + + +# --- Pages --- + + +async def test_get_pages(mocker): + """Test listing pages in a collective.""" + mock_response = _ocs_response( + {"pages": [_sample_page(10, "Page A"), _sample_page(20, "Page B")]} + ) + mocker.patch.object(CollectivesClient, "_make_request", return_value=mock_response) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.get_pages(collective_id=1) + + assert len(result) == 2 + assert result[0]["title"] == "Page A" + assert result[1]["id"] == 20 + + +async def test_get_page(mocker): + """Test getting a single page metadata.""" + mock_response = _ocs_response({"page": _sample_page(10, "My Page")}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.get_page(collective_id=1, page_id=10) + + assert result["id"] == 10 + assert result["title"] == "My Page" + assert result["collectivePath"] == "Collectives/Test Wiki" + + call_args = mock_request.call_args + assert "/collectives/1/pages/10" in call_args[0][1] + + +async def test_create_page(mocker): + """Test creating a page under a parent.""" + mock_response = _ocs_response({"page": _sample_page(30, "New Page", parent_id=10)}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.create_page(collective_id=1, parent_id=10, title="New Page") + + assert result["id"] == 30 + assert result["parentId"] == 10 + + call_args = mock_request.call_args + assert call_args[0][0] == "POST" + assert "/collectives/1/pages/10" in call_args[0][1] + assert call_args[1]["json"]["title"] == "New Page" + + +async def test_trash_page(mocker): + """Test trashing a page sends DELETE.""" + mock_response = create_mock_response(status_code=200, json_data={}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.trash_page(collective_id=1, page_id=10) + + call_args = mock_request.call_args + assert call_args[0][0] == "DELETE" + assert "/collectives/1/pages/10" in call_args[0][1] + + +async def test_move_page(mocker): + """Test moving a page sends PUT with correct params.""" + mock_response = _ocs_response( + {"page": _sample_page(10, "Moved Page", parent_id=20)} + ) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.move_page( + collective_id=1, page_id=10, parent_id=20, title="Moved Page" + ) + + assert result["parentId"] == 20 + call_args = mock_request.call_args + assert call_args[0][0] == "PUT" + assert call_args[1]["json"]["parentId"] == 20 + + +# --- Search --- + + +async def test_search_pages(mocker): + """Test full-text search sends query parameter.""" + mock_response = _ocs_response({"pages": [_sample_page(10, "Result Page")]}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.search_pages(collective_id=1, query="test query") + + assert len(result) == 1 + assert result[0]["title"] == "Result Page" + + call_args = mock_request.call_args + assert call_args[1]["params"]["searchString"] == "test query" + + +# --- Tags --- + + +async def test_get_tags(mocker): + """Test listing tags.""" + mock_response = _ocs_response( + {"tags": [_sample_tag(1, "important"), _sample_tag(2, "draft", "00FF00")]} + ) + mocker.patch.object(CollectivesClient, "_make_request", return_value=mock_response) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.get_tags(collective_id=1) + + assert len(result) == 2 + assert result[0]["name"] == "important" + assert result[1]["color"] == "00FF00" + + +async def test_create_tag(mocker): + """Test creating a tag.""" + mock_response = _ocs_response({"tag": _sample_tag(3, "review", "0000FF")}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.create_tag(collective_id=1, name="review", color="0000FF") + + assert result["id"] == 3 + assert result["name"] == "review" + + call_args = mock_request.call_args + assert call_args[0][0] == "POST" + assert call_args[1]["json"]["name"] == "review" + + +async def test_assign_tag(mocker): + """Test assigning a tag to a page.""" + mock_response = create_mock_response(status_code=200, json_data={}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.assign_tag(collective_id=1, page_id=10, tag_id=3) + + call_args = mock_request.call_args + assert call_args[0][0] == "PUT" + assert "/pages/10/tags/3" in call_args[0][1] + + +async def test_remove_tag(mocker): + """Test removing a tag from a page.""" + mock_response = create_mock_response(status_code=200, json_data={}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.remove_tag(collective_id=1, page_id=10, tag_id=3) + + call_args = mock_request.call_args + assert call_args[0][0] == "DELETE" + assert "/pages/10/tags/3" in call_args[0][1] + + +# --- Trash --- + + +async def test_get_trashed_pages(mocker): + """Test listing trashed pages.""" + trashed = _sample_page(10, "Trashed Page") + trashed["trashTimestamp"] = 1700000000 + mock_response = _ocs_response({"pages": [trashed]}) + mocker.patch.object(CollectivesClient, "_make_request", return_value=mock_response) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.get_trashed_pages(collective_id=1) + + assert len(result) == 1 + assert result[0]["title"] == "Trashed Page" + + +async def test_restore_page(mocker): + """Test restoring a page from trash.""" + mock_response = _ocs_response({"page": _sample_page(10, "Restored Page")}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.restore_page(collective_id=1, page_id=10) + + assert result["title"] == "Restored Page" + call_args = mock_request.call_args + assert call_args[0][0] == "PATCH" + assert "/pages/trash/10" in call_args[0][1] + + +# --- Error Handling --- + + +async def test_get_collectives_403(mocker): + """Test 403 response raises HTTPStatusError.""" + mock_response = create_mock_response( + status_code=403, json_data={"message": "Forbidden"} + ) + mock_response.raise_for_status = lambda: (_ for _ in ()).throw( + httpx.HTTPStatusError( + "Forbidden", request=mock_response.request, response=mock_response + ) + ) + mocker.patch.object( + CollectivesClient, + "_make_request", + side_effect=httpx.HTTPStatusError( + "Forbidden", + request=httpx.Request("GET", "http://test"), + response=mock_response, + ), + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + with pytest.raises(httpx.HTTPStatusError): + await client.get_collectives() + + +async def test_get_page_404(mocker): + """Test 404 response raises HTTPStatusError.""" + mock_response = create_mock_response( + status_code=404, json_data={"message": "Not found"} + ) + mocker.patch.object( + CollectivesClient, + "_make_request", + side_effect=httpx.HTTPStatusError( + "Not Found", + request=httpx.Request("GET", "http://test"), + response=mock_response, + ), + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + with pytest.raises(httpx.HTTPStatusError): + await client.get_page(collective_id=1, page_id=999) diff --git a/tests/server/test_collectives_mcp.py b/tests/server/test_collectives_mcp.py new file mode 100644 index 00000000..dee52889 --- /dev/null +++ b/tests/server/test_collectives_mcp.py @@ -0,0 +1,412 @@ +"""Integration tests for Nextcloud Collectives MCP tools.""" + +import json +import logging +import uuid + +import httpx +import pytest +from mcp import ClientSession + +logger = logging.getLogger(__name__) +pytestmark = pytest.mark.integration + +# Nextcloud credentials for direct API cleanup (matches docker-compose.yml) +_NC_BASE = "http://localhost:8080" +_NC_AUTH = ("admin", "admin") +_OCS_HEADERS = { + "OCS-APIRequest": "true", + "Accept": "application/json", +} + + +# --- Fixtures --- + + +@pytest.fixture(scope="session") +async def temporary_collective(nc_mcp_client: ClientSession): + """Create a temporary collective for testing. Cleaned up after session.""" + unique_suffix = uuid.uuid4().hex[:8] + name = f"MCP Test Collective {unique_suffix}" + + result = await nc_mcp_client.call_tool( + "collectives_create_collective", + {"name": name, "emoji": "๐Ÿงช"}, + ) + assert result.isError is False, f"Failed to create collective: {result.content}" + data = json.loads(result.content[0].text) + collective_id = data["id"] + logger.info(f"Created temporary collective: {name} (ID: {collective_id})") + + # Get the landing page ID (auto-created with each collective) + pages_result = await nc_mcp_client.call_tool( + "collectives_get_pages", + {"collective_id": collective_id}, + ) + pages_data = json.loads(pages_result.content[0].text) + landing_page_id = pages_data["pages"][0]["id"] + + yield { + "id": collective_id, + "name": name, + "landing_page_id": landing_page_id, + } + + # Cleanup: trash and permanently delete the collective via direct OCS API + try: + async with httpx.AsyncClient(base_url=_NC_BASE, auth=_NC_AUTH) as client: + api = "/ocs/v2.php/apps/collectives/api/v1.0" + await client.delete( + f"{api}/collectives/{collective_id}", + headers=_OCS_HEADERS, + ) + await client.delete( + f"{api}/collectives/trash/{collective_id}", + headers=_OCS_HEADERS, + ) + logger.info(f"Cleaned up collective: {collective_id}") + except Exception as e: + logger.warning(f"Cleanup of collective {collective_id} failed: {e}") + + +# --- Tool Discovery --- + + +async def test_collectives_tools_available(nc_mcp_client: ClientSession): + """Verify all Collectives MCP tools are registered.""" + tools = await nc_mcp_client.list_tools() + tool_names = [tool.name for tool in tools.tools] + + expected_tools = [ + "collectives_get_collectives", + "collectives_create_collective", + "collectives_update_collective", + "collectives_get_pages", + "collectives_get_page", + "collectives_create_page", + "collectives_move_page", + "collectives_trash_page", + "collectives_restore_page", + "collectives_set_page_emoji", + "collectives_search_pages", + "collectives_get_tags", + "collectives_create_tag", + "collectives_assign_tag", + "collectives_remove_tag", + "collectives_get_trashed_pages", + ] + + for expected in expected_tools: + assert expected in tool_names, ( + f"Expected tool '{expected}' not found in available tools" + ) + + logger.info(f"All {len(expected_tools)} Collectives tools registered") + + +# --- Collective CRUD --- + + +async def test_collectives_list( + nc_mcp_client: ClientSession, temporary_collective: dict +): + """Test listing collectives includes the temporary one.""" + result = await nc_mcp_client.call_tool("collectives_get_collectives", {}) + assert result.isError is False + + data = json.loads(result.content[0].text) + assert data["success"] is True + assert data["total"] >= 1 + + collective_ids = [c["id"] for c in data["collectives"]] + assert temporary_collective["id"] in collective_ids + logger.info(f"Found {data['total']} collectives") + + +async def test_collectives_update_emoji( + nc_mcp_client: ClientSession, temporary_collective: dict +): + """Test updating a collective's emoji.""" + result = await nc_mcp_client.call_tool( + "collectives_update_collective", + {"collective_id": temporary_collective["id"], "emoji": "๐Ÿ“–"}, + ) + assert result.isError is False + + data = json.loads(result.content[0].text) + assert data["success"] is True + assert data["collective_id"] == temporary_collective["id"] + logger.info("Collective emoji updated") + + +# --- Page CRUD --- + + +async def test_collectives_page_workflow( + nc_mcp_client: ClientSession, temporary_collective: dict +): + """Test the full page lifecycle: create, read, set emoji, trash, restore.""" + cid = temporary_collective["id"] + landing_id = temporary_collective["landing_page_id"] + + # 1. Create a page + unique_title = f"Test Page {uuid.uuid4().hex[:8]}" + create_result = await nc_mcp_client.call_tool( + "collectives_create_page", + {"collective_id": cid, "parent_id": landing_id, "title": unique_title}, + ) + assert create_result.isError is False + create_data = json.loads(create_result.content[0].text) + page_id = create_data["id"] + assert create_data["collective_id"] == cid + assert create_data["parent_id"] == landing_id + logger.info(f"Created page: {unique_title} (ID: {page_id})") + + # 2. List pages โ€” should include the new page + list_result = await nc_mcp_client.call_tool( + "collectives_get_pages", + {"collective_id": cid}, + ) + assert list_result.isError is False + list_data = json.loads(list_result.content[0].text) + page_ids = [p["id"] for p in list_data["pages"]] + assert page_id in page_ids + logger.info(f"Page found in list ({list_data['total']} pages)") + + # 3. Get page with content + get_result = await nc_mcp_client.call_tool( + "collectives_get_page", + {"collective_id": cid, "page_id": page_id}, + ) + assert get_result.isError is False + get_data = json.loads(get_result.content[0].text) + assert get_data["page"]["id"] == page_id + assert get_data["page"]["title"] == unique_title + # New pages have empty content (empty string or None) + logger.info("Page metadata retrieved") + + # 4. Set page emoji + emoji_result = await nc_mcp_client.call_tool( + "collectives_set_page_emoji", + {"collective_id": cid, "page_id": page_id, "emoji": "๐Ÿš€"}, + ) + assert emoji_result.isError is False + logger.info("Page emoji set") + + # 5. Trash the page + trash_result = await nc_mcp_client.call_tool( + "collectives_trash_page", + {"collective_id": cid, "page_id": page_id}, + ) + assert trash_result.isError is False + logger.info("Page trashed") + + # 6. Verify page is in trash + trashed_result = await nc_mcp_client.call_tool( + "collectives_get_trashed_pages", + {"collective_id": cid}, + ) + assert trashed_result.isError is False + trashed_data = json.loads(trashed_result.content[0].text) + trashed_ids = [p["id"] for p in trashed_data["pages"]] + assert page_id in trashed_ids + logger.info("Page found in trash") + + # 7. Restore from trash + restore_result = await nc_mcp_client.call_tool( + "collectives_restore_page", + {"collective_id": cid, "page_id": page_id}, + ) + assert restore_result.isError is False + logger.info("Page restored from trash") + + # 8. Verify page is back in pages list + list_result2 = await nc_mcp_client.call_tool( + "collectives_get_pages", + {"collective_id": cid}, + ) + list_data2 = json.loads(list_result2.content[0].text) + page_ids2 = [p["id"] for p in list_data2["pages"]] + assert page_id in page_ids2 + logger.info("Page verified restored to pages list") + + +async def test_collectives_get_landing_page_content( + nc_mcp_client: ClientSession, temporary_collective: dict +): + """Test that the landing page has auto-generated content readable via WebDAV.""" + cid = temporary_collective["id"] + landing_id = temporary_collective["landing_page_id"] + + result = await nc_mcp_client.call_tool( + "collectives_get_page", + {"collective_id": cid, "page_id": landing_id}, + ) + assert result.isError is False + + data = json.loads(result.content[0].text) + assert data["page"]["fileName"] == "Readme.md" + assert data["content"] is not None, ( + "Landing page should have auto-generated content" + ) + assert "Welcome" in data["content"], "Landing page should contain welcome text" + logger.info(f"Landing page content: {len(data['content'])} bytes") + + +async def test_collectives_move_page( + nc_mcp_client: ClientSession, temporary_collective: dict +): + """Test moving a page (rename).""" + cid = temporary_collective["id"] + landing_id = temporary_collective["landing_page_id"] + + # Create a page to move + create_result = await nc_mcp_client.call_tool( + "collectives_create_page", + { + "collective_id": cid, + "parent_id": landing_id, + "title": f"Movable Page {uuid.uuid4().hex[:8]}", + }, + ) + assert create_result.isError is False + page_id = json.loads(create_result.content[0].text)["id"] + + # Move (rename) the page + new_title = f"Renamed Page {uuid.uuid4().hex[:8]}" + move_result = await nc_mcp_client.call_tool( + "collectives_move_page", + { + "collective_id": cid, + "page_id": page_id, + "title": new_title, + }, + ) + assert move_result.isError is False + + data = json.loads(move_result.content[0].text) + assert data["page_id"] == page_id + assert "moved" in data["message"] + logger.info(f"Page renamed to: {new_title}") + + # Cleanup + await nc_mcp_client.call_tool( + "collectives_trash_page", + {"collective_id": cid, "page_id": page_id}, + ) + + +# --- Tags --- + + +async def test_collectives_tag_workflow( + nc_mcp_client: ClientSession, temporary_collective: dict +): + """Test tag lifecycle: create tag, assign to page, remove from page.""" + cid = temporary_collective["id"] + landing_id = temporary_collective["landing_page_id"] + + # 1. Create a tag + tag_name = f"test-tag-{uuid.uuid4().hex[:6]}" + create_tag_result = await nc_mcp_client.call_tool( + "collectives_create_tag", + {"collective_id": cid, "name": tag_name, "color": "FF5733"}, + ) + assert create_tag_result.isError is False + tag_data = json.loads(create_tag_result.content[0].text) + tag_id = tag_data["id"] + assert tag_data["name"] == tag_name + assert tag_data["color"] == "FF5733" + logger.info(f"Created tag: {tag_name} (ID: {tag_id})") + + # 2. List tags โ€” should include the new tag + list_tags_result = await nc_mcp_client.call_tool( + "collectives_get_tags", + {"collective_id": cid}, + ) + assert list_tags_result.isError is False + tags_data = json.loads(list_tags_result.content[0].text) + tag_ids = [t["id"] for t in tags_data["tags"]] + assert tag_id in tag_ids + logger.info(f"Tag found in list ({tags_data['total']} tags)") + + # 3. Create a page to tag + page_result = await nc_mcp_client.call_tool( + "collectives_create_page", + { + "collective_id": cid, + "parent_id": landing_id, + "title": f"Tagged Page {uuid.uuid4().hex[:8]}", + }, + ) + assert page_result.isError is False + page_id = json.loads(page_result.content[0].text)["id"] + + # 4. Assign tag to page + assign_result = await nc_mcp_client.call_tool( + "collectives_assign_tag", + {"collective_id": cid, "page_id": page_id, "tag_id": tag_id}, + ) + assert assign_result.isError is False + logger.info(f"Tag {tag_id} assigned to page {page_id}") + + # 5. Remove tag from page + remove_result = await nc_mcp_client.call_tool( + "collectives_remove_tag", + {"collective_id": cid, "page_id": page_id, "tag_id": tag_id}, + ) + assert remove_result.isError is False + logger.info(f"Tag {tag_id} removed from page {page_id}") + + # Cleanup + await nc_mcp_client.call_tool( + "collectives_trash_page", + {"collective_id": cid, "page_id": page_id}, + ) + + +# --- Search --- + + +async def test_collectives_search( + nc_mcp_client: ClientSession, temporary_collective: dict +): + """Test full-text search within a collective.""" + cid = temporary_collective["id"] + + # Search for text in the landing page (contains "Welcome") + result = await nc_mcp_client.call_tool( + "collectives_search_pages", + {"collective_id": cid, "query": "Welcome"}, + ) + assert result.isError is False + + data = json.loads(result.content[0].text) + assert data["success"] is True + assert data["query"] == "Welcome" + assert data["collective_id"] == cid + # Search may or may not find results depending on indexing timing + logger.info(f"Search returned {data['total']} results for 'Welcome'") + + +# --- Error Handling --- + + +async def test_collectives_get_page_not_found(nc_mcp_client: ClientSession): + """Test getting a non-existent page returns an error.""" + result = await nc_mcp_client.call_tool( + "collectives_get_page", + {"collective_id": 999999, "page_id": 999999}, + ) + assert result.isError is True + logger.info("Non-existent page correctly returned error") + + +async def test_collectives_get_pages_not_found(nc_mcp_client: ClientSession): + """Test listing pages for a non-existent collective returns an error.""" + result = await nc_mcp_client.call_tool( + "collectives_get_pages", + {"collective_id": 999999}, + ) + assert result.isError is True + logger.info("Non-existent collective correctly returned error") From e5ad625a6654c9a080d43a8ce566a4bbd114ca9b Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 25 Mar 2026 08:07:37 +0100 Subject: [PATCH 02/11] 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( From 3393cd975660996a3282dc6b282805139b3ff5da Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 25 Mar 2026 08:14:59 +0100 Subject: [PATCH 03/11] fix: correct tool annotations to match ADR-017 conventions - Add destructiveHint=True to collectives_remove_tag (matches "remove" keyword pattern in annotation tests) - Change collectives_update_collective to idempotentHint=False (update operations are non-idempotent per project convention) Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/server/collectives.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index cdea6a33..3a5dc4ef 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -205,7 +205,7 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Update Collective", - annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), + annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), ) @require_scopes("collectives:write") @instrument_tool @@ -422,7 +422,9 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Remove Tag from Collective Page", - annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), + annotations=ToolAnnotations( + destructiveHint=True, idempotentHint=True, openWorldHint=True + ), ) @require_scopes("collectives:write") @instrument_tool From 44a27bd9e9c68fa9076c8f371f434d73e1eb2e5f Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 25 Mar 2026 09:08:13 +0100 Subject: [PATCH 04/11] fix: address PR review feedback (round 2) Bug fixes: - Catch OCSError/HTTPStatusError in all server tools, convert to McpError - Guard update_collective against empty body (raise ValueError) - Use restore_page response data in status message ADR-017 annotation fix: - Distinguish "remove" (reversible association) from "delete" (permanent): remove_tag and deck_remove_label_from_card no longer set destructiveHint - Update annotation test to exclude "remove" from destructive keywords Data model improvements: - Add trashTimestamp field to PageInfo - Create ListTrashedPagesResponse with is_trash context flag - Add collective_id to ListTagsResponse Test robustness: - Read NC credentials from environment variables (not hardcoded) - Filter landing page by parentId == 0 instead of assuming pages[0] Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/client/collectives.py | 8 +- nextcloud_mcp_server/models/collectives.py | 12 +++ nextcloud_mcp_server/server/collectives.py | 118 ++++++++++++++++----- nextcloud_mcp_server/server/deck.py | 4 +- tests/server/test_annotations.py | 5 +- tests/server/test_collectives_mcp.py | 18 ++-- 6 files changed, 125 insertions(+), 40 deletions(-) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index cb23ebe4..82439eb2 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -71,10 +71,16 @@ class CollectivesClient(BaseNextcloudClient): async def update_collective( self, collective_id: int, emoji: str | None = None ) -> dict[str, Any]: - """Update a collective (emoji).""" + """Update a collective (emoji). + + Raises: + ValueError: If no fields are provided to update. + """ json_data: dict[str, Any] = {} if emoji is not None: json_data["emoji"] = emoji + if not json_data: + raise ValueError("At least one field must be provided to update") response = await self._make_request( "PUT", f"{API_BASE}/collectives/{collective_id}", diff --git a/nextcloud_mcp_server/models/collectives.py b/nextcloud_mcp_server/models/collectives.py index f6dcb180..3e377f67 100644 --- a/nextcloud_mcp_server/models/collectives.py +++ b/nextcloud_mcp_server/models/collectives.py @@ -42,6 +42,9 @@ class PageInfo(BaseModel): default_factory=list, description="Ordered subpage IDs" ) isFullWidth: bool | None = Field(default=None, description="Full-width page layout") + trashTimestamp: int | None = Field( + default=None, description="Timestamp when the page was trashed" + ) class CollectiveTag(BaseModel): @@ -120,11 +123,20 @@ class SearchPagesResponse(BaseResponse): collective_id: int = Field(description="Collective ID") +class ListTrashedPagesResponse(ListPagesResponse): + """Response for listing trashed pages in a collective.""" + + is_trash: bool = Field( + default=True, description="Indicates these are trashed pages" + ) + + class ListTagsResponse(BaseResponse): """Response for listing tags in a collective.""" tags: list[CollectiveTag] = Field(description="List of tags") total: int = Field(description="Total number of tags") + collective_id: int = Field(description="Collective ID") class CreateTagResponse(BaseResponse): diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index 3a5dc4ef..d98811e0 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -4,9 +4,11 @@ import logging from httpx import HTTPStatusError from mcp.server.fastmcp import Context, FastMCP -from mcp.types import ToolAnnotations +from mcp.shared.exceptions import McpError +from mcp.types import ErrorData, ToolAnnotations from nextcloud_mcp_server.auth import require_scopes +from nextcloud_mcp_server.client.collectives import OCSError from nextcloud_mcp_server.context import get_client from nextcloud_mcp_server.models.collectives import ( Collective, @@ -19,6 +21,7 @@ from nextcloud_mcp_server.models.collectives import ( ListCollectivesResponse, ListPagesResponse, ListTagsResponse, + ListTrashedPagesResponse, PageInfo, PageOperationResponse, SearchPagesResponse, @@ -28,6 +31,13 @@ from nextcloud_mcp_server.observability.metrics import instrument_tool logger = logging.getLogger(__name__) +def _handle_collectives_error(e: OCSError | HTTPStatusError) -> McpError: + """Convert OCS or HTTP errors to McpError.""" + if isinstance(e, OCSError): + return McpError(ErrorData(code=e.status_code, message=e.message)) + return McpError(ErrorData(code=e.response.status_code, message=str(e))) + + def configure_collectives_tools(mcp: FastMCP): """Configure Nextcloud Collectives tools for the MCP server.""" @@ -44,7 +54,10 @@ def configure_collectives_tools(mcp: FastMCP): ) -> ListCollectivesResponse: """List all Nextcloud Collectives the user has access to""" client = await get_client(ctx) - raw_collectives = await client.collectives.get_collectives() + try: + raw_collectives = await client.collectives.get_collectives() + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e collectives = [Collective(**c) for c in raw_collectives] return ListCollectivesResponse(collectives=collectives, total=len(collectives)) @@ -63,7 +76,10 @@ def configure_collectives_tools(mcp: FastMCP): collective_id: ID of the collective """ client = await get_client(ctx) - raw_pages = await client.collectives.get_pages(collective_id) + try: + raw_pages = await client.collectives.get_pages(collective_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e pages = [PageInfo(**p) for p in raw_pages] return ListPagesResponse( pages=pages, total=len(pages), collective_id=collective_id @@ -89,7 +105,10 @@ def configure_collectives_tools(mcp: FastMCP): page_id: ID of the page """ client = await get_client(ctx) - raw_page = await client.collectives.get_page(collective_id, page_id) + try: + raw_page = await client.collectives.get_page(collective_id, page_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e page = PageInfo(**raw_page) # Fetch content via WebDAV @@ -130,7 +149,10 @@ def configure_collectives_tools(mcp: FastMCP): query: Search query string """ client = await get_client(ctx) - raw_pages = await client.collectives.search_pages(collective_id, query) + try: + raw_pages = await client.collectives.search_pages(collective_id, query) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e pages = [PageInfo(**p) for p in raw_pages] return SearchPagesResponse( results=pages, @@ -154,9 +176,12 @@ def configure_collectives_tools(mcp: FastMCP): collective_id: ID of the collective """ client = await get_client(ctx) - raw_tags = await client.collectives.get_tags(collective_id) + try: + raw_tags = await client.collectives.get_tags(collective_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e tags = [CollectiveTag(**t) for t in raw_tags] - return ListTagsResponse(tags=tags, total=len(tags)) + return ListTagsResponse(tags=tags, total=len(tags), collective_id=collective_id) @mcp.tool( title="List Trashed Collective Pages", @@ -166,16 +191,19 @@ def configure_collectives_tools(mcp: FastMCP): @instrument_tool async def collectives_get_trashed_pages( ctx: Context, collective_id: int - ) -> ListPagesResponse: + ) -> ListTrashedPagesResponse: """List trashed pages in a Nextcloud Collective Args: collective_id: ID of the collective """ client = await get_client(ctx) - raw_pages = await client.collectives.get_trashed_pages(collective_id) + try: + raw_pages = await client.collectives.get_trashed_pages(collective_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e pages = [PageInfo(**p) for p in raw_pages] - return ListPagesResponse( + return ListTrashedPagesResponse( pages=pages, total=len(pages), collective_id=collective_id ) @@ -197,7 +225,10 @@ def configure_collectives_tools(mcp: FastMCP): emoji: Optional emoji for the collective """ client = await get_client(ctx) - raw = await client.collectives.create_collective(name, emoji) + try: + raw = await client.collectives.create_collective(name, emoji) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e collective = Collective(**raw) return CreateCollectiveResponse( id=collective.id, name=collective.name, emoji=collective.emoji @@ -219,7 +250,12 @@ def configure_collectives_tools(mcp: FastMCP): emoji: New emoji for the collective """ client = await get_client(ctx) - raw = await client.collectives.update_collective(collective_id, emoji) + try: + raw = await client.collectives.update_collective(collective_id, emoji) + except ValueError as e: + raise McpError(ErrorData(code=400, message=str(e))) from e + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e collective = Collective(**raw) return CollectiveOperationResponse( collective_id=collective.id, @@ -248,7 +284,10 @@ def configure_collectives_tools(mcp: FastMCP): title: Title of the new page """ client = await get_client(ctx) - raw = await client.collectives.create_page(collective_id, parent_id, title) + try: + raw = await client.collectives.create_page(collective_id, parent_id, title) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e page = PageInfo(**raw) return CreatePageResponse( id=page.id, @@ -283,9 +322,12 @@ def configure_collectives_tools(mcp: FastMCP): copy: If true, copy instead of move """ client = await get_client(ctx) - raw = await client.collectives.move_page( - collective_id, page_id, parent_id, title, index, copy - ) + try: + raw = await client.collectives.move_page( + collective_id, page_id, parent_id, title, index, copy + ) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e page = PageInfo(**raw) action = "copied" if copy else "moved" return PageOperationResponse( @@ -313,7 +355,10 @@ def configure_collectives_tools(mcp: FastMCP): page_id: ID of the page to trash """ client = await get_client(ctx) - await client.collectives.trash_page(collective_id, page_id) + try: + await client.collectives.trash_page(collective_id, page_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e return PageOperationResponse( page_id=page_id, collective_id=collective_id, @@ -337,12 +382,16 @@ def configure_collectives_tools(mcp: FastMCP): page_id: ID of the page to restore """ client = await get_client(ctx) - await client.collectives.restore_page(collective_id, page_id) + try: + raw = await client.collectives.restore_page(collective_id, page_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e + page = PageInfo(**raw) return PageOperationResponse( - page_id=page_id, + page_id=page.id, collective_id=collective_id, status_code=200, - message="Page restored from trash", + message=f"Page restored from trash (title: {page.title})", ) @mcp.tool( @@ -365,7 +414,10 @@ def configure_collectives_tools(mcp: FastMCP): emoji: Emoji to set, or null to clear """ client = await get_client(ctx) - raw = await client.collectives.set_page_emoji(collective_id, page_id, emoji) + try: + raw = await client.collectives.set_page_emoji(collective_id, page_id, emoji) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e page = PageInfo(**raw) return PageOperationResponse( page_id=page.id, @@ -391,7 +443,10 @@ def configure_collectives_tools(mcp: FastMCP): color: Hex color code (e.g. "FF0000") """ client = await get_client(ctx) - raw = await client.collectives.create_tag(collective_id, name, color) + try: + raw = await client.collectives.create_tag(collective_id, name, color) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e tag = CollectiveTag(**raw) return CreateTagResponse(id=tag.id, name=tag.name, color=tag.color) @@ -412,7 +467,10 @@ def configure_collectives_tools(mcp: FastMCP): tag_id: ID of the tag to assign """ client = await get_client(ctx) - await client.collectives.assign_tag(collective_id, page_id, tag_id) + try: + await client.collectives.assign_tag(collective_id, page_id, tag_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e return PageOperationResponse( page_id=page_id, collective_id=collective_id, @@ -422,16 +480,17 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Remove Tag from Collective Page", - annotations=ToolAnnotations( - destructiveHint=True, idempotentHint=True, openWorldHint=True - ), + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), ) @require_scopes("collectives:write") @instrument_tool async def collectives_remove_tag( ctx: Context, collective_id: int, page_id: int, tag_id: int ) -> PageOperationResponse: - """Remove a tag from a page in a Nextcloud Collective + """Remove a tag from a page in a Nextcloud Collective. + + This is a reversible operation โ€” the tag still exists and can be + reassigned with collectives_assign_tag. Args: collective_id: ID of the collective @@ -439,7 +498,10 @@ def configure_collectives_tools(mcp: FastMCP): tag_id: ID of the tag to remove """ client = await get_client(ctx) - await client.collectives.remove_tag(collective_id, page_id, tag_id) + try: + await client.collectives.remove_tag(collective_id, page_id, tag_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e return PageOperationResponse( page_id=page_id, collective_id=collective_id, diff --git a/nextcloud_mcp_server/server/deck.py b/nextcloud_mcp_server/server/deck.py index f48e1345..c8d589d2 100644 --- a/nextcloud_mcp_server/server/deck.py +++ b/nextcloud_mcp_server/server/deck.py @@ -641,9 +641,7 @@ def configure_deck_tools(mcp: FastMCP): @mcp.tool( title="Remove Label from Deck Card", - annotations=ToolAnnotations( - destructiveHint=True, idempotentHint=True, openWorldHint=True - ), + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), ) @require_scopes("deck:write") @instrument_tool diff --git a/tests/server/test_annotations.py b/tests/server/test_annotations.py index 5d770e87..4ef1acd7 100644 --- a/tests/server/test_annotations.py +++ b/tests/server/test_annotations.py @@ -58,8 +58,9 @@ async def test_destructive_tools_have_correct_annotations(nc_mcp_client: ClientS """Verify destructive operations are marked correctly.""" tools = await nc_mcp_client.list_tools() - # Known destructive operations - destructive_keywords = ["delete", "remove", "revoke"] + # Known destructive operations (permanently delete data). + # "remove" is excluded โ€” removing associations (labels, tags) is reversible. + destructive_keywords = ["delete", "revoke"] for tool in tools.tools: has_destructive_keyword = any( diff --git a/tests/server/test_collectives_mcp.py b/tests/server/test_collectives_mcp.py index dee52889..e11f11c5 100644 --- a/tests/server/test_collectives_mcp.py +++ b/tests/server/test_collectives_mcp.py @@ -2,6 +2,7 @@ import json import logging +import os import uuid import httpx @@ -11,9 +12,10 @@ from mcp import ClientSession logger = logging.getLogger(__name__) pytestmark = pytest.mark.integration -# Nextcloud credentials for direct API cleanup (matches docker-compose.yml) -_NC_BASE = "http://localhost:8080" -_NC_AUTH = ("admin", "admin") +# Nextcloud credentials from environment (matches .envrc / docker-compose.yml defaults) +_NC_BASE = os.environ.get("NEXTCLOUD_HOST", "http://localhost:8080") +_NC_USER = os.environ.get("NEXTCLOUD_USERNAME", "admin") +_NC_PASS = os.environ.get("NEXTCLOUD_PASSWORD", "admin") _OCS_HEADERS = { "OCS-APIRequest": "true", "Accept": "application/json", @@ -38,13 +40,15 @@ async def temporary_collective(nc_mcp_client: ClientSession): collective_id = data["id"] logger.info(f"Created temporary collective: {name} (ID: {collective_id})") - # Get the landing page ID (auto-created with each collective) + # Get the landing page ID โ€” filter by parentId == 0 (root page) pages_result = await nc_mcp_client.call_tool( "collectives_get_pages", {"collective_id": collective_id}, ) pages_data = json.loads(pages_result.content[0].text) - landing_page_id = pages_data["pages"][0]["id"] + root_pages = [p for p in pages_data["pages"] if p["parentId"] == 0] + assert root_pages, "Expected at least one root page (landing page)" + landing_page_id = root_pages[0]["id"] yield { "id": collective_id, @@ -54,7 +58,9 @@ async def temporary_collective(nc_mcp_client: ClientSession): # Cleanup: trash and permanently delete the collective via direct OCS API try: - async with httpx.AsyncClient(base_url=_NC_BASE, auth=_NC_AUTH) as client: + async with httpx.AsyncClient( + base_url=_NC_BASE, auth=(_NC_USER, _NC_PASS) + ) as client: api = "/ocs/v2.php/apps/collectives/api/v1.0" await client.delete( f"{api}/collectives/{collective_id}", From f3caad122dd2a4b7347a02731c64f55e41a73cfd Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 25 Mar 2026 13:25:09 +0100 Subject: [PATCH 05/11] fix: address PR review feedback (round 3) Bugs: - assign_tag/remove_tag now call _unwrap_ocs to surface OCS-level errors - trash_page changed to idempotentHint=False (trashing twice errors) - WebDAV path parts stripped of slashes to prevent double-slash paths Robustness: - _unwrap_ocs uses ocs.get("data", {}) instead of ocs["data"] - Unit test added for missing data key in OCS envelope Minor: - MCP error codes use -1 (project convention) instead of HTTP status codes - update_collective docstring notes that emoji is required - CollectiveTag.color validated as hex format via field_validator Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/client/collectives.py | 8 ++++--- nextcloud_mcp_server/models/collectives.py | 13 +++++++++-- nextcloud_mcp_server/server/collectives.py | 14 ++++++----- .../collectives/test_collectives_api.py | 23 +++++++++++++++++-- 4 files changed, 45 insertions(+), 13 deletions(-) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index 82439eb2..49e048b6 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -40,7 +40,7 @@ class CollectivesClient(BaseNextcloudClient): if status_code >= 400: message = meta.get("message", "OCS error") raise OCSError(status_code, message) - return ocs["data"] + return ocs.get("data", {}) # Collectives @@ -215,19 +215,21 @@ class CollectivesClient(BaseNextcloudClient): async def assign_tag(self, collective_id: int, page_id: int, tag_id: int) -> None: """Assign a tag to a page.""" - await self._make_request( + response = await self._make_request( "PUT", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/tags/{tag_id}", headers=self._get_ocs_headers(), ) + self._unwrap_ocs(response.json()) async def remove_tag(self, collective_id: int, page_id: int, tag_id: int) -> None: """Remove a tag from a page.""" - await self._make_request( + response = await self._make_request( "DELETE", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/tags/{tag_id}", headers=self._get_ocs_headers(), ) + self._unwrap_ocs(response.json()) # Trash diff --git a/nextcloud_mcp_server/models/collectives.py b/nextcloud_mcp_server/models/collectives.py index 3e377f67..03eafafe 100644 --- a/nextcloud_mcp_server/models/collectives.py +++ b/nextcloud_mcp_server/models/collectives.py @@ -1,6 +1,8 @@ """Pydantic models for Nextcloud Collectives app.""" -from pydantic import BaseModel, Field +import re + +from pydantic import BaseModel, Field, field_validator from .base import BaseResponse, StatusResponse @@ -53,7 +55,14 @@ class CollectiveTag(BaseModel): id: int = Field(description="Tag ID") collectiveId: int = Field(description="Parent collective ID") name: str = Field(description="Tag name") - color: str = Field(description="Hex color code") + color: str = Field(description="Hex color code (e.g. 'FF0000')") + + @field_validator("color") + @classmethod + def validate_hex_color(cls, v: str) -> str: + if not re.fullmatch(r"[0-9A-Fa-f]{3,8}", v): + raise ValueError(f"Invalid hex color: {v!r}") + return v # Response Models diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index d98811e0..f3a346cc 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -34,8 +34,8 @@ logger = logging.getLogger(__name__) def _handle_collectives_error(e: OCSError | HTTPStatusError) -> McpError: """Convert OCS or HTTP errors to McpError.""" if isinstance(e, OCSError): - return McpError(ErrorData(code=e.status_code, message=e.message)) - return McpError(ErrorData(code=e.response.status_code, message=str(e))) + return McpError(ErrorData(code=-1, message=e.message)) + return McpError(ErrorData(code=-1, message=str(e))) def configure_collectives_tools(mcp: FastMCP): @@ -120,7 +120,7 @@ def configure_collectives_tools(mcp: FastMCP): if page.filePath: parts.append(page.filePath) parts.append(page.fileName) - webdav_path = "/".join(parts) + webdav_path = "/".join(p.strip("/") for p in parts) try: file_bytes, _ = await client.webdav.read_file(webdav_path) content = file_bytes.decode("utf-8") @@ -243,11 +243,13 @@ def configure_collectives_tools(mcp: FastMCP): async def collectives_update_collective( ctx: Context, collective_id: int, emoji: str | None = None ) -> CollectiveOperationResponse: - """Update a Nextcloud Collective (emoji) + """Update a Nextcloud Collective (emoji). + + At least one field must be provided. Args: collective_id: ID of the collective - emoji: New emoji for the collective + emoji: New emoji for the collective (required) """ client = await get_client(ctx) try: @@ -340,7 +342,7 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Trash Collective Page", annotations=ToolAnnotations( - destructiveHint=True, idempotentHint=True, openWorldHint=True + destructiveHint=True, idempotentHint=False, openWorldHint=True ), ) @require_scopes("collectives:write") diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py index 5bdc3ca4..9207b35d 100644 --- a/tests/client/collectives/test_collectives_api.py +++ b/tests/client/collectives/test_collectives_api.py @@ -263,7 +263,7 @@ async def test_create_tag(mocker): async def test_assign_tag(mocker): """Test assigning a tag to a page.""" - mock_response = create_mock_response(status_code=200, json_data={}) + mock_response = _ocs_response({}) mock_request = mocker.patch.object( CollectivesClient, "_make_request", return_value=mock_response ) @@ -278,7 +278,7 @@ async def test_assign_tag(mocker): async def test_remove_tag(mocker): """Test removing a tag from a page.""" - mock_response = create_mock_response(status_code=200, json_data={}) + mock_response = _ocs_response({}) mock_request = mocker.patch.object( CollectivesClient, "_make_request", return_value=mock_response ) @@ -327,6 +327,25 @@ async def test_restore_page(mocker): # --- Error Handling --- +async def test_ocs_missing_data_returns_empty(mocker): + """Test that OCS envelope without 'data' key returns empty dict.""" + mock_response = create_mock_response( + status_code=200, + json_data={ + "ocs": { + "meta": {"status": "ok", "statuscode": 200}, + } + }, + ) + mocker.patch.object(CollectivesClient, "_make_request", return_value=mock_response) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + # get_collectives accesses data["collectives"], which will KeyError on empty dict + # This tests that _unwrap_ocs itself doesn't crash โ€” it returns {} + with pytest.raises(KeyError): + await client.get_collectives() + + async def test_ocs_error_status_raises(mocker): """Test that OCS envelope with error statuscode raises OCSError.""" mock_response = create_mock_response( From 95edd9ba8e85ccfe7b6c4d595af8af281d827a61 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 26 Mar 2026 07:36:40 +0100 Subject: [PATCH 06/11] fix: add trash/delete collective tools and address review feedback (round 4) Add collectives_trash_collective and collectives_delete_collective MCP tools with proper destructiveHint annotations. Refactor integration test fixture to use MCP tools for cleanup instead of direct httpx/OCS calls. Optimize _get_ocs_headers() to class-level constant. Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/client/collectives.py | 32 ++++++++-- nextcloud_mcp_server/server/collectives.py | 60 +++++++++++++++++++ pyproject.toml | 1 + .../collectives/test_collectives_api.py | 31 ++++++++++ tests/server/test_collectives_mcp.py | 35 ++++------- uv.lock | 48 +++++++++++++++ 6 files changed, 178 insertions(+), 29 deletions(-) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index 49e048b6..0f1c1629 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -24,13 +24,15 @@ class CollectivesClient(BaseNextcloudClient): app_name = "collectives" + _OCS_HEADERS: dict[str, str] = { + "OCS-APIRequest": "true", + "Content-Type": "application/json", + "Accept": "application/json", + } + def _get_ocs_headers(self) -> dict[str, str]: """Get standard headers required for OCS API calls.""" - return { - "OCS-APIRequest": "true", - "Content-Type": "application/json", - "Accept": "application/json", - } + return self._OCS_HEADERS def _unwrap_ocs(self, response_json: dict[str, Any]) -> Any: """Unwrap OCS envelope, validating the status before returning data.""" @@ -90,6 +92,26 @@ class CollectivesClient(BaseNextcloudClient): data = self._unwrap_ocs(response.json()) return data["collective"] + async def trash_collective(self, collective_id: int) -> None: + """Move a collective to trash (soft delete).""" + await self._make_request( + "DELETE", + f"{API_BASE}/collectives/{collective_id}", + headers=self._get_ocs_headers(), + ) + + async def delete_collective(self, collective_id: int) -> None: + """Permanently delete a collective (must be trashed first). + + This is irreversible. The collective must be in the trash before + calling this method. + """ + await self._make_request( + "DELETE", + f"{API_BASE}/collectives/trash/{collective_id}", + headers=self._get_ocs_headers(), + ) + # Pages async def get_pages(self, collective_id: int) -> list[dict[str, Any]]: diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index f3a346cc..44fed112 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -265,6 +265,66 @@ def configure_collectives_tools(mcp: FastMCP): message=f"Collective updated (emoji: {collective.emoji})", ) + @mcp.tool( + title="Trash Collective", + annotations=ToolAnnotations( + destructiveHint=True, idempotentHint=False, openWorldHint=True + ), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_trash_collective( + ctx: Context, collective_id: int + ) -> CollectiveOperationResponse: + """Move a Nextcloud Collective to trash (soft delete). + + The collective can be restored or permanently deleted afterwards. + + Args: + collective_id: ID of the collective to trash + """ + client = await get_client(ctx) + try: + await client.collectives.trash_collective(collective_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e + return CollectiveOperationResponse( + collective_id=collective_id, + status_code=200, + message="Collective moved to trash", + ) + + @mcp.tool( + title="Delete Collective", + annotations=ToolAnnotations( + destructiveHint=True, idempotentHint=True, openWorldHint=True + ), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_delete_collective( + ctx: Context, collective_id: int + ) -> CollectiveOperationResponse: + """Permanently delete a Nextcloud Collective. + + WARNING: This is irreversible. The collective must be in the trash + first (use collectives_trash_collective). All pages and content + will be permanently destroyed. + + Args: + collective_id: ID of the trashed collective to permanently delete + """ + client = await get_client(ctx) + try: + await client.collectives.delete_collective(collective_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e + return CollectiveOperationResponse( + collective_id=collective_id, + status_code=200, + message="Collective permanently deleted", + ) + @mcp.tool( title="Create Collective Page", annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), diff --git a/pyproject.toml b/pyproject.toml index 18b14f04..d4d21f70 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -135,6 +135,7 @@ dev = [ "ruff>=0.11.13", "reportlab>=4.0.0", "ty>=0.0.1a25", + "pytest-otel>=2.0.1", ] [project.scripts] diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py index 9207b35d..3da227d6 100644 --- a/tests/client/collectives/test_collectives_api.py +++ b/tests/client/collectives/test_collectives_api.py @@ -115,6 +115,37 @@ async def test_create_collective(mocker): assert call_args[1]["json"]["emoji"] == "๐Ÿ“š" +async def test_trash_collective(mocker): + """Test trashing a collective sends DELETE to correct endpoint.""" + mock_response = create_mock_response(status_code=200, json_data={}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.trash_collective(collective_id=5) + + call_args = mock_request.call_args + assert call_args[0][0] == "DELETE" + assert "/collectives/5" in call_args[0][1] + assert "/trash" not in call_args[0][1] + + +async def test_delete_collective(mocker): + """Test permanently deleting a collective sends DELETE to trash endpoint.""" + mock_response = create_mock_response(status_code=200, json_data={}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.delete_collective(collective_id=5) + + call_args = mock_request.call_args + assert call_args[0][0] == "DELETE" + assert "/collectives/trash/5" in call_args[0][1] + + # --- Pages --- diff --git a/tests/server/test_collectives_mcp.py b/tests/server/test_collectives_mcp.py index e11f11c5..2e134bf5 100644 --- a/tests/server/test_collectives_mcp.py +++ b/tests/server/test_collectives_mcp.py @@ -2,25 +2,14 @@ import json import logging -import os import uuid -import httpx import pytest from mcp import ClientSession logger = logging.getLogger(__name__) pytestmark = pytest.mark.integration -# Nextcloud credentials from environment (matches .envrc / docker-compose.yml defaults) -_NC_BASE = os.environ.get("NEXTCLOUD_HOST", "http://localhost:8080") -_NC_USER = os.environ.get("NEXTCLOUD_USERNAME", "admin") -_NC_PASS = os.environ.get("NEXTCLOUD_PASSWORD", "admin") -_OCS_HEADERS = { - "OCS-APIRequest": "true", - "Accept": "application/json", -} - # --- Fixtures --- @@ -56,20 +45,16 @@ async def temporary_collective(nc_mcp_client: ClientSession): "landing_page_id": landing_page_id, } - # Cleanup: trash and permanently delete the collective via direct OCS API + # Cleanup: trash and permanently delete the collective via MCP tools try: - async with httpx.AsyncClient( - base_url=_NC_BASE, auth=(_NC_USER, _NC_PASS) - ) as client: - api = "/ocs/v2.php/apps/collectives/api/v1.0" - await client.delete( - f"{api}/collectives/{collective_id}", - headers=_OCS_HEADERS, - ) - await client.delete( - f"{api}/collectives/trash/{collective_id}", - headers=_OCS_HEADERS, - ) + await nc_mcp_client.call_tool( + "collectives_trash_collective", + {"collective_id": collective_id}, + ) + await nc_mcp_client.call_tool( + "collectives_delete_collective", + {"collective_id": collective_id}, + ) logger.info(f"Cleaned up collective: {collective_id}") except Exception as e: logger.warning(f"Cleanup of collective {collective_id} failed: {e}") @@ -87,6 +72,8 @@ async def test_collectives_tools_available(nc_mcp_client: ClientSession): "collectives_get_collectives", "collectives_create_collective", "collectives_update_collective", + "collectives_trash_collective", + "collectives_delete_collective", "collectives_get_pages", "collectives_get_page", "collectives_create_page", diff --git a/uv.lock b/uv.lock index 45f39fe1..79f6ffdf 100644 --- a/uv.lock +++ b/uv.lock @@ -2131,6 +2131,7 @@ dev = [ { name = "pytest" }, { name = "pytest-cov" }, { name = "pytest-mock" }, + { name = "pytest-otel" }, { name = "pytest-playwright-asyncio" }, { name = "pytest-timeout" }, { name = "reportlab" }, @@ -2182,6 +2183,7 @@ dev = [ { name = "pytest", specifier = ">=8.3.5" }, { name = "pytest-cov", specifier = ">=6.1.1" }, { name = "pytest-mock", specifier = ">=3.15.1" }, + { name = "pytest-otel", specifier = ">=2.0.1" }, { name = "pytest-playwright-asyncio", specifier = ">=0.7.1" }, { name = "pytest-timeout", specifier = ">=2.3.1" }, { name = "reportlab", specifier = ">=4.0.0" }, @@ -2348,6 +2350,19 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/05/85/d831a9bc0a9e0e1a304ff3d12c1489a5fbc9bf6690a15dcbdae372bbca45/opentelemetry_api-1.39.0-py3-none-any.whl", hash = "sha256:3c3b3ca5c5687b1b5b37e5c5027ff68eacea8675241b29f13110a8ffbb8f0459", size = 66357, upload-time = "2025-12-03T13:19:33.043Z" }, ] +[[package]] +name = "opentelemetry-exporter-otlp" +version = "1.39.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "opentelemetry-exporter-otlp-proto-grpc" }, + { name = "opentelemetry-exporter-otlp-proto-http" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/13/be/0e9d889f47e55cadc4041e5b53d4e0cc688f9a74811134fb0ba7cbee6905/opentelemetry_exporter_otlp-1.39.0.tar.gz", hash = "sha256:b405da0287b895fe4e2450dedb2a5b072debba1dfcfed5bdb3d1d183d8daa296", size = 6146, upload-time = "2025-12-03T13:19:58.381Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/fc/35/212d2cae4fa9a2c02e74438612268b640ab577b8ccb04590371eb4e0f542/opentelemetry_exporter_otlp-1.39.0-py3-none-any.whl", hash = "sha256:fe155d6968d581b325574ad6dc267c8de299397b18d11feeda2206d0a47928a9", size = 7017, upload-time = "2025-12-03T13:19:35.686Z" }, +] + [[package]] name = "opentelemetry-exporter-otlp-proto-common" version = "1.39.0" @@ -2378,6 +2393,24 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/56/e8/d420b94ffddfd8cff85bb4aa5d98da26ce7935dc3cf3eca6b83cd39ab436/opentelemetry_exporter_otlp_proto_grpc-1.39.0-py3-none-any.whl", hash = "sha256:758641278050de9bb895738f35ff8840e4a47685b7e6ef4a201fe83196ba7a05", size = 19765, upload-time = "2025-12-03T13:19:38.143Z" }, ] +[[package]] +name = "opentelemetry-exporter-otlp-proto-http" +version = "1.39.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "googleapis-common-protos" }, + { name = "opentelemetry-api" }, + { name = "opentelemetry-exporter-otlp-proto-common" }, + { name = "opentelemetry-proto" }, + { name = "opentelemetry-sdk" }, + { name = "requests" }, + { name = "typing-extensions" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/81/dc/1e9bf3f6a28e29eba516bc0266e052996d02bc7e92675f3cd38169607609/opentelemetry_exporter_otlp_proto_http-1.39.0.tar.gz", hash = "sha256:28d78fc0eb82d5a71ae552263d5012fa3ebad18dfd189bf8d8095ba0e65ee1ed", size = 17287, upload-time = "2025-12-03T13:20:01.134Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/bc/46/e4a102e17205bb05a50dbf24ef0e92b66b648cd67db9a68865af06a242fd/opentelemetry_exporter_otlp_proto_http-1.39.0-py3-none-any.whl", hash = "sha256:5789cb1375a8b82653328c0ce13a054d285f774099faf9d068032a49de4c7862", size = 19639, upload-time = "2025-12-03T13:19:39.536Z" }, +] + [[package]] name = "opentelemetry-instrumentation" version = "0.60b0" @@ -3296,6 +3329,21 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/5a/cc/06253936f4a7fa2e0f48dfe6d851d9c56df896a9ab09ac019d70b760619c/pytest_mock-3.15.1-py3-none-any.whl", hash = "sha256:0a25e2eb88fe5168d535041d09a4529a188176ae608a6d249ee65abc0949630d", size = 10095, upload-time = "2025-09-16T16:37:25.734Z" }, ] +[[package]] +name = "pytest-otel" +version = "2.0.1" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "opentelemetry-api" }, + { name = "opentelemetry-exporter-otlp" }, + { name = "opentelemetry-sdk" }, + { name = "pytest" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/ca/5e/771f8dbdf55ae57603d89bb26e046c13e8bee3492c74cb3afb6044166e9a/pytest_otel-2.0.1.tar.gz", hash = "sha256:3d529dc34105862cca39fd1258d00dd3d17f3b7d92ebf0953d55326bb017af3d", size = 17880, upload-time = "2025-12-08T14:56:40.393Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/16/fa/0cfe23bac571f68b4f427ad7ae313ddd8de881ec2ff49dcea196a2a96592/pytest_otel-2.0.1-py2.py3-none-any.whl", hash = "sha256:501f36f02f55578ca34c3ccbe55e81cc1e14bb130da13f7cf0b6480d54a9e5db", size = 14530, upload-time = "2025-12-08T14:56:41.731Z" }, +] + [[package]] name = "pytest-playwright-asyncio" version = "0.7.2" From aa46c6147bf42bb4b6357a4efcbe089dc4e3c7fc Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 26 Mar 2026 13:49:28 +0100 Subject: [PATCH 07/11] fix: address PR review feedback (round 5) - Validate OCS envelope in trash_collective, delete_collective, trash_page - Guard _unwrap_ocs against non-OCS responses with informative OCSError - Remove _get_ocs_headers() indirection, use class constants directly - Split headers: _OCS_HEADERS (GET) vs _OCS_HEADERS_JSON (with body) - Fix docstring claiming emoji param is required when it is optional - Rename misleading test, add test for non-OCS envelope handling Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/client/collectives.py | 57 ++++++++++--------- nextcloud_mcp_server/server/collectives.py | 2 +- .../collectives/test_collectives_api.py | 27 ++++++--- 3 files changed, 52 insertions(+), 34 deletions(-) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index 0f1c1629..5a3a44f7 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -26,17 +26,19 @@ class CollectivesClient(BaseNextcloudClient): _OCS_HEADERS: dict[str, str] = { "OCS-APIRequest": "true", - "Content-Type": "application/json", "Accept": "application/json", } - def _get_ocs_headers(self) -> dict[str, str]: - """Get standard headers required for OCS API calls.""" - return self._OCS_HEADERS + _OCS_HEADERS_JSON: dict[str, str] = { + **_OCS_HEADERS, + "Content-Type": "application/json", + } def _unwrap_ocs(self, response_json: dict[str, Any]) -> Any: """Unwrap OCS envelope, validating the status before returning data.""" - ocs = response_json["ocs"] + ocs = response_json.get("ocs") + if ocs is None: + raise OCSError(500, "Response is not an OCS envelope") meta = ocs.get("meta", {}) status_code = meta.get("statuscode", 200) if status_code >= 400: @@ -49,7 +51,7 @@ class CollectivesClient(BaseNextcloudClient): async def get_collectives(self) -> list[dict[str, Any]]: """List all collectives the user has access to.""" response = await self._make_request( - "GET", f"{API_BASE}/collectives", headers=self._get_ocs_headers() + "GET", f"{API_BASE}/collectives", headers=self._OCS_HEADERS ) data = self._unwrap_ocs(response.json()) return data["collectives"] @@ -65,7 +67,7 @@ class CollectivesClient(BaseNextcloudClient): "POST", f"{API_BASE}/collectives", json=json_data, - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS_JSON, ) data = self._unwrap_ocs(response.json()) return data["collective"] @@ -87,18 +89,19 @@ class CollectivesClient(BaseNextcloudClient): "PUT", f"{API_BASE}/collectives/{collective_id}", json=json_data, - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS_JSON, ) data = self._unwrap_ocs(response.json()) return data["collective"] async def trash_collective(self, collective_id: int) -> None: """Move a collective to trash (soft delete).""" - await self._make_request( + response = await self._make_request( "DELETE", f"{API_BASE}/collectives/{collective_id}", - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS, ) + self._unwrap_ocs(response.json()) async def delete_collective(self, collective_id: int) -> None: """Permanently delete a collective (must be trashed first). @@ -106,11 +109,12 @@ class CollectivesClient(BaseNextcloudClient): This is irreversible. The collective must be in the trash before calling this method. """ - await self._make_request( + response = await self._make_request( "DELETE", f"{API_BASE}/collectives/trash/{collective_id}", - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS, ) + self._unwrap_ocs(response.json()) # Pages @@ -119,7 +123,7 @@ class CollectivesClient(BaseNextcloudClient): response = await self._make_request( "GET", f"{API_BASE}/collectives/{collective_id}/pages", - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS, ) data = self._unwrap_ocs(response.json()) return data["pages"] @@ -129,7 +133,7 @@ class CollectivesClient(BaseNextcloudClient): response = await self._make_request( "GET", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}", - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS, ) data = self._unwrap_ocs(response.json()) return data["page"] @@ -143,7 +147,7 @@ class CollectivesClient(BaseNextcloudClient): "POST", f"{API_BASE}/collectives/{collective_id}/pages/{parent_id}", json=json_data, - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS_JSON, ) data = self._unwrap_ocs(response.json()) return data["page"] @@ -167,18 +171,19 @@ class CollectivesClient(BaseNextcloudClient): "PUT", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}", json=json_data, - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS_JSON, ) data = self._unwrap_ocs(response.json()) return data["page"] async def trash_page(self, collective_id: int, page_id: int) -> None: """Move a page to trash (soft delete).""" - await self._make_request( + response = await self._make_request( "DELETE", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}", - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS, ) + self._unwrap_ocs(response.json()) async def set_page_emoji( self, collective_id: int, page_id: int, emoji: str | None @@ -189,7 +194,7 @@ class CollectivesClient(BaseNextcloudClient): "PUT", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/emoji", json=json_data, - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS_JSON, ) data = self._unwrap_ocs(response.json()) return data["page"] @@ -204,7 +209,7 @@ class CollectivesClient(BaseNextcloudClient): "GET", f"{API_BASE}/collectives/{collective_id}/search", params={"searchString": query}, - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS, ) data = self._unwrap_ocs(response.json()) return data["pages"] @@ -216,7 +221,7 @@ class CollectivesClient(BaseNextcloudClient): response = await self._make_request( "GET", f"{API_BASE}/collectives/{collective_id}/tags", - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS, ) data = self._unwrap_ocs(response.json()) return data["tags"] @@ -230,7 +235,7 @@ class CollectivesClient(BaseNextcloudClient): "POST", f"{API_BASE}/collectives/{collective_id}/tags", json=json_data, - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS_JSON, ) data = self._unwrap_ocs(response.json()) return data["tag"] @@ -240,7 +245,7 @@ class CollectivesClient(BaseNextcloudClient): response = await self._make_request( "PUT", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/tags/{tag_id}", - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS_JSON, ) self._unwrap_ocs(response.json()) @@ -249,7 +254,7 @@ class CollectivesClient(BaseNextcloudClient): response = await self._make_request( "DELETE", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/tags/{tag_id}", - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS, ) self._unwrap_ocs(response.json()) @@ -260,7 +265,7 @@ class CollectivesClient(BaseNextcloudClient): response = await self._make_request( "GET", f"{API_BASE}/collectives/{collective_id}/pages/trash", - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS, ) data = self._unwrap_ocs(response.json()) return data["pages"] @@ -270,7 +275,7 @@ class CollectivesClient(BaseNextcloudClient): response = await self._make_request( "PATCH", f"{API_BASE}/collectives/{collective_id}/pages/trash/{page_id}", - headers=self._get_ocs_headers(), + headers=self._OCS_HEADERS, ) data = self._unwrap_ocs(response.json()) return data["page"] diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index 44fed112..540993f7 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -249,7 +249,7 @@ def configure_collectives_tools(mcp: FastMCP): Args: collective_id: ID of the collective - emoji: New emoji for the collective (required) + emoji: New emoji for the collective """ client = await get_client(ctx) try: diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py index 3da227d6..98c712f6 100644 --- a/tests/client/collectives/test_collectives_api.py +++ b/tests/client/collectives/test_collectives_api.py @@ -117,7 +117,7 @@ async def test_create_collective(mocker): async def test_trash_collective(mocker): """Test trashing a collective sends DELETE to correct endpoint.""" - mock_response = create_mock_response(status_code=200, json_data={}) + mock_response = _ocs_response({}) mock_request = mocker.patch.object( CollectivesClient, "_make_request", return_value=mock_response ) @@ -133,7 +133,7 @@ async def test_trash_collective(mocker): async def test_delete_collective(mocker): """Test permanently deleting a collective sends DELETE to trash endpoint.""" - mock_response = create_mock_response(status_code=200, json_data={}) + mock_response = _ocs_response({}) mock_request = mocker.patch.object( CollectivesClient, "_make_request", return_value=mock_response ) @@ -203,7 +203,7 @@ async def test_create_page(mocker): async def test_trash_page(mocker): """Test trashing a page sends DELETE.""" - mock_response = create_mock_response(status_code=200, json_data={}) + mock_response = _ocs_response({}) mock_request = mocker.patch.object( CollectivesClient, "_make_request", return_value=mock_response ) @@ -358,8 +358,8 @@ async def test_restore_page(mocker): # --- Error Handling --- -async def test_ocs_missing_data_returns_empty(mocker): - """Test that OCS envelope without 'data' key returns empty dict.""" +async def test_ocs_missing_data_raises_key_error(mocker): + """Test that OCS envelope without 'data' key causes KeyError on field access.""" mock_response = create_mock_response( status_code=200, json_data={ @@ -371,12 +371,25 @@ async def test_ocs_missing_data_returns_empty(mocker): mocker.patch.object(CollectivesClient, "_make_request", return_value=mock_response) client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") - # get_collectives accesses data["collectives"], which will KeyError on empty dict - # This tests that _unwrap_ocs itself doesn't crash โ€” it returns {} + # _unwrap_ocs returns {} when "data" is absent; the caller then + # raises KeyError when accessing the expected key (e.g. "collectives") with pytest.raises(KeyError): await client.get_collectives() +async def test_non_ocs_envelope_raises_ocs_error(mocker): + """Test that a non-OCS response (e.g. proxy error) raises OCSError.""" + mock_response = create_mock_response( + status_code=200, + json_data={"error": "Bad Gateway"}, + ) + mocker.patch.object(CollectivesClient, "_make_request", return_value=mock_response) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + with pytest.raises(OCSError, match="not an OCS envelope"): + await client.get_collectives() + + async def test_ocs_error_status_raises(mocker): """Test that OCS envelope with error statuscode raises OCSError.""" mock_response = create_mock_response( From 85119bde91f950b6c92a66aa6dd1d465974d5fc3 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 26 Mar 2026 14:38:11 +0100 Subject: [PATCH 08/11] fix: address PR review feedback (round 6) - Fix assign_tag sending Content-Type header with no body - Mark collectives_update_collective as idempotent (no ETag involved) - Raise OCSError when 'data' key missing instead of silent fallback - Tighten color validator to 3 or 6 hex chars only - Add comment explaining null emoji semantics in set_page_emoji Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/client/collectives.py | 7 +++++-- nextcloud_mcp_server/models/collectives.py | 2 +- nextcloud_mcp_server/server/collectives.py | 2 +- tests/client/collectives/test_collectives_api.py | 8 +++----- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index 5a3a44f7..6fd140bb 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -44,7 +44,9 @@ class CollectivesClient(BaseNextcloudClient): if status_code >= 400: message = meta.get("message", "OCS error") raise OCSError(status_code, message) - return ocs.get("data", {}) + if "data" not in ocs: + raise OCSError(500, "OCS response missing 'data' field") + return ocs["data"] # Collectives @@ -189,6 +191,7 @@ class CollectivesClient(BaseNextcloudClient): self, collective_id: int, page_id: int, emoji: str | None ) -> dict[str, Any]: """Set or clear the emoji on a page.""" + # Sending {"emoji": null} intentionally clears the emoji on the server json_data = {"emoji": emoji} response = await self._make_request( "PUT", @@ -245,7 +248,7 @@ class CollectivesClient(BaseNextcloudClient): response = await self._make_request( "PUT", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/tags/{tag_id}", - headers=self._OCS_HEADERS_JSON, + headers=self._OCS_HEADERS, ) self._unwrap_ocs(response.json()) diff --git a/nextcloud_mcp_server/models/collectives.py b/nextcloud_mcp_server/models/collectives.py index 03eafafe..25259407 100644 --- a/nextcloud_mcp_server/models/collectives.py +++ b/nextcloud_mcp_server/models/collectives.py @@ -60,7 +60,7 @@ class CollectiveTag(BaseModel): @field_validator("color") @classmethod def validate_hex_color(cls, v: str) -> str: - if not re.fullmatch(r"[0-9A-Fa-f]{3,8}", v): + if not re.fullmatch(r"[0-9A-Fa-f]{3}(?:[0-9A-Fa-f]{3})?", v): raise ValueError(f"Invalid hex color: {v!r}") return v diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index 540993f7..44eba34d 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -236,7 +236,7 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Update Collective", - annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), ) @require_scopes("collectives:write") @instrument_tool diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py index 98c712f6..e6865c59 100644 --- a/tests/client/collectives/test_collectives_api.py +++ b/tests/client/collectives/test_collectives_api.py @@ -358,8 +358,8 @@ async def test_restore_page(mocker): # --- Error Handling --- -async def test_ocs_missing_data_raises_key_error(mocker): - """Test that OCS envelope without 'data' key causes KeyError on field access.""" +async def test_ocs_missing_data_raises_ocs_error(mocker): + """Test that OCS envelope without 'data' key raises OCSError.""" mock_response = create_mock_response( status_code=200, json_data={ @@ -371,9 +371,7 @@ async def test_ocs_missing_data_raises_key_error(mocker): mocker.patch.object(CollectivesClient, "_make_request", return_value=mock_response) client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") - # _unwrap_ocs returns {} when "data" is absent; the caller then - # raises KeyError when accessing the expected key (e.g. "collectives") - with pytest.raises(KeyError): + with pytest.raises(OCSError, match="missing 'data' field"): await client.get_collectives() From 7224a2ebe32ea966d764e5cd760a734510514dee Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 26 Mar 2026 23:18:42 +0100 Subject: [PATCH 09/11] fix: address PR review feedback (round 7) and fix CI - Rename collectives_update_collective to collectives_set_collective_emoji (more precise since only emoji is settable) - Use standard JSON-RPC error code -32603 (INTERNAL_ERROR) instead of -1 - Handle UnicodeDecodeError when reading page content via WebDAV - Replace brittle 'Welcome' content assertion with length check Fixes CI: test_update_operations_not_idempotent no longer matches the renamed tool, which is correctly idempotent (no ETag involved). Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/server/collectives.py | 20 ++++++++++---------- tests/server/test_collectives_mcp.py | 10 +++++----- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index 44eba34d..85e8f194 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -34,8 +34,8 @@ logger = logging.getLogger(__name__) def _handle_collectives_error(e: OCSError | HTTPStatusError) -> McpError: """Convert OCS or HTTP errors to McpError.""" if isinstance(e, OCSError): - return McpError(ErrorData(code=-1, message=e.message)) - return McpError(ErrorData(code=-1, message=str(e))) + return McpError(ErrorData(code=-32603, message=e.message)) + return McpError(ErrorData(code=-32603, message=str(e))) def configure_collectives_tools(mcp: FastMCP): @@ -124,7 +124,7 @@ def configure_collectives_tools(mcp: FastMCP): try: file_bytes, _ = await client.webdav.read_file(webdav_path) content = file_bytes.decode("utf-8") - except (HTTPStatusError, OSError) as e: + except (HTTPStatusError, OSError, UnicodeDecodeError) as e: logger.warning( "Failed to read page content via WebDAV: %s: %s", webdav_path, @@ -235,21 +235,21 @@ def configure_collectives_tools(mcp: FastMCP): ) @mcp.tool( - title="Update Collective", + title="Set Collective Emoji", annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), ) @require_scopes("collectives:write") @instrument_tool - async def collectives_update_collective( - ctx: Context, collective_id: int, emoji: str | None = None + async def collectives_set_collective_emoji( + ctx: Context, collective_id: int, emoji: str ) -> CollectiveOperationResponse: - """Update a Nextcloud Collective (emoji). + """Set the emoji on a Nextcloud Collective. - At least one field must be provided. + Setting the same emoji twice produces the same result (idempotent). Args: collective_id: ID of the collective - emoji: New emoji for the collective + emoji: Emoji to set on the collective """ client = await get_client(ctx) try: @@ -262,7 +262,7 @@ def configure_collectives_tools(mcp: FastMCP): return CollectiveOperationResponse( collective_id=collective.id, status_code=200, - message=f"Collective updated (emoji: {collective.emoji})", + message=f"Collective emoji set to: {collective.emoji}", ) @mcp.tool( diff --git a/tests/server/test_collectives_mcp.py b/tests/server/test_collectives_mcp.py index 2e134bf5..c69272ba 100644 --- a/tests/server/test_collectives_mcp.py +++ b/tests/server/test_collectives_mcp.py @@ -71,7 +71,7 @@ async def test_collectives_tools_available(nc_mcp_client: ClientSession): expected_tools = [ "collectives_get_collectives", "collectives_create_collective", - "collectives_update_collective", + "collectives_set_collective_emoji", "collectives_trash_collective", "collectives_delete_collective", "collectives_get_pages", @@ -116,12 +116,12 @@ async def test_collectives_list( logger.info(f"Found {data['total']} collectives") -async def test_collectives_update_emoji( +async def test_collectives_set_collective_emoji( nc_mcp_client: ClientSession, temporary_collective: dict ): - """Test updating a collective's emoji.""" + """Test setting a collective's emoji.""" result = await nc_mcp_client.call_tool( - "collectives_update_collective", + "collectives_set_collective_emoji", {"collective_id": temporary_collective["id"], "emoji": "๐Ÿ“–"}, ) assert result.isError is False @@ -242,7 +242,7 @@ async def test_collectives_get_landing_page_content( assert data["content"] is not None, ( "Landing page should have auto-generated content" ) - assert "Welcome" in data["content"], "Landing page should contain welcome text" + assert len(data["content"]) > 0, "Landing page should have non-empty content" logger.info(f"Landing page content: {len(data['content'])} bytes") From cb16060b1b450e5803fb32daa78d98a1d73baea8 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 27 Mar 2026 11:26:26 +0100 Subject: [PATCH 10/11] fix: address PR review feedback (round 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix inconsistent error code in set_collective_emoji (400 โ†’ -32603) - Allow clearing emoji via set_collective_emoji(emoji=None) - Remove destructiveHint from trash operations (soft deletes are recoverable) - Change delete_collective to idempotentHint=False (requires trash precondition) - Add restore_collective and get_trashed_collectives tools - Add unit tests for ValueError guard, clear-emoji path, and new tools - Add integration test for full trash/restore/delete lifecycle - Verify move_page returns new title in response message Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/client/collectives.py | 20 ++++++ nextcloud_mcp_server/models/collectives.py | 7 ++ nextcloud_mcp_server/server/collectives.py | 70 ++++++++++++++++--- .../collectives/test_collectives_api.py | 64 +++++++++++++++++ tests/server/test_annotations.py | 6 +- tests/server/test_collectives_mcp.py | 68 ++++++++++++++++++ 6 files changed, 223 insertions(+), 12 deletions(-) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index 6fd140bb..4df636fb 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -118,6 +118,26 @@ class CollectivesClient(BaseNextcloudClient): ) self._unwrap_ocs(response.json()) + # Trash (collectives) + + async def get_trashed_collectives(self) -> list[dict[str, Any]]: + """List trashed collectives.""" + response = await self._make_request( + "GET", f"{API_BASE}/collectives/trash", headers=self._OCS_HEADERS + ) + data = self._unwrap_ocs(response.json()) + return data["collectives"] + + async def restore_collective(self, collective_id: int) -> dict[str, Any]: + """Restore a collective from trash.""" + response = await self._make_request( + "PATCH", + f"{API_BASE}/collectives/trash/{collective_id}", + headers=self._OCS_HEADERS, + ) + data = self._unwrap_ocs(response.json()) + return data["collective"] + # Pages async def get_pages(self, collective_id: int) -> list[dict[str, Any]]: diff --git a/nextcloud_mcp_server/models/collectives.py b/nextcloud_mcp_server/models/collectives.py index 25259407..538b366e 100644 --- a/nextcloud_mcp_server/models/collectives.py +++ b/nextcloud_mcp_server/models/collectives.py @@ -140,6 +140,13 @@ class ListTrashedPagesResponse(ListPagesResponse): ) +class ListTrashedCollectivesResponse(BaseResponse): + """Response for listing trashed collectives.""" + + collectives: list[Collective] = Field(description="List of trashed collectives") + total: int = Field(description="Total number of trashed collectives") + + class ListTagsResponse(BaseResponse): """Response for listing tags in a collective.""" diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index 85e8f194..6d216022 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -21,6 +21,7 @@ from nextcloud_mcp_server.models.collectives import ( ListCollectivesResponse, ListPagesResponse, ListTagsResponse, + ListTrashedCollectivesResponse, ListTrashedPagesResponse, PageInfo, PageOperationResponse, @@ -241,21 +242,22 @@ def configure_collectives_tools(mcp: FastMCP): @require_scopes("collectives:write") @instrument_tool async def collectives_set_collective_emoji( - ctx: Context, collective_id: int, emoji: str + ctx: Context, collective_id: int, emoji: str | None = None ) -> CollectiveOperationResponse: - """Set the emoji on a Nextcloud Collective. + """Set or clear the emoji on a Nextcloud Collective. Setting the same emoji twice produces the same result (idempotent). + Pass emoji=None to clear the emoji. Args: collective_id: ID of the collective - emoji: Emoji to set on the collective + emoji: Emoji to set, or None to clear """ client = await get_client(ctx) try: raw = await client.collectives.update_collective(collective_id, emoji) except ValueError as e: - raise McpError(ErrorData(code=400, message=str(e))) from e + raise McpError(ErrorData(code=-32603, message=str(e))) from e except (OCSError, HTTPStatusError) as e: raise _handle_collectives_error(e) from e collective = Collective(**raw) @@ -267,9 +269,7 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Trash Collective", - annotations=ToolAnnotations( - destructiveHint=True, idempotentHint=False, openWorldHint=True - ), + annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), ) @require_scopes("collectives:write") @instrument_tool @@ -297,7 +297,7 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Delete Collective", annotations=ToolAnnotations( - destructiveHint=True, idempotentHint=True, openWorldHint=True + destructiveHint=True, idempotentHint=False, openWorldHint=True ), ) @require_scopes("collectives:write") @@ -325,6 +325,56 @@ def configure_collectives_tools(mcp: FastMCP): message="Collective permanently deleted", ) + @mcp.tool( + title="List Trashed Collectives", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("collectives:read") + @instrument_tool + async def collectives_get_trashed_collectives( + ctx: Context, + ) -> ListTrashedCollectivesResponse: + """List all trashed Nextcloud Collectives. + + Returns collectives that have been soft-deleted and can be restored + or permanently deleted. + """ + client = await get_client(ctx) + try: + raw = await client.collectives.get_trashed_collectives() + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e + collectives = [Collective(**c) for c in raw] + return ListTrashedCollectivesResponse( + collectives=collectives, total=len(collectives) + ) + + @mcp.tool( + title="Restore Collective", + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_restore_collective( + ctx: Context, collective_id: int + ) -> CollectiveOperationResponse: + """Restore a Nextcloud Collective from trash. + + Args: + collective_id: ID of the trashed collective to restore + """ + client = await get_client(ctx) + try: + raw = await client.collectives.restore_collective(collective_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e + collective = Collective(**raw) + return CollectiveOperationResponse( + collective_id=collective.id, + status_code=200, + message=f"Collective '{collective.name}' restored from trash", + ) + @mcp.tool( title="Create Collective Page", annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), @@ -401,9 +451,7 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Trash Collective Page", - annotations=ToolAnnotations( - destructiveHint=True, idempotentHint=False, openWorldHint=True - ), + annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), ) @require_scopes("collectives:write") @instrument_tool diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py index e6865c59..04ae40cf 100644 --- a/tests/client/collectives/test_collectives_api.py +++ b/tests/client/collectives/test_collectives_api.py @@ -453,3 +453,67 @@ async def test_get_page_404(mocker): client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") with pytest.raises(httpx.HTTPStatusError): await client.get_page(collective_id=1, page_id=999) + + +# --- Additional coverage --- + + +async def test_update_collective_no_fields_raises_value_error(mocker): + """Test that update_collective raises ValueError when called with no fields.""" + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + with pytest.raises(ValueError, match="At least one field"): + await client.update_collective(collective_id=1) + + +async def test_set_page_emoji_clear(mocker): + """Test that set_page_emoji sends null emoji to clear it.""" + page = _sample_page(10, "Test Page") + page["emoji"] = None + mock_response = _ocs_response({"page": page}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.set_page_emoji(collective_id=1, page_id=10, emoji=None) + + assert result["emoji"] is None + call_args = mock_request.call_args + assert call_args[1]["json"] == {"emoji": None} + + +async def test_get_trashed_collectives(mocker): + """Test listing trashed collectives.""" + mock_response = _ocs_response( + {"collectives": [_sample_collective(1, "Trashed Wiki")]} + ) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.get_trashed_collectives() + + assert len(result) == 1 + assert result[0]["name"] == "Trashed Wiki" + call_args = mock_request.call_args + assert "/collectives/trash" in call_args[0][1] + assert call_args[0][0] == "GET" + + +async def test_restore_collective(mocker): + """Test restoring a collective from trash.""" + mock_response = _ocs_response( + {"collective": _sample_collective(5, "Restored Wiki")} + ) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.restore_collective(collective_id=5) + + assert result["name"] == "Restored Wiki" + call_args = mock_request.call_args + assert call_args[0][0] == "PATCH" + assert "/collectives/trash/5" in call_args[0][1] diff --git a/tests/server/test_annotations.py b/tests/server/test_annotations.py index 4ef1acd7..9424eb71 100644 --- a/tests/server/test_annotations.py +++ b/tests/server/test_annotations.py @@ -78,8 +78,12 @@ async def test_delete_operations_are_idempotent(nc_mcp_client: ClientSession): """Verify delete operations are marked as idempotent (ADR-017 decision).""" tools = await nc_mcp_client.list_tools() + # Exceptions: delete operations that require a precondition (e.g. must be + # trashed first), so calling twice produces an error on the second call. + non_idempotent_deletes = {"collectives_delete_collective"} + for tool in tools.tools: - if "delete" in tool.name.lower(): + if "delete" in tool.name.lower() and tool.name not in non_idempotent_deletes: assert tool.annotations is not None, f"Tool {tool.name} missing annotations" assert tool.annotations.idempotentHint is True, ( f"Delete tool {tool.name} should be idempotent (same end state)" diff --git a/tests/server/test_collectives_mcp.py b/tests/server/test_collectives_mcp.py index c69272ba..aae68c07 100644 --- a/tests/server/test_collectives_mcp.py +++ b/tests/server/test_collectives_mcp.py @@ -87,6 +87,8 @@ async def test_collectives_tools_available(nc_mcp_client: ClientSession): "collectives_assign_tag", "collectives_remove_tag", "collectives_get_trashed_pages", + "collectives_get_trashed_collectives", + "collectives_restore_collective", ] for expected in expected_tools: @@ -280,6 +282,7 @@ async def test_collectives_move_page( data = json.loads(move_result.content[0].text) assert data["page_id"] == page_id assert "moved" in data["message"] + assert new_title in data["message"] logger.info(f"Page renamed to: {new_title}") # Cleanup @@ -382,6 +385,71 @@ async def test_collectives_search( logger.info(f"Search returned {data['total']} results for 'Welcome'") +# --- Collective Trash / Restore / Delete --- + + +async def test_collectives_trash_restore_delete_workflow( + nc_mcp_client: ClientSession, +): + """Test the full collective lifecycle: create, trash, list trashed, restore, trash, delete.""" + # Create a throwaway collective + name = f"Lifecycle Test {uuid.uuid4().hex[:8]}" + create_result = await nc_mcp_client.call_tool( + "collectives_create_collective", + {"name": name}, + ) + assert create_result.isError is False + created = json.loads(create_result.content[0].text) + cid = created["id"] + logger.info(f"Created collective {name} (ID: {cid})") + + # Trash the collective + trash_result = await nc_mcp_client.call_tool( + "collectives_trash_collective", + {"collective_id": cid}, + ) + assert trash_result.isError is False + logger.info("Collective moved to trash") + + # List trashed collectives โ€” should include ours + list_trash_result = await nc_mcp_client.call_tool( + "collectives_get_trashed_collectives", + {}, + ) + assert list_trash_result.isError is False + trash_data = json.loads(list_trash_result.content[0].text) + trashed_ids = [c["id"] for c in trash_data["collectives"]] + assert cid in trashed_ids + logger.info(f"Found {trash_data['total']} trashed collectives") + + # Restore the collective + restore_result = await nc_mcp_client.call_tool( + "collectives_restore_collective", + {"collective_id": cid}, + ) + assert restore_result.isError is False + restore_data = json.loads(restore_result.content[0].text) + assert restore_data["collective_id"] == cid + assert "restored" in restore_data["message"].lower() + logger.info("Collective restored from trash") + + # Trash again, then permanently delete + trash_result2 = await nc_mcp_client.call_tool( + "collectives_trash_collective", + {"collective_id": cid}, + ) + assert trash_result2.isError is False + + delete_result = await nc_mcp_client.call_tool( + "collectives_delete_collective", + {"collective_id": cid}, + ) + assert delete_result.isError is False + delete_data = json.loads(delete_result.content[0].text) + assert "permanently deleted" in delete_data["message"].lower() + logger.info("Collective permanently deleted") + + # --- Error Handling --- From 52470ea713c422c769c7976a29215861905d1777 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 28 Mar 2026 09:19:53 +0100 Subject: [PATCH 11/11] fix: address PR review feedback (round 9) - Fix emoji clearing bug: use _UNSET sentinel in update_collective so emoji=None sends {"emoji": null} instead of raising ValueError - Move collectives_get_trashed_collectives to Read Tools section - Remove redundant is_trash field from ListTrashedPagesResponse - Add page lifecycle note to collectives_trash_page docstring - Add unit test for clearing collective emoji via update_collective - Add integration test for clearing collective emoji via MCP tool Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/client/collectives.py | 10 +++- nextcloud_mcp_server/models/collectives.py | 4 -- nextcloud_mcp_server/server/collectives.py | 54 ++++++++++--------- .../collectives/test_collectives_api.py | 18 +++++++ tests/server/test_collectives_mcp.py | 24 +++++++++ 5 files changed, 79 insertions(+), 31 deletions(-) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index 4df636fb..ec2569a2 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -9,6 +9,9 @@ logger = logging.getLogger(__name__) API_BASE = "/ocs/v2.php/apps/collectives/api/v1.0" +_UNSET = object() +"""Sentinel to distinguish 'not provided' from an explicit None.""" + class OCSError(Exception): """Error returned in the OCS response envelope.""" @@ -75,15 +78,18 @@ class CollectivesClient(BaseNextcloudClient): return data["collective"] async def update_collective( - self, collective_id: int, emoji: str | None = None + self, collective_id: int, emoji: str | None | object = _UNSET ) -> dict[str, Any]: """Update a collective (emoji). + Pass emoji=None to clear the emoji. Omit emoji entirely to leave + it unchanged. + Raises: ValueError: If no fields are provided to update. """ json_data: dict[str, Any] = {} - if emoji is not None: + if emoji is not _UNSET: json_data["emoji"] = emoji if not json_data: raise ValueError("At least one field must be provided to update") diff --git a/nextcloud_mcp_server/models/collectives.py b/nextcloud_mcp_server/models/collectives.py index 538b366e..b4c0f6e0 100644 --- a/nextcloud_mcp_server/models/collectives.py +++ b/nextcloud_mcp_server/models/collectives.py @@ -135,10 +135,6 @@ class SearchPagesResponse(BaseResponse): class ListTrashedPagesResponse(ListPagesResponse): """Response for listing trashed pages in a collective.""" - is_trash: bool = Field( - default=True, description="Indicates these are trashed pages" - ) - class ListTrashedCollectivesResponse(BaseResponse): """Response for listing trashed collectives.""" diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index 6d216022..cd0dbd66 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -208,6 +208,30 @@ def configure_collectives_tools(mcp: FastMCP): pages=pages, total=len(pages), collective_id=collective_id ) + @mcp.tool( + title="List Trashed Collectives", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("collectives:read") + @instrument_tool + async def collectives_get_trashed_collectives( + ctx: Context, + ) -> ListTrashedCollectivesResponse: + """List all trashed Nextcloud Collectives. + + Returns collectives that have been soft-deleted and can be restored + or permanently deleted. + """ + client = await get_client(ctx) + try: + raw = await client.collectives.get_trashed_collectives() + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e + collectives = [Collective(**c) for c in raw] + return ListTrashedCollectivesResponse( + collectives=collectives, total=len(collectives) + ) + # --- Write Tools --- @mcp.tool( @@ -325,30 +349,6 @@ def configure_collectives_tools(mcp: FastMCP): message="Collective permanently deleted", ) - @mcp.tool( - title="List Trashed Collectives", - annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), - ) - @require_scopes("collectives:read") - @instrument_tool - async def collectives_get_trashed_collectives( - ctx: Context, - ) -> ListTrashedCollectivesResponse: - """List all trashed Nextcloud Collectives. - - Returns collectives that have been soft-deleted and can be restored - or permanently deleted. - """ - client = await get_client(ctx) - try: - raw = await client.collectives.get_trashed_collectives() - except (OCSError, HTTPStatusError) as e: - raise _handle_collectives_error(e) from e - collectives = [Collective(**c) for c in raw] - return ListTrashedCollectivesResponse( - collectives=collectives, total=len(collectives) - ) - @mcp.tool( title="Restore Collective", annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), @@ -458,7 +458,11 @@ def configure_collectives_tools(mcp: FastMCP): async def collectives_trash_page( ctx: Context, collective_id: int, page_id: int ) -> PageOperationResponse: - """Move a page to trash in a Nextcloud Collective (soft delete) + """Move a page to trash in a Nextcloud Collective (soft delete). + + Trashed pages can be restored with collectives_restore_page. The + Collectives API does not support permanent page deletion; trashed + pages are cleaned up by Nextcloud's retention policy. Args: collective_id: ID of the collective diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py index 04ae40cf..46460bc4 100644 --- a/tests/client/collectives/test_collectives_api.py +++ b/tests/client/collectives/test_collectives_api.py @@ -461,10 +461,28 @@ async def test_get_page_404(mocker): async def test_update_collective_no_fields_raises_value_error(mocker): """Test that update_collective raises ValueError when called with no fields.""" client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + # Omitting emoji entirely (not passing it) should raise with pytest.raises(ValueError, match="At least one field"): await client.update_collective(collective_id=1) +async def test_update_collective_clear_emoji(mocker): + """Test that update_collective sends null emoji to clear it.""" + mock_response = _ocs_response( + {"collective": _sample_collective(1, "Test Wiki", emoji=None)} + ) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.update_collective(collective_id=1, emoji=None) + + assert result["emoji"] is None + call_args = mock_request.call_args + assert call_args[1]["json"] == {"emoji": None} + + async def test_set_page_emoji_clear(mocker): """Test that set_page_emoji sends null emoji to clear it.""" page = _sample_page(10, "Test Page") diff --git a/tests/server/test_collectives_mcp.py b/tests/server/test_collectives_mcp.py index aae68c07..96ebd635 100644 --- a/tests/server/test_collectives_mcp.py +++ b/tests/server/test_collectives_mcp.py @@ -134,6 +134,30 @@ async def test_collectives_set_collective_emoji( logger.info("Collective emoji updated") +async def test_collectives_clear_collective_emoji( + nc_mcp_client: ClientSession, temporary_collective: dict +): + """Test clearing a collective's emoji by passing null.""" + cid = temporary_collective["id"] + + # Set an emoji first + set_result = await nc_mcp_client.call_tool( + "collectives_set_collective_emoji", + {"collective_id": cid, "emoji": "๐Ÿ”ฌ"}, + ) + assert set_result.isError is False + + # Clear the emoji by passing null + clear_result = await nc_mcp_client.call_tool( + "collectives_set_collective_emoji", + {"collective_id": cid, "emoji": None}, + ) + assert clear_result.isError is False + data = json.loads(clear_result.content[0].text) + assert data["collective_id"] == cid + logger.info("Collective emoji cleared") + + # --- Page CRUD ---