fix: wrap raw list returns in response models (#568)

[cbcoutinho]

Summary

• Wrapped 9 MCP tool functions that returned raw list[PydanticModel] in proper response objects, so each tool now produces a single JSON TextContent block instead of one block per element
• Added 2 new response models (ListCardsResponse, ListLabelsResponse) in models/deck.py; all other models already existed
• Updated integration tests to parse the new wrapped response format

Affected tools: deck_get_boards, deck_get_stacks, deck_get_cards, deck_get_labels, nc_calendar_list_events, nc_calendar_get_upcoming_events, nc_contacts_list_addressbooks, nc_contacts_list_contacts, nc_tables_list_tables

Test plan

• uv run ruff check && uv run ruff format — pass
• uv run ty check -- nextcloud_mcp_server — pass
• uv run pytest tests/unit/ -v — 334 passed (10 pre-existing failures unrelated)
• docker-compose up --build -d mcp && uv run pytest -m smoke -v — 6 passed
• uv run pytest -m "integration and not oauth" -v -k "calendar or deck or contacts or tables" — 69 passed, 0 failed

Closes #568

---

This PR was generated with the help of AI, and reviewed by a Human

🤖 Generated with Claude Code

[claude]

Code Review

This PR correctly addresses the core issue from #568 — wrapping raw list returns in proper response models. The approach is sound and aligns with the pattern documented in CLAUDE.md. That said, there are a few issues worth addressing before merging.

---

Bugs / Regressions

1. Contacts list loses almost all contact data (Major)

In server/contacts.py, nc_contacts_list_contacts maps raw contact dicts to Contact objects, but only preserves three fields:

```python
Contact(
uid=c["vcard_id"],
fn=c.get("contact", {}).get("fullname", ""),
etag=c.get("getetag"),
)
```

The Contact model has emails, phones, given_name, family_name, organization, birthday, categories, etc. — all of which come back from list_contacts() (via contact.email, contact.bday, contact.nickname). Before this PR, the raw dict with email, nickname, and birthday was returned. Now those fields silently go to their defaults (empty list/None). This is a meaningful regression for any agent using this tool.

The mapping should also populate at least emails from the raw data (or the full Contact model should be built from the available data), to avoid this silent data loss.

2. nc_calendar_get_upcoming_events loses calendar source information (Moderate)

The original code explicitly annotated each event with which calendar it came from:

```python

Original (removed in this PR)

event["calendar_name"] = calendar["name"]
event["calendar_display_name"] = calendar["display_name"]
```

This was the only way to know which calendar an event belongs to when fetching across all calendars. The CalendarEventSummary model has no calendar_name field, so after this PR, a user calling nc_calendar_get_upcoming_events with no calendar_name filter gets events with no indication of their source calendar. Consider adding calendar_name: Optional[str] to CalendarEventSummary, or including per-event calendar context another way.

---

Inconsistencies

3. Count field naming is inconsistent across response models

The PR introduces new response models with different naming for the count field:

• ListCardsResponse.total, ListLabelsResponse.total (new in this PR)
• ListBoardsResponse.total, ListStacksResponse.total (pre-existing)
• ListTablesResponse.total_count, ListAddressBooksResponse.total_count, ListContactsResponse.total_count
• ListEventsResponse.total_found, ListTodosResponse.total_count

ListCardsResponse and ListLabelsResponse follow the Deck naming (total), which is consistent within that module. But the broader project has no single convention. This isn't introduced by this PR, but it's worth noting for future work.

4. UpcomingEventsResponse lacks a count field

ListEventsResponse has total_found: int, but UpcomingEventsResponse has no equivalent. Since both are event listing responses, this inconsistency could confuse callers. Minor, but easy to fix.

---

Test Coverage

5. OAuth deck tests use a fragile fallback pattern

The updated test code:

```python
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 []
```

The elif branch still handles the old format. Since deck_get_boards now always returns a ListBoardsResponse dict, the elif branch is dead code. It could be simplified to just:

```python
assert isinstance(response_data, dict) and "boards" in response_data
response_data = response_data["boards"]
```

This appears three times in test_oauth_deck_permissions.py.

6. No unit/integration test coverage for new contacts mapping

The PR description mentions calendar or deck or contacts or tables tests passed, but the contacts changes introduce new data mapping logic that isn't verified in the diff. A unit test covering that nickname, birthday, and email survive the mapping (or intentionally don't) would prevent silent regressions here.

---

Minor / Nits

7. _event_dict_to_summary handles str categories but the upstream model already uses List[str]

The helper defensively handles categories as either str or list. That's fine, but it's worth checking whether the calendar client ever actually returns a string here — if not, the isinstance(raw_categories, str) branch is dead code.

8. deck.py still imports List from typing

nextcloud_mcp_server/models/deck.py uses from typing import Any, Dict, List, Optional, Union — CLAUDE.md specifies Python 3.10+ lowercase generics (list not List). The PR adds new response models using List[DeckCard] / List[DeckLabel] following the existing pattern, so this is pre-existing tech debt, not a new violation. Just flagging for awareness.

---

Summary

The core direction is correct and the approach aligns with project conventions. The two functional issues (contacts data loss and missing calendar source info in upcoming events) should be addressed before merging. The rest are improvements or cleanup.

[claude]

Code Review

This PR correctly addresses the core issue — FastMCP was mangling raw list[PydanticModel] returns into dicts with numeric string keys. The approach of wrapping everything in a BaseResponse-derived model is the right fix and aligns with the project's established pattern.

What works well

• _event_dict_to_summary() cleanly handles the categories string/list ambiguity and the title → summary rename.
• Deck changes are minimal and correct — stack.cards or [] is a nice simplification.
• Test updates properly reflect the new response schema, and the new unit tests for contact mapping cover the key edge cases (single email string, list of emails, missing fields).
• The OAuth permission tests are updated correctly.

---

Issues

1. Naming inconsistency: total vs total_count

The two new deck models use total, while contacts and tables use total_count. This PR adds ListCardsResponse and ListLabelsResponse with total, but existing ListBoardsResponse and ListStacksResponse also use total. The inconsistency is now spread across 5 files:

deck:     total        (boards, stacks, cards, labels)
contacts: total_count  (addressbooks, contacts)
tables:   total_count  (tables)
calendar: total_found  (events)

Not a blocking issue, but for a client consuming multiple tools this will be confusing. Worth standardizing in a follow-up.

2. Per-event calendar_name is never populated in nc_calendar_list_events

CalendarEventSummary now has calendar_name and calendar_display_name fields, and _event_dict_to_summary maps them via event.get("calendar_name"). But in nc_calendar_list_events, neither the single-calendar nor the search-all-calendars path enriches individual event dicts with calendar provenance — the response-level calendar_name field carries that for single-calendar, but per-event it will always be None.

Contrast with nc_calendar_get_upcoming_events, which does enrich each event dict in the multi-calendar path:

for event in cal_events:
    event["calendar_name"] = calendar["name"]
    event["calendar_display_name"] = calendar["display_name"]

nc_calendar_list_events (both paths) skips this. An LLM agent iterating over events from list_events in all-calendar mode won't know which calendar each event belongs to. This seems unintentional.

3. Test helper duplicates production mapping logic

_map_contact in tests/unit/test_response_models.py (lines 273–293) is a copy of the loop body from server/contacts.py. If the production mapping changes (e.g. adding phone support), the test helper silently stays out of sync.

The function _event_dict_to_summary is a module-level function in server/calendar.py — it could be extracted to models/calendar.py or a utility module so it can be imported and tested directly. For contacts, the same extraction would allow tests/unit/test_response_models.py to import _map_contact_to_model instead of duplicating it.

4. No unit test for _event_dict_to_summary

The calendar mapping helper has more non-trivial logic than the contact mapper (category string-splitting, title→summary rename, start_datetime→start rename, or None coercions) but there are no unit tests for it. A test covering the category parsing edge case (comma-separated string vs list) would be particularly valuable.

---

Minor observations

• calendar_name or None in UpcomingEventsResponse(calendar_name=calendar_name or None, ...) is correct given the calendar_name: str = "" default, but the intent (empty string = all calendars = None in response) could use a brief comment for readers unfamiliar with this pattern.
• custom_fields: dict[str, str] = {} in the contacts loop has a narrower type annotation than the model field Dict[str, Any]. Not a runtime issue, but worth aligning.
• AddressBook(uri=ab["name"], ...) maps the addressbook's name to uri — the field names suggest these are different concepts (name is a short identifier, uri is a full path). Might warrant a comment or a check of what ab["name"] actually contains (short name vs /remote.php/dav/... path).

---

Summary

The fix is correct and the test coverage for the new patterns is solid. The main actionable items before merging are issue #2 (missing per-event calendar enrichment in nc_calendar_list_events) which looks like a genuine behaviour gap, and optionally issue #4 (add a test for _event_dict_to_summary). The rest are housekeeping items suitable for follow-up issues.

[claude]

PR Review: Fix raw list returns (#574)

This is a solid, well-motivated fix. The CLAUDE.md convention is explicit about this: "Never return raw List[Dict] from MCP tools" and this PR addresses 9 tools that were silently violating it. The test plan is thorough and the new unit tests covering _event_dict_to_summary and _raw_contact_to_model are a genuinely useful addition. A few issues worth addressing before merge:

---

Bug: board.labels may be None in deck_get_labels

# server/deck.py
board = await client.deck.get_board(board_id)
return ListLabelsResponse(labels=board.labels, total=len(board.labels))
#                                                     ^^^ TypeError if None

The DeckBoard model likely declares labels: list[DeckLabel] | None, matching the original code which returned board.labels directly (None-safe). The new code calls len() on it unconditionally. Apply the same guard used for cards:

labels = board.labels or []
return ListLabelsResponse(labels=labels, total=len(labels))

---

Data loss in _raw_contact_to_model

The helper only maps uid, fn, etag, birthday, emails, and nickname. If the Contact model has fields for phone numbers, physical addresses, organisation, photo URLs, etc. (typical CardDAV data), those fields are silently dropped even when the underlying client returns them. The unit tests confirm the three named fields are preserved, but they don't verify that nothing else is lost.

If the intent is to keep the mapping minimal for now, a comment explaining the intentional scope limitation would be helpful. Otherwise, auditing the full Contact model and mapping all available fields is the safer choice.

---

Potential ValidationError in nc_tables_list_tables

tables = [Table(**t) for t in tables_data]

If the raw dicts from list_tables() contain keys not declared in the Table model and the model doesn't set model_config = ConfigDict(extra='ignore'), Pydantic v2 raises a ValidationError at runtime. Worth checking that Table tolerates extra fields, or using model_validate with an explicit config, or pre-filtering the dict.

---

Inconsistent metadata field naming

The three total variants across the new response models will confuse tool consumers:

Model	Field
ListCardsResponse / ListLabelsResponse	total
ListContactsResponse / ListAddressBooksResponse	total_count
ListEventsResponse	total_found

Existing models in the codebase (e.g. SearchNotesResponse) use total_found. Picking one name consistently (or at minimum matching the existing convention) makes the API easier to work with programmatically.

---

calendar_display_name missing in single-calendar paths

In both nc_calendar_list_events and nc_calendar_get_upcoming_events, when fetching from a specific named calendar the code enriches events with calendar_name but not calendar_display_name:

for event in events:
    event["calendar_name"] = calendar_name   # set
    # calendar_display_name never set → always None in summaries

The all-calendars path sets both fields. The asymmetry means single-calendar calls return "calendar_display_name": null even when the display name is known. This is a pre-existing limitation of the single-calendar path, but it is now surfaced in the API contract; a follow-up issue or a TODO comment would be useful.

---

Minor: start defaults to "" on missing start_datetime

start=event.get("start_datetime", ""),

Events without a start_datetime key silently produce an empty-string timestamp. If start is a required semantic field, a more explicit fallback (or a log warning) would make debugging easier when upstream data is malformed. Not a blocker, but worth noting.

---

What's working well

• The core fix is correct: wrapping in response objects eliminates the FastMCP numeric-key mangling.
• deck_get_cards already guards against None with stack.cards or [].
• The unit tests for _event_dict_to_summary cover the edge cases well (comma-separated categories, falsy location/description, missing fields).
• Removing the isinstance(data, dict)→[data] workarounds from the integration tests is a clean simplification.
• The found_event["title"] → found_event["summary"] fix in the integration test is correct and necessary.

[claude]

PR Review: Wrap Raw List Returns in Response Models

Good direction overall — this correctly fixes the FastMCP serialization bug (issue #568) where raw lists were mangled into dicts with numeric string keys. The test coverage for the new mapping helpers is solid. Here are some observations:

---

Issues

1. List[...] vs list[...] in new deck models

In models/deck.py, the two new response models use capital-L List:

cards: List[DeckCard] = Field(...)
labels: List[DeckLabel] = Field(...)

CLAUDE.md requires lowercase generics (list[DeckCard]). If the rest of deck.py already uses List, this is consistent within the file — but it's still worth fixing for the new code to align with the project convention.

---

2. Inconsistent total-count field naming

Three different field names are now used across equivalent response models:

Domain	Field
Deck	total
Contacts, Tables	total_count
Calendar	total_found

This forces clients to use different keys for a semantically identical concept. Since all the affected models are being touched in this PR, it's a reasonable time to standardise on one name (e.g. total_count). The deck models (ListBoardsResponse, ListStacksResponse) are pre-existing, so this would require a separate pass — but at minimum the two new deck models (ListCardsResponse, ListLabelsResponse) should match the existing naming rather than introduce a third variant.

---

3. calendar_display_name inconsistency between single- and multi-calendar paths

In nc_calendar_list_events:

• All-calendars path: events have both calendar_name and calendar_display_name (populated by get_calendar_events_across_calendars).
• Single-calendar path: only calendar_name is injected; calendar_display_name stays None.

The comment acknowledges this, but clients that conditionally display calendar_display_name will get different shapes depending on how the tool was called. The same asymmetry exists in nc_calendar_get_upcoming_events. Since calendar_display_name is on the summary-level field (not just the envelope), silently omitting it for the single-calendar case is a latent bug. Consider either:

• Making a single list_calendars() call to resolve the display name, or
• Falling back to calendar_name when calendar_display_name is absent (document the omission only at the model level).

---

4. uri mapping in nc_contacts_list_addressbooks

AddressBook(
    uri=ab["name"],           # ← is "name" actually the URI?
    displayname=ab.get("display_name", ab["name"]),
    ctag=ab.get("getctag"),
)

The AddressBook model has a uri field (presumably a CardDAV path like /remote.php/dav/addressbooks/users/admin/contacts/), but the client's list_addressbooks() returns a name key. If name is just a short identifier rather than the full URI, clients relying on uri for subsequent CardDAV operations will get a wrong value. Worth verifying what the contacts client actually returns for that key.

---

5. Table(**t) is fragile

tables = [Table(**t) for t in tables_data]

This relies on the raw API dict keys matching the Table Pydantic field names exactly. If the API ever returns an unexpected key and the model isn't configured with extra='ignore', this will raise a ValidationError at runtime. The pattern used elsewhere (explicit field mapping, as in _raw_contact_to_model) is more robust and self-documenting.

---

6. Testing internal helpers directly

tests/unit/test_response_models.py imports private functions:

from nextcloud_mcp_server.server.calendar import _event_dict_to_summary
from nextcloud_mcp_server.server.contacts import _raw_contact_to_model

Testing private functions directly couples the test suite to implementation details. These helpers could be moved to a _mapping.py or utils.py module alongside their respective server files, making the import intentional and the boundary clearer. Alternatively, testing them through the public tool interfaces (integration-style) avoids the coupling entirely.

---

Positives

• The board.labels or [] / stack.cards or [] fixes are correct and prevent NoneType errors.
• The _event_dict_to_summary and _raw_contact_to_model helpers are well-documented with their limitations.
• Unit tests for the mapping functions are thorough and cover edge cases (multi-email, missing fields, comma-separated categories, falsy location/description).
• The test updates cleanly remove the workaround (isinstance(data, dict)) that was papering over the original bug.

---

Overall this is a solid fix for the serialization issue. The naming inconsistency (point 2) and the uri mapping question (point 4) are the items I'd most want resolved before merging.

[claude]

Code Review

This PR correctly addresses the FastMCP list-mangling issue documented in CLAUDE.md. The approach is sound and the test coverage is thorough. A few observations:

---

What's Well Done

• Root cause fix is correct. Returning list[PydanticModel] directly from MCP tools caused FastMCP to emit one TextContent block per element with numeric string keys. Wrapping in a response object solves this cleanly.
• _event_dict_to_summary consolidates conversion logic that was previously scattered across two tool functions. The fallback calendar_display_name=event.get("calendar_display_name") or event.get("calendar_name") handles the asymmetry between single- and multi-calendar paths gracefully.
• _raw_contact_to_model handles edge cases well — str vs. list emails, empty string → None coercion, and the acknowledged limitation on unmapped fields is documented clearly in the docstring.
• Null safety added for board.labels (board.labels or []) — the previous code would have returned None if a board had no labels, which is a latent bug fix.
• Unit tests for the mapping helpers are comprehensive, covering happy paths, missing optional fields, type variations (string vs. list categories), and the display-name fallback.

---

Minor Issues / Suggestions

1. Mapping helpers in the server layer

_event_dict_to_summary and _raw_contact_to_model are model-conversion logic living in server/calendar.py and server/contacts.py. The tests import them directly from there:

from nextcloud_mcp_server.server.calendar import _event_dict_to_summary
from nextcloud_mcp_server.server.contacts import _raw_contact_to_model

Testing private functions from the server module is pragmatic here (the logic is complex enough to warrant unit tests), but if these helpers grow or are needed elsewhere, they'd be better placed in a utils/ or models/ module. Not a blocker, just something to watch.

2. Unnecessary _map_contact wrapper in tests

def _map_contact(raw: dict) -> Contact:
    """Thin wrapper around the production mapping function for test readability."""
    return _raw_contact_to_model(raw)

This is a no-op indirection. Tests could call _raw_contact_to_model directly — the wrapper adds noise without adding clarity.

3. AddressBook.uri stores a slug, not a URI

AddressBook(
    uri=ab["name"],  # short slug like "contacts", not a full CardDAV URI
    ...
)

The inline comment explains this, but a field named uri holding a path segment is semantically misleading for anyone reading the model definition or the MCP tool output. Consider whether renaming the field to name or path in the model would be cleaner in a follow-up — though that's a broader change.

4. ListCardsResponse / ListLabelsResponse field naming vs. project convention

CLAUDE.md says response models should include a results field, following SearchNotesResponse and SearchFilesResponse. The new models use cards and labels. The existing ListBoardsResponse/ListStacksResponse also use descriptive names, so this PR is consistent with the existing deck patterns — just noting the inconsistency with the broader project convention exists and wasn't introduced here.

5. Pre-existing: private method access

events = client.calendar._apply_event_filters(events, filters)

This was present before the PR, but it's worth noting — accessing a private method from outside the class is a code smell. Not a PR issue to fix here.

---

Correctness Checks

• Both branches of nc_calendar_list_events (search_all_calendars and single-calendar) now assign to events and fall through to the common summaries = [...] path. ✓
• The limit is correctly applied: via events[:limit] in the all-calendars path, and via limit=limit parameter in the single-calendar path. ✓
• calendar_name=None if search_all_calendars else calendar_name in ListEventsResponse correctly omits the field for cross-calendar queries. ✓
• Tests update found_event["title"] → found_event["summary"] to match CalendarEventSummary.summary field name — catches a field rename that would otherwise silently fail. ✓

---

Overall this is a clean, well-tested fix for a real serialization bug. The suggestions above are minor and mostly stylistic.