From 85119bde91f950b6c92a66aa6dd1d465974d5fc3 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 26 Mar 2026 14:38:11 +0100 Subject: [PATCH] fix: address PR review feedback (round 6) - Fix assign_tag sending Content-Type header with no body - Mark collectives_update_collective as idempotent (no ETag involved) - Raise OCSError when 'data' key missing instead of silent fallback - Tighten color validator to 3 or 6 hex chars only - Add comment explaining null emoji semantics in set_page_emoji Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/client/collectives.py | 7 +++++-- nextcloud_mcp_server/models/collectives.py | 2 +- nextcloud_mcp_server/server/collectives.py | 2 +- tests/client/collectives/test_collectives_api.py | 8 +++----- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index 5a3a44f7..6fd140bb 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -44,7 +44,9 @@ class CollectivesClient(BaseNextcloudClient): if status_code >= 400: message = meta.get("message", "OCS error") raise OCSError(status_code, message) - return ocs.get("data", {}) + if "data" not in ocs: + raise OCSError(500, "OCS response missing 'data' field") + return ocs["data"] # Collectives @@ -189,6 +191,7 @@ class CollectivesClient(BaseNextcloudClient): self, collective_id: int, page_id: int, emoji: str | None ) -> dict[str, Any]: """Set or clear the emoji on a page.""" + # Sending {"emoji": null} intentionally clears the emoji on the server json_data = {"emoji": emoji} response = await self._make_request( "PUT", @@ -245,7 +248,7 @@ class CollectivesClient(BaseNextcloudClient): response = await self._make_request( "PUT", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/tags/{tag_id}", - headers=self._OCS_HEADERS_JSON, + headers=self._OCS_HEADERS, ) self._unwrap_ocs(response.json()) diff --git a/nextcloud_mcp_server/models/collectives.py b/nextcloud_mcp_server/models/collectives.py index 03eafafe..25259407 100644 --- a/nextcloud_mcp_server/models/collectives.py +++ b/nextcloud_mcp_server/models/collectives.py @@ -60,7 +60,7 @@ class CollectiveTag(BaseModel): @field_validator("color") @classmethod def validate_hex_color(cls, v: str) -> str: - if not re.fullmatch(r"[0-9A-Fa-f]{3,8}", v): + if not re.fullmatch(r"[0-9A-Fa-f]{3}(?:[0-9A-Fa-f]{3})?", v): raise ValueError(f"Invalid hex color: {v!r}") return v diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index 540993f7..44eba34d 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -236,7 +236,7 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Update Collective", - annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), ) @require_scopes("collectives:write") @instrument_tool diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py index 98c712f6..e6865c59 100644 --- a/tests/client/collectives/test_collectives_api.py +++ b/tests/client/collectives/test_collectives_api.py @@ -358,8 +358,8 @@ async def test_restore_page(mocker): # --- Error Handling --- -async def test_ocs_missing_data_raises_key_error(mocker): - """Test that OCS envelope without 'data' key causes KeyError on field access.""" +async def test_ocs_missing_data_raises_ocs_error(mocker): + """Test that OCS envelope without 'data' key raises OCSError.""" mock_response = create_mock_response( status_code=200, json_data={ @@ -371,9 +371,7 @@ async def test_ocs_missing_data_raises_key_error(mocker): mocker.patch.object(CollectivesClient, "_make_request", return_value=mock_response) client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") - # _unwrap_ocs returns {} when "data" is absent; the caller then - # raises KeyError when accessing the expected key (e.g. "collectives") - with pytest.raises(KeyError): + with pytest.raises(OCSError, match="missing 'data' field"): await client.get_collectives()