From 69814f30e37c34aa420458a15a54679c139a7204 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 29 Apr 2026 23:19:57 +0200 Subject: [PATCH 1/5] feat(talk): add MCP integration for Nextcloud Talk (spreed) Adds 6 MCP tools so an LLM can read a user's Talk conversations and post messages on their behalf, addressing the "read my chats and reply" use case from issue #720: - talk_list_conversations - talk_get_conversation - talk_get_messages - talk_list_participants - talk_send_message (auto-attaches a referenceId for retry dedup) - talk_mark_as_read Edit/delete messages, reactions, threads, and call/session ops are intentionally out of scope for this first PR. The TalkClient also exposes create_conversation/delete_conversation for the integration test fixture; these are not registered as MCP tools. A post-installation hook enables spreed in the docker dev env so the integration suite has a real Talk backend to talk to. Closes #720 Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 1 + .../10-install-spreed-app.sh | 9 + .../auth/scope_authorization.py | 2 + nextcloud_mcp_server/client/__init__.py | 2 + nextcloud_mcp_server/client/talk.py | 238 +++++++++++ nextcloud_mcp_server/models/talk.py | 152 +++++++ nextcloud_mcp_server/server/__init__.py | 3 + nextcloud_mcp_server/server/talk.py | 220 ++++++++++ tests/client/conftest.py | 79 ++++ tests/client/talk/test_talk_api.py | 386 ++++++++++++++++++ tests/conftest.py | 46 ++- tests/server/test_talk_mcp.py | 143 +++++++ 12 files changed, 1278 insertions(+), 3 deletions(-) create mode 100755 app-hooks/post-installation/10-install-spreed-app.sh create mode 100644 nextcloud_mcp_server/client/talk.py create mode 100644 nextcloud_mcp_server/models/talk.py create mode 100644 nextcloud_mcp_server/server/talk.py create mode 100644 tests/client/talk/test_talk_api.py create mode 100644 tests/server/test_talk_mcp.py diff --git a/README.md b/README.md index 396cb33b..7b58f598 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,7 @@ docker compose --profile login-flow up -d # Port 8004 | **Sharing** | 10+ | Create and manage shares | | **News** | 8 | Feeds, folders, items, feed health monitoring | | **Collectives** | 16 | Full CRUD on collectives, pages, and tags | +| **Talk (spreed)** | 6 | List conversations, read/post messages, mark as read, list participants | | **Semantic Search** | 2+ | Vector search for Notes, Files, News items, and Deck cards (experimental, opt-in, requires infrastructure) | Want to see another Nextcloud app supported? [Open an issue](https://github.com/cbcoutinho/nextcloud-mcp-server/issues) or contribute a pull request! diff --git a/app-hooks/post-installation/10-install-spreed-app.sh b/app-hooks/post-installation/10-install-spreed-app.sh new file mode 100755 index 00000000..a4b84974 --- /dev/null +++ b/app-hooks/post-installation/10-install-spreed-app.sh @@ -0,0 +1,9 @@ +#!/bin/bash + +set -euox pipefail + +# Talk (spreed) ships its appstore release on demand; on a fresh container the +# app is not yet installed, so app:install pulls it. On rebuilds where the +# volume already has it, app:install would fail, so fall through to +# app:enable. +php /var/www/html/occ app:install spreed || php /var/www/html/occ app:enable spreed diff --git a/nextcloud_mcp_server/auth/scope_authorization.py b/nextcloud_mcp_server/auth/scope_authorization.py index 1a5e7a15..5317d271 100644 --- a/nextcloud_mcp_server/auth/scope_authorization.py +++ b/nextcloud_mcp_server/auth/scope_authorization.py @@ -209,6 +209,7 @@ def require_scopes(*required_scopes: str): "files.", "tables.", "deck.", + "talk.", ] ) ] @@ -229,6 +230,7 @@ def require_scopes(*required_scopes: str): "files.", "tables.", "deck.", + "talk.", ] ) diff --git a/nextcloud_mcp_server/client/__init__.py b/nextcloud_mcp_server/client/__init__.py index 4b567efb..e28e419d 100644 --- a/nextcloud_mcp_server/client/__init__.py +++ b/nextcloud_mcp_server/client/__init__.py @@ -23,6 +23,7 @@ from .news import NewsClient from .notes import NotesClient from .sharing import SharingClient from .tables import TablesClient +from .talk import TalkClient from .users import UsersClient from .webdav import WebDAVClient from .webhooks import WebhooksClient @@ -85,6 +86,7 @@ class NextcloudClient: self.collectives = CollectivesClient(self._client, username) self.deck = DeckClient(self._client, username) self.news = NewsClient(self._client, username) + self.talk = TalkClient(self._client, username) self.users = UsersClient(self._client, username) self.groups = GroupsClient(self._client, username) self.sharing = SharingClient(self._client, username) diff --git a/nextcloud_mcp_server/client/talk.py b/nextcloud_mcp_server/client/talk.py new file mode 100644 index 00000000..8ee01e58 --- /dev/null +++ b/nextcloud_mcp_server/client/talk.py @@ -0,0 +1,238 @@ +"""HTTP client for the Nextcloud Talk (spreed) app. + +Talk exposes its REST API under ``/ocs/v2.php/apps/spreed/api/{v}/...``. +The current versions used here are: + +- conversations & participants: ``api/v4`` (Nextcloud 22+) +- chat: ``api/v1`` (Nextcloud 13+) + +All endpoints follow the OCS envelope ``{"ocs": {"meta": ..., "data": ...}}``, +require ``OCS-APIRequest: true`` and respond as JSON when ``Accept: +application/json`` is sent. +""" + +from typing import Any + +from nextcloud_mcp_server.client.base import BaseNextcloudClient +from nextcloud_mcp_server.models.talk import ( + TalkConversation, + TalkMessage, + TalkParticipant, +) + + +class TalkClient(BaseNextcloudClient): + """Client for Nextcloud Talk (spreed) app operations.""" + + app_name = "talk" + + _ROOM_BASE = "/ocs/v2.php/apps/spreed/api/v4/room" + _CHAT_BASE = "/ocs/v2.php/apps/spreed/api/v1/chat" + + def _talk_headers(self) -> dict[str, str]: + """Standard OCS+JSON headers for spreed API calls.""" + return { + "OCS-APIRequest": "true", + "Accept": "application/json", + "Content-Type": "application/json", + } + + # Conversations (rooms) + + async def list_conversations( + self, + *, + modified_since: int | None = None, + include_status: bool = False, + no_status_update: bool = True, + ) -> list[TalkConversation]: + """Return the user's Talk conversations. + + Args: + modified_since: If provided, only return conversations modified + after this Unix timestamp (server-side filter). + include_status: Include user-status info for one-to-one rooms. + no_status_update: When True (default), the call does not bump + the user's "online" status — appropriate for an MCP server + acting in the background. + """ + params: dict[str, Any] = {} + if modified_since is not None: + params["modifiedSince"] = modified_since + if include_status: + params["includeStatus"] = "true" + if no_status_update: + params["noStatusUpdate"] = 1 + response = await self._make_request( + "GET", self._ROOM_BASE, params=params, headers=self._talk_headers() + ) + data = response.json()["ocs"]["data"] + return [TalkConversation(**room) for room in data] + + async def get_conversation(self, token: str) -> TalkConversation: + """Fetch a single Talk conversation by its room token.""" + response = await self._make_request( + "GET", f"{self._ROOM_BASE}/{token}", headers=self._talk_headers() + ) + return TalkConversation(**response.json()["ocs"]["data"]) + + async def create_conversation( + self, + *, + room_type: int = 2, + room_name: str, + invite: str | None = None, + ) -> TalkConversation: + """Create a new conversation (used for tests/fixtures). + + Args: + room_type: 1=one-to-one, 2=group, 3=public. Defaults to 2. + room_name: Display name (required for group/public rooms). + invite: Optional user/group ID to invite at creation time. + + This client method is not exposed as an MCP tool in the initial + Talk integration; it exists so integration tests can spin up + scratch rooms. + """ + body: dict[str, Any] = {"roomType": room_type, "roomName": room_name} + if invite is not None: + body["invite"] = invite + response = await self._make_request( + "POST", self._ROOM_BASE, json=body, headers=self._talk_headers() + ) + return TalkConversation(**response.json()["ocs"]["data"]) + + async def delete_conversation(self, token: str) -> None: + """Delete a conversation. Used by integration test cleanup.""" + await self._make_request( + "DELETE", f"{self._ROOM_BASE}/{token}", headers=self._talk_headers() + ) + + # Chat + + async def get_messages( + self, + token: str, + *, + limit: int = 50, + last_known_message_id: int | None = None, + look_into_future: bool = False, + set_read_marker: bool = False, + include_last_known: bool = False, + ) -> tuple[list[TalkMessage], int | None]: + """Fetch chat messages for a conversation. + + Args: + token: Conversation token. + limit: Max messages to return (spreed caps this at 200). + last_known_message_id: Pagination cursor — pass the value + from the previous response's ``X-Chat-Last-Given`` header. + look_into_future: When False (default), return *older* + messages relative to ``last_known_message_id`` — i.e., + read history. When True, this becomes a long-poll for + new messages, which we don't expose via MCP. + set_read_marker: When False (default), the call does not move + the user's read marker — consumers can call + ``mark_as_read`` explicitly. + include_last_known: Include the message identified by + ``last_known_message_id`` itself in the page. + + Returns: + ``(messages, x_chat_last_given)`` where the integer is the + value of the ``X-Chat-Last-Given`` response header (or None + if the header was absent), suitable for pagination. + """ + params: dict[str, Any] = { + "limit": limit, + "lookIntoFuture": 1 if look_into_future else 0, + "setReadMarker": 1 if set_read_marker else 0, + "includeLastKnown": 1 if include_last_known else 0, + } + if last_known_message_id is not None: + params["lastKnownMessageId"] = last_known_message_id + response = await self._make_request( + "GET", + f"{self._CHAT_BASE}/{token}", + params=params, + headers=self._talk_headers(), + ) + # 200 OK → JSON body with messages; 304 Not Modified → no body. + # _make_request's raise_for_status() lets 3xx through for GET, but + # spreed returns 200 with an empty data list when there's nothing + # new, so we trust the JSON body here. + last_given_header = response.headers.get("X-Chat-Last-Given") + last_given = int(last_given_header) if last_given_header else None + data = response.json()["ocs"]["data"] + return [TalkMessage(**msg) for msg in data], last_given + + async def send_message( + self, + token: str, + message: str, + *, + reply_to: int | None = None, + reference_id: str | None = None, + silent: bool = False, + ) -> TalkMessage: + """Post a chat message to a conversation. + + Args: + token: Conversation token. + message: Message text (max 32000 chars per spreed docs/chat.md). + reply_to: Optional parent message ID to thread this reply. + reference_id: Optional client-provided UUID for idempotency on + retry (spreed dedupes on this within the conversation). + silent: When True, the message is delivered without push + notifications. + """ + body: dict[str, Any] = {"message": message} + if reply_to is not None: + body["replyTo"] = reply_to + if reference_id is not None: + body["referenceId"] = reference_id + if silent: + body["silent"] = True + response = await self._make_request( + "POST", + f"{self._CHAT_BASE}/{token}", + json=body, + headers=self._talk_headers(), + ) + return TalkMessage(**response.json()["ocs"]["data"]) + + async def mark_as_read( + self, token: str, *, last_read_message: int | None = None + ) -> None: + """Mark the conversation as read. + + If ``last_read_message`` is provided it sets the read marker to + that message; otherwise spreed marks everything currently in the + room as read. + """ + body: dict[str, Any] = {} + if last_read_message is not None: + body["lastReadMessage"] = last_read_message + await self._make_request( + "POST", + f"{self._CHAT_BASE}/{token}/read", + json=body, + headers=self._talk_headers(), + ) + + # Participants + + async def list_participants( + self, token: str, *, include_status: bool = False + ) -> list[TalkParticipant]: + """List participants of a Talk conversation.""" + params: dict[str, Any] = {} + if include_status: + params["includeStatus"] = "true" + response = await self._make_request( + "GET", + f"{self._ROOM_BASE}/{token}/participants", + params=params, + headers=self._talk_headers(), + ) + data = response.json()["ocs"]["data"] + return [TalkParticipant(**p) for p in data] diff --git a/nextcloud_mcp_server/models/talk.py b/nextcloud_mcp_server/models/talk.py new file mode 100644 index 00000000..35b66c72 --- /dev/null +++ b/nextcloud_mcp_server/models/talk.py @@ -0,0 +1,152 @@ +"""Pydantic models for the Nextcloud Talk (spreed) integration.""" + +from typing import Any + +from pydantic import BaseModel, Field, field_validator + +from .base import BaseResponse, StatusResponse + +# Domain models + + +class TalkMessage(BaseModel): + """A single chat message in a Talk conversation. + + See spreed docs/chat.md for the field definitions. We map only the + fields that are useful to MCP consumers; spreed returns more. + """ + + id: int + token: str + actorType: str + actorId: str + actorDisplayName: str + timestamp: int + systemMessage: str = "" + messageType: str + message: str + messageParameters: dict[str, Any] = Field(default_factory=dict) + expirationTimestamp: int | None = None + referenceId: str | None = None + markdown: bool | None = None + + @field_validator("messageParameters", mode="before") + @classmethod + def _coerce_empty_list_params(cls, v: Any) -> Any: + # spreed serializes an empty parameter map as `[]` (PHP array) rather + # than `{}`; normalize so pydantic accepts it as a dict. + if isinstance(v, list) and not v: + return {} + return v + + +class TalkConversation(BaseModel): + """A Talk conversation (room). + + See spreed docs/conversation.md for the full field reference. Many + optional fields are omitted; we keep the ones useful for chat-centric + flows. + """ + + id: int + token: str + type: int + name: str + displayName: str + description: str | None = "" + participantType: int | None = None + unreadMessages: int = 0 + unreadMention: bool = False + lastActivity: int | None = None + lastReadMessage: int | None = None + lastMessage: TalkMessage | None = None + readOnly: int | None = None + isFavorite: bool | None = None + notificationLevel: int | None = None + objectType: str | None = None + objectId: str | None = None + + @field_validator("lastMessage", mode="before") + @classmethod + def _coerce_empty_last_message(cls, v: Any) -> Any: + # spreed returns `lastMessage: []` (PHP empty array) when there has + # never been a message in the room; normalize to None. + if isinstance(v, list) and not v: + return None + return v + + +class TalkParticipant(BaseModel): + """A participant (attendee) in a Talk conversation.""" + + attendeeId: int + actorType: str + actorId: str + displayName: str + participantType: int + inCall: int = 0 + lastPing: int = 0 + sessionIds: list[str] = Field(default_factory=list) + status: str | None = None + statusIcon: str | None = None + statusMessage: str | None = None + + +# Response wrappers for MCP tools + + +class ListConversationsResponse(BaseResponse): + """Response model for listing Talk conversations.""" + + results: list[TalkConversation] = Field( + description="Talk conversations the user participates in" + ) + total: int = Field(description="Number of conversations returned") + + +class GetConversationResponse(BaseResponse): + """Response model for fetching a single Talk conversation.""" + + conversation: TalkConversation = Field(description="The Talk conversation") + + +class ListMessagesResponse(BaseResponse): + """Response model for fetching chat history of a conversation.""" + + conversation_token: str = Field(description="Token of the conversation") + results: list[TalkMessage] = Field(description="Chat messages in this page") + count: int = Field(description="Number of messages returned in this page") + last_known_message_id: int | None = Field( + default=None, + description=( + "ID to pass back as `last_known_message_id` to fetch the next " + "page (older history). Sourced from the `X-Chat-Last-Given` " + "response header." + ), + ) + + +class ListParticipantsResponse(BaseResponse): + """Response model for listing participants in a Talk conversation.""" + + conversation_token: str = Field(description="Token of the conversation") + results: list[TalkParticipant] = Field( + description="Participants of the conversation" + ) + count: int = Field(description="Number of participants returned") + + +class SendMessageResponse(BaseResponse): + """Response model returned after posting a chat message.""" + + message: TalkMessage = Field(description="The posted chat message") + + +class MarkAsReadResponse(StatusResponse): + """Response model for the mark-as-read operation.""" + + conversation_token: str = Field(description="Token of the conversation") + last_read_message: int | None = Field( + default=None, + description="The message ID that was marked as the last-read marker", + ) diff --git a/nextcloud_mcp_server/server/__init__.py b/nextcloud_mcp_server/server/__init__.py index 45fab885..e2f4616b 100644 --- a/nextcloud_mcp_server/server/__init__.py +++ b/nextcloud_mcp_server/server/__init__.py @@ -12,6 +12,7 @@ from .notes import configure_notes_tools from .semantic import configure_semantic_tools from .sharing import configure_sharing_tools from .tables import configure_tables_tools +from .talk import configure_talk_tools from .webdav import configure_webdav_tools # Canonical mapping of app name → tool registration function. @@ -29,6 +30,7 @@ AVAILABLE_APPS: dict[str, Callable[[FastMCP], None]] = { "cookbook": configure_cookbook_tools, "deck": configure_deck_tools, "news": configure_news_tools, + "talk": configure_talk_tools, } __all__ = [ @@ -43,5 +45,6 @@ __all__ = [ "configure_semantic_tools", "configure_sharing_tools", "configure_tables_tools", + "configure_talk_tools", "configure_webdav_tools", ] diff --git a/nextcloud_mcp_server/server/talk.py b/nextcloud_mcp_server/server/talk.py new file mode 100644 index 00000000..d1d91fd2 --- /dev/null +++ b/nextcloud_mcp_server/server/talk.py @@ -0,0 +1,220 @@ +"""MCP tool registration for the Nextcloud Talk (spreed) integration.""" + +import logging +import uuid + +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.talk import ( + GetConversationResponse, + ListConversationsResponse, + ListMessagesResponse, + ListParticipantsResponse, + MarkAsReadResponse, + SendMessageResponse, +) +from nextcloud_mcp_server.observability.metrics import instrument_tool + +logger = logging.getLogger(__name__) + + +# spreed advertises a 32000-character limit on chat messages (docs/chat.md); +# we enforce it client-side for a clearer error than the server's 413. +_MESSAGE_MAX_LENGTH = 32000 + + +def _validate_message_text(message: str) -> None: + if not message: + raise ValueError("Message text must not be empty") + if len(message) > _MESSAGE_MAX_LENGTH: + raise ValueError( + f"Message too long: {len(message)} characters (max {_MESSAGE_MAX_LENGTH})" + ) + + +def configure_talk_tools(mcp: FastMCP) -> None: + """Configure Nextcloud Talk (spreed) MCP tools.""" + + # Read tools + + @mcp.tool( + title="List Talk Conversations", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("talk.read") + @instrument_tool + async def talk_list_conversations( + ctx: Context, + modified_since: int | None = None, + include_status: bool = False, + ) -> ListConversationsResponse: + """List the user's Talk conversations (rooms). + + Args: + modified_since: Optional Unix timestamp; only conversations + modified after this time are returned. + include_status: Whether to include user-status info for + one-to-one conversations. + """ + client = await get_client(ctx) + rooms = await client.talk.list_conversations( + modified_since=modified_since, + include_status=include_status, + ) + return ListConversationsResponse(results=rooms, total=len(rooms)) + + @mcp.tool( + title="Get Talk Conversation", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("talk.read") + @instrument_tool + async def talk_get_conversation( + ctx: Context, token: str + ) -> GetConversationResponse: + """Get details of a Talk conversation by its token. + + Args: + token: Unique room token (returned by ``talk_list_conversations``). + """ + client = await get_client(ctx) + conversation = await client.talk.get_conversation(token) + return GetConversationResponse(conversation=conversation) + + @mcp.tool( + title="Get Talk Messages", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("talk.read") + @instrument_tool + async def talk_get_messages( + ctx: Context, + token: str, + limit: int = 50, + last_known_message_id: int | None = None, + include_last_known: bool = False, + ) -> ListMessagesResponse: + """Read chat history for a Talk conversation. + + Returns the most recent messages (older first when paginated). + Does not move the user's read marker; call + ``talk_mark_as_read`` separately if desired. + + Args: + token: Conversation token. + limit: Max messages per page (spreed caps at 200). + last_known_message_id: Pagination cursor — pass the + ``last_known_message_id`` from the previous response to + fetch the next (older) page. + include_last_known: Include the cursor message in the page + instead of starting just before it. + """ + client = await get_client(ctx) + messages, last_given = await client.talk.get_messages( + token, + limit=limit, + last_known_message_id=last_known_message_id, + look_into_future=False, + set_read_marker=False, + include_last_known=include_last_known, + ) + return ListMessagesResponse( + conversation_token=token, + results=messages, + count=len(messages), + last_known_message_id=last_given, + ) + + @mcp.tool( + title="List Talk Conversation Participants", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("talk.read") + @instrument_tool + async def talk_list_participants( + ctx: Context, token: str, include_status: bool = False + ) -> ListParticipantsResponse: + """List the participants of a Talk conversation. + + Args: + token: Conversation token. + include_status: Include each participant's user-status info. + """ + client = await get_client(ctx) + participants = await client.talk.list_participants( + token, include_status=include_status + ) + return ListParticipantsResponse( + conversation_token=token, + results=participants, + count=len(participants), + ) + + # Write tools + + @mcp.tool( + title="Send Talk Message", + annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), + ) + @require_scopes("talk.write") + @instrument_tool + async def talk_send_message( + ctx: Context, + token: str, + message: str, + reply_to: int | None = None, + silent: bool = False, + ) -> SendMessageResponse: + """Post a chat message into a Talk conversation as the user. + + A random ``referenceId`` is attached so spreed dedupes the post + if the request is retried. + + Args: + token: Conversation token. + message: Message text (max 32000 characters). + reply_to: Optional parent message ID to thread the reply. + silent: When True the message is delivered without push + notifications (e.g. for status updates). + """ + _validate_message_text(message) + client = await get_client(ctx) + posted = await client.talk.send_message( + token, + message, + reply_to=reply_to, + reference_id=uuid.uuid4().hex, + silent=silent, + ) + return SendMessageResponse(message=posted) + + @mcp.tool( + title="Mark Talk Conversation as Read", + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), + ) + @require_scopes("talk.write") + @instrument_tool + async def talk_mark_as_read( + ctx: Context, + token: str, + last_read_message: int | None = None, + ) -> MarkAsReadResponse: + """Move the user's read marker forward in a Talk conversation. + + Args: + token: Conversation token. + last_read_message: Optional message ID to mark as the new + read position. When omitted, spreed marks everything + currently in the room as read. + """ + client = await get_client(ctx) + await client.talk.mark_as_read(token, last_read_message=last_read_message) + return MarkAsReadResponse( + success=True, + message="Conversation marked as read", + conversation_token=token, + last_read_message=last_read_message, + ) diff --git a/tests/client/conftest.py b/tests/client/conftest.py index 4fc8cc7e..c4649b4f 100644 --- a/tests/client/conftest.py +++ b/tests/client/conftest.py @@ -358,6 +358,85 @@ def create_mock_deck_comment_response( return create_mock_response(status_code=200, json_data=ocs_response) +def create_mock_talk_room_response( + conversation_id: int = 1, + token: str = "abc123", + name: str = "Test Conversation", + room_type: int = 2, + **kwargs, +) -> httpx.Response: + """Create a mock OCS response for a Nextcloud Talk conversation. + + Args: + conversation_id: Numeric conversation ID + token: Unique room token + name: Conversation name + room_type: 1=one-to-one, 2=group, 3=public, 4=changelog + **kwargs: Additional conversation fields (override defaults) + + Returns: + Mock httpx.Response with conversation data wrapped in OCS format. + """ + room_data = { + "id": conversation_id, + "token": token, + "type": room_type, + "name": name, + "displayName": name, + "description": "", + "participantType": 1, + "unreadMessages": 0, + "unreadMention": False, + "lastActivity": 1700000000, + "lastReadMessage": 0, + "lastMessage": [], + "readOnly": 0, + "isFavorite": False, + "notificationLevel": 0, + **kwargs, + } + + ocs_response = {"ocs": {"meta": {"status": "ok"}, "data": room_data}} + return create_mock_response(status_code=200, json_data=ocs_response) + + +def create_mock_talk_message_response( + message_id: int = 1, + token: str = "abc123", + text: str = "Hello, world!", + actor_id: str = "testuser", + **kwargs, +) -> httpx.Response: + """Create a mock OCS response for a Nextcloud Talk chat message. + + Args: + message_id: Numeric message ID + token: Conversation token the message belongs to + text: Message body + actor_id: User ID of the message author + **kwargs: Additional message fields (override defaults) + + Returns: + Mock httpx.Response with message data wrapped in OCS format. + """ + message_data = { + "id": message_id, + "token": token, + "actorType": "users", + "actorId": actor_id, + "actorDisplayName": "Test User", + "timestamp": 1700000000, + "systemMessage": "", + "messageType": "comment", + "message": text, + "messageParameters": {}, + **kwargs, + } + + ocs_response = {"ocs": {"meta": {"status": "ok"}, "data": message_data}} + return create_mock_response(status_code=200, json_data=ocs_response) + + def create_mock_tables_list_response( tables: list[dict] = None, ) -> httpx.Response: diff --git a/tests/client/talk/test_talk_api.py b/tests/client/talk/test_talk_api.py new file mode 100644 index 00000000..74f2b341 --- /dev/null +++ b/tests/client/talk/test_talk_api.py @@ -0,0 +1,386 @@ +"""Unit tests for the Nextcloud Talk (spreed) HTTP client.""" + +import logging + +import httpx +import pytest + +from nextcloud_mcp_server.client.talk import TalkClient +from nextcloud_mcp_server.models.talk import ( + TalkConversation, + TalkMessage, + TalkParticipant, +) +from tests.client.conftest import ( + create_mock_error_response, + create_mock_response, + create_mock_talk_message_response, + create_mock_talk_room_response, +) + +logger = logging.getLogger(__name__) +pytestmark = pytest.mark.unit + + +# Conversation tests + + +async def test_talk_list_conversations(mocker): + """list_conversations parses the OCS-wrapped room list.""" + mock_response = create_mock_response( + status_code=200, + json_data={ + "ocs": { + "meta": {"status": "ok"}, + "data": [ + { + "id": 1, + "token": "abc", + "type": 2, + "name": "Group 1", + "displayName": "Group 1", + "lastMessage": [], + }, + { + "id": 2, + "token": "def", + "type": 3, + "name": "Public Room", + "displayName": "Public Room", + "lastMessage": [], + }, + ], + } + }, + ) + + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + rooms = await client.list_conversations() + + assert len(rooms) == 2 + assert all(isinstance(r, TalkConversation) for r in rooms) + assert rooms[0].token == "abc" + assert rooms[1].type == 3 + + mock_make_request.assert_called_once() + call_args = mock_make_request.call_args + assert call_args[0][0] == "GET" + assert call_args[0][1] == "/ocs/v2.php/apps/spreed/api/v4/room" + # noStatusUpdate defaults to True + assert call_args[1]["params"]["noStatusUpdate"] == 1 + + +async def test_talk_list_conversations_with_modified_since(mocker): + """modifiedSince and includeStatus are forwarded as query params.""" + mock_response = create_mock_response( + status_code=200, + json_data={"ocs": {"meta": {"status": "ok"}, "data": []}}, + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.list_conversations(modified_since=1700000000, include_status=True) + + params = mock_make_request.call_args[1]["params"] + assert params["modifiedSince"] == 1700000000 + assert params["includeStatus"] == "true" + + +async def test_talk_get_conversation(mocker): + """get_conversation parses the OCS-wrapped room object.""" + mock_response = create_mock_talk_room_response( + conversation_id=42, token="xyz", name="My Room" + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + room = await client.get_conversation("xyz") + + assert isinstance(room, TalkConversation) + assert room.id == 42 + assert room.token == "xyz" + assert room.name == "My Room" + + assert mock_make_request.call_args[0][0] == "GET" + assert "/api/v4/room/xyz" in mock_make_request.call_args[0][1] + + +async def test_talk_get_conversation_not_found(mocker): + """A 404 from spreed propagates as HTTPStatusError.""" + error_response = create_mock_error_response(404, "Conversation not found") + mock_make_request = mocker.patch.object(TalkClient, "_make_request") + mock_make_request.side_effect = httpx.HTTPStatusError( + "404 Not Found", + request=httpx.Request("GET", "http://test.local"), + response=error_response, + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + with pytest.raises(httpx.HTTPStatusError) as excinfo: + await client.get_conversation("nope") + assert excinfo.value.response.status_code == 404 + + +async def test_talk_create_conversation(mocker): + """create_conversation builds the right POST body.""" + mock_response = create_mock_talk_room_response( + conversation_id=10, token="new", name="New Room" + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + room = await client.create_conversation(room_name="New Room") + + assert room.token == "new" + call_args = mock_make_request.call_args + assert call_args[0][0] == "POST" + assert call_args[1]["json"] == {"roomType": 2, "roomName": "New Room"} + + +async def test_talk_delete_conversation(mocker): + """delete_conversation issues DELETE on the room URL.""" + mock_response = create_mock_response( + status_code=200, + json_data={"ocs": {"meta": {"status": "ok"}, "data": []}}, + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.delete_conversation("doomed") + + call_args = mock_make_request.call_args + assert call_args[0][0] == "DELETE" + assert "/api/v4/room/doomed" in call_args[0][1] + + +# Chat tests + + +async def test_talk_get_messages(mocker): + """get_messages parses the OCS-wrapped message list.""" + mock_response = create_mock_response( + status_code=200, + headers={"X-Chat-Last-Given": "100"}, + json_data={ + "ocs": { + "meta": {"status": "ok"}, + "data": [ + { + "id": 101, + "token": "abc", + "actorType": "users", + "actorId": "alice", + "actorDisplayName": "Alice", + "timestamp": 1700000000, + "messageType": "comment", + "message": "Hi", + }, + { + "id": 102, + "token": "abc", + "actorType": "users", + "actorId": "bob", + "actorDisplayName": "Bob", + "timestamp": 1700000010, + "messageType": "comment", + "message": "Hello", + }, + ], + } + }, + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + messages, last_given = await client.get_messages("abc", limit=20) + + assert len(messages) == 2 + assert all(isinstance(m, TalkMessage) for m in messages) + assert messages[0].id == 101 + assert messages[1].actorId == "bob" + assert last_given == 100 + + call_args = mock_make_request.call_args + assert call_args[0][0] == "GET" + assert "/chat/abc" in call_args[0][1] + params = call_args[1]["params"] + assert params["limit"] == 20 + # Defaults: not look_into_future, not set_read_marker + assert params["lookIntoFuture"] == 0 + assert params["setReadMarker"] == 0 + + +async def test_talk_get_messages_pagination_cursor(mocker): + """last_known_message_id is forwarded as the spreed cursor param.""" + mock_response = create_mock_response( + status_code=200, + json_data={"ocs": {"meta": {"status": "ok"}, "data": []}}, + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + messages, last_given = await client.get_messages( + "abc", last_known_message_id=500, include_last_known=True + ) + + assert messages == [] + assert last_given is None # No header on this response + + params = mock_make_request.call_args[1]["params"] + assert params["lastKnownMessageId"] == 500 + assert params["includeLastKnown"] == 1 + + +async def test_talk_send_message(mocker): + """send_message posts the message text and parses the response.""" + mock_response = create_mock_talk_message_response( + message_id=999, token="abc", text="Posted from MCP" + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + posted = await client.send_message("abc", "Posted from MCP") + + assert isinstance(posted, TalkMessage) + assert posted.id == 999 + assert posted.message == "Posted from MCP" + + call_args = mock_make_request.call_args + assert call_args[0][0] == "POST" + assert "/chat/abc" in call_args[0][1] + body = call_args[1]["json"] + assert body["message"] == "Posted from MCP" + # Optional fields are not sent unless explicitly set + assert "replyTo" not in body + assert "referenceId" not in body + assert "silent" not in body + + +async def test_talk_send_message_with_reference_id_and_reply(mocker): + """send_message forwards reply_to, reference_id, silent.""" + mock_response = create_mock_talk_message_response( + message_id=1000, token="abc", text="A reply" + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.send_message( + "abc", + "A reply", + reply_to=999, + reference_id="dedupe-token", + silent=True, + ) + + body = mock_make_request.call_args[1]["json"] + assert body == { + "message": "A reply", + "replyTo": 999, + "referenceId": "dedupe-token", + "silent": True, + } + + +async def test_talk_mark_as_read_no_message(mocker): + """mark_as_read with no message ID sends an empty body.""" + mock_response = create_mock_response( + status_code=200, + json_data={"ocs": {"meta": {"status": "ok"}, "data": []}}, + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.mark_as_read("abc") + + call_args = mock_make_request.call_args + assert call_args[0][0] == "POST" + assert "/chat/abc/read" in call_args[0][1] + assert call_args[1]["json"] == {} + + +async def test_talk_mark_as_read_with_message(mocker): + """mark_as_read sends lastReadMessage when provided.""" + mock_response = create_mock_response( + status_code=200, + json_data={"ocs": {"meta": {"status": "ok"}, "data": []}}, + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.mark_as_read("abc", last_read_message=500) + + assert mock_make_request.call_args[1]["json"] == {"lastReadMessage": 500} + + +# Participant tests + + +async def test_talk_list_participants(mocker): + """list_participants parses the OCS-wrapped participant list.""" + mock_response = create_mock_response( + status_code=200, + json_data={ + "ocs": { + "meta": {"status": "ok"}, + "data": [ + { + "attendeeId": 1, + "actorType": "users", + "actorId": "alice", + "displayName": "Alice", + "participantType": 1, + "inCall": 0, + "lastPing": 1700000000, + }, + { + "attendeeId": 2, + "actorType": "users", + "actorId": "bob", + "displayName": "Bob", + "participantType": 3, + "inCall": 0, + "lastPing": 1700000000, + }, + ], + } + }, + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + participants = await client.list_participants("abc") + + assert len(participants) == 2 + assert all(isinstance(p, TalkParticipant) for p in participants) + assert participants[0].actorId == "alice" + assert participants[1].participantType == 3 + + call_args = mock_make_request.call_args + assert call_args[0][0] == "GET" + assert "/api/v4/room/abc/participants" in call_args[0][1] diff --git a/tests/conftest.py b/tests/conftest.py index a7a3ccfc..e8aade3f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -38,7 +38,8 @@ DEFAULT_FULL_SCOPES = ( "deck.read deck.write " "tables.read tables.write " "files.read files.write " - "sharing.read sharing.write" + "sharing.read sharing.write " + "talk.read talk.write" ) # Read-only scopes (all read scopes across apps) - should match DEFAULT_FULL_SCOPES read portion @@ -52,7 +53,8 @@ DEFAULT_READ_SCOPES = ( "deck.read " "tables.read " "files.read " - "sharing.read" + "sharing.read " + "talk.read" ) # Write-only scopes (all write scopes across apps) - should match DEFAULT_FULL_SCOPES write portion @@ -66,7 +68,8 @@ DEFAULT_WRITE_SCOPES = ( "deck.write " "tables.write " "files.write " - "sharing.write" + "sharing.write " + "talk.write" ) @@ -916,6 +919,43 @@ async def temporary_board_with_card( logger.error(f"Unexpected error deleting temporary card {card.id}: {e}") +@pytest.fixture +async def temporary_conversation(nc_client: NextcloudClient): + """Create a temporary Talk conversation and clean it up after the test. + + Yields a dict with the room ``token``, ``id``, and ``name``. + """ + unique_suffix = uuid.uuid4().hex[:8] + room_name = f"MCP Test Room {unique_suffix}" + token = None + + logger.info(f"Creating temporary Talk conversation: {room_name}") + try: + room = await nc_client.talk.create_conversation(room_name=room_name) + token = room.token + logger.info(f"Temporary Talk conversation created (token={token})") + yield {"token": token, "id": room.id, "name": room_name} + + finally: + if token: + logger.info(f"Cleaning up temporary Talk conversation token={token}") + try: + await nc_client.talk.delete_conversation(token) + logger.info(f"Successfully deleted Talk conversation token={token}") + except HTTPStatusError as e: + if e.response.status_code not in [404, 403]: + logger.error(f"HTTP error deleting Talk conversation {token}: {e}") + else: + logger.warning( + f"Talk conversation {token} already deleted or access denied " + f"({e.response.status_code})." + ) + except Exception as e: + logger.error( + f"Unexpected error deleting Talk conversation {token}: {e}" + ) + + @pytest.fixture(scope="session") def shared_test_calendar_name(): """Unique calendar name for the entire test session.""" diff --git a/tests/server/test_talk_mcp.py b/tests/server/test_talk_mcp.py new file mode 100644 index 00000000..d5d63214 --- /dev/null +++ b/tests/server/test_talk_mcp.py @@ -0,0 +1,143 @@ +"""Integration tests for the Nextcloud Talk (spreed) MCP tools.""" + +import json +import logging + +import pytest +from mcp import ClientSession + +from nextcloud_mcp_server.client import NextcloudClient + +logger = logging.getLogger(__name__) +pytestmark = pytest.mark.integration + + +EXPECTED_TALK_TOOLS = { + "talk_list_conversations", + "talk_get_conversation", + "talk_get_messages", + "talk_list_participants", + "talk_send_message", + "talk_mark_as_read", +} + + +async def test_talk_mcp_connectivity(nc_mcp_client: ClientSession): + """All six Talk tools should be registered with the MCP server.""" + tools = await nc_mcp_client.list_tools() + tool_names = {tool.name for tool in tools.tools} + + missing = EXPECTED_TALK_TOOLS - tool_names + assert not missing, f"Missing Talk tools: {missing}" + + +async def test_talk_send_and_read_workflow( + nc_mcp_client: ClientSession, + nc_client: NextcloudClient, + temporary_conversation: dict, +): + """End-to-end: post a message via MCP, read it back, mark read.""" + token = temporary_conversation["token"] + + # 1. Send a message via MCP + send_result = await nc_mcp_client.call_tool( + "talk_send_message", + {"token": token, "message": "Hello from MCP integration test"}, + ) + assert send_result.isError is False, ( + f"talk_send_message failed: {send_result.content}" + ) + send_payload = json.loads(send_result.content[0].text) + assert send_payload["success"] is True + posted = send_payload["message"] + assert posted["message"] == "Hello from MCP integration test" + assert posted["token"] == token + posted_id = posted["id"] + logger.info(f"Posted message id={posted_id} into token={token}") + + # 2. Cross-check via direct client + direct_messages, _ = await nc_client.talk.get_messages(token, limit=10) + direct_ids = [m.id for m in direct_messages] + assert posted_id in direct_ids, "Posted message not visible via direct client" + + # 3. Read messages via MCP + get_result = await nc_mcp_client.call_tool( + "talk_get_messages", {"token": token, "limit": 10} + ) + assert get_result.isError is False, ( + f"talk_get_messages failed: {get_result.content}" + ) + get_payload = json.loads(get_result.content[0].text) + assert get_payload["conversation_token"] == token + listed_ids = [m["id"] for m in get_payload["results"]] + assert posted_id in listed_ids, "Posted message not in MCP get_messages results" + + # 4. Mark conversation as read up to that message + mark_result = await nc_mcp_client.call_tool( + "talk_mark_as_read", + {"token": token, "last_read_message": posted_id}, + ) + assert mark_result.isError is False, ( + f"talk_mark_as_read failed: {mark_result.content}" + ) + mark_payload = json.loads(mark_result.content[0].text) + assert mark_payload["success"] is True + assert mark_payload["conversation_token"] == token + assert mark_payload["last_read_message"] == posted_id + + +async def test_talk_list_conversations_includes_temp_room( + nc_mcp_client: ClientSession, temporary_conversation: dict +): + """Newly created conversation should appear in talk_list_conversations.""" + token = temporary_conversation["token"] + + list_result = await nc_mcp_client.call_tool("talk_list_conversations", {}) + assert list_result.isError is False, ( + f"talk_list_conversations failed: {list_result.content}" + ) + payload = json.loads(list_result.content[0].text) + tokens = [r["token"] for r in payload["results"]] + assert token in tokens, "Temporary conversation not found in list" + + +async def test_talk_get_conversation( + nc_mcp_client: ClientSession, temporary_conversation: dict +): + """talk_get_conversation returns the same room we created.""" + token = temporary_conversation["token"] + name = temporary_conversation["name"] + + result = await nc_mcp_client.call_tool("talk_get_conversation", {"token": token}) + assert result.isError is False, f"talk_get_conversation failed: {result.content}" + payload = json.loads(result.content[0].text) + conversation = payload["conversation"] + assert conversation["token"] == token + assert conversation["name"] == name + + +async def test_talk_list_participants( + nc_mcp_client: ClientSession, temporary_conversation: dict +): + """talk_list_participants returns the room creator as a participant.""" + token = temporary_conversation["token"] + + result = await nc_mcp_client.call_tool("talk_list_participants", {"token": token}) + assert result.isError is False, f"talk_list_participants failed: {result.content}" + payload = json.loads(result.content[0].text) + assert payload["conversation_token"] == token + actor_ids = [p["actorId"] for p in payload["results"]] + # The user that created the room is always a participant. + assert len(actor_ids) >= 1 + + +async def test_talk_send_message_validation_empty_text( + nc_mcp_client: ClientSession, temporary_conversation: dict +): + """Empty message text is rejected client-side.""" + token = temporary_conversation["token"] + + result = await nc_mcp_client.call_tool( + "talk_send_message", {"token": token, "message": ""} + ) + assert result.isError is True, "Expected validation error for empty message" From b6eb7a6bb8189574c4c543fc15d3bb9354e3c8d9 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 00:03:24 +0200 Subject: [PATCH 2/5] fix(talk): address PR #741 reviewer feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four targeted fixes from the AI code review: 1. TalkConversation.description: drop the misleading `str | None` union (spreed always sends `""`, never null) — type is now `str` with default `""`. 2. get_messages: guard the X-Chat-Last-Given int parse with try/except so a misbehaving proxy can't crash the read flow; logs a warning and falls back to None. 3. get_messages: clamp `limit` to [1, 200] in the client (spreed caps server-side at 200 and silently truncates) so the returned `count` always matches what was actually requested. Both client and server-tool docstrings updated to state the valid range. 4. Add an integration test covering the 32000-char message ceiling in talk_send_message — the empty-message case was already tested, the over-length case was not. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/talk.py | 26 ++++++++++++++++++++++---- nextcloud_mcp_server/models/talk.py | 2 +- nextcloud_mcp_server/server/talk.py | 4 +++- tests/server/test_talk_mcp.py | 15 +++++++++++++++ 4 files changed, 41 insertions(+), 6 deletions(-) diff --git a/nextcloud_mcp_server/client/talk.py b/nextcloud_mcp_server/client/talk.py index 8ee01e58..1127e1d2 100644 --- a/nextcloud_mcp_server/client/talk.py +++ b/nextcloud_mcp_server/client/talk.py @@ -11,6 +11,7 @@ require ``OCS-APIRequest: true`` and respond as JSON when ``Accept: application/json`` is sent. """ +import logging from typing import Any from nextcloud_mcp_server.client.base import BaseNextcloudClient @@ -20,6 +21,8 @@ from nextcloud_mcp_server.models.talk import ( TalkParticipant, ) +logger = logging.getLogger(__name__) + class TalkClient(BaseNextcloudClient): """Client for Nextcloud Talk (spreed) app operations.""" @@ -124,7 +127,10 @@ class TalkClient(BaseNextcloudClient): Args: token: Conversation token. - limit: Max messages to return (spreed caps this at 200). + limit: Max messages to return. spreed caps this server-side + at 200; values outside ``[1, 200]`` are clamped here so + callers don't get a confusing mismatch between the + requested limit and the returned ``count``. last_known_message_id: Pagination cursor — pass the value from the previous response's ``X-Chat-Last-Given`` header. look_into_future: When False (default), return *older* @@ -140,10 +146,12 @@ class TalkClient(BaseNextcloudClient): Returns: ``(messages, x_chat_last_given)`` where the integer is the value of the ``X-Chat-Last-Given`` response header (or None - if the header was absent), suitable for pagination. + if the header was absent or unparseable), suitable for + pagination. """ + clamped_limit = min(max(1, limit), 200) params: dict[str, Any] = { - "limit": limit, + "limit": clamped_limit, "lookIntoFuture": 1 if look_into_future else 0, "setReadMarker": 1 if set_read_marker else 0, "includeLastKnown": 1 if include_last_known else 0, @@ -161,7 +169,17 @@ class TalkClient(BaseNextcloudClient): # spreed returns 200 with an empty data list when there's nothing # new, so we trust the JSON body here. last_given_header = response.headers.get("X-Chat-Last-Given") - last_given = int(last_given_header) if last_given_header else None + last_given: int | None = None + if last_given_header: + try: + last_given = int(last_given_header) + except ValueError: + # Defensive: spreed always sends an int, but a misbehaving + # proxy could mangle the header. Don't crash the read flow. + logger.warning( + "Invalid X-Chat-Last-Given header from spreed: %r", + last_given_header, + ) data = response.json()["ocs"]["data"] return [TalkMessage(**msg) for msg in data], last_given diff --git a/nextcloud_mcp_server/models/talk.py b/nextcloud_mcp_server/models/talk.py index 35b66c72..536ab2cd 100644 --- a/nextcloud_mcp_server/models/talk.py +++ b/nextcloud_mcp_server/models/talk.py @@ -53,7 +53,7 @@ class TalkConversation(BaseModel): type: int name: str displayName: str - description: str | None = "" + description: str = "" participantType: int | None = None unreadMessages: int = 0 unreadMention: bool = False diff --git a/nextcloud_mcp_server/server/talk.py b/nextcloud_mcp_server/server/talk.py index d1d91fd2..cdf8a9a3 100644 --- a/nextcloud_mcp_server/server/talk.py +++ b/nextcloud_mcp_server/server/talk.py @@ -105,7 +105,9 @@ def configure_talk_tools(mcp: FastMCP) -> None: Args: token: Conversation token. - limit: Max messages per page (spreed caps at 200). + limit: Max messages per page. Valid range is 1-200 (spreed + caps server-side at 200); values outside this range are + clamped. Default 50. last_known_message_id: Pagination cursor — pass the ``last_known_message_id`` from the previous response to fetch the next (older) page. diff --git a/tests/server/test_talk_mcp.py b/tests/server/test_talk_mcp.py index d5d63214..f9dfe766 100644 --- a/tests/server/test_talk_mcp.py +++ b/tests/server/test_talk_mcp.py @@ -141,3 +141,18 @@ async def test_talk_send_message_validation_empty_text( "talk_send_message", {"token": token, "message": ""} ) assert result.isError is True, "Expected validation error for empty message" + + +async def test_talk_send_message_validation_too_long( + nc_mcp_client: ClientSession, temporary_conversation: dict +): + """A message exceeding the 32000-char ceiling is rejected client-side.""" + token = temporary_conversation["token"] + + result = await nc_mcp_client.call_tool( + "talk_send_message", + {"token": token, "message": "x" * 32001}, + ) + assert result.isError is True, ( + "Expected validation error for message longer than 32000 characters" + ) From 8f92a7ea29e4ceb27dbed7673bea80cf23014d7e Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 00:29:04 +0200 Subject: [PATCH 3/5] docs(talk): address PR #741 reviewer nits Comment-only follow-up to surface non-obvious behavior at the call sites flagged in review: - server/talk.py: note the `uuid.uuid4().hex` 32-char no-dashes format (spreed accepts either form). - models/talk.py: warn that spreed returns `lastReadMessage: 0` rather than `null` for unread rooms, so consumers should compare to ``None`` rather than rely on truthiness. - 10-install-spreed-app.sh: document that the `app:install || app:enable` fallback also masks unrelated install failures, and limit its use to dev fixtures. No runtime behavior changes; tests unchanged (still 13 unit + 7 integ). Co-Authored-By: Claude Opus 4.7 (1M context) --- app-hooks/post-installation/10-install-spreed-app.sh | 5 +++++ nextcloud_mcp_server/models/talk.py | 3 +++ nextcloud_mcp_server/server/talk.py | 1 + 3 files changed, 9 insertions(+) diff --git a/app-hooks/post-installation/10-install-spreed-app.sh b/app-hooks/post-installation/10-install-spreed-app.sh index a4b84974..f1df23b6 100755 --- a/app-hooks/post-installation/10-install-spreed-app.sh +++ b/app-hooks/post-installation/10-install-spreed-app.sh @@ -6,4 +6,9 @@ set -euox pipefail # app is not yet installed, so app:install pulls it. On rebuilds where the # volume already has it, app:install would fail, so fall through to # app:enable. +# +# Caveat: this `||` also masks unrelated install failures (network outage, +# bad version pin, etc.) — they will fall through to app:enable, which +# will then fail with a clearer "app not found" error. Acceptable for a +# dev-fixture script; do not copy this pattern into production tooling. php /var/www/html/occ app:install spreed || php /var/www/html/occ app:enable spreed diff --git a/nextcloud_mcp_server/models/talk.py b/nextcloud_mcp_server/models/talk.py index 536ab2cd..0312bb03 100644 --- a/nextcloud_mcp_server/models/talk.py +++ b/nextcloud_mcp_server/models/talk.py @@ -58,6 +58,9 @@ class TalkConversation(BaseModel): unreadMessages: int = 0 unreadMention: bool = False lastActivity: int | None = None + # spreed returns `0` (not `null`) when the user has not read any + # message in the room yet — compare to ``None`` explicitly rather + # than relying on truthiness, since `0` is falsy but valid. lastReadMessage: int | None = None lastMessage: TalkMessage | None = None readOnly: int | None = None diff --git a/nextcloud_mcp_server/server/talk.py b/nextcloud_mcp_server/server/talk.py index cdf8a9a3..a8bdfb36 100644 --- a/nextcloud_mcp_server/server/talk.py +++ b/nextcloud_mcp_server/server/talk.py @@ -188,6 +188,7 @@ def configure_talk_tools(mcp: FastMCP) -> None: token, message, reply_to=reply_to, + # 32 hex chars, no dashes — spreed accepts either UUID format. reference_id=uuid.uuid4().hex, silent=silent, ) From 9614c0b361e34cef35e8348ce77efe7cb56b46b9 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 00:47:20 +0200 Subject: [PATCH 4/5] test(talk): cover include_status + malformed-header paths; drop Content-Type from default headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the missing-test and Content-Type points from the latest PR #741 review: - client/talk.py _talk_headers(): drop the manual `Content-Type: application/json`. httpx sets it automatically on requests that pass `json=`, and we no longer leak it onto bodyless GETs and DELETEs. - tests/client/talk/test_talk_api.py: - new `test_talk_list_participants_with_include_status` asserting `includeStatus=true` is forwarded. - new `test_talk_get_messages_invalid_last_given_header` covering the defensive try/except around the `X-Chat-Last-Given` parse — asserts the fallback `last_given=None` and that a warning is logged. - existing `test_talk_list_participants` extended to assert that `includeStatus` is *absent* by default. Unit tests: 13 → 15. Integration tests still 7/7. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/talk.py | 9 +++++-- tests/client/talk/test_talk_api.py | 39 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/nextcloud_mcp_server/client/talk.py b/nextcloud_mcp_server/client/talk.py index 1127e1d2..4f2b1280 100644 --- a/nextcloud_mcp_server/client/talk.py +++ b/nextcloud_mcp_server/client/talk.py @@ -33,11 +33,16 @@ class TalkClient(BaseNextcloudClient): _CHAT_BASE = "/ocs/v2.php/apps/spreed/api/v1/chat" def _talk_headers(self) -> dict[str, str]: - """Standard OCS+JSON headers for spreed API calls.""" + """Standard OCS+JSON headers for spreed API calls. + + ``Content-Type`` is intentionally omitted — httpx adds it + automatically (and correctly) on requests that pass ``json=``, + so setting it here would also leak it onto bodyless GETs and + DELETEs. + """ return { "OCS-APIRequest": "true", "Accept": "application/json", - "Content-Type": "application/json", } # Conversations (rooms) diff --git a/tests/client/talk/test_talk_api.py b/tests/client/talk/test_talk_api.py index 74f2b341..a38facbc 100644 --- a/tests/client/talk/test_talk_api.py +++ b/tests/client/talk/test_talk_api.py @@ -247,6 +247,26 @@ async def test_talk_get_messages_pagination_cursor(mocker): assert params["includeLastKnown"] == 1 +async def test_talk_get_messages_invalid_last_given_header(mocker, caplog): + """A non-numeric X-Chat-Last-Given falls back to None and logs a warning.""" + mock_response = create_mock_response( + status_code=200, + headers={"X-Chat-Last-Given": "not-a-number"}, + json_data={"ocs": {"meta": {"status": "ok"}, "data": []}}, + ) + mocker.patch.object(TalkClient, "_make_request", return_value=mock_response) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + with caplog.at_level(logging.WARNING, logger="nextcloud_mcp_server.client.talk"): + messages, last_given = await client.get_messages("abc") + + assert messages == [] + assert last_given is None + assert any( + "Invalid X-Chat-Last-Given" in record.message for record in caplog.records + ), "Expected a warning log for the malformed header" + + async def test_talk_send_message(mocker): """send_message posts the message text and parses the response.""" mock_response = create_mock_talk_message_response( @@ -384,3 +404,22 @@ async def test_talk_list_participants(mocker): call_args = mock_make_request.call_args assert call_args[0][0] == "GET" assert "/api/v4/room/abc/participants" in call_args[0][1] + # Default: include_status off → no includeStatus param + assert "includeStatus" not in call_args[1].get("params", {}) + + +async def test_talk_list_participants_with_include_status(mocker): + """include_status=True forwards includeStatus=true as a query param.""" + mock_response = create_mock_response( + status_code=200, + json_data={"ocs": {"meta": {"status": "ok"}, "data": []}}, + ) + mock_make_request = mocker.patch.object( + TalkClient, "_make_request", return_value=mock_response + ) + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.list_participants("abc", include_status=True) + + params = mock_make_request.call_args[1]["params"] + assert params["includeStatus"] == "true" From f075540232ea509d356199386841b2264fdb748e Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 01:14:32 +0200 Subject: [PATCH 5/5] fix(talk): address remaining PR #741 reviewer feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the seven outstanding items from the @claude review on PR #741: 1. Add empty `tests/client/talk/__init__.py` for pytest discovery parity with `tests/client/{collectives,news}/`. 2. Standardise boolean query params to integers — `includeStatus` was the string `"true"` in `list_conversations`/`list_participants` while every other flag (`noStatusUpdate`, `lookIntoFuture`, `setReadMarker`, `includeLastKnown`) used `1`/`0`. 3. Replace the `app:install || app:enable` chain in the spreed install hook with `app:install --keep-disabled --force || true; app:enable spreed`, so unrelated install failures surface as a clear "app not found" from `app:enable` rather than being silently masked. 4. Add `_validate_token()` (alphanumeric whitelist) and call it from all six TalkClient methods that interpolate the token into a URL path — defence-in-depth against pathological tokens reaching httpx. 5. Rename `TalkConversation.type` to `room_type` with `Field(alias="type")` and `populate_by_name=True`, so the field no longer shadows Python's builtin while preserving spreed's wire format on input. MCP responses now serialize `room_type` (field name) instead of `type`. 6. `mark_as_read` now passes `json=body or None` so the bodyless "mark everything as read" call doesn't send a spurious `{}` body and `Content-Type: application/json` header. 7. `_validate_message_text` rejects whitespace-only messages, not just empty strings. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../10-install-spreed-app.sh | 22 +++++--- nextcloud_mcp_server/client/talk.py | 28 ++++++++-- nextcloud_mcp_server/models/talk.py | 20 ++++++- nextcloud_mcp_server/server/talk.py | 6 ++- tests/client/talk/__init__.py | 0 tests/client/talk/test_talk_api.py | 54 ++++++++++++++++--- tests/server/test_talk_mcp.py | 15 ++++-- 7 files changed, 119 insertions(+), 26 deletions(-) create mode 100644 tests/client/talk/__init__.py diff --git a/app-hooks/post-installation/10-install-spreed-app.sh b/app-hooks/post-installation/10-install-spreed-app.sh index f1df23b6..8992e379 100755 --- a/app-hooks/post-installation/10-install-spreed-app.sh +++ b/app-hooks/post-installation/10-install-spreed-app.sh @@ -4,11 +4,19 @@ set -euox pipefail # Talk (spreed) ships its appstore release on demand; on a fresh container the # app is not yet installed, so app:install pulls it. On rebuilds where the -# volume already has it, app:install would fail, so fall through to -# app:enable. +# volume already has it, app:install fails with "already installed" — that +# specific failure is benign, hence the trailing `|| true`. We then run +# app:enable separately, which is the action that actually has to succeed. # -# Caveat: this `||` also masks unrelated install failures (network outage, -# bad version pin, etc.) — they will fall through to app:enable, which -# will then fail with a clearer "app not found" error. Acceptable for a -# dev-fixture script; do not copy this pattern into production tooling. -php /var/www/html/occ app:install spreed || php /var/www/html/occ app:enable spreed +# This split (install || true; then enable) is preferred over the previous +# `app:install || app:enable` chain because: +# - `--keep-disabled` keeps install side-effects strictly to fetching/extracting +# the app, so the enable step is the single source of truth for whether the +# app is active. +# - `--force` skips the compatibility check, locking the script to spreed's +# current behaviour rather than the appstore's view of NC compatibility. +# - If app:install dies for an unrelated reason (network outage, appstore +# unreachable on a fresh install), app:enable now fails with the clearer +# "app not found" rather than the install-time error being masked entirely. +php /var/www/html/occ app:install spreed --keep-disabled --force || true +php /var/www/html/occ app:enable spreed diff --git a/nextcloud_mcp_server/client/talk.py b/nextcloud_mcp_server/client/talk.py index 4f2b1280..8673eb88 100644 --- a/nextcloud_mcp_server/client/talk.py +++ b/nextcloud_mcp_server/client/talk.py @@ -12,6 +12,7 @@ application/json`` is sent. """ import logging +import re from typing import Any from nextcloud_mcp_server.client.base import BaseNextcloudClient @@ -24,6 +25,18 @@ from nextcloud_mcp_server.models.talk import ( logger = logging.getLogger(__name__) +# Spreed conversation tokens are short alphanumeric strings (e.g. "a1b2c3d4"). +# httpx does not normalise path traversal sequences, so a pathological token +# like ``"../foo"`` would be sent verbatim. Validate up-front for clearer +# errors and defence-in-depth. +_TALK_TOKEN_RE = re.compile(r"^[A-Za-z0-9]+$") + + +def _validate_token(token: str) -> None: + if not _TALK_TOKEN_RE.fullmatch(token): + raise ValueError(f"Invalid Talk conversation token: {token!r}") + + class TalkClient(BaseNextcloudClient): """Client for Nextcloud Talk (spreed) app operations.""" @@ -68,7 +81,7 @@ class TalkClient(BaseNextcloudClient): if modified_since is not None: params["modifiedSince"] = modified_since if include_status: - params["includeStatus"] = "true" + params["includeStatus"] = 1 if no_status_update: params["noStatusUpdate"] = 1 response = await self._make_request( @@ -79,6 +92,7 @@ class TalkClient(BaseNextcloudClient): async def get_conversation(self, token: str) -> TalkConversation: """Fetch a single Talk conversation by its room token.""" + _validate_token(token) response = await self._make_request( "GET", f"{self._ROOM_BASE}/{token}", headers=self._talk_headers() ) @@ -112,6 +126,7 @@ class TalkClient(BaseNextcloudClient): async def delete_conversation(self, token: str) -> None: """Delete a conversation. Used by integration test cleanup.""" + _validate_token(token) await self._make_request( "DELETE", f"{self._ROOM_BASE}/{token}", headers=self._talk_headers() ) @@ -154,6 +169,7 @@ class TalkClient(BaseNextcloudClient): if the header was absent or unparseable), suitable for pagination. """ + _validate_token(token) clamped_limit = min(max(1, limit), 200) params: dict[str, Any] = { "limit": clamped_limit, @@ -208,6 +224,7 @@ class TalkClient(BaseNextcloudClient): silent: When True, the message is delivered without push notifications. """ + _validate_token(token) body: dict[str, Any] = {"message": message} if reply_to is not None: body["replyTo"] = reply_to @@ -232,13 +249,17 @@ class TalkClient(BaseNextcloudClient): that message; otherwise spreed marks everything currently in the room as read. """ + _validate_token(token) body: dict[str, Any] = {} if last_read_message is not None: body["lastReadMessage"] = last_read_message + # ``json=None`` makes httpx skip both the body and the + # ``Content-Type: application/json`` header — semantically correct + # for the bodyless "mark everything as read" call. await self._make_request( "POST", f"{self._CHAT_BASE}/{token}/read", - json=body, + json=body or None, headers=self._talk_headers(), ) @@ -248,9 +269,10 @@ class TalkClient(BaseNextcloudClient): self, token: str, *, include_status: bool = False ) -> list[TalkParticipant]: """List participants of a Talk conversation.""" + _validate_token(token) params: dict[str, Any] = {} if include_status: - params["includeStatus"] = "true" + params["includeStatus"] = 1 response = await self._make_request( "GET", f"{self._ROOM_BASE}/{token}/participants", diff --git a/nextcloud_mcp_server/models/talk.py b/nextcloud_mcp_server/models/talk.py index 0312bb03..9f5bf2f6 100644 --- a/nextcloud_mcp_server/models/talk.py +++ b/nextcloud_mcp_server/models/talk.py @@ -2,7 +2,7 @@ from typing import Any -from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator from .base import BaseResponse, StatusResponse @@ -48,9 +48,25 @@ class TalkConversation(BaseModel): flows. """ + # ``populate_by_name=True`` lets us deserialize spreed's ``type`` key + # into the ``room_type`` field while still allowing internal callers + # to construct the model with ``room_type=...`` directly. + model_config = ConfigDict(populate_by_name=True) + id: int token: str - type: int + # The spreed JSON wire format uses ``type`` for the room kind, but + # ``type`` shadows Python's builtin within the class scope, which + # would silently call this int field if anyone wrote ``type(...)`` + # in a validator or method on this model. Map to ``room_type`` and + # alias the wire field instead. + room_type: int = Field( + alias="type", + description=( + "Conversation kind: 1=one-to-one, 2=group, 3=public, " + "4=changelog, 5=former one-to-one, 6=note-to-self." + ), + ) name: str displayName: str description: str = "" diff --git a/nextcloud_mcp_server/server/talk.py b/nextcloud_mcp_server/server/talk.py index a8bdfb36..52cf787d 100644 --- a/nextcloud_mcp_server/server/talk.py +++ b/nextcloud_mcp_server/server/talk.py @@ -27,8 +27,10 @@ _MESSAGE_MAX_LENGTH = 32000 def _validate_message_text(message: str) -> None: - if not message: - raise ValueError("Message text must not be empty") + # Reject both empty strings and whitespace-only strings — spreed + # would happily post the latter as a visually-blank message. + if not message or not message.strip(): + raise ValueError("Message text must not be empty or whitespace-only") if len(message) > _MESSAGE_MAX_LENGTH: raise ValueError( f"Message too long: {len(message)} characters (max {_MESSAGE_MAX_LENGTH})" diff --git a/tests/client/talk/__init__.py b/tests/client/talk/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/client/talk/test_talk_api.py b/tests/client/talk/test_talk_api.py index a38facbc..277bbbb3 100644 --- a/tests/client/talk/test_talk_api.py +++ b/tests/client/talk/test_talk_api.py @@ -5,7 +5,7 @@ import logging import httpx import pytest -from nextcloud_mcp_server.client.talk import TalkClient +from nextcloud_mcp_server.client.talk import TalkClient, _validate_token from nextcloud_mcp_server.models.talk import ( TalkConversation, TalkMessage, @@ -22,6 +22,44 @@ logger = logging.getLogger(__name__) pytestmark = pytest.mark.unit +# Token validation + + +@pytest.mark.parametrize( + "bad_token", + [ + "", + "../foo", + "a/b", + "a b", + "a.b", + "a-b", + "token!", + ], +) +def test_validate_token_rejects_invalid(bad_token): + """_validate_token rejects anything outside the alphanumeric whitelist.""" + with pytest.raises(ValueError, match="Invalid Talk conversation token"): + _validate_token(bad_token) + + +@pytest.mark.parametrize("good_token", ["a1b2c3d4", "ABC123", "abcdef", "1"]) +def test_validate_token_accepts_valid(good_token): + """Real spreed tokens — short alphanumeric strings — pass through.""" + _validate_token(good_token) + + +async def test_talk_get_conversation_rejects_path_traversal(mocker): + """Pathological tokens never reach the HTTP layer.""" + mock_make_request = mocker.patch.object(TalkClient, "_make_request") + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + with pytest.raises(ValueError, match="Invalid Talk conversation token"): + await client.get_conversation("../etc/passwd") + + mock_make_request.assert_not_called() + + # Conversation tests @@ -64,7 +102,7 @@ async def test_talk_list_conversations(mocker): assert len(rooms) == 2 assert all(isinstance(r, TalkConversation) for r in rooms) assert rooms[0].token == "abc" - assert rooms[1].type == 3 + assert rooms[1].room_type == 3 mock_make_request.assert_called_once() call_args = mock_make_request.call_args @@ -89,7 +127,7 @@ async def test_talk_list_conversations_with_modified_since(mocker): params = mock_make_request.call_args[1]["params"] assert params["modifiedSince"] == 1700000000 - assert params["includeStatus"] == "true" + assert params["includeStatus"] == 1 async def test_talk_get_conversation(mocker): @@ -322,7 +360,7 @@ async def test_talk_send_message_with_reference_id_and_reply(mocker): async def test_talk_mark_as_read_no_message(mocker): - """mark_as_read with no message ID sends an empty body.""" + """mark_as_read with no message ID sends no body (json=None).""" mock_response = create_mock_response( status_code=200, json_data={"ocs": {"meta": {"status": "ok"}, "data": []}}, @@ -337,7 +375,9 @@ async def test_talk_mark_as_read_no_message(mocker): call_args = mock_make_request.call_args assert call_args[0][0] == "POST" assert "/chat/abc/read" in call_args[0][1] - assert call_args[1]["json"] == {} + # Empty body is sent as ``json=None`` so httpx skips both the body and + # the ``Content-Type: application/json`` header for this bodyless POST. + assert call_args[1]["json"] is None async def test_talk_mark_as_read_with_message(mocker): @@ -409,7 +449,7 @@ async def test_talk_list_participants(mocker): async def test_talk_list_participants_with_include_status(mocker): - """include_status=True forwards includeStatus=true as a query param.""" + """include_status=True forwards includeStatus=1 as a query param.""" mock_response = create_mock_response( status_code=200, json_data={"ocs": {"meta": {"status": "ok"}, "data": []}}, @@ -422,4 +462,4 @@ async def test_talk_list_participants_with_include_status(mocker): await client.list_participants("abc", include_status=True) params = mock_make_request.call_args[1]["params"] - assert params["includeStatus"] == "true" + assert params["includeStatus"] == 1 diff --git a/tests/server/test_talk_mcp.py b/tests/server/test_talk_mcp.py index f9dfe766..2eb52757 100644 --- a/tests/server/test_talk_mcp.py +++ b/tests/server/test_talk_mcp.py @@ -131,16 +131,21 @@ async def test_talk_list_participants( assert len(actor_ids) >= 1 -async def test_talk_send_message_validation_empty_text( - nc_mcp_client: ClientSession, temporary_conversation: dict +@pytest.mark.parametrize("blank_text", ["", " ", "\t\n", " \t \n "]) +async def test_talk_send_message_validation_blank_text( + nc_mcp_client: ClientSession, + temporary_conversation: dict, + blank_text: str, ): - """Empty message text is rejected client-side.""" + """Empty and whitespace-only message text are rejected client-side.""" token = temporary_conversation["token"] result = await nc_mcp_client.call_tool( - "talk_send_message", {"token": token, "message": ""} + "talk_send_message", {"token": token, "message": blank_text} + ) + assert result.isError is True, ( + f"Expected validation error for blank message {blank_text!r}" ) - assert result.isError is True, "Expected validation error for empty message" async def test_talk_send_message_validation_too_long(