From a995155bd48c93ad12eea2646824c439fe668d9b Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 01:09:54 +0200 Subject: [PATCH] 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 -------------------------------------------------------