From 2129bd6fac2d20fa5e644d3798e8b273ecd602af Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 29 Apr 2026 16:57:46 +0200 Subject: [PATCH] fix(deck): address review feedback on card comment tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Wrap raw DeckComment returns in CardCommentResponse(BaseResponse) for create/update so the success/timestamp envelope matches other deck tools (#737 review issue 2). - Rename ListCardCommentsResponse.total → count and clarify in the description that it's the page size, not a server-side total — the Deck list endpoint does not expose one (#737 review issue 3). - Validate the documented 1000-character limit on create/update with an inline length check + ValueError, matching the pattern in api/management.py (#737 review issue 4). - Use modern int | None union syntax for the new parent_id parameter (#737 review issue 1); rest of the file is left in the existing Optional[...] style. Also add an MCP-level test that the >1000 char message is rejected, and update the existing comment tests to unwrap the new comment field. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/models/deck.py | 17 ++++++++++++++--- nextcloud_mcp_server/server/deck.py | 29 ++++++++++++++++++++++------- tests/server/test_deck_mcp.py | 28 +++++++++++++++++++++++----- 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/nextcloud_mcp_server/models/deck.py b/nextcloud_mcp_server/models/deck.py index 6fa7357e..46f3eb5d 100644 --- a/nextcloud_mcp_server/models/deck.py +++ b/nextcloud_mcp_server/models/deck.py @@ -288,12 +288,23 @@ class LabelOperationResponse(StatusResponse): class ListCardCommentsResponse(BaseResponse): """Response model for listing card comments.""" - results: list[DeckComment] = Field(description="List of card comments") - total: int = Field(description="Number of comments returned") + results: list[DeckComment] = Field(description="Card comments in this page") + count: int = Field( + description=( + "Number of comments returned in this page (page size, not the " + "server-side total — the Deck list endpoint does not expose a total)." + ) + ) + + +class CardCommentResponse(BaseResponse): + """Response model returned when a single card comment is created or updated.""" + + comment: DeckComment = Field(description="The created or updated card comment") class CardCommentOperationResponse(StatusResponse): - """Response model for card comment create/update/delete operations.""" + """Response model for card comment operations that don't return comment data (e.g. delete).""" card_id: int = Field(description="ID of the card the comment belongs to") comment_id: int = Field(description="ID of the affected comment") diff --git a/nextcloud_mcp_server/server/deck.py b/nextcloud_mcp_server/server/deck.py index aa3ef1c9..6b7a89ba 100644 --- a/nextcloud_mcp_server/server/deck.py +++ b/nextcloud_mcp_server/server/deck.py @@ -8,6 +8,7 @@ from nextcloud_mcp_server.auth import require_scopes from nextcloud_mcp_server.context import get_client from nextcloud_mcp_server.models.deck import ( CardCommentOperationResponse, + CardCommentResponse, CardOperationResponse, CreateBoardResponse, CreateCardResponse, @@ -15,7 +16,6 @@ from nextcloud_mcp_server.models.deck import ( CreateStackResponse, DeckBoard, DeckCard, - DeckComment, DeckLabel, DeckStack, LabelOperationResponse, @@ -728,6 +728,15 @@ def configure_deck_tools(mcp: FastMCP): # Card Comment Tools + _COMMENT_MAX_LENGTH = 1000 + + def _validate_comment_message(message: str) -> None: + if len(message) > _COMMENT_MAX_LENGTH: + raise ValueError( + f"Comment message too long: {len(message)} characters " + f"(max {_COMMENT_MAX_LENGTH})" + ) + @mcp.tool( title="List Deck Card Comments", annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), @@ -746,7 +755,7 @@ def configure_deck_tools(mcp: FastMCP): """ client = await get_client(ctx) comments = await client.deck.get_comments(card_id, limit=limit, offset=offset) - return ListCardCommentsResponse(results=comments, total=len(comments)) + return ListCardCommentsResponse(results=comments, count=len(comments)) @mcp.tool( title="Create Deck Card Comment", @@ -758,8 +767,8 @@ def configure_deck_tools(mcp: FastMCP): ctx: Context, card_id: int, message: str, - parent_id: Optional[int] = None, - ) -> DeckComment: + parent_id: int | None = None, + ) -> CardCommentResponse: """Create a comment on a Nextcloud Deck card Supports @-mentions (e.g. "@alice"). Pass parent_id to reply to an @@ -770,8 +779,12 @@ def configure_deck_tools(mcp: FastMCP): message: The comment text (max 1000 characters) parent_id: Optional ID of a parent comment to reply to """ + _validate_comment_message(message) client = await get_client(ctx) - return await client.deck.create_comment(card_id, message, parent_id=parent_id) + comment = await client.deck.create_comment( + card_id, message, parent_id=parent_id + ) + return CardCommentResponse(comment=comment) @mcp.tool( title="Update Deck Card Comment", @@ -781,7 +794,7 @@ def configure_deck_tools(mcp: FastMCP): @instrument_tool async def deck_update_card_comment( ctx: Context, card_id: int, comment_id: int, message: str - ) -> DeckComment: + ) -> CardCommentResponse: """Update a Nextcloud Deck card comment Only the comment's author can update it; the server returns 403 otherwise. @@ -791,8 +804,10 @@ def configure_deck_tools(mcp: FastMCP): comment_id: The ID of the comment to update message: The new comment text (max 1000 characters) """ + _validate_comment_message(message) client = await get_client(ctx) - return await client.deck.update_comment(card_id, comment_id, message) + comment = await client.deck.update_comment(card_id, comment_id, message) + return CardCommentResponse(comment=comment) @mcp.tool( title="Delete Deck Card Comment", diff --git a/tests/server/test_deck_mcp.py b/tests/server/test_deck_mcp.py index acca2746..d3fa6a9e 100644 --- a/tests/server/test_deck_mcp.py +++ b/tests/server/test_deck_mcp.py @@ -243,7 +243,9 @@ async def test_deck_card_comment_crud_workflow_mcp( assert create_result.isError is False, ( f"Comment creation failed: {create_result.content}" ) - comment = json.loads(create_result.content[0].text) + create_response = json.loads(create_result.content[0].text) + assert create_response["success"] is True + comment = create_response["comment"] comment_id = comment["id"] assert comment["objectId"] == card_id assert comment["message"] == "Initial comment" @@ -256,7 +258,7 @@ async def test_deck_card_comment_crud_workflow_mcp( ) assert list_result.isError is False, f"List comments failed: {list_result.content}" listed = json.loads(list_result.content[0].text) - assert listed["total"] >= 1 + assert listed["count"] >= 1 listed_ids = [c["id"] for c in listed["results"]] assert comment_id in listed_ids, "Created comment not in list" @@ -277,7 +279,8 @@ async def test_deck_card_comment_crud_workflow_mcp( assert update_result.isError is False, ( f"Comment update failed: {update_result.content}" ) - updated = json.loads(update_result.content[0].text) + update_response = json.loads(update_result.content[0].text) + updated = update_response["comment"] assert updated["id"] == comment_id assert updated["message"] == "Edited comment" @@ -316,7 +319,7 @@ async def test_deck_card_comment_reply_mcp( {"card_id": card_id, "message": "Parent message"}, ) assert parent_result.isError is False - parent = json.loads(parent_result.content[0].text) + parent = json.loads(parent_result.content[0].text)["comment"] parent_id = parent["id"] # Create a reply @@ -329,9 +332,24 @@ async def test_deck_card_comment_reply_mcp( }, ) assert reply_result.isError is False, f"Reply failed: {reply_result.content}" - reply = json.loads(reply_result.content[0].text) + reply = json.loads(reply_result.content[0].text)["comment"] assert reply["message"] == "Reply message" assert reply["replyTo"] is not None, "replyTo should be populated for replies" assert reply["replyTo"]["id"] == parent_id assert reply["replyTo"]["message"] == "Parent message" + + +async def test_deck_card_comment_message_too_long_mcp( + nc_mcp_client: ClientSession, temporary_board_with_card: tuple +): + """Creating a comment longer than 1000 chars is rejected client-side.""" + _, _, card_data = temporary_board_with_card + card_id = card_data["id"] + + too_long = "x" * 1001 + result = await nc_mcp_client.call_tool( + "deck_create_card_comment", + {"card_id": card_id, "message": too_long}, + ) + assert result.isError is True, "Expected validation error for >1000 char message"