From 271904c227acf0c687185e0369669bcf4cb2b0a9 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 10 May 2026 23:28:37 +0200 Subject: [PATCH] =?UTF-8?q?fix(deck):=20address=20review=20=E2=80=94=20not?= =?UTF-8?q?esPath=20key,=20scopes,=20modernize=20types?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #781 review round 1: - 🔴 Fix notesPath key: the Notes API returns the folder under camelCase ``notesPath`` (see models/notes.py:43), but `deck_attach_note` was looking up snake_case ``notes_path`` and silently falling back to ``"Notes"``. Users with a non-default notes folder would have produced shares pointing at non-existent files (404 on click in Deck UI). - 🔴 Add wire-through unit test that would have caught the above: extract `_resolve_note_attach_path(client, note_id)` as a testable helper that encapsulates the camelCase-key lookup. Three new tests: custom notesPath honored, missing key falls back to default, null category handled. - 🟡 Modernize new fields on `DeckAttachmentExtendedData` to PEP 604 (`X | None`) per CLAUDE.md. - 🟡 Drop unnecessary string forward reference on `ListAttachmentsResponse.results` — DeckAttachment is defined earlier in the same module. - 🟢 Move `pytestmark = pytest.mark.unit` to module level in test_sharing_client.py to match the convention in test_deck_server.py. Per user request: `deck_attach_file` is now scoped `deck.write` + ``files.read`` (was just `deck.write`) so the generic file-share permission story is consistent — only `deck_attach_note` keeps `notes.read` since it specifically reads from the Notes app. Docstring updated to emphasise the tool is generic over the user's Files (PDFs/images/etc., not just markdown). Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/models/deck.py | 10 ++--- nextcloud_mcp_server/server/deck.py | 43 ++++++++++++++------- tests/client/test_sharing_client.py | 4 +- tests/unit/test_deck_server.py | 60 +++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 21 deletions(-) diff --git a/nextcloud_mcp_server/models/deck.py b/nextcloud_mcp_server/models/deck.py index e0ee76bd..1c29c401 100644 --- a/nextcloud_mcp_server/models/deck.py +++ b/nextcloud_mcp_server/models/deck.py @@ -145,10 +145,10 @@ class DeckAttachmentExtendedData(BaseModel): mimetype: str info: Dict[str, str] # Populated for type="file" (Files share) attachments via FilesAppService. - path: Optional[str] = None - fileid: Optional[int] = None - hasPreview: Optional[bool] = None - permissions: Optional[int] = None + path: str | None = None + fileid: int | None = None + hasPreview: bool | None = None + permissions: int | None = None class DeckAttachment(BaseModel): @@ -336,7 +336,7 @@ class AttachFileResponse(BaseResponse): class ListAttachmentsResponse(BaseResponse): """Response model for listing card attachments.""" - results: list["DeckAttachment"] = Field( + results: list[DeckAttachment] = Field( description="Attachments on the card (both type='file' and type='deck_file')" ) count: int = Field(description="Number of attachments returned") diff --git a/nextcloud_mcp_server/server/deck.py b/nextcloud_mcp_server/server/deck.py index 9a99f939..ec8344c5 100644 --- a/nextcloud_mcp_server/server/deck.py +++ b/nextcloud_mcp_server/server/deck.py @@ -127,6 +127,26 @@ def _resolve_note_path(notes_folder: str, category: str, title: str) -> str: return "/" + "/".join(p for p in parts if p) +async def _resolve_note_attach_path(client, note_id: int) -> str: + """Resolve a Notes-app note ID to its filesystem path for sharing. + + Hits the Notes API twice (settings + note metadata) and reconstructs + the path. Encapsulates the camelCase key (``notesPath``, see + ``models/notes.py:43``) so a typo there can't silently route to the + default ``"Notes"`` folder for users who've configured a non-default + notes location — that bug is exactly what this helper exists to make + testable. + """ + settings = await client.notes.get_settings() + note = await client.notes.get_note(note_id) + notes_folder = settings.get("notesPath") or "Notes" + return _resolve_note_path( + notes_folder=notes_folder, + category=note.get("category") or "", + title=note["title"], + ) + + def configure_deck_tools(mcp: FastMCP): """Configure Nextcloud Deck tools and resources for the MCP server.""" @@ -1079,7 +1099,7 @@ def configure_deck_tools(mcp: FastMCP): title="Attach File to Deck Card", annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), ) - @require_scopes("deck.write") + @require_scopes("deck.write", "files.read") @instrument_tool async def deck_attach_file( ctx: Context, card_id: int, path: str @@ -1090,15 +1110,17 @@ def configure_deck_tools(mcp: FastMCP): ``shareWith=``). The file stays in its original location; clicking the attachment in the Deck UI opens the file in place. - Use this to link Notes (``/Notes/.md``) or any Files entry to - a card as a discoverable attachment, replacing the older pattern of - long card comments. Calling twice with the same ``path`` creates two - distinct shares — caller is responsible for de-duping. + Generic over the user's Files: works for any file the caller can + read — markdown notes, PDFs, images, spreadsheets, etc. Use + :func:`deck_attach_note` if you have a Notes-app note ID and want + the path resolved automatically. Calling twice with the same + ``path`` creates two distinct shares — caller is responsible for + de-duping. Args: card_id: The ID of the Deck card to attach to path: Path to the file in the user's Nextcloud Files (must start - with "/", e.g. "/Notes/My Note.md") + with "/", e.g. "/Documents/spec.pdf" or "/Notes/My Note.md") """ if not path.startswith("/"): raise ValueError( @@ -1145,14 +1167,7 @@ def configure_deck_tools(mcp: FastMCP): note_id: The ID of the Note to attach """ client = await get_client(ctx) - settings = await client.notes.get_settings() - note = await client.notes.get_note(note_id) - notes_folder = settings.get("notes_path") or "Notes" - path = _resolve_note_path( - notes_folder=notes_folder, - category=note.get("category") or "", - title=note["title"], - ) + path = await _resolve_note_attach_path(client, note_id) share = await client.sharing.create_share( path=path, share_with=str(card_id), diff --git a/tests/client/test_sharing_client.py b/tests/client/test_sharing_client.py index 1723b1c5..f4e40d14 100644 --- a/tests/client/test_sharing_client.py +++ b/tests/client/test_sharing_client.py @@ -11,6 +11,8 @@ from httpx import AsyncClient from nextcloud_mcp_server.client.sharing import SharingClient +pytestmark = pytest.mark.unit + @pytest.fixture def sharing_client(mocker): @@ -32,7 +34,6 @@ def _ok_share_response(mocker, share_id: int = 4242, **extra): return response -@pytest.mark.unit async def test_create_share_deck_type_payload(sharing_client, mocker): """create_share(share_type=12) must POST exactly what the Deck UI does: {path, shareType: 12, shareWith: "<cardId>"} to /ocs/v2.php/apps/files_sharing/api/v1/shares. @@ -65,7 +66,6 @@ async def test_create_share_deck_type_payload(sharing_client, mocker): assert call.kwargs["headers"]["OCS-APIRequest"] == "true" -@pytest.mark.unit async def test_create_share_raises_on_ocs_failure(sharing_client, mocker): """OCS error responses (statuscode != 100/200) raise RuntimeError.""" response = mocker.Mock() diff --git a/tests/unit/test_deck_server.py b/tests/unit/test_deck_server.py index f7ace51c..ff0ab859 100644 --- a/tests/unit/test_deck_server.py +++ b/tests/unit/test_deck_server.py @@ -14,6 +14,7 @@ from nextcloud_mcp_server.server.deck import ( _apply_board_filters, _apply_card_filters, _apply_stack_filters, + _resolve_note_attach_path, _resolve_note_path, _truncate_card_descriptions, _validate_description_max_length, @@ -408,3 +409,62 @@ def test_resolve_note_path_custom_notes_folder(): def test_share_type_deck_constant_matches_deck_app(): """Deck UI uses shareType=12 (IShare::TYPE_DECK) — must not drift.""" assert _SHARE_TYPE_DECK == 12 + + +# _resolve_note_attach_path (camelCase notesPath guard) --------------------- + + +async def test_resolve_note_attach_path_honors_camelcase_notes_path(mocker): + """Custom notes folders configured in the Notes app must be honored. + + Regression: the Notes API returns the folder under ``notesPath`` (camelCase, + see ``models/notes.py:43``). An earlier draft of this code looked up + ``notes_path`` (snake_case) and silently fell back to the default ``"Notes"``, + producing 404s for users with a non-default folder. This test pins the + correct key so that bug can't reappear. + """ + client = mocker.AsyncMock() + client.notes.get_settings.return_value = {"notesPath": "Documents/MyNotes"} + client.notes.get_note.return_value = { + "id": 42, + "title": "Q4 Plan", + "category": "Work", + } + + path = await _resolve_note_attach_path(client, note_id=42) + + assert path == "/Documents/MyNotes/Work/Q4 Plan.md" + client.notes.get_settings.assert_awaited_once() + client.notes.get_note.assert_awaited_once_with(42) + + +async def test_resolve_note_attach_path_falls_back_to_default_when_setting_missing( + mocker, +): + """Missing/empty ``notesPath`` falls back to the documented default.""" + client = mocker.AsyncMock() + client.notes.get_settings.return_value = {} + client.notes.get_note.return_value = { + "id": 1, + "title": "Idea", + "category": "", + } + + path = await _resolve_note_attach_path(client, note_id=1) + + assert path == "/Notes/Idea.md" + + +async def test_resolve_note_attach_path_handles_null_category(mocker): + """A note with ``category=None`` (rather than ``""``) must not crash.""" + client = mocker.AsyncMock() + client.notes.get_settings.return_value = {"notesPath": "Notes"} + client.notes.get_note.return_value = { + "id": 7, + "title": "Bare", + "category": None, + } + + path = await _resolve_note_attach_path(client, note_id=7) + + assert path == "/Notes/Bare.md"