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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
961449be30
commit
90494674d9
@@ -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
|
||||
|
||||
@@ -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)
|
||||
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)
|
||||
# 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)
|
||||
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)
|
||||
|
||||
@@ -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"}
|
||||
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 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"
|
||||
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"}
|
||||
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 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"])
|
||||
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]
|
||||
|
||||
Reference in New Issue
Block a user