fix: address PR review feedback (round 9)

- Fix emoji clearing bug: use _UNSET sentinel in update_collective so
  emoji=None sends {"emoji": null} instead of raising ValueError
- Move collectives_get_trashed_collectives to Read Tools section
- Remove redundant is_trash field from ListTrashedPagesResponse
- Add page lifecycle note to collectives_trash_page docstring
- Add unit test for clearing collective emoji via update_collective
- Add integration test for clearing collective emoji via MCP tool

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

nextcloud_mcp_server/client/collectives.py

@@ -9,6 +9,9 @@ logger = logging.getLogger(__name__)
 
 API_BASE = "/ocs/v2.php/apps/collectives/api/v1.0"
 
+_UNSET = object()
+"""Sentinel to distinguish 'not provided' from an explicit None."""
+
 
 class OCSError(Exception):
     """Error returned in the OCS response envelope."""
@@ -75,15 +78,18 @@ class CollectivesClient(BaseNextcloudClient):
         return data["collective"]
 
     async def update_collective(
-        self, collective_id: int, emoji: str | None = None
+        self, collective_id: int, emoji: str | None | object = _UNSET
     ) -> dict[str, Any]:
         """Update a collective (emoji).
 
+        Pass emoji=None to clear the emoji. Omit emoji entirely to leave
+        it unchanged.
+
         Raises:
             ValueError: If no fields are provided to update.
         """
         json_data: dict[str, Any] = {}
-        if emoji is not None:
+        if emoji is not _UNSET:
             json_data["emoji"] = emoji
         if not json_data:
             raise ValueError("At least one field must be provided to update")

nextcloud_mcp_server/models/collectives.py

@@ -135,10 +135,6 @@ class SearchPagesResponse(BaseResponse):
 class ListTrashedPagesResponse(ListPagesResponse):
     """Response for listing trashed pages in a collective."""
 
-    is_trash: bool = Field(
-        default=True, description="Indicates these are trashed pages"
-    )
-
 
 class ListTrashedCollectivesResponse(BaseResponse):
     """Response for listing trashed collectives."""

nextcloud_mcp_server/server/collectives.py

@@ -208,6 +208,30 @@ def configure_collectives_tools(mcp: FastMCP):
             pages=pages, total=len(pages), collective_id=collective_id
         )
 
+    @mcp.tool(
+        title="List Trashed Collectives",
+        annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True),
+    )
+    @require_scopes("collectives:read")
+    @instrument_tool
+    async def collectives_get_trashed_collectives(
+        ctx: Context,
+    ) -> ListTrashedCollectivesResponse:
+        """List all trashed Nextcloud Collectives.
+
+        Returns collectives that have been soft-deleted and can be restored
+        or permanently deleted.
+        """
+        client = await get_client(ctx)
+        try:
+            raw = await client.collectives.get_trashed_collectives()
+        except (OCSError, HTTPStatusError) as e:
+            raise _handle_collectives_error(e) from e
+        collectives = [Collective(**c) for c in raw]
+        return ListTrashedCollectivesResponse(
+            collectives=collectives, total=len(collectives)
+        )
+
     # --- Write Tools ---
 
     @mcp.tool(
@@ -325,30 +349,6 @@ def configure_collectives_tools(mcp: FastMCP):
             message="Collective permanently deleted",
         )
 
-    @mcp.tool(
-        title="List Trashed Collectives",
-        annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True),
-    )
-    @require_scopes("collectives:read")
-    @instrument_tool
-    async def collectives_get_trashed_collectives(
-        ctx: Context,
-    ) -> ListTrashedCollectivesResponse:
-        """List all trashed Nextcloud Collectives.
-
-        Returns collectives that have been soft-deleted and can be restored
-        or permanently deleted.
-        """
-        client = await get_client(ctx)
-        try:
-            raw = await client.collectives.get_trashed_collectives()
-        except (OCSError, HTTPStatusError) as e:
-            raise _handle_collectives_error(e) from e
-        collectives = [Collective(**c) for c in raw]
-        return ListTrashedCollectivesResponse(
-            collectives=collectives, total=len(collectives)
-        )
-
     @mcp.tool(
         title="Restore Collective",
         annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True),
@@ -458,7 +458,11 @@ def configure_collectives_tools(mcp: FastMCP):
     async def collectives_trash_page(
         ctx: Context, collective_id: int, page_id: int
     ) -> PageOperationResponse:
-        """Move a page to trash in a Nextcloud Collective (soft delete)
+        """Move a page to trash in a Nextcloud Collective (soft delete).
+
+        Trashed pages can be restored with collectives_restore_page. The
+        Collectives API does not support permanent page deletion; trashed
+        pages are cleaned up by Nextcloud's retention policy.
 
         Args:
             collective_id: ID of the collective

tests/client/collectives/test_collectives_api.py

@@ -461,10 +461,28 @@ async def test_get_page_404(mocker):
 async def test_update_collective_no_fields_raises_value_error(mocker):
     """Test that update_collective raises ValueError when called with no fields."""
     client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser")
+    # Omitting emoji entirely (not passing it) should raise
     with pytest.raises(ValueError, match="At least one field"):
         await client.update_collective(collective_id=1)
 
 
+async def test_update_collective_clear_emoji(mocker):
+    """Test that update_collective sends null emoji to clear it."""
+    mock_response = _ocs_response(
+        {"collective": _sample_collective(1, "Test Wiki", emoji=None)}
+    )
+    mock_request = mocker.patch.object(
+        CollectivesClient, "_make_request", return_value=mock_response
+    )
+
+    client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser")
+    result = await client.update_collective(collective_id=1, emoji=None)
+
+    assert result["emoji"] is None
+    call_args = mock_request.call_args
+    assert call_args[1]["json"] == {"emoji": None}
+
+
 async def test_set_page_emoji_clear(mocker):
     """Test that set_page_emoji sends null emoji to clear it."""
     page = _sample_page(10, "Test Page")

tests/server/test_collectives_mcp.py

@@ -134,6 +134,30 @@ async def test_collectives_set_collective_emoji(
     logger.info("Collective emoji updated")
 
 
+async def test_collectives_clear_collective_emoji(
+    nc_mcp_client: ClientSession, temporary_collective: dict
+):
+    """Test clearing a collective's emoji by passing null."""
+    cid = temporary_collective["id"]
+
+    # Set an emoji first
+    set_result = await nc_mcp_client.call_tool(
+        "collectives_set_collective_emoji",
+        {"collective_id": cid, "emoji": "🔬"},
+    )
+    assert set_result.isError is False
+
+    # Clear the emoji by passing null
+    clear_result = await nc_mcp_client.call_tool(
+        "collectives_set_collective_emoji",
+        {"collective_id": cid, "emoji": None},
+    )
+    assert clear_result.isError is False
+    data = json.loads(clear_result.content[0].text)
+    assert data["collective_id"] == cid
+    logger.info("Collective emoji cleared")
+
+
 # --- Page CRUD ---