From 8887aa241a1a28d232b7a3a7ea3b79a68916b73d Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 20 Feb 2026 09:22:16 +0100 Subject: [PATCH 1/5] fix: wrap raw list returns in response models to produce single TextContent block MCP tools returning raw lists caused FastMCP's _convert_to_content() to create one TextContent block per element. Most MCP clients only read content[0], so they saw a single result instead of the full list. Wrapped 9 tool functions in proper response objects: - deck: deck_get_boards, deck_get_stacks, deck_get_cards, deck_get_labels - calendar: nc_calendar_list_events, nc_calendar_get_upcoming_events - contacts: nc_contacts_list_addressbooks, nc_contacts_list_contacts - tables: nc_tables_list_tables Closes #568 Co-Authored-By: Claude Opus 4.6 --- nextcloud_mcp_server/models/deck.py | 14 +++++ nextcloud_mcp_server/server/calendar.py | 54 +++++++++++++++---- nextcloud_mcp_server/server/contacts.py | 38 +++++++++++-- nextcloud_mcp_server/server/deck.py | 23 ++++---- nextcloud_mcp_server/server/tables.py | 7 ++- .../oauth/test_oauth_deck_permissions.py | 24 ++++++--- tests/server/test_mcp.py | 36 ++++++------- 7 files changed, 141 insertions(+), 55 deletions(-) diff --git a/nextcloud_mcp_server/models/deck.py b/nextcloud_mcp_server/models/deck.py index b636ddd..ca46562 100644 --- a/nextcloud_mcp_server/models/deck.py +++ b/nextcloud_mcp_server/models/deck.py @@ -261,6 +261,20 @@ class CreateLabelResponse(BaseResponse): color: str = Field(description="The created label color") +class ListCardsResponse(BaseResponse): + """Response model for listing deck cards.""" + + cards: List[DeckCard] = Field(description="List of deck cards") + total: int = Field(description="Total number of cards") + + +class ListLabelsResponse(BaseResponse): + """Response model for listing deck labels.""" + + labels: List[DeckLabel] = Field(description="List of deck labels") + total: int = Field(description="Total number of labels") + + class LabelOperationResponse(StatusResponse): """Response model for label operations like update/delete.""" diff --git a/nextcloud_mcp_server/server/calendar.py b/nextcloud_mcp_server/server/calendar.py index b54ae82..5974264 100644 --- a/nextcloud_mcp_server/server/calendar.py +++ b/nextcloud_mcp_server/server/calendar.py @@ -9,15 +9,39 @@ from nextcloud_mcp_server.auth import require_scopes from nextcloud_mcp_server.context import get_client from nextcloud_mcp_server.models.calendar import ( Calendar, + CalendarEventSummary, ListCalendarsResponse, + ListEventsResponse, ListTodosResponse, Todo, + UpcomingEventsResponse, ) from nextcloud_mcp_server.observability.metrics import instrument_tool logger = logging.getLogger(__name__) +def _event_dict_to_summary(event: dict) -> CalendarEventSummary: + """Convert a raw event dict from the calendar client to a CalendarEventSummary.""" + raw_categories = event.get("categories", []) + if isinstance(raw_categories, str): + categories = [c.strip() for c in raw_categories.split(",") if c.strip()] + else: + categories = raw_categories + + return CalendarEventSummary( + uid=event.get("uid", ""), + summary=event.get("title", ""), + start=event.get("start_datetime", ""), + end=event.get("end_datetime"), + all_day=event.get("all_day", False), + location=event.get("location") or None, + description=event.get("description") or None, + categories=categories, + status=event.get("status"), + ) + + def configure_calendar_tools(mcp: FastMCP): # Calendar tools @mcp.tool( @@ -204,7 +228,7 @@ def configure_calendar_tools(mcp: FastMCP): end_datetime=end_datetime, filters=filters if filters else None, ) - return events[:limit] + events = events[:limit] else: # Search in specific calendar events = await client.calendar.get_calendar_events( @@ -218,7 +242,14 @@ def configure_calendar_tools(mcp: FastMCP): if filters: events = client.calendar._apply_event_filters(events, filters) - return events + summaries = [_event_dict_to_summary(e) for e in events] + return ListEventsResponse( + events=summaries, + calendar_name=None if search_all_calendars else calendar_name, + start_date=start_date or None, + end_date=end_date or None, + total_found=len(summaries), + ) @mcp.tool( title="Get Calendar Event", @@ -420,7 +451,7 @@ def configure_calendar_tools(mcp: FastMCP): if calendar_name: # Get events from specific calendar - return await client.calendar.get_calendar_events( + events = await client.calendar.get_calendar_events( calendar_name=calendar_name, start_datetime=now, end_datetime=end_datetime, @@ -433,17 +464,13 @@ def configure_calendar_tools(mcp: FastMCP): for calendar in all_calendars: try: - events = await client.calendar.get_calendar_events( + cal_events = await client.calendar.get_calendar_events( calendar_name=calendar["name"], start_datetime=now, end_datetime=end_datetime, limit=limit, ) - # Add calendar info to each event - for event in events: - event["calendar_name"] = calendar["name"] - event["calendar_display_name"] = calendar["display_name"] - all_events.extend(events) + all_events.extend(cal_events) except Exception as e: logger.warning( f"Error getting events from calendar {calendar['name']}: {e}" @@ -452,7 +479,14 @@ def configure_calendar_tools(mcp: FastMCP): # Sort by start time and limit all_events.sort(key=lambda x: x.get("start_datetime", "")) - return all_events[:limit] + events = all_events[:limit] + + summaries = [_event_dict_to_summary(e) for e in events] + return UpcomingEventsResponse( + events=summaries, + days_ahead=days_ahead, + calendar_name=calendar_name or None, + ) @mcp.tool( title="Find Availability", diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index 9f5eafa..593c913 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -5,6 +5,12 @@ from mcp.types import ToolAnnotations from nextcloud_mcp_server.auth import require_scopes from nextcloud_mcp_server.context import get_client +from nextcloud_mcp_server.models.contacts import ( + AddressBook, + Contact, + ListAddressBooksResponse, + ListContactsResponse, +) from nextcloud_mcp_server.observability.metrics import instrument_tool logger = logging.getLogger(__name__) @@ -18,10 +24,21 @@ def configure_contacts_tools(mcp: FastMCP): ) @require_scopes("contacts:read") @instrument_tool - async def nc_contacts_list_addressbooks(ctx: Context): + async def nc_contacts_list_addressbooks(ctx: Context) -> ListAddressBooksResponse: """List all addressbooks for the user.""" client = await get_client(ctx) - return await client.contacts.list_addressbooks() + addressbooks_data = await client.contacts.list_addressbooks() + addressbooks = [ + AddressBook( + uri=ab["name"], + displayname=ab.get("display_name", ab["name"]), + ctag=ab.get("getctag"), + ) + for ab in addressbooks_data + ] + return ListAddressBooksResponse( + addressbooks=addressbooks, total_count=len(addressbooks) + ) @mcp.tool( title="List Contacts", @@ -29,10 +46,23 @@ def configure_contacts_tools(mcp: FastMCP): ) @require_scopes("contacts:read") @instrument_tool - async def nc_contacts_list_contacts(ctx: Context, *, addressbook: str): + async def nc_contacts_list_contacts( + ctx: Context, *, addressbook: str + ) -> ListContactsResponse: """List all contacts in the specified addressbook.""" client = await get_client(ctx) - return await client.contacts.list_contacts(addressbook=addressbook) + contacts_data = await client.contacts.list_contacts(addressbook=addressbook) + contacts = [ + Contact( + uid=c["vcard_id"], + fn=c.get("contact", {}).get("fullname", ""), + etag=c.get("getetag"), + ) + for c in contacts_data + ] + return ListContactsResponse( + contacts=contacts, addressbook=addressbook, total_count=len(contacts) + ) @mcp.tool( title="Create Address Book", diff --git a/nextcloud_mcp_server/server/deck.py b/nextcloud_mcp_server/server/deck.py index 93b5d64..ee6f499 100644 --- a/nextcloud_mcp_server/server/deck.py +++ b/nextcloud_mcp_server/server/deck.py @@ -17,6 +17,10 @@ from nextcloud_mcp_server.models.deck import ( DeckLabel, DeckStack, LabelOperationResponse, + ListBoardsResponse, + ListCardsResponse, + ListLabelsResponse, + ListStacksResponse, StackOperationResponse, ) from nextcloud_mcp_server.observability.metrics import instrument_tool @@ -124,11 +128,11 @@ def configure_deck_tools(mcp: FastMCP): ) @require_scopes("deck:read") @instrument_tool - async def deck_get_boards(ctx: Context) -> list[DeckBoard]: + async def deck_get_boards(ctx: Context) -> ListBoardsResponse: """Get all Nextcloud Deck boards""" client = await get_client(ctx) boards = await client.deck.get_boards() - return boards + return ListBoardsResponse(boards=boards, total=len(boards)) @mcp.tool( title="Get Deck Board", @@ -148,11 +152,11 @@ def configure_deck_tools(mcp: FastMCP): ) @require_scopes("deck:read") @instrument_tool - async def deck_get_stacks(ctx: Context, board_id: int) -> list[DeckStack]: + async def deck_get_stacks(ctx: Context, board_id: int) -> ListStacksResponse: """Get all stacks in a Nextcloud Deck board""" client = await get_client(ctx) stacks = await client.deck.get_stacks(board_id) - return stacks + return ListStacksResponse(stacks=stacks, total=len(stacks)) @mcp.tool( title="Get Deck Stack", @@ -174,13 +178,12 @@ def configure_deck_tools(mcp: FastMCP): @instrument_tool async def deck_get_cards( ctx: Context, board_id: int, stack_id: int - ) -> list[DeckCard]: + ) -> ListCardsResponse: """Get all cards in a Nextcloud Deck stack""" client = await get_client(ctx) stack = await client.deck.get_stack(board_id, stack_id) - if stack.cards: - return stack.cards - return [] + cards = stack.cards or [] + return ListCardsResponse(cards=cards, total=len(cards)) @mcp.tool( title="Get Deck Card", @@ -202,11 +205,11 @@ def configure_deck_tools(mcp: FastMCP): ) @require_scopes("deck:read") @instrument_tool - async def deck_get_labels(ctx: Context, board_id: int) -> list[DeckLabel]: + async def deck_get_labels(ctx: Context, board_id: int) -> ListLabelsResponse: """Get all labels in a Nextcloud Deck board""" client = await get_client(ctx) board = await client.deck.get_board(board_id) - return board.labels + return ListLabelsResponse(labels=board.labels, total=len(board.labels)) @mcp.tool( title="Get Deck Label", diff --git a/nextcloud_mcp_server/server/tables.py b/nextcloud_mcp_server/server/tables.py index 76e3c66..5a7f071 100644 --- a/nextcloud_mcp_server/server/tables.py +++ b/nextcloud_mcp_server/server/tables.py @@ -5,6 +5,7 @@ from mcp.types import ToolAnnotations from nextcloud_mcp_server.auth import require_scopes from nextcloud_mcp_server.context import get_client +from nextcloud_mcp_server.models.tables import ListTablesResponse, Table from nextcloud_mcp_server.observability.metrics import instrument_tool logger = logging.getLogger(__name__) @@ -18,10 +19,12 @@ def configure_tables_tools(mcp: FastMCP): ) @require_scopes("tables:read") @instrument_tool - async def nc_tables_list_tables(ctx: Context): + async def nc_tables_list_tables(ctx: Context) -> ListTablesResponse: """List all tables available to the user""" client = await get_client(ctx) - return await client.tables.list_tables() + tables_data = await client.tables.list_tables() + tables = [Table(**t) for t in tables_data] + return ListTablesResponse(tables=tables, total_count=len(tables)) @mcp.tool( title="Get Table Schema", diff --git a/tests/server/oauth/test_oauth_deck_permissions.py b/tests/server/oauth/test_oauth_deck_permissions.py index 1b12537..b5ade0e 100644 --- a/tests/server/oauth/test_oauth_deck_permissions.py +++ b/tests/server/oauth/test_oauth_deck_permissions.py @@ -78,8 +78,10 @@ async def test_deck_board_view_permissions( if not result.isError: response_data = json.loads(result.content[0].text) - # The response is directly a list of boards - if not isinstance(response_data, list): + # Response is a ListBoardsResponse with a "boards" field + if isinstance(response_data, dict) and "boards" in response_data: + response_data = response_data["boards"] + elif not isinstance(response_data, list): response_data = [response_data] if response_data else [] board_ids = [b["id"] for b in response_data] logger.info(f"Bob can see {len(response_data)} boards: {board_ids}") @@ -98,8 +100,10 @@ async def test_deck_board_view_permissions( if not result.isError: response_data = json.loads(result.content[0].text) - # The response is directly a list of boards - if not isinstance(response_data, list): + # Response is a ListBoardsResponse with a "boards" field + if isinstance(response_data, dict) and "boards" in response_data: + response_data = response_data["boards"] + elif not isinstance(response_data, list): response_data = [response_data] if response_data else [] board_ids = [b["id"] for b in response_data] logger.info(f"Diana can see {len(response_data)} boards") @@ -313,8 +317,10 @@ async def test_deck_user_isolation(nc_client, alice_mcp_client, bob_mcp_client): if not result.isError: response_data = json.loads(result.content[0].text) - # The response is directly a list of boards - if not isinstance(response_data, list): + # Response is a ListBoardsResponse with a "boards" field + if isinstance(response_data, dict) and "boards" in response_data: + response_data = response_data["boards"] + elif not isinstance(response_data, list): response_data = [response_data] if response_data else [] board_ids = [b["id"] for b in response_data] logger.info(f"Alice can see boards: {board_ids}") @@ -332,8 +338,10 @@ async def test_deck_user_isolation(nc_client, alice_mcp_client, bob_mcp_client): if not result.isError: response_data = json.loads(result.content[0].text) - # The response is directly a list of boards - if not isinstance(response_data, list): + # Response is a ListBoardsResponse with a "boards" field + if isinstance(response_data, dict) and "boards" in response_data: + response_data = response_data["boards"] + elif not isinstance(response_data, list): response_data = [response_data] if response_data else [] board_ids = [b["id"] for b in response_data] logger.info(f"Bob can see boards: {board_ids}") diff --git a/tests/server/test_mcp.py b/tests/server/test_mcp.py index 9bcef10..03e379b 100644 --- a/tests/server/test_mcp.py +++ b/tests/server/test_mcp.py @@ -683,17 +683,15 @@ async def test_mcp_calendar_workflow( f"MCP list events failed: {list_result.content}" ) - events_data = json.loads(list_result.content[0].text) + events_response = json.loads(list_result.content[0].text) # Debug output to understand what nc_calendar_list_events returns - logger.info(f"list_events result type: {type(events_data)}") - logger.info(f"list_events result content: {events_data}") - - # Handle single event returned as dict instead of list (same fix as calendars) - if isinstance(events_data, dict): - # Single event returned as dict instead of list - events_data = [events_data] + logger.info(f"list_events result type: {type(events_response)}") + logger.info(f"list_events result content: {events_response}") + # Response is now a ListEventsResponse with an "events" field + assert isinstance(events_response, dict), "Expected response dict" + events_data = events_response.get("events", []) assert isinstance(events_data, list), "Expected events list" # Our created event should be in the list @@ -706,7 +704,7 @@ async def test_mcp_calendar_workflow( assert found_event is not None, ( f"Created event {event_uid} not found in events list" ) - assert found_event["title"] == test_event_title + assert found_event["summary"] == test_event_title # 6. Test list events across all calendars logger.info("Testing nc_calendar_list_events across all calendars") @@ -727,13 +725,11 @@ async def test_mcp_calendar_workflow( f"MCP list all events failed: {all_list_result.content}" ) - all_events_data = json.loads(all_list_result.content[0].text) - - # Handle single event returned as dict instead of list (same fix as calendars) - if isinstance(all_events_data, dict): - # Single event returned as dict instead of list - all_events_data = [all_events_data] + all_events_response = json.loads(all_list_result.content[0].text) + # Response is now a ListEventsResponse with an "events" field + assert isinstance(all_events_response, dict), "Expected response dict" + all_events_data = all_events_response.get("events", []) assert isinstance(all_events_data, list), "Expected events list" # Our event should still be found when searching all calendars @@ -780,13 +776,11 @@ async def test_mcp_calendar_workflow( f"MCP upcoming events failed: {upcoming_result.content}" ) - upcoming_events = json.loads(upcoming_result.content[0].text) - - # Handle single event returned as dict instead of list (same fix as other tools) - if isinstance(upcoming_events, dict): - # Single event returned as dict instead of list - upcoming_events = [upcoming_events] + upcoming_response = json.loads(upcoming_result.content[0].text) + # Response is now an UpcomingEventsResponse with an "events" field + assert isinstance(upcoming_response, dict), "Expected response dict" + upcoming_events = upcoming_response.get("events", []) assert isinstance(upcoming_events, list), "Expected upcoming events list" # 10. Delete event via MCP From 76e305006ceddec833fb16f900f1f98245826598 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 20 Feb 2026 09:39:46 +0100 Subject: [PATCH 2/5] fix: address PR #574 review comments Restore contact email/birthday/nickname data and per-event calendar source that were silently dropped during response model wrapping. Remove dead elif branches in OAuth deck tests, add regression tests. Co-Authored-By: Claude Opus 4.6 --- nextcloud_mcp_server/models/calendar.py | 6 + nextcloud_mcp_server/server/calendar.py | 5 + nextcloud_mcp_server/server/contacts.py | 35 ++++- .../oauth/test_oauth_deck_permissions.py | 32 ++--- tests/unit/test_response_models.py | 127 ++++++++++++++++++ 5 files changed, 176 insertions(+), 29 deletions(-) diff --git a/nextcloud_mcp_server/models/calendar.py b/nextcloud_mcp_server/models/calendar.py index fb1bf8f..3884b50 100644 --- a/nextcloud_mcp_server/models/calendar.py +++ b/nextcloud_mcp_server/models/calendar.py @@ -34,6 +34,12 @@ class CalendarEventSummary(BaseModel): status: Optional[str] = Field( None, description="Event status (CONFIRMED, TENTATIVE, CANCELLED)" ) + calendar_name: Optional[str] = Field( + None, description="Calendar containing this event" + ) + calendar_display_name: Optional[str] = Field( + None, description="Display name of calendar containing this event" + ) class CalendarEvent(CalendarEventSummary): diff --git a/nextcloud_mcp_server/server/calendar.py b/nextcloud_mcp_server/server/calendar.py index 5974264..8bf4033 100644 --- a/nextcloud_mcp_server/server/calendar.py +++ b/nextcloud_mcp_server/server/calendar.py @@ -39,6 +39,8 @@ def _event_dict_to_summary(event: dict) -> CalendarEventSummary: description=event.get("description") or None, categories=categories, status=event.get("status"), + calendar_name=event.get("calendar_name"), + calendar_display_name=event.get("calendar_display_name"), ) @@ -470,6 +472,9 @@ def configure_calendar_tools(mcp: FastMCP): end_datetime=end_datetime, limit=limit, ) + for event in cal_events: + event["calendar_name"] = calendar["name"] + event["calendar_display_name"] = calendar["display_name"] all_events.extend(cal_events) except Exception as e: logger.warning( diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index 593c913..bd51734 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -8,6 +8,7 @@ from nextcloud_mcp_server.context import get_client from nextcloud_mcp_server.models.contacts import ( AddressBook, Contact, + ContactField, ListAddressBooksResponse, ListContactsResponse, ) @@ -52,14 +53,34 @@ def configure_contacts_tools(mcp: FastMCP): """List all contacts in the specified addressbook.""" client = await get_client(ctx) contacts_data = await client.contacts.list_contacts(addressbook=addressbook) - contacts = [ - Contact( - uid=c["vcard_id"], - fn=c.get("contact", {}).get("fullname", ""), - etag=c.get("getetag"), + contacts = [] + for c in contacts_data: + contact_info = c.get("contact", {}) + + # Convert email field (str, list, or None) to list[ContactField] + raw_email = contact_info.get("email") + emails: list[ContactField] = [] + if isinstance(raw_email, list): + emails = [ContactField(type="email", value=e) for e in raw_email if e] + elif isinstance(raw_email, str) and raw_email: + emails = [ContactField(type="email", value=raw_email)] + + # Nickname goes into custom_fields (no dedicated model field) + custom_fields: dict[str, str] = {} + nickname = contact_info.get("nickname") + if nickname: + custom_fields["nickname"] = nickname + + contacts.append( + Contact( + uid=c["vcard_id"], + fn=contact_info.get("fullname", ""), + etag=c.get("getetag"), + birthday=contact_info.get("birthday"), + emails=emails, + custom_fields=custom_fields, + ) ) - for c in contacts_data - ] return ListContactsResponse( contacts=contacts, addressbook=addressbook, total_count=len(contacts) ) diff --git a/tests/server/oauth/test_oauth_deck_permissions.py b/tests/server/oauth/test_oauth_deck_permissions.py index b5ade0e..ee444aa 100644 --- a/tests/server/oauth/test_oauth_deck_permissions.py +++ b/tests/server/oauth/test_oauth_deck_permissions.py @@ -79,12 +79,9 @@ async def test_deck_board_view_permissions( if not result.isError: response_data = json.loads(result.content[0].text) # Response is a ListBoardsResponse with a "boards" field - if isinstance(response_data, dict) and "boards" in response_data: - response_data = response_data["boards"] - elif not isinstance(response_data, list): - response_data = [response_data] if response_data else [] - board_ids = [b["id"] for b in response_data] - logger.info(f"Bob can see {len(response_data)} boards: {board_ids}") + board_list = response_data.get("boards", []) + board_ids = [b["id"] for b in board_list] + logger.info(f"Bob can see {len(board_list)} boards: {board_ids}") # Bob should see the shared board if board_id in board_ids: @@ -101,12 +98,9 @@ async def test_deck_board_view_permissions( if not result.isError: response_data = json.loads(result.content[0].text) # Response is a ListBoardsResponse with a "boards" field - if isinstance(response_data, dict) and "boards" in response_data: - response_data = response_data["boards"] - elif not isinstance(response_data, list): - response_data = [response_data] if response_data else [] - board_ids = [b["id"] for b in response_data] - logger.info(f"Diana can see {len(response_data)} boards") + board_list = response_data.get("boards", []) + board_ids = [b["id"] for b in board_list] + logger.info(f"Diana can see {len(board_list)} boards") # Diana should NOT see the board assert board_id not in board_ids, "Diana should not see board without ACL" @@ -318,11 +312,8 @@ async def test_deck_user_isolation(nc_client, alice_mcp_client, bob_mcp_client): if not result.isError: response_data = json.loads(result.content[0].text) # Response is a ListBoardsResponse with a "boards" field - if isinstance(response_data, dict) and "boards" in response_data: - response_data = response_data["boards"] - elif not isinstance(response_data, list): - response_data = [response_data] if response_data else [] - board_ids = [b["id"] for b in response_data] + board_list = response_data.get("boards", []) + board_ids = [b["id"] for b in board_list] logger.info(f"Alice can see boards: {board_ids}") # Alice should NOT see Bob's board @@ -339,11 +330,8 @@ async def test_deck_user_isolation(nc_client, alice_mcp_client, bob_mcp_client): if not result.isError: response_data = json.loads(result.content[0].text) # Response is a ListBoardsResponse with a "boards" field - if isinstance(response_data, dict) and "boards" in response_data: - response_data = response_data["boards"] - elif not isinstance(response_data, list): - response_data = [response_data] if response_data else [] - board_ids = [b["id"] for b in response_data] + board_list = response_data.get("boards", []) + board_ids = [b["id"] for b in board_list] logger.info(f"Bob can see boards: {board_ids}") # Bob should NOT see Alice's board diff --git a/tests/unit/test_response_models.py b/tests/unit/test_response_models.py index bbe44dc..272901e 100644 --- a/tests/unit/test_response_models.py +++ b/tests/unit/test_response_models.py @@ -2,6 +2,11 @@ import pytest +from nextcloud_mcp_server.models.contacts import ( + Contact, + ContactField, + ListContactsResponse, +) from nextcloud_mcp_server.models.notes import ( CreateNoteResponse, Note, @@ -267,3 +272,125 @@ def test_sampling_search_response_serialization(): assert data["model_used"] == "claude-3-5-sonnet" assert data["stop_reason"] == "maxTokens" assert data["success"] is True + + +def _map_contact(raw: dict) -> Contact: + """Replicate the mapping logic from server/contacts.py for testing.""" + contact_info = raw.get("contact", {}) + + raw_email = contact_info.get("email") + emails: list[ContactField] = [] + if isinstance(raw_email, list): + emails = [ContactField(type="email", value=e) for e in raw_email if e] + elif isinstance(raw_email, str) and raw_email: + emails = [ContactField(type="email", value=raw_email)] + + custom_fields: dict[str, str] = {} + nickname = contact_info.get("nickname") + if nickname: + custom_fields["nickname"] = nickname + + return Contact( + uid=raw["vcard_id"], + fn=contact_info.get("fullname", ""), + etag=raw.get("getetag"), + birthday=contact_info.get("birthday"), + emails=emails, + custom_fields=custom_fields, + ) + + +@pytest.mark.unit +def test_contact_mapping_preserves_email_birthday_nickname(): + """Test that list_contacts mapping preserves email, birthday, and nickname. + + Regression test for PR #574: the original mapping only kept uid, fn, etag + and silently dropped email, birthday, and nickname. + """ + raw_contact = { + "vcard_id": "abc-123", + "getetag": '"etag-val"', + "contact": { + "fullname": "Jane Doe", + "email": "jane@example.com", + "birthday": "1990-05-15", + "nickname": "JD", + }, + } + + contact = _map_contact(raw_contact) + + assert contact.uid == "abc-123" + assert contact.fn == "Jane Doe" + assert contact.etag == '"etag-val"' + assert contact.birthday == "1990-05-15" + assert len(contact.emails) == 1 + assert contact.emails[0].value == "jane@example.com" + assert contact.emails[0].type == "email" + assert contact.custom_fields["nickname"] == "JD" + + +@pytest.mark.unit +def test_contact_mapping_multiple_emails(): + """Test that multiple emails are mapped correctly.""" + raw_contact = { + "vcard_id": "def-456", + "contact": { + "fullname": "John Smith", + "email": ["john@work.com", "john@home.com"], + }, + } + + contact = _map_contact(raw_contact) + + assert len(contact.emails) == 2 + assert contact.emails[0].value == "john@work.com" + assert contact.emails[1].value == "john@home.com" + + +@pytest.mark.unit +def test_contact_mapping_missing_optional_fields(): + """Test mapping when email, birthday, and nickname are absent.""" + raw_contact = { + "vcard_id": "ghi-789", + "contact": {"fullname": "No Details"}, + } + + contact = _map_contact(raw_contact) + + assert contact.uid == "ghi-789" + assert contact.fn == "No Details" + assert contact.birthday is None + assert contact.emails == [] + assert contact.custom_fields == {} + + +@pytest.mark.unit +def test_list_contacts_response_wraps_contacts(): + """Test ListContactsResponse wraps contacts correctly for MCP output.""" + contacts = [ + _map_contact( + { + "vcard_id": "a", + "getetag": '"e1"', + "contact": { + "fullname": "Alice", + "email": "alice@test.com", + "birthday": "2000-01-01", + "nickname": "Ali", + }, + } + ), + ] + + response = ListContactsResponse( + contacts=contacts, addressbook="personal", total_count=1 + ) + + data = response.model_dump() + assert data["total_count"] == 1 + assert len(data["contacts"]) == 1 + c = data["contacts"][0] + assert c["birthday"] == "2000-01-01" + assert c["emails"][0]["value"] == "alice@test.com" + assert c["custom_fields"]["nickname"] == "Ali" From 76e6c12b561232aaa7890505cf03989fe488bbad Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 20 Feb 2026 10:57:09 +0100 Subject: [PATCH 3/5] fix: address PR #574 second review round - Enrich single-calendar event dicts with calendar_name before mapping to CalendarEventSummary (list_events and upcoming_events paths) - Extract _raw_contact_to_model() from inline mapping in contacts.py, fix custom_fields type annotation to dict[str, Any] - Add unit tests for _event_dict_to_summary covering categories parsing, falsy coercion, and calendar name passthrough - Replace duplicated test helper with import of production function Co-Authored-By: Claude Opus 4.6 --- nextcloud_mcp_server/server/calendar.py | 6 + nextcloud_mcp_server/server/contacts.py | 58 +++++----- tests/unit/test_response_models.py | 142 ++++++++++++++++++++---- 3 files changed, 154 insertions(+), 52 deletions(-) diff --git a/nextcloud_mcp_server/server/calendar.py b/nextcloud_mcp_server/server/calendar.py index 8bf4033..8c7f939 100644 --- a/nextcloud_mcp_server/server/calendar.py +++ b/nextcloud_mcp_server/server/calendar.py @@ -240,6 +240,10 @@ def configure_calendar_tools(mcp: FastMCP): limit=limit, ) + # Enrich events with calendar context for per-event mapping + for event in events: + event["calendar_name"] = calendar_name + # Apply filters if provided if filters: events = client.calendar._apply_event_filters(events, filters) @@ -459,6 +463,8 @@ def configure_calendar_tools(mcp: FastMCP): end_datetime=end_datetime, limit=limit, ) + for event in events: + event["calendar_name"] = calendar_name else: # Get events from all calendars all_calendars = await client.calendar.list_calendars() diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index bd51734..dc44956 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -1,4 +1,5 @@ import logging +from typing import Any from mcp.server.fastmcp import Context, FastMCP from mcp.types import ToolAnnotations @@ -17,6 +18,34 @@ from nextcloud_mcp_server.observability.metrics import instrument_tool logger = logging.getLogger(__name__) +def _raw_contact_to_model(raw: dict) -> Contact: + """Convert a raw contact dict from the contacts client to a Contact model.""" + contact_info = raw.get("contact", {}) + + # Convert email field (str, list, or None) to list[ContactField] + raw_email = contact_info.get("email") + emails: list[ContactField] = [] + if isinstance(raw_email, list): + emails = [ContactField(type="email", value=e) for e in raw_email if e] + elif isinstance(raw_email, str) and raw_email: + emails = [ContactField(type="email", value=raw_email)] + + # Nickname goes into custom_fields (no dedicated model field) + custom_fields: dict[str, Any] = {} + nickname = contact_info.get("nickname") + if nickname: + custom_fields["nickname"] = nickname + + return Contact( + uid=raw["vcard_id"], + fn=contact_info.get("fullname", ""), + etag=raw.get("getetag"), + birthday=contact_info.get("birthday"), + emails=emails, + custom_fields=custom_fields, + ) + + def configure_contacts_tools(mcp: FastMCP): # Contacts tools @mcp.tool( @@ -53,34 +82,7 @@ def configure_contacts_tools(mcp: FastMCP): """List all contacts in the specified addressbook.""" client = await get_client(ctx) contacts_data = await client.contacts.list_contacts(addressbook=addressbook) - contacts = [] - for c in contacts_data: - contact_info = c.get("contact", {}) - - # Convert email field (str, list, or None) to list[ContactField] - raw_email = contact_info.get("email") - emails: list[ContactField] = [] - if isinstance(raw_email, list): - emails = [ContactField(type="email", value=e) for e in raw_email if e] - elif isinstance(raw_email, str) and raw_email: - emails = [ContactField(type="email", value=raw_email)] - - # Nickname goes into custom_fields (no dedicated model field) - custom_fields: dict[str, str] = {} - nickname = contact_info.get("nickname") - if nickname: - custom_fields["nickname"] = nickname - - contacts.append( - Contact( - uid=c["vcard_id"], - fn=contact_info.get("fullname", ""), - etag=c.get("getetag"), - birthday=contact_info.get("birthday"), - emails=emails, - custom_fields=custom_fields, - ) - ) + contacts = [_raw_contact_to_model(c) for c in contacts_data] return ListContactsResponse( contacts=contacts, addressbook=addressbook, total_count=len(contacts) ) diff --git a/tests/unit/test_response_models.py b/tests/unit/test_response_models.py index 272901e..10cec40 100644 --- a/tests/unit/test_response_models.py +++ b/tests/unit/test_response_models.py @@ -4,7 +4,6 @@ import pytest from nextcloud_mcp_server.models.contacts import ( Contact, - ContactField, ListContactsResponse, ) from nextcloud_mcp_server.models.notes import ( @@ -17,6 +16,8 @@ from nextcloud_mcp_server.models.semantic import ( SamplingSearchResponse, SemanticSearchResult, ) +from nextcloud_mcp_server.server.calendar import _event_dict_to_summary +from nextcloud_mcp_server.server.contacts import _raw_contact_to_model @pytest.mark.unit @@ -275,29 +276,8 @@ def test_sampling_search_response_serialization(): def _map_contact(raw: dict) -> Contact: - """Replicate the mapping logic from server/contacts.py for testing.""" - contact_info = raw.get("contact", {}) - - raw_email = contact_info.get("email") - emails: list[ContactField] = [] - if isinstance(raw_email, list): - emails = [ContactField(type="email", value=e) for e in raw_email if e] - elif isinstance(raw_email, str) and raw_email: - emails = [ContactField(type="email", value=raw_email)] - - custom_fields: dict[str, str] = {} - nickname = contact_info.get("nickname") - if nickname: - custom_fields["nickname"] = nickname - - return Contact( - uid=raw["vcard_id"], - fn=contact_info.get("fullname", ""), - etag=raw.get("getetag"), - birthday=contact_info.get("birthday"), - emails=emails, - custom_fields=custom_fields, - ) + """Thin wrapper around the production mapping function for test readability.""" + return _raw_contact_to_model(raw) @pytest.mark.unit @@ -394,3 +374,117 @@ def test_list_contacts_response_wraps_contacts(): assert c["birthday"] == "2000-01-01" assert c["emails"][0]["value"] == "alice@test.com" assert c["custom_fields"]["nickname"] == "Ali" + + +# ============= _event_dict_to_summary tests ============= + + +@pytest.mark.unit +def test_event_dict_to_summary_basic(): + """Test basic mapping with all fields populated.""" + event = { + "uid": "evt-001", + "title": "Team Standup", + "start_datetime": "2025-07-28T09:00:00", + "end_datetime": "2025-07-28T09:30:00", + "all_day": False, + "location": "Room 42", + "description": "Daily sync", + "categories": ["work", "meeting"], + "status": "CONFIRMED", + "calendar_name": "office", + "calendar_display_name": "Office Calendar", + } + + summary = _event_dict_to_summary(event) + + assert summary.uid == "evt-001" + assert summary.summary == "Team Standup" + assert summary.start == "2025-07-28T09:00:00" + assert summary.end == "2025-07-28T09:30:00" + assert summary.all_day is False + assert summary.location == "Room 42" + assert summary.description == "Daily sync" + assert summary.categories == ["work", "meeting"] + assert summary.status == "CONFIRMED" + assert summary.calendar_name == "office" + assert summary.calendar_display_name == "Office Calendar" + + +@pytest.mark.unit +def test_event_dict_to_summary_categories_string(): + """Test that comma-separated category string is split into a list.""" + event = { + "uid": "evt-002", + "title": "Review", + "categories": "work, meeting, important", + } + + summary = _event_dict_to_summary(event) + + assert summary.categories == ["work", "meeting", "important"] + + +@pytest.mark.unit +def test_event_dict_to_summary_categories_list_passthrough(): + """Test that a list of categories passes through unchanged.""" + event = { + "uid": "evt-003", + "title": "Review", + "categories": ["personal", "health"], + } + + summary = _event_dict_to_summary(event) + + assert summary.categories == ["personal", "health"] + + +@pytest.mark.unit +def test_event_dict_to_summary_falsy_location_description(): + """Test that empty/falsy location and description are coerced to None.""" + event = { + "uid": "evt-004", + "title": "Quick Chat", + "location": "", + "description": "", + } + + summary = _event_dict_to_summary(event) + + assert summary.location is None + assert summary.description is None + + +@pytest.mark.unit +def test_event_dict_to_summary_missing_optional_fields(): + """Test mapping with only required fields present.""" + event = {"uid": "evt-005", "title": "Minimal Event"} + + summary = _event_dict_to_summary(event) + + assert summary.uid == "evt-005" + assert summary.summary == "Minimal Event" + assert summary.start == "" + assert summary.end is None + assert summary.all_day is False + assert summary.location is None + assert summary.description is None + assert summary.categories == [] + assert summary.status is None + assert summary.calendar_name is None + assert summary.calendar_display_name is None + + +@pytest.mark.unit +def test_event_dict_to_summary_calendar_name_without_display_name(): + """Test single-calendar path: calendar_name set, display_name absent.""" + event = { + "uid": "evt-006", + "title": "Personal Errand", + "calendar_name": "personal", + } + + summary = _event_dict_to_summary(event) + + assert summary.calendar_name == "personal" + assert summary.calendar_display_name is None From f51b27ba190bd4b70aad0e9095c5c4067ddc9436 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 20 Feb 2026 13:49:55 +0100 Subject: [PATCH 4/5] fix: address PR #574 third review round - Guard board.labels against None in deck_get_labels and resource - Add TODO comments for calendar_display_name in single-calendar paths - Document _raw_contact_to_model scope limitation (maps only what the client returns; expanding requires changes to vCard parsing) - Log debug warning when event has no start_datetime - Verified Table model is safe with extra fields (Pydantic v2 ignores) Co-Authored-By: Claude Opus 4.6 --- nextcloud_mcp_server/server/calendar.py | 12 ++++++++++-- nextcloud_mcp_server/server/contacts.py | 8 +++++++- nextcloud_mcp_server/server/deck.py | 5 +++-- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/nextcloud_mcp_server/server/calendar.py b/nextcloud_mcp_server/server/calendar.py index 8c7f939..0761a72 100644 --- a/nextcloud_mcp_server/server/calendar.py +++ b/nextcloud_mcp_server/server/calendar.py @@ -29,10 +29,14 @@ def _event_dict_to_summary(event: dict) -> CalendarEventSummary: else: categories = raw_categories + start = event.get("start_datetime", "") + if not start: + logger.debug("Event %s has no start_datetime", event.get("uid", "unknown")) + return CalendarEventSummary( uid=event.get("uid", ""), summary=event.get("title", ""), - start=event.get("start_datetime", ""), + start=start, end=event.get("end_datetime"), all_day=event.get("all_day", False), location=event.get("location") or None, @@ -240,7 +244,10 @@ def configure_calendar_tools(mcp: FastMCP): limit=limit, ) - # Enrich events with calendar context for per-event mapping + # Enrich events with calendar context for per-event mapping. + # Note: calendar_display_name is not available here without an + # extra list_calendars() call; the response-level calendar_name + # already identifies the calendar for single-calendar queries. for event in events: event["calendar_name"] = calendar_name @@ -463,6 +470,7 @@ def configure_calendar_tools(mcp: FastMCP): end_datetime=end_datetime, limit=limit, ) + # calendar_display_name not available without extra API call for event in events: event["calendar_name"] = calendar_name else: diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index dc44956..ca64af7 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -19,7 +19,13 @@ logger = logging.getLogger(__name__) def _raw_contact_to_model(raw: dict) -> Contact: - """Convert a raw contact dict from the contacts client to a Contact model.""" + """Convert a raw contact dict from the contacts client to a Contact model. + + Only maps fields the client's list_contacts() currently returns: + fullname, nickname, birthday, and email. Additional Contact model fields + (phones, addresses, organization, etc.) require expanding the client's + vCard parsing in ContactsClient.list_contacts(). + """ contact_info = raw.get("contact", {}) # Convert email field (str, list, or None) to list[ContactField] diff --git a/nextcloud_mcp_server/server/deck.py b/nextcloud_mcp_server/server/deck.py index ee6f499..f48e134 100644 --- a/nextcloud_mcp_server/server/deck.py +++ b/nextcloud_mcp_server/server/deck.py @@ -107,7 +107,7 @@ def configure_deck_tools(mcp: FastMCP): ) client = await get_client(ctx) board = await client.deck.get_board(board_id) - return [label.model_dump() for label in board.labels] + return [label.model_dump() for label in (board.labels or [])] @mcp.resource("nc://Deck/boards/{board_id}/labels/{label_id}") async def deck_label_resource(board_id: int, label_id: int): @@ -209,7 +209,8 @@ def configure_deck_tools(mcp: FastMCP): """Get all labels in a Nextcloud Deck board""" client = await get_client(ctx) board = await client.deck.get_board(board_id) - return ListLabelsResponse(labels=board.labels, total=len(board.labels)) + labels = board.labels or [] + return ListLabelsResponse(labels=labels, total=len(labels)) @mcp.tool( title="Get Deck Label", From 18e5baf2a5cdbcde47db4efa50b57b29d8af11f8 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 21 Feb 2026 09:36:14 +0100 Subject: [PATCH 5/5] fix: address PR #574 fourth review round - Use lowercase generics (list[...]) in new deck response models - Add clarifying comment on AddressBook.uri slug semantics - Fall back calendar_display_name to calendar_name when absent Co-Authored-By: Claude Opus 4.6 --- nextcloud_mcp_server/models/deck.py | 4 ++-- nextcloud_mcp_server/server/calendar.py | 3 ++- nextcloud_mcp_server/server/contacts.py | 2 ++ tests/unit/test_response_models.py | 4 ++-- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/nextcloud_mcp_server/models/deck.py b/nextcloud_mcp_server/models/deck.py index ca46562..8c42bff 100644 --- a/nextcloud_mcp_server/models/deck.py +++ b/nextcloud_mcp_server/models/deck.py @@ -264,14 +264,14 @@ class CreateLabelResponse(BaseResponse): class ListCardsResponse(BaseResponse): """Response model for listing deck cards.""" - cards: List[DeckCard] = Field(description="List of deck cards") + cards: list[DeckCard] = Field(description="List of deck cards") total: int = Field(description="Total number of cards") class ListLabelsResponse(BaseResponse): """Response model for listing deck labels.""" - labels: List[DeckLabel] = Field(description="List of deck labels") + labels: list[DeckLabel] = Field(description="List of deck labels") total: int = Field(description="Total number of labels") diff --git a/nextcloud_mcp_server/server/calendar.py b/nextcloud_mcp_server/server/calendar.py index 0761a72..8881186 100644 --- a/nextcloud_mcp_server/server/calendar.py +++ b/nextcloud_mcp_server/server/calendar.py @@ -44,7 +44,8 @@ def _event_dict_to_summary(event: dict) -> CalendarEventSummary: categories=categories, status=event.get("status"), calendar_name=event.get("calendar_name"), - calendar_display_name=event.get("calendar_display_name"), + calendar_display_name=event.get("calendar_display_name") + or event.get("calendar_name"), ) diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index ca64af7..7618988 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -66,6 +66,8 @@ def configure_contacts_tools(mcp: FastMCP): addressbooks_data = await client.contacts.list_addressbooks() addressbooks = [ AddressBook( + # ab["name"] is a short slug like "contacts", not a full CardDAV URI; + # all tools use it as a path segment: f"{carddav_path}/{name}/" uri=ab["name"], displayname=ab.get("display_name", ab["name"]), ctag=ab.get("getctag"), diff --git a/tests/unit/test_response_models.py b/tests/unit/test_response_models.py index 10cec40..56c781e 100644 --- a/tests/unit/test_response_models.py +++ b/tests/unit/test_response_models.py @@ -477,7 +477,7 @@ def test_event_dict_to_summary_missing_optional_fields(): @pytest.mark.unit def test_event_dict_to_summary_calendar_name_without_display_name(): - """Test single-calendar path: calendar_name set, display_name absent.""" + """Test single-calendar path: calendar_name set, display_name absent falls back.""" event = { "uid": "evt-006", "title": "Personal Errand", @@ -487,4 +487,4 @@ def test_event_dict_to_summary_calendar_name_without_display_name(): summary = _event_dict_to_summary(event) assert summary.calendar_name == "personal" - assert summary.calendar_display_name is None + assert summary.calendar_display_name == "personal"