fix(deck): address review feedback on card comment tools
- 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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
13abaf3db7
commit
2129bd6fac
@@ -288,12 +288,23 @@ class LabelOperationResponse(StatusResponse):
|
|||||||
class ListCardCommentsResponse(BaseResponse):
|
class ListCardCommentsResponse(BaseResponse):
|
||||||
"""Response model for listing card comments."""
|
"""Response model for listing card comments."""
|
||||||
|
|
||||||
results: list[DeckComment] = Field(description="List of card comments")
|
results: list[DeckComment] = Field(description="Card comments in this page")
|
||||||
total: int = Field(description="Number of comments returned")
|
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):
|
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")
|
card_id: int = Field(description="ID of the card the comment belongs to")
|
||||||
comment_id: int = Field(description="ID of the affected comment")
|
comment_id: int = Field(description="ID of the affected comment")
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ from nextcloud_mcp_server.auth import require_scopes
|
|||||||
from nextcloud_mcp_server.context import get_client
|
from nextcloud_mcp_server.context import get_client
|
||||||
from nextcloud_mcp_server.models.deck import (
|
from nextcloud_mcp_server.models.deck import (
|
||||||
CardCommentOperationResponse,
|
CardCommentOperationResponse,
|
||||||
|
CardCommentResponse,
|
||||||
CardOperationResponse,
|
CardOperationResponse,
|
||||||
CreateBoardResponse,
|
CreateBoardResponse,
|
||||||
CreateCardResponse,
|
CreateCardResponse,
|
||||||
@@ -15,7 +16,6 @@ from nextcloud_mcp_server.models.deck import (
|
|||||||
CreateStackResponse,
|
CreateStackResponse,
|
||||||
DeckBoard,
|
DeckBoard,
|
||||||
DeckCard,
|
DeckCard,
|
||||||
DeckComment,
|
|
||||||
DeckLabel,
|
DeckLabel,
|
||||||
DeckStack,
|
DeckStack,
|
||||||
LabelOperationResponse,
|
LabelOperationResponse,
|
||||||
@@ -728,6 +728,15 @@ def configure_deck_tools(mcp: FastMCP):
|
|||||||
|
|
||||||
# Card Comment Tools
|
# 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(
|
@mcp.tool(
|
||||||
title="List Deck Card Comments",
|
title="List Deck Card Comments",
|
||||||
annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True),
|
annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True),
|
||||||
@@ -746,7 +755,7 @@ def configure_deck_tools(mcp: FastMCP):
|
|||||||
"""
|
"""
|
||||||
client = await get_client(ctx)
|
client = await get_client(ctx)
|
||||||
comments = await client.deck.get_comments(card_id, limit=limit, offset=offset)
|
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(
|
@mcp.tool(
|
||||||
title="Create Deck Card Comment",
|
title="Create Deck Card Comment",
|
||||||
@@ -758,8 +767,8 @@ def configure_deck_tools(mcp: FastMCP):
|
|||||||
ctx: Context,
|
ctx: Context,
|
||||||
card_id: int,
|
card_id: int,
|
||||||
message: str,
|
message: str,
|
||||||
parent_id: Optional[int] = None,
|
parent_id: int | None = None,
|
||||||
) -> DeckComment:
|
) -> CardCommentResponse:
|
||||||
"""Create a comment on a Nextcloud Deck card
|
"""Create a comment on a Nextcloud Deck card
|
||||||
|
|
||||||
Supports @-mentions (e.g. "@alice"). Pass parent_id to reply to an
|
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)
|
message: The comment text (max 1000 characters)
|
||||||
parent_id: Optional ID of a parent comment to reply to
|
parent_id: Optional ID of a parent comment to reply to
|
||||||
"""
|
"""
|
||||||
|
_validate_comment_message(message)
|
||||||
client = await get_client(ctx)
|
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(
|
@mcp.tool(
|
||||||
title="Update Deck Card Comment",
|
title="Update Deck Card Comment",
|
||||||
@@ -781,7 +794,7 @@ def configure_deck_tools(mcp: FastMCP):
|
|||||||
@instrument_tool
|
@instrument_tool
|
||||||
async def deck_update_card_comment(
|
async def deck_update_card_comment(
|
||||||
ctx: Context, card_id: int, comment_id: int, message: str
|
ctx: Context, card_id: int, comment_id: int, message: str
|
||||||
) -> DeckComment:
|
) -> CardCommentResponse:
|
||||||
"""Update a Nextcloud Deck card comment
|
"""Update a Nextcloud Deck card comment
|
||||||
|
|
||||||
Only the comment's author can update it; the server returns 403 otherwise.
|
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
|
comment_id: The ID of the comment to update
|
||||||
message: The new comment text (max 1000 characters)
|
message: The new comment text (max 1000 characters)
|
||||||
"""
|
"""
|
||||||
|
_validate_comment_message(message)
|
||||||
client = await get_client(ctx)
|
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(
|
@mcp.tool(
|
||||||
title="Delete Deck Card Comment",
|
title="Delete Deck Card Comment",
|
||||||
|
|||||||
@@ -243,7 +243,9 @@ async def test_deck_card_comment_crud_workflow_mcp(
|
|||||||
assert create_result.isError is False, (
|
assert create_result.isError is False, (
|
||||||
f"Comment creation failed: {create_result.content}"
|
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"]
|
comment_id = comment["id"]
|
||||||
assert comment["objectId"] == card_id
|
assert comment["objectId"] == card_id
|
||||||
assert comment["message"] == "Initial comment"
|
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}"
|
assert list_result.isError is False, f"List comments failed: {list_result.content}"
|
||||||
listed = json.loads(list_result.content[0].text)
|
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"]]
|
listed_ids = [c["id"] for c in listed["results"]]
|
||||||
assert comment_id in listed_ids, "Created comment not in list"
|
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, (
|
assert update_result.isError is False, (
|
||||||
f"Comment update failed: {update_result.content}"
|
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["id"] == comment_id
|
||||||
assert updated["message"] == "Edited comment"
|
assert updated["message"] == "Edited comment"
|
||||||
|
|
||||||
@@ -316,7 +319,7 @@ async def test_deck_card_comment_reply_mcp(
|
|||||||
{"card_id": card_id, "message": "Parent message"},
|
{"card_id": card_id, "message": "Parent message"},
|
||||||
)
|
)
|
||||||
assert parent_result.isError is False
|
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"]
|
parent_id = parent["id"]
|
||||||
|
|
||||||
# Create a reply
|
# 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}"
|
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["message"] == "Reply message"
|
||||||
assert reply["replyTo"] is not None, "replyTo should be populated for replies"
|
assert reply["replyTo"] is not None, "replyTo should be populated for replies"
|
||||||
assert reply["replyTo"]["id"] == parent_id
|
assert reply["replyTo"]["id"] == parent_id
|
||||||
assert reply["replyTo"]["message"] == "Parent message"
|
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"
|
||||||
|
|||||||
Reference in New Issue
Block a user