From b6eb7a6bb8189574c4c543fc15d3bb9354e3c8d9 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 00:03:24 +0200 Subject: [PATCH] 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" + )