From d8cd073e66f6dea4fdb8a772f042506ebc1da4f9 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 00:00:09 +0200 Subject: [PATCH 1/4] feat(deck): add response filters and archived stacks tool Add filtering options to deck read tools to keep responses compact on boards with accumulated cards/comments, and expose archived stacks so agents can audit completed work that has been archived off the active board. - deck_get_board: include_acl, include_users, include_labels - deck_get_stacks/deck_get_stack: include_cards, include_archived_cards, description_max_length - deck_get_cards: include_archived, description_max_length - New deck_get_archived_stacks tool wrapping the existing client method Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/server/deck.py | 146 ++++++++++++++++++++++++++-- tests/client/deck/test_deck_api.py | 31 ++++++ tests/unit/test_deck_server.py | 51 ++++++++++ 3 files changed, 220 insertions(+), 8 deletions(-) create mode 100644 tests/unit/test_deck_server.py diff --git a/nextcloud_mcp_server/server/deck.py b/nextcloud_mcp_server/server/deck.py index 6b7a89ba..b4dd7d3d 100644 --- a/nextcloud_mcp_server/server/deck.py +++ b/nextcloud_mcp_server/server/deck.py @@ -31,6 +31,17 @@ from nextcloud_mcp_server.observability.metrics import instrument_tool logger = logging.getLogger(__name__) +def _truncate_card_descriptions( + cards: list[DeckCard], description_max_length: int | None +) -> None: + """Truncate each card's description in-place when it exceeds the limit.""" + if description_max_length is None: + return + for card in cards: + if card.description and len(card.description) > description_max_length: + card.description = card.description[:description_max_length] + "…" + + def configure_deck_tools(mcp: FastMCP): """Configure Nextcloud Deck tools and resources for the MCP server.""" @@ -143,10 +154,33 @@ def configure_deck_tools(mcp: FastMCP): ) @require_scopes("deck.read") @instrument_tool - async def deck_get_board(ctx: Context, board_id: int) -> DeckBoard: - """Get details of a specific Nextcloud Deck board""" + async def deck_get_board( + ctx: Context, + board_id: int, + include_acl: bool = True, + include_users: bool = True, + include_labels: bool = True, + ) -> DeckBoard: + """Get details of a specific Nextcloud Deck board. + + Args: + board_id: The ID of the board + include_acl: Include the board's ACL entries (default True). Set + False to reduce response size when ACLs are not needed. + include_users: Include the board's user list (default True). Set + False to reduce response size when users are not needed. + include_labels: Include the board's label definitions (default + True). Set False to reduce response size; labels can still be + retrieved via deck_get_labels. + """ client = await get_client(ctx) board = await client.deck.get_board(board_id) + if not include_acl: + board.acl = [] + if not include_users: + board.users = [] + if not include_labels: + board.labels = [] return board @mcp.tool( @@ -155,10 +189,36 @@ def configure_deck_tools(mcp: FastMCP): ) @require_scopes("deck.read") @instrument_tool - async def deck_get_stacks(ctx: Context, board_id: int) -> ListStacksResponse: - """Get all stacks in a Nextcloud Deck board""" + async def deck_get_stacks( + ctx: Context, + board_id: int, + include_cards: bool = True, + include_archived_cards: bool = False, + description_max_length: int | None = None, + ) -> ListStacksResponse: + """Get all stacks in a Nextcloud Deck board. + + Args: + board_id: The ID of the board + include_cards: Include cards inside each stack (default True). Set + False for a lightweight stack listing; fetch cards separately + via deck_get_cards. + include_archived_cards: Include archived cards (default False). + Only relevant when include_cards is True. + description_max_length: If set, truncate each card's description + to this many characters. Useful for keeping responses compact + on boards with long card specs. + """ client = await get_client(ctx) stacks = await client.deck.get_stacks(board_id) + for stack in stacks: + if not include_cards: + stack.cards = None + continue + if stack.cards: + if not include_archived_cards: + stack.cards = [c for c in stack.cards if not c.archived] + _truncate_card_descriptions(stack.cards, description_max_length) return ListStacksResponse(stacks=stacks, total=len(stacks)) @mcp.tool( @@ -167,12 +227,65 @@ def configure_deck_tools(mcp: FastMCP): ) @require_scopes("deck.read") @instrument_tool - async def deck_get_stack(ctx: Context, board_id: int, stack_id: int) -> DeckStack: - """Get details of a specific Nextcloud Deck stack""" + async def deck_get_stack( + ctx: Context, + board_id: int, + stack_id: int, + include_cards: bool = True, + include_archived_cards: bool = False, + description_max_length: int | None = None, + ) -> DeckStack: + """Get details of a specific Nextcloud Deck stack. + + Args: + board_id: The ID of the board + stack_id: The ID of the stack + include_cards: Include cards in the stack (default True). + include_archived_cards: Include archived cards (default False). + Only relevant when include_cards is True. + description_max_length: If set, truncate each card's description + to this many characters. + """ client = await get_client(ctx) stack = await client.deck.get_stack(board_id, stack_id) + if not include_cards: + stack.cards = None + elif stack.cards: + if not include_archived_cards: + stack.cards = [c for c in stack.cards if not c.archived] + _truncate_card_descriptions(stack.cards, description_max_length) return stack + @mcp.tool( + title="List Archived Deck Stacks", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("deck.read") + @instrument_tool + async def deck_get_archived_stacks( + ctx: Context, + board_id: int, + description_max_length: int | None = None, + ) -> ListStacksResponse: + """List archived stacks (with their archived cards) for a Nextcloud + Deck board. + + Use this to audit completed work that has been archived off the + active board (e.g. cards moved through a "Done" stack and then + archived via deck_archive_card). The shape mirrors deck_get_stacks. + + Args: + board_id: The ID of the board + description_max_length: If set, truncate each card's description + to this many characters. + """ + client = await get_client(ctx) + stacks = await client.deck.get_archived_stacks(board_id) + for stack in stacks: + if stack.cards: + _truncate_card_descriptions(stack.cards, description_max_length) + return ListStacksResponse(stacks=stacks, total=len(stacks)) + @mcp.tool( title="List Deck Cards", annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), @@ -180,12 +293,29 @@ def configure_deck_tools(mcp: FastMCP): @require_scopes("deck.read") @instrument_tool async def deck_get_cards( - ctx: Context, board_id: int, stack_id: int + ctx: Context, + board_id: int, + stack_id: int, + include_archived: bool = False, + description_max_length: int | None = None, ) -> ListCardsResponse: - """Get all cards in a Nextcloud Deck stack""" + """Get all cards in a Nextcloud Deck stack. + + Args: + board_id: The ID of the board + stack_id: The ID of the stack + include_archived: Include archived cards (default False). Archived + cards can also be retrieved per-board via + deck_get_archived_stacks. + description_max_length: If set, truncate each card's description + to this many characters. + """ client = await get_client(ctx) stack = await client.deck.get_stack(board_id, stack_id) cards = stack.cards or [] + if not include_archived: + cards = [c for c in cards if not c.archived] + _truncate_card_descriptions(cards, description_max_length) return ListCardsResponse(cards=cards, total=len(cards)) @mcp.tool( diff --git a/tests/client/deck/test_deck_api.py b/tests/client/deck/test_deck_api.py index c5bec571..9b988e63 100644 --- a/tests/client/deck/test_deck_api.py +++ b/tests/client/deck/test_deck_api.py @@ -255,6 +255,37 @@ async def test_deck_get_stacks(mocker): mock_make_request.assert_called_once() +async def test_deck_get_archived_stacks(mocker): + """Test that get_archived_stacks targets the archived endpoint and parses the response.""" + mock_response = create_mock_response( + status_code=200, + json_data=[ + { + "id": 9, + "title": "Archived Stack", + "boardId": 123, + "order": 1, + "deletedAt": 0, + }, + ], + ) + + mock_client = mocker.AsyncMock(spec=httpx.AsyncClient) + mock_make_request = mocker.patch.object( + DeckClient, "_make_request", return_value=mock_response + ) + + client = DeckClient(mock_client, "testuser") + stacks = await client.get_archived_stacks(board_id=123) + + assert isinstance(stacks, list) + assert len(stacks) == 1 + assert stacks[0].id == 9 + + mock_make_request.assert_called_once() + assert "/boards/123/stacks/archived" in mock_make_request.call_args[0][1] + + # Card Tests diff --git a/tests/unit/test_deck_server.py b/tests/unit/test_deck_server.py new file mode 100644 index 00000000..3f48fbdf --- /dev/null +++ b/tests/unit/test_deck_server.py @@ -0,0 +1,51 @@ +import pytest + +from nextcloud_mcp_server.models.deck import DeckCard +from nextcloud_mcp_server.server.deck import _truncate_card_descriptions + +pytestmark = pytest.mark.unit + + +def _make_card(card_id: int, description: str | None) -> DeckCard: + return DeckCard( + id=card_id, + title=f"Card {card_id}", + stackId=1, + type="plain", + order=card_id, + archived=False, + owner="testuser", + description=description, + ) + + +def test_truncate_card_descriptions_no_op_when_limit_is_none(): + """When description_max_length is None, descriptions are left untouched.""" + cards = [_make_card(1, "x" * 5000)] + _truncate_card_descriptions(cards, None) + assert cards[0].description is not None + assert len(cards[0].description) == 5000 + + +def test_truncate_card_descriptions_truncates_long_descriptions(): + """Descriptions over the limit are truncated and marked with an ellipsis.""" + cards = [_make_card(1, "x" * 5000), _make_card(2, "short")] + _truncate_card_descriptions(cards, 100) + assert cards[0].description is not None + assert len(cards[0].description) == 101 # 100 chars + ellipsis + assert cards[0].description.endswith("…") + assert cards[1].description == "short" + + +def test_truncate_card_descriptions_handles_none_description(): + """Cards with no description are skipped without error.""" + cards = [_make_card(1, None)] + _truncate_card_descriptions(cards, 100) + assert cards[0].description is None + + +def test_truncate_card_descriptions_at_exact_boundary(): + """Descriptions at exactly the limit should not be truncated.""" + cards = [_make_card(1, "x" * 100)] + _truncate_card_descriptions(cards, 100) + assert cards[0].description == "x" * 100 From 7d633a945dead7613ab5e65bec41806229c9d387 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 00:41:14 +0200 Subject: [PATCH 2/4] fix(deck): address PR #759 review feedback - Validate description_max_length is positive (raises ValueError on 0 or negative); the prior code would have wiped descriptions to a single ellipsis character on description_max_length=0. - Extract filter logic into testable module-level helpers (_apply_board_filters, _apply_stack_filters, _apply_card_filters) and replace the dense `continue`-based loop in deck_get_stacks with the reviewer's elif form. - Document the truncation length quirk in the helper docstring (result is description_max_length + 1 chars when truncation fires). - Add 13 new unit tests covering the include/exclude flags on board, stacks, and flat card lists, plus the new validation paths and an explicit "description fits within limit, no ellipsis" case. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/server/deck.py | 117 +++++++++--- tests/unit/test_deck_server.py | 281 +++++++++++++++++++++++++++- 2 files changed, 368 insertions(+), 30 deletions(-) diff --git a/nextcloud_mcp_server/server/deck.py b/nextcloud_mcp_server/server/deck.py index b4dd7d3d..0c16b58e 100644 --- a/nextcloud_mcp_server/server/deck.py +++ b/nextcloud_mcp_server/server/deck.py @@ -34,14 +34,81 @@ logger = logging.getLogger(__name__) def _truncate_card_descriptions( cards: list[DeckCard], description_max_length: int | None ) -> None: - """Truncate each card's description in-place when it exceeds the limit.""" + """Truncate each card's description in-place when it strictly exceeds the + limit. + + Descriptions whose length is less than or equal to ``description_max_length`` + are left untouched (no ellipsis is appended). Descriptions longer than the + limit are truncated to ``description_max_length`` characters and an + ellipsis ("…") is appended, so the resulting string is + ``description_max_length + 1`` characters total. + + Args: + cards: Cards to mutate in place. + description_max_length: Positive truncation threshold, or ``None`` to + skip truncation entirely. + + Raises: + ValueError: If ``description_max_length`` is not positive. + """ if description_max_length is None: return + if description_max_length <= 0: + raise ValueError( + f"description_max_length must be positive, got {description_max_length}" + ) for card in cards: if card.description and len(card.description) > description_max_length: card.description = card.description[:description_max_length] + "…" +def _apply_board_filters( + board: DeckBoard, + *, + include_acl: bool, + include_users: bool, + include_labels: bool, +) -> DeckBoard: + """Drop board sub-fields the caller didn't request (in-place).""" + if not include_acl: + board.acl = [] + if not include_users: + board.users = [] + if not include_labels: + board.labels = [] + return board + + +def _apply_stack_filters( + stack: DeckStack, + *, + include_cards: bool, + include_archived_cards: bool, + description_max_length: int | None, +) -> DeckStack: + """Apply card-shaping filters to a single stack (in-place).""" + if not include_cards: + stack.cards = None + elif stack.cards: + if not include_archived_cards: + stack.cards = [c for c in stack.cards if not c.archived] + _truncate_card_descriptions(stack.cards, description_max_length) + return stack + + +def _apply_card_filters( + cards: list[DeckCard], + *, + include_archived: bool, + description_max_length: int | None, +) -> list[DeckCard]: + """Apply filters to a flat list of cards. Returns a (possibly new) list.""" + if not include_archived: + cards = [c for c in cards if not c.archived] + _truncate_card_descriptions(cards, description_max_length) + return cards + + def configure_deck_tools(mcp: FastMCP): """Configure Nextcloud Deck tools and resources for the MCP server.""" @@ -175,13 +242,12 @@ def configure_deck_tools(mcp: FastMCP): """ client = await get_client(ctx) board = await client.deck.get_board(board_id) - if not include_acl: - board.acl = [] - if not include_users: - board.users = [] - if not include_labels: - board.labels = [] - return board + return _apply_board_filters( + board, + include_acl=include_acl, + include_users=include_users, + include_labels=include_labels, + ) @mcp.tool( title="List Deck Stacks", @@ -212,13 +278,12 @@ def configure_deck_tools(mcp: FastMCP): client = await get_client(ctx) stacks = await client.deck.get_stacks(board_id) for stack in stacks: - if not include_cards: - stack.cards = None - continue - if stack.cards: - if not include_archived_cards: - stack.cards = [c for c in stack.cards if not c.archived] - _truncate_card_descriptions(stack.cards, description_max_length) + _apply_stack_filters( + stack, + include_cards=include_cards, + include_archived_cards=include_archived_cards, + description_max_length=description_max_length, + ) return ListStacksResponse(stacks=stacks, total=len(stacks)) @mcp.tool( @@ -248,13 +313,12 @@ def configure_deck_tools(mcp: FastMCP): """ client = await get_client(ctx) stack = await client.deck.get_stack(board_id, stack_id) - if not include_cards: - stack.cards = None - elif stack.cards: - if not include_archived_cards: - stack.cards = [c for c in stack.cards if not c.archived] - _truncate_card_descriptions(stack.cards, description_max_length) - return stack + return _apply_stack_filters( + stack, + include_cards=include_cards, + include_archived_cards=include_archived_cards, + description_max_length=description_max_length, + ) @mcp.tool( title="List Archived Deck Stacks", @@ -312,10 +376,11 @@ def configure_deck_tools(mcp: FastMCP): """ client = await get_client(ctx) stack = await client.deck.get_stack(board_id, stack_id) - cards = stack.cards or [] - if not include_archived: - cards = [c for c in cards if not c.archived] - _truncate_card_descriptions(cards, description_max_length) + cards = _apply_card_filters( + stack.cards or [], + include_archived=include_archived, + description_max_length=description_max_length, + ) return ListCardsResponse(cards=cards, total=len(cards)) @mcp.tool( diff --git a/tests/unit/test_deck_server.py b/tests/unit/test_deck_server.py index 3f48fbdf..4306f00f 100644 --- a/tests/unit/test_deck_server.py +++ b/tests/unit/test_deck_server.py @@ -1,24 +1,94 @@ import pytest -from nextcloud_mcp_server.models.deck import DeckCard -from nextcloud_mcp_server.server.deck import _truncate_card_descriptions +from nextcloud_mcp_server.models.deck import ( + DeckACL, + DeckBoard, + DeckCard, + DeckLabel, + DeckPermissions, + DeckStack, + DeckUser, +) +from nextcloud_mcp_server.server.deck import ( + _apply_board_filters, + _apply_card_filters, + _apply_stack_filters, + _truncate_card_descriptions, +) pytestmark = pytest.mark.unit -def _make_card(card_id: int, description: str | None) -> DeckCard: +# Fixtures ------------------------------------------------------------------ + + +def _make_card( + card_id: int, + description: str | None = "desc", + archived: bool = False, +) -> DeckCard: return DeckCard( id=card_id, title=f"Card {card_id}", stackId=1, type="plain", order=card_id, - archived=False, + archived=archived, owner="testuser", description=description, ) +def _make_user(uid: str = "testuser") -> DeckUser: + return DeckUser(primaryKey=uid, uid=uid, displayname=uid) + + +def _make_board( + board_id: int = 1, + *, + labels: list[DeckLabel] | None = None, + acl: list[DeckACL] | None = None, + users: list[DeckUser] | None = None, +) -> DeckBoard: + return DeckBoard( + id=board_id, + title=f"Board {board_id}", + owner=_make_user(), + color="FF0000", + archived=False, + labels=labels + if labels is not None + else [DeckLabel(id=1, title="L1", color="00FF00")], + acl=acl if acl is not None else [], + permissions=DeckPermissions( + PERMISSION_READ=True, + PERMISSION_EDIT=True, + PERMISSION_MANAGE=True, + PERMISSION_SHARE=True, + ), + users=users if users is not None else [_make_user("alice"), _make_user("bob")], + deletedAt=0, + ) + + +def _make_stack( + stack_id: int = 1, + *, + cards: list[DeckCard] | None = None, +) -> DeckStack: + return DeckStack( + id=stack_id, + title=f"Stack {stack_id}", + boardId=1, + order=stack_id, + deletedAt=0, + cards=cards, + ) + + +# _truncate_card_descriptions ---------------------------------------------- + + def test_truncate_card_descriptions_no_op_when_limit_is_none(): """When description_max_length is None, descriptions are left untouched.""" cards = [_make_card(1, "x" * 5000)] @@ -49,3 +119,206 @@ def test_truncate_card_descriptions_at_exact_boundary(): cards = [_make_card(1, "x" * 100)] _truncate_card_descriptions(cards, 100) assert cards[0].description == "x" * 100 + + +def test_truncate_card_descriptions_shorter_than_limit_no_ellipsis(): + """A description shorter than the limit must not have an ellipsis appended.""" + cards = [_make_card(1, "hello")] + _truncate_card_descriptions(cards, 1000) + assert cards[0].description == "hello" + + +def test_truncate_card_descriptions_rejects_zero(): + """A zero limit is invalid (would wipe descriptions to a single ellipsis).""" + cards = [_make_card(1, "anything")] + with pytest.raises(ValueError, match="must be positive"): + _truncate_card_descriptions(cards, 0) + + +def test_truncate_card_descriptions_rejects_negative(): + """Negative limits are invalid.""" + cards = [_make_card(1, "anything")] + with pytest.raises(ValueError, match="must be positive"): + _truncate_card_descriptions(cards, -10) + + +# _apply_board_filters ------------------------------------------------------ + + +def test_apply_board_filters_defaults_preserve_fields(): + """With all include_* flags True, no fields are cleared.""" + board = _make_board() + result = _apply_board_filters( + board, include_acl=True, include_users=True, include_labels=True + ) + assert len(result.labels) == 1 + assert len(result.users) == 2 + + +def test_apply_board_filters_excludes_acl(): + """include_acl=False clears the acl list.""" + board = _make_board( + acl=[ + DeckACL( + id=1, + participant=_make_user("alice"), + type=0, + boardId=1, + permissionEdit=True, + permissionShare=True, + permissionManage=False, + owner=False, + ) + ] + ) + result = _apply_board_filters( + board, include_acl=False, include_users=True, include_labels=True + ) + assert result.acl == [] + + +def test_apply_board_filters_excludes_users(): + """include_users=False clears the users list.""" + board = _make_board() + result = _apply_board_filters( + board, include_acl=True, include_users=False, include_labels=True + ) + assert result.users == [] + + +def test_apply_board_filters_excludes_labels(): + """include_labels=False clears the labels list.""" + board = _make_board() + result = _apply_board_filters( + board, include_acl=True, include_users=True, include_labels=False + ) + assert result.labels == [] + + +def test_apply_board_filters_excludes_all(): + """All include_* flags False clears every filterable list.""" + board = _make_board() + result = _apply_board_filters( + board, include_acl=False, include_users=False, include_labels=False + ) + assert result.acl == [] + assert result.users == [] + assert result.labels == [] + + +# _apply_stack_filters ------------------------------------------------------ + + +def test_apply_stack_filters_include_cards_false_strips_cards(): + """include_cards=False sets cards to None regardless of other flags.""" + stack = _make_stack(cards=[_make_card(1), _make_card(2, archived=True)]) + result = _apply_stack_filters( + stack, + include_cards=False, + include_archived_cards=True, + description_max_length=None, + ) + assert result.cards is None + + +def test_apply_stack_filters_excludes_archived_by_default(): + """include_archived_cards=False filters out archived cards.""" + stack = _make_stack( + cards=[_make_card(1, archived=False), _make_card(2, archived=True)] + ) + result = _apply_stack_filters( + stack, + include_cards=True, + include_archived_cards=False, + description_max_length=None, + ) + assert result.cards is not None + assert [c.id for c in result.cards] == [1] + + +def test_apply_stack_filters_keeps_archived_when_requested(): + """include_archived_cards=True retains archived cards.""" + stack = _make_stack( + cards=[_make_card(1, archived=False), _make_card(2, archived=True)] + ) + result = _apply_stack_filters( + stack, + include_cards=True, + include_archived_cards=True, + description_max_length=None, + ) + assert result.cards is not None + assert [c.id for c in result.cards] == [1, 2] + + +def test_apply_stack_filters_truncates_descriptions_after_archive_filter(): + """Truncation runs on the post-archive-filter card set.""" + stack = _make_stack( + cards=[ + _make_card(1, description="x" * 50, archived=False), + _make_card(2, description="y" * 50, archived=True), + ] + ) + result = _apply_stack_filters( + stack, + include_cards=True, + include_archived_cards=False, + description_max_length=10, + ) + assert result.cards is not None + assert len(result.cards) == 1 + assert result.cards[0].description is not None + assert result.cards[0].description.endswith("…") + + +def test_apply_stack_filters_handles_none_cards(): + """A stack with no cards (cards=None) is left untouched.""" + stack = _make_stack(cards=None) + result = _apply_stack_filters( + stack, + include_cards=True, + include_archived_cards=False, + description_max_length=10, + ) + assert result.cards is None + + +# _apply_card_filters ------------------------------------------------------- + + +def test_apply_card_filters_excludes_archived_by_default(): + """include_archived=False filters archived cards out of the flat list.""" + cards = [ + _make_card(1, archived=False), + _make_card(2, archived=True), + _make_card(3, archived=False), + ] + result = _apply_card_filters( + cards, include_archived=False, description_max_length=None + ) + assert [c.id for c in result] == [1, 3] + + +def test_apply_card_filters_keeps_archived_when_requested(): + """include_archived=True retains archived cards.""" + cards = [_make_card(1, archived=False), _make_card(2, archived=True)] + result = _apply_card_filters( + cards, include_archived=True, description_max_length=None + ) + assert [c.id for c in result] == [1, 2] + + +def test_apply_card_filters_truncates_descriptions(): + """description_max_length is honored on the returned cards.""" + cards = [_make_card(1, description="x" * 50)] + result = _apply_card_filters( + cards, include_archived=True, description_max_length=10 + ) + assert result[0].description is not None + assert result[0].description.endswith("…") + + +def test_apply_card_filters_empty_list_is_noop(): + """An empty input returns an empty output.""" + result = _apply_card_filters([], include_archived=False, description_max_length=10) + assert result == [] From b7805c2180475f6ae82f3198b7a5200f7875ca78 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 00:50:56 +0200 Subject: [PATCH 3/4] fix(deck): address PR #759 round-2 review feedback - Move description_max_length validation to tool layer (_validate_description_max_length), matching the existing _validate_comment_message pattern; helper now trusts callers per CLAUDE.md ("validate at system boundaries only"). - Fix mutation/return inconsistency: deck_get_stacks now uses a list comprehension to capture _apply_stack_filters' return, matching deck_get_stack / deck_get_archived_stacks. - Rename include_archived -> include_archived_cards on deck_get_cards and _apply_card_filters for consistency with deck_get_stacks. - Route deck_get_archived_stacks through _apply_stack_filters so future filters apply uniformly to active + archived paths. - Trim _truncate_card_descriptions docstring to one line; add inline comment in _apply_stack_filters explaining the breaking-change default (mirrors Deck UI archived-card filtering). - Replace fragile call_args[0][1] with call_args.args[1] in the archived-stacks client test. - Modernize Optional[X] -> X | None throughout deck.py (adjacent cleanup called out in the review). Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/server/deck.py | 103 +++++++++++++++------------- tests/client/deck/test_deck_api.py | 2 +- tests/unit/test_deck_server.py | 45 ++++++++---- 3 files changed, 86 insertions(+), 64 deletions(-) diff --git a/nextcloud_mcp_server/server/deck.py b/nextcloud_mcp_server/server/deck.py index 0c16b58e..cc595456 100644 --- a/nextcloud_mcp_server/server/deck.py +++ b/nextcloud_mcp_server/server/deck.py @@ -1,5 +1,4 @@ import logging -from typing import Optional from mcp.server.fastmcp import Context, FastMCP from mcp.types import ToolAnnotations @@ -31,32 +30,21 @@ from nextcloud_mcp_server.observability.metrics import instrument_tool logger = logging.getLogger(__name__) -def _truncate_card_descriptions( - cards: list[DeckCard], description_max_length: int | None -) -> None: - """Truncate each card's description in-place when it strictly exceeds the - limit. - - Descriptions whose length is less than or equal to ``description_max_length`` - are left untouched (no ellipsis is appended). Descriptions longer than the - limit are truncated to ``description_max_length`` characters and an - ellipsis ("…") is appended, so the resulting string is - ``description_max_length + 1`` characters total. - - Args: - cards: Cards to mutate in place. - description_max_length: Positive truncation threshold, or ``None`` to - skip truncation entirely. - - Raises: - ValueError: If ``description_max_length`` is not positive. - """ - if description_max_length is None: - return - if description_max_length <= 0: +def _validate_description_max_length(description_max_length: int | None) -> None: + """Tool-layer guard: reject zero/negative truncation thresholds.""" + if description_max_length is not None and description_max_length <= 0: raise ValueError( f"description_max_length must be positive, got {description_max_length}" ) + + +def _truncate_card_descriptions( + cards: list[DeckCard], description_max_length: int | None +) -> None: + """Truncate descriptions strictly longer than the limit; appends "…" so + the truncated result is ``description_max_length + 1`` chars.""" + if description_max_length is None: + return for card in cards: if card.description and len(card.description) > description_max_length: card.description = card.description[:description_max_length] + "…" @@ -87,6 +75,10 @@ def _apply_stack_filters( description_max_length: int | None, ) -> DeckStack: """Apply card-shaping filters to a single stack (in-place).""" + # Note: the upstream Deck API returns archived cards inline within + # active stacks (the Deck UI filters them frontend-side). Defaulting + # include_archived_cards to False mirrors that UI behavior — this is + # the breaking change called out in the PR description. if not include_cards: stack.cards = None elif stack.cards: @@ -99,11 +91,11 @@ def _apply_stack_filters( def _apply_card_filters( cards: list[DeckCard], *, - include_archived: bool, + include_archived_cards: bool, description_max_length: int | None, ) -> list[DeckCard]: """Apply filters to a flat list of cards. Returns a (possibly new) list.""" - if not include_archived: + if not include_archived_cards: cards = [c for c in cards if not c.archived] _truncate_card_descriptions(cards, description_max_length) return cards @@ -275,15 +267,18 @@ def configure_deck_tools(mcp: FastMCP): to this many characters. Useful for keeping responses compact on boards with long card specs. """ + _validate_description_max_length(description_max_length) client = await get_client(ctx) stacks = await client.deck.get_stacks(board_id) - for stack in stacks: + stacks = [ _apply_stack_filters( stack, include_cards=include_cards, include_archived_cards=include_archived_cards, description_max_length=description_max_length, ) + for stack in stacks + ] return ListStacksResponse(stacks=stacks, total=len(stacks)) @mcp.tool( @@ -311,6 +306,7 @@ def configure_deck_tools(mcp: FastMCP): description_max_length: If set, truncate each card's description to this many characters. """ + _validate_description_max_length(description_max_length) client = await get_client(ctx) stack = await client.deck.get_stack(board_id, stack_id) return _apply_stack_filters( @@ -343,11 +339,21 @@ def configure_deck_tools(mcp: FastMCP): description_max_length: If set, truncate each card's description to this many characters. """ + _validate_description_max_length(description_max_length) client = await get_client(ctx) stacks = await client.deck.get_archived_stacks(board_id) - for stack in stacks: - if stack.cards: - _truncate_card_descriptions(stack.cards, description_max_length) + # All cards in archived stacks are themselves archived; route through + # the same helper as the active-stack path so future filter additions + # apply uniformly. + stacks = [ + _apply_stack_filters( + stack, + include_cards=True, + include_archived_cards=True, + description_max_length=description_max_length, + ) + for stack in stacks + ] return ListStacksResponse(stacks=stacks, total=len(stacks)) @mcp.tool( @@ -360,7 +366,7 @@ def configure_deck_tools(mcp: FastMCP): ctx: Context, board_id: int, stack_id: int, - include_archived: bool = False, + include_archived_cards: bool = False, description_max_length: int | None = None, ) -> ListCardsResponse: """Get all cards in a Nextcloud Deck stack. @@ -368,17 +374,18 @@ def configure_deck_tools(mcp: FastMCP): Args: board_id: The ID of the board stack_id: The ID of the stack - include_archived: Include archived cards (default False). Archived - cards can also be retrieved per-board via + include_archived_cards: Include archived cards (default False). + Archived cards can also be retrieved per-board via deck_get_archived_stacks. description_max_length: If set, truncate each card's description to this many characters. """ + _validate_description_max_length(description_max_length) client = await get_client(ctx) stack = await client.deck.get_stack(board_id, stack_id) cards = _apply_card_filters( stack.cards or [], - include_archived=include_archived, + include_archived_cards=include_archived_cards, description_max_length=description_max_length, ) return ListCardsResponse(cards=cards, total=len(cards)) @@ -475,8 +482,8 @@ def configure_deck_tools(mcp: FastMCP): ctx: Context, board_id: int, stack_id: int, - title: Optional[str] = None, - order: Optional[int] = None, + title: str | None = None, + order: int | None = None, ) -> StackOperationResponse: """Update a Nextcloud Deck stack @@ -535,8 +542,8 @@ def configure_deck_tools(mcp: FastMCP): title: str, type: str = "plain", order: int = 999, - description: Optional[str] = None, - duedate: Optional[str] = None, + description: str | None = None, + duedate: str | None = None, ) -> CreateCardResponse: """Create a new card in a Nextcloud Deck stack @@ -571,14 +578,14 @@ def configure_deck_tools(mcp: FastMCP): board_id: int, stack_id: int, card_id: int, - title: Optional[str] = None, - description: Optional[str] = None, - type: Optional[str] = None, - owner: Optional[str] = None, - order: Optional[int] = None, - duedate: Optional[str] = None, - archived: Optional[bool] = None, - done: Optional[str] = None, + title: str | None = None, + description: str | None = None, + type: str | None = None, + owner: str | None = None, + order: int | None = None, + duedate: str | None = None, + archived: bool | None = None, + done: str | None = None, ) -> CardOperationResponse: """Update a Nextcloud Deck card @@ -763,8 +770,8 @@ def configure_deck_tools(mcp: FastMCP): ctx: Context, board_id: int, label_id: int, - title: Optional[str] = None, - color: Optional[str] = None, + title: str | None = None, + color: str | None = None, ) -> LabelOperationResponse: """Update a Nextcloud Deck label diff --git a/tests/client/deck/test_deck_api.py b/tests/client/deck/test_deck_api.py index 9b988e63..cbe1e6f9 100644 --- a/tests/client/deck/test_deck_api.py +++ b/tests/client/deck/test_deck_api.py @@ -283,7 +283,7 @@ async def test_deck_get_archived_stacks(mocker): assert stacks[0].id == 9 mock_make_request.assert_called_once() - assert "/boards/123/stacks/archived" in mock_make_request.call_args[0][1] + assert "/boards/123/stacks/archived" in mock_make_request.call_args.args[1] # Card Tests diff --git a/tests/unit/test_deck_server.py b/tests/unit/test_deck_server.py index 4306f00f..cd4a38a5 100644 --- a/tests/unit/test_deck_server.py +++ b/tests/unit/test_deck_server.py @@ -14,6 +14,7 @@ from nextcloud_mcp_server.server.deck import ( _apply_card_filters, _apply_stack_filters, _truncate_card_descriptions, + _validate_description_max_length, ) pytestmark = pytest.mark.unit @@ -128,18 +129,30 @@ def test_truncate_card_descriptions_shorter_than_limit_no_ellipsis(): assert cards[0].description == "hello" -def test_truncate_card_descriptions_rejects_zero(): - """A zero limit is invalid (would wipe descriptions to a single ellipsis).""" - cards = [_make_card(1, "anything")] - with pytest.raises(ValueError, match="must be positive"): - _truncate_card_descriptions(cards, 0) +# _validate_description_max_length ---------------------------------------- -def test_truncate_card_descriptions_rejects_negative(): - """Negative limits are invalid.""" - cards = [_make_card(1, "anything")] +def test_validate_description_max_length_accepts_none(): + """None is the documented sentinel for "no truncation".""" + _validate_description_max_length(None) + + +def test_validate_description_max_length_accepts_positive(): + """Positive values pass through silently.""" + _validate_description_max_length(1) + _validate_description_max_length(1000) + + +def test_validate_description_max_length_rejects_zero(): + """Zero would wipe descriptions to a single ellipsis — reject at the boundary.""" with pytest.raises(ValueError, match="must be positive"): - _truncate_card_descriptions(cards, -10) + _validate_description_max_length(0) + + +def test_validate_description_max_length_rejects_negative(): + """Negative values produce surprising slice semantics — reject at the boundary.""" + with pytest.raises(ValueError, match="must be positive"): + _validate_description_max_length(-10) # _apply_board_filters ------------------------------------------------------ @@ -287,23 +300,23 @@ def test_apply_stack_filters_handles_none_cards(): def test_apply_card_filters_excludes_archived_by_default(): - """include_archived=False filters archived cards out of the flat list.""" + """include_archived_cards=False filters archived cards out of the flat list.""" cards = [ _make_card(1, archived=False), _make_card(2, archived=True), _make_card(3, archived=False), ] result = _apply_card_filters( - cards, include_archived=False, description_max_length=None + cards, include_archived_cards=False, description_max_length=None ) assert [c.id for c in result] == [1, 3] def test_apply_card_filters_keeps_archived_when_requested(): - """include_archived=True retains archived cards.""" + """include_archived_cards=True retains archived cards.""" cards = [_make_card(1, archived=False), _make_card(2, archived=True)] result = _apply_card_filters( - cards, include_archived=True, description_max_length=None + cards, include_archived_cards=True, description_max_length=None ) assert [c.id for c in result] == [1, 2] @@ -312,7 +325,7 @@ def test_apply_card_filters_truncates_descriptions(): """description_max_length is honored on the returned cards.""" cards = [_make_card(1, description="x" * 50)] result = _apply_card_filters( - cards, include_archived=True, description_max_length=10 + cards, include_archived_cards=True, description_max_length=10 ) assert result[0].description is not None assert result[0].description.endswith("…") @@ -320,5 +333,7 @@ def test_apply_card_filters_truncates_descriptions(): def test_apply_card_filters_empty_list_is_noop(): """An empty input returns an empty output.""" - result = _apply_card_filters([], include_archived=False, description_max_length=10) + result = _apply_card_filters( + [], include_archived_cards=False, description_max_length=10 + ) assert result == [] From a995155bd48c93ad12eea2646824c439fe668d9b Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 01:09:54 +0200 Subject: [PATCH 4/4] fix(deck): address PR #759 round-3 review feedback - Drop "(in-place)" from filter-helper docstrings; callers should consume the return value, mutation is an implementation detail. - Document that deck_get_archived_stacks always returns cards (an archived stack without its cards has no audit value); point to description_max_length for size control. - Document that deck_get_cards applies filtering client-side, so it is network-equivalent to deck_get_stack(include_cards=True). - Pin the empty-list contract: a stack with all-archived cards and include_archived_cards=False yields cards == [] (loaded but empty), not cards is None (explicitly suppressed). - Add explicit one-character-over-limit truncation test alongside the existing exact-boundary test. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/server/deck.py | 16 ++++++++++++--- tests/unit/test_deck_server.py | 31 +++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/nextcloud_mcp_server/server/deck.py b/nextcloud_mcp_server/server/deck.py index cc595456..b6205c3d 100644 --- a/nextcloud_mcp_server/server/deck.py +++ b/nextcloud_mcp_server/server/deck.py @@ -57,7 +57,7 @@ def _apply_board_filters( include_users: bool, include_labels: bool, ) -> DeckBoard: - """Drop board sub-fields the caller didn't request (in-place).""" + """Drop board sub-fields the caller didn't request; returns the board.""" if not include_acl: board.acl = [] if not include_users: @@ -74,7 +74,7 @@ def _apply_stack_filters( include_archived_cards: bool, description_max_length: int | None, ) -> DeckStack: - """Apply card-shaping filters to a single stack (in-place).""" + """Apply card-shaping filters to a single stack; returns the stack.""" # Note: the upstream Deck API returns archived cards inline within # active stacks (the Deck UI filters them frontend-side). Defaulting # include_archived_cards to False mirrors that UI behavior — this is @@ -94,7 +94,7 @@ def _apply_card_filters( include_archived_cards: bool, description_max_length: int | None, ) -> list[DeckCard]: - """Apply filters to a flat list of cards. Returns a (possibly new) list.""" + """Apply filters to a flat list of cards; returns the (possibly new) list.""" if not include_archived_cards: cards = [c for c in cards if not c.archived] _truncate_card_descriptions(cards, description_max_length) @@ -334,6 +334,10 @@ def configure_deck_tools(mcp: FastMCP): active board (e.g. cards moved through a "Done" stack and then archived via deck_archive_card). The shape mirrors deck_get_stacks. + Cards are always included on the returned stacks (an archived stack + without its cards would have no audit value); pass + ``description_max_length`` if you need to keep the response compact. + Args: board_id: The ID of the board description_max_length: If set, truncate each card's description @@ -371,6 +375,12 @@ def configure_deck_tools(mcp: FastMCP): ) -> ListCardsResponse: """Get all cards in a Nextcloud Deck stack. + Filtering is applied client-side after the API returns the full + stack, so ``include_archived_cards=False`` and + ``description_max_length`` reduce response size visible to the + caller but not network bandwidth — network-wise this tool is + equivalent to deck_get_stack(include_cards=True). + Args: board_id: The ID of the board stack_id: The ID of the stack diff --git a/tests/unit/test_deck_server.py b/tests/unit/test_deck_server.py index cd4a38a5..d810dbc0 100644 --- a/tests/unit/test_deck_server.py +++ b/tests/unit/test_deck_server.py @@ -122,6 +122,15 @@ def test_truncate_card_descriptions_at_exact_boundary(): assert cards[0].description == "x" * 100 +def test_truncate_card_descriptions_one_over_limit(): + """A description one character over the limit triggers truncation.""" + cards = [_make_card(1, "x" * 101)] + _truncate_card_descriptions(cards, 100) + assert cards[0].description is not None + assert len(cards[0].description) == 101 # 100 chars + ellipsis + assert cards[0].description.endswith("…") + + def test_truncate_card_descriptions_shorter_than_limit_no_ellipsis(): """A description shorter than the limit must not have an ellipsis appended.""" cards = [_make_card(1, "hello")] @@ -296,6 +305,28 @@ def test_apply_stack_filters_handles_none_cards(): assert result.cards is None +def test_apply_stack_filters_all_archived_yields_empty_list_not_none(): + """A stack whose cards are all archived yields cards == [], not None. + + Pin the contract: include_cards=True with all cards filtered out + means "the stack was loaded but had nothing to show", which is + semantically distinct from include_cards=False (cards=None, + "explicitly suppressed"). Callers checking ``stack.cards is None`` + can use that to distinguish the two states. + """ + stack = _make_stack( + cards=[_make_card(1, archived=True), _make_card(2, archived=True)] + ) + result = _apply_stack_filters( + stack, + include_cards=True, + include_archived_cards=False, + description_max_length=None, + ) + assert result.cards == [] + assert result.cards is not None + + # _apply_card_filters -------------------------------------------------------