From 90494674d9386cf524acf8512efc83e1fdf1038a Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 7 Jun 2026 18:21:00 +0200 Subject: [PATCH] refactor(deck): address PR #872 round-1 review - deck_get_stack: fetch active + archived concurrently for status="all", and for status="archived" source the stack from /stacks/archived in a single call (skip the active fetch whose open cards are filtered out anyway), matching deck_get_cards' pattern. - Type the `client` param of _archived_cards_by_stack as NextcloudClient. - Extend the stacks/overview integration test to assert status="archived" (only the archived card) in addition to status="all". - Document the third_party/astrolabe submodule mount policy in CLAUDE.md: unmounted by default (CI installs the published app-store version); mount only for tightly-coupled feature work needing CI integration, then revert. Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 14 +++++++++ nextcloud_mcp_server/server/deck.py | 40 +++++++++++++++++++++--- tests/server/test_deck_mcp.py | 48 +++++++++++++++++------------ 3 files changed, 77 insertions(+), 25 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 332bb36f..56567079 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -223,6 +223,20 @@ docker compose up --build -d mcp-login-flow # Login Flow v2 (port 8004) docker compose up --build -d mcp-keycloak # Keycloak OAuth (port 8002) ``` +### Astrolabe submodule mount (do NOT mount by default) + +The `third_party/astrolabe` submodule mount in `docker-compose.yml` +(`./third_party/astrolabe:/opt/apps/astrolabe:ro`) is **commented out by +default and should stay that way**. With it unmounted, the stack installs the +most recently **published** Astrolabe version from the Nextcloud app store — +which is the correct baseline for almost all work, including CI. + +Only uncomment the mount when developing features that are **tightly coupled** +to unreleased Astrolabe changes and need the local submodule build integration- +tested in CI. Re-comment it before the change is considered done — a left-on +mount silently pins CI to the local checkout instead of the published app, and +breaks for anyone without the submodule built. (See PR #872.) + ### Environment Setup ```bash uv sync # Install dependencies diff --git a/nextcloud_mcp_server/server/deck.py b/nextcloud_mcp_server/server/deck.py index 998a0eab..19942c4a 100644 --- a/nextcloud_mcp_server/server/deck.py +++ b/nextcloud_mcp_server/server/deck.py @@ -6,6 +6,7 @@ from mcp.server.fastmcp import Context, FastMCP from mcp.types import ToolAnnotations from nextcloud_mcp_server.auth import require_scopes +from nextcloud_mcp_server.client import NextcloudClient from nextcloud_mcp_server.context import get_client from nextcloud_mcp_server.models.deck import ( AttachFileResponse, @@ -222,7 +223,9 @@ def _apply_stack_filters( _ARCHIVED_STATUSES: frozenset[str] = frozenset({"all", "archived"}) -async def _archived_cards_by_stack(client, board_id: int) -> dict[int, list[DeckCard]]: +async def _archived_cards_by_stack( + client: NextcloudClient, board_id: int +) -> dict[int, list[DeckCard]]: """Map stack_id -> archived DeckCards for a board. The active stack/card listing endpoints filter out archived cards in SQL; @@ -608,10 +611,37 @@ def configure_deck_tools(mcp: FastMCP): description_preview_length, "description_preview_length" ) client = await get_client(ctx) - stack = await client.deck.get_stack(board_id, stack_id) - # Merge archived cards (omitted by the active endpoint) when in scope. - if include_cards and status in _ARCHIVED_STATUSES: - archived_by_stack = await _archived_cards_by_stack(client, board_id) + if status == "archived" and include_cards: + # Archived-only: the /stacks/archived endpoint already returns the + # stack (metadata + archived cards) in one call, so skip the active + # fetch whose open cards would all be filtered out anyway. + archived = await client.deck.get_archived_stacks(board_id) + stack = next((s for s in archived if s.id == stack_id), None) + if stack is None: + # findAllArchived returns every stack, so this is defensive; + # fall back to the active endpoint for the stack metadata. + stack = await client.deck.get_stack(board_id, stack_id) + else: + # Active stack always needed (for metadata + open cards); fetch the + # archived cards concurrently when status="all" needs both sets. + stack_holder: list[DeckStack] = [] + archived_by_stack: dict[int, list[DeckCard]] = {} + merge_archived = include_cards and status == "all" + + async def _get_active() -> None: + stack_holder.append(await client.deck.get_stack(board_id, stack_id)) + + async def _get_archived() -> None: + archived_by_stack.update( + await _archived_cards_by_stack(client, board_id) + ) + + async with anyio.create_task_group() as tg: + tg.start_soon(_get_active) + if merge_archived: + tg.start_soon(_get_archived) + + stack = stack_holder[0] extra = archived_by_stack.get(stack_id) if extra: _append_archived_cards(stack, extra) diff --git a/tests/server/test_deck_mcp.py b/tests/server/test_deck_mcp.py index 00475083..5800cbe5 100644 --- a/tests/server/test_deck_mcp.py +++ b/tests/server/test_deck_mcp.py @@ -520,24 +520,32 @@ async def test_deck_get_stacks_and_overview_include_archived_mcp( stack_id = stack_data["id"] archived_id = await _create_and_archive_card(nc_mcp_client, board_id, stack_id) - # deck_get_stacks(status="all") - stacks_result = await nc_mcp_client.call_tool( - "deck_get_stacks", {"board_id": board_id, "status": "all"} - ) - assert stacks_result.isError is False, "deck_get_stacks(all) failed" - stacks_payload = json.loads(stacks_result.content[0].text) - stack = next(s for s in stacks_payload["stacks"] if s["id"] == stack_id) - stack_card_ids = [c["id"] for c in (stack.get("cards") or [])] - assert card_data["id"] in stack_card_ids - assert archived_id in stack_card_ids, "archived card missing from deck_get_stacks" + async def stacks_card_ids(status: str) -> list[int]: + result = await nc_mcp_client.call_tool( + "deck_get_stacks", {"board_id": board_id, "status": status} + ) + assert result.isError is False, f"deck_get_stacks({status}) failed" + payload = json.loads(result.content[0].text) + stack = next(s for s in payload["stacks"] if s["id"] == stack_id) + return [c["id"] for c in (stack.get("cards") or [])] - # deck_get_board_overview(status="all") - overview_result = await nc_mcp_client.call_tool( - "deck_get_board_overview", {"board_id": board_id, "status": "all"} - ) - assert overview_result.isError is False, "board overview failed" - overview_payload = json.loads(overview_result.content[0].text) - ov_stack = next(s for s in overview_payload["stacks"] if s["id"] == stack_id) - ov_card_ids = [c["id"] for c in ov_stack["cards"]] - assert archived_id in ov_card_ids, "archived card missing from board overview" - assert ov_stack["card_count"] == len(ov_stack["cards"]) + async def overview_card_ids(status: str) -> list[int]: + result = await nc_mcp_client.call_tool( + "deck_get_board_overview", {"board_id": board_id, "status": status} + ) + assert result.isError is False, f"board overview({status}) failed" + payload = json.loads(result.content[0].text) + stack = next(s for s in payload["stacks"] if s["id"] == stack_id) + assert stack["card_count"] == len(stack["cards"]) + return [c["id"] for c in stack["cards"]] + + # status="all": both the open and archived card present. + stacks_all = await stacks_card_ids("all") + assert card_data["id"] in stacks_all + assert archived_id in stacks_all, "archived card missing from deck_get_stacks(all)" + overview_all = await overview_card_ids("all") + assert archived_id in overview_all, "archived card missing from board overview(all)" + + # status="archived": only the archived card, open card excluded. + assert await stacks_card_ids("archived") == [archived_id] + assert await overview_card_ids("archived") == [archived_id]