feat: add Nextcloud Collectives app support

[cbcoutinho]

Summary

• Implements MCP tools for the Nextcloud Collectives wiki/documentation app (20 tools)
• Enables agentic workflows for team knowledge base management: document retrieval, creation, search, tagging, and trash management
• Page content (markdown) is fetched via WebDAV, composing the path from the Collectives API metadata

Tools

Read (8): list collectives, list/get pages (with WebDAV content), search pages, list tags, list trashed pages, list trashed collectives

Write (12): create/set-emoji collective, trash/delete/restore collective, create/move/trash/restore pages, set page emoji, create/assign/remove tags

Changes

• app-hooks/post-installation/10-install-collectives-app.sh — Docker hook (enables Circles dep + installs Collectives)
• nextcloud_mcp_server/client/collectives.py — OCS API client with envelope unwrapping
• nextcloud_mcp_server/models/collectives.py — Domain + response Pydantic models
• nextcloud_mcp_server/server/collectives.py — 20 MCP tool definitions with annotations and scopes
• nextcloud_mcp_server/models/auth.py — Added collectives:read/collectives:write scopes
• Registration in client/__init__.py, server/__init__.py, app.py

Other changes

• nextcloud_mcp_server/server/deck.py — Removed destructiveHint=True from "Remove Label from Deck Card" (reversible operation)
• tests/server/test_annotations.py — Updated destructive keyword list to exclude "remove"; added collectives_delete_collective exception for idempotency check
• pyproject.toml — Added pytest-otel>=2.0.1 dev dependency (intentional: enables OpenTelemetry trace export from test runs for observability infrastructure)

Test plan

• 26 unit tests (mocked OCS responses) — tests/client/collectives/test_collectives_api.py
• Integration tests (end-to-end via MCP client) — tests/server/test_collectives_mcp.py
• All existing unit tests pass
• ruff check/format clean
• ty type check clean
• CI pipeline

Closes #621

---

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

🤖 Generated with Claude Code

[claude]

Code Review: feat: add Nextcloud Collectives app support

Overall this is well-structured and follows the project conventions closely. The 16 tools are correctly annotated, scoped, and wired up. A few issues worth addressing before merging.

---

Issues

1. Silent failure in trash_collective and trash_page (client/collectives.py)

Both methods call _make_request but never call _unwrap_ocs. If Nextcloud returns HTTP 200 with an OCS error envelope (statuscode >= 400), the error is silently swallowed. Both should capture the response and call self._unwrap_ocs(response.json()), consistent with all other mutating methods.

2. _unwrap_ocs raises bare KeyError on non-OCS responses

The line ocs = response_json["ocs"] raises an untyped KeyError if the response is not an OCS envelope (e.g. a proxy error page). Consider using .get("ocs") and raising OCSError(500, "Response is not an OCS envelope") for a more informative error.

3. Docstring mismatch in collectives_update_collective (server/collectives.py)

The Args section says "emoji: New emoji for the collective (required)" but the signature has emoji: str | None = None. The parameter is optional.

---

Minor Issues

4. Unnecessary _get_ocs_headers() indirection

_get_ocs_headers() simply returns self._OCS_HEADERS. Every call site could reference self._OCS_HEADERS directly - the wrapper method adds no value.

5. Content-Type: application/json on GET requests

_OCS_HEADERS includes Content-Type: application/json and is sent on every request including GETs that have no body. Harmless in practice but semantically wrong.

6. Unrelated pyproject.toml change

pytest-otel>=2.0.1 was added to dev dependencies but appears unrelated to Collectives support. Accidental inclusion from another branch?

7. Misleading test name

test_ocs_missing_data_returns_empty - the body asserts a KeyError is raised, which is the opposite of what the name implies. Consider renaming to test_ocs_missing_data_raises_key_error, and consider whether this behavior is intentional.

---

What's Good

• OCS envelope unwrapping with typed OCSError is clean and reusable
• WebDAV content fetching in collectives_get_page with graceful degradation on failure
• Annotations are correct throughout: destructiveHint=True on soft-delete (trash), idempotentHint=True on permanent delete, consistent with ADR-017
• The test_annotations.py fix removing "remove" from destructive_keywords is a good general improvement (removing tag/label associations is reversible and should not be flagged as destructive)
• Comprehensive unit test helpers make the test suite easy to extend
• The app-hook script correctly handles all three installation paths

---

Issues 1 and 2 are the most important as they can cause silent failures or confusing errors in production. The rest are low-priority cleanup items.

[claude]

Code Review

This is a solid, well-structured addition that follows the project's conventions closely. The OCS envelope abstraction, error handling, Pydantic models, tool annotations, and test coverage are all good. A few issues worth addressing:

---

Bug: assign_tag sends Content-Type: application/json with no body

In client/collectives.py, assign_tag uses _OCS_HEADERS_JSON but passes no json= argument to _make_request. This sends Content-Type: application/json with an empty body, which could confuse some server implementations. Should use self._OCS_HEADERS instead.

---

Annotation: collectives_update_collective should be idempotentHint=True

The current annotation is idempotentHint=False. Unlike ETag-based updates where the input changes after each call, setting the same emoji twice produces the same end state — that is idempotent. This differs from notes/calendar tools where ETag is part of the input.

---

Robustness: _unwrap_ocs silent fallback on missing data key

ocs.get("data", {}) silently returns {} when the key is absent, and test_ocs_missing_data_raises_key_error documents this as expected behavior. The caller then gets an opaque KeyError rather than a clear OCSError. Consider raising OCSError(500, "OCS response missing 'data' field") directly in _unwrap_ocs for clearer debugging.

---

Minor: CollectiveTag.color validator is too permissive

r"[0-9A-Fa-f]{3,8}" allows 4, 5, 7, and 8-character values which are not valid CSS hex colors. The Collectives API likely only accepts 3 or 6 characters. Consider r"[0-9A-Fa-f]{3}(?:[0-9A-Fa-f]{3})?" (3 or 6 only).

---

Observation: set_page_emoji(emoji=None) sends {"emoji": null}

This is intentional to clear an emoji, but worth a brief comment since null JSON handling varies across APIs. Worth adding an integration test for the clear-emoji case if not already present.

---

Positives

• OCS envelope abstraction (_unwrap_ocs, OCSError) is clean and reusable
• WebDAV content fetch in collectives_get_page with graceful degradation is well done
• Tool annotations accurately reflect semantics per ADR-017 — the trash vs permanent delete distinction is especially good
• The deck.py fix removing destructiveHint=True from label removal is correct (tag/label de-association is reversible)
• 16 unit tests + 10 integration tests with session-scoped fixture and proper cleanup is thorough
• Error conversion via _handle_collectives_error is consistent with other server modules

[claude]

Nextcloud Webhook Testing Findings

Date: 2025-11-11
Purpose: Manual validation of Nextcloud webhook schemas and behavior for vector sync integration (ADR-010)

Executive Summary

Successfully tested and validated Nextcloud webhook payloads for file/note events and calendar events. 5 out of 6 webhook types were captured and validated against expected schemas from ADR-010 and Nextcloud documentation. One calendar deletion webhook did not fire during testing (potential Nextcloud issue or configuration).

Test Environment

• Nextcloud Version: 30+ (Docker compose setup)
• Webhook App: webhook_listeners (bundled, enabled)
• MCP Server: Test endpoint at http://mcp:8000/webhooks/nextcloud
• Background Worker: Running with 60s timeout
• Authentication: None (test environment)

Webhooks Registered

ID	Event Class	Status
1	OCP\Files\Events\Node\NodeCreatedEvent	✓ Tested
2	OCP\Files\Events\Node\NodeWrittenEvent	✓ Tested
3	OCP\Files\Events\Node\NodeDeletedEvent	✓ Tested
4	OCP\Calendar\Events\CalendarObjectCreatedEvent	✓ Tested
5	OCP\Calendar\Events\CalendarObjectUpdatedEvent	✓ Tested
6	OCP\Calendar\Events\CalendarObjectDeletedEvent	✗ Not received

Captured Webhook Payloads

1. NodeCreatedEvent (File/Note Creation)

Test Action: Created note via Notes API
Trigger Time: 2025-11-11 08:37:25
Webhooks Fired: 3 events (folder creation + file creation + file written)

Payload:

{
  "user": {
    "uid": "admin",
    "displayName": "admin"
  },
  "time": 1762850245,
  "event": {
    "class": "OCP\\Files\\Events\\Node\\NodeCreatedEvent",
    "node": {
      "id": 437,
      "path": "/admin/files/Notes/Webhooks/Webhook Test Note.md"
    }
  }
}

Validation:

• ✅ Schema matches ADR-010 specification
• ✅ Contains user object with uid and displayName
• ✅ Contains time (Unix timestamp)
• ✅ Contains event.class (fully qualified event name)
• ✅ Contains event.node.id (file ID)
• ✅ Contains event.node.path (absolute path)

Observations:

• Creating a note via Notes API triggers 3 webhook events:
  1. NodeCreatedEvent for the parent folder (if new)
  2. NodeWrittenEvent for the parent folder
  3. NodeCreatedEvent for the actual file
  4. NodeWrittenEvent for the file (sometimes fired 2x)

2. NodeWrittenEvent (File/Note Update)

Test Action: Updated note content via Notes API
Trigger Time: 2025-11-11 08:49:20

Payload:

{
  "user": {
    "uid": "admin",
    "displayName": "admin"
  },
  "time": 1762850960,
  "event": {
    "class": "OCP\\Files\\Events\\Node\\NodeWrittenEvent",
    "node": {
      "id": 437,
      "path": "/admin/files/Notes/Webhooks/Webhook Test Note.md"
    }
  }
}

Validation:

• ✅ Schema identical to NodeCreatedEvent except for event.class
• ✅ Same file ID (437) as creation event
• ✅ Updated timestamp reflects actual modification time

Observations:

• File updates trigger a single NodeWrittenEvent
• No duplicate events fired for update operations

3. NodeDeletedEvent (File/Note Deletion)

Test Action: Deleted note via Notes API
Trigger Time: 2025-11-11 08:51:34
Webhooks Fired: 2 events (file + folder deletion)

Payload:

{
  "user": {
    "uid": "admin",
    "displayName": "admin"
  },
  "time": 1762851093,
  "event": {
    "class": "OCP\\Files\\Events\\Node\\NodeDeletedEvent",
    "node": {
      "path": "/admin/files/Notes/Webhooks/Webhook Test Note.md"
    }
  }
}

Validation:

• ✅ Schema matches ADR-010 specification
• ⚠️ IMPORTANT: No node.id field in deletion events (only path)
• ✅ Folder deletion triggered after file deletion (empty folder cleanup)

Observations:

• Critical Difference: Deletion events do NOT include node.id, only node.path
• This differs from Create/Write events which include both id and path
• ADR-010 implementation must handle missing id field for deletions
• Deleting a file also triggers deletion of empty parent folders

4. CalendarObjectCreatedEvent (Calendar Event Creation)

Test Action: Created calendar event via CalDAV PUT
Trigger Time: 2025-11-11 08:52:50

Payload (partial - calendarData omitted for brevity):

{
  "user": {
    "uid": "admin",
    "displayName": "admin"
  },
  "time": 1762851169,
  "event": {
    "calendarId": 1,
    "class": "OCP\\Calendar\\Events\\CalendarObjectCreatedEvent",
    "calendarData": {
      "id": 1,
      "uri": "personal",
      "{http://calendarserver.org/ns/}getctag": "...",
      "{http://sabredav.org/ns}sync-token": 21,
      "{urn:ietf:params:xml:ns:caldav}supported-calendar-component-set": [],
      "{urn:ietf:params:xml:ns:caldav}schedule-calendar-transp": [],
      "{urn:ietf:params:xml:ns:caldav}calendar-timezone": null
    },
    "objectData": {
      "id": 3,
      "uri": "webhook-test-event-001.ics",
      "lastmodified": 1762851169,
      "etag": "\"2b937b7d77dc83c77329dfdb210ba9d0\"",
      "calendarid": 1,
      "size": 297,
      "component": "vevent",
      "classification": 0,
      "uid": "webhook-test-event-001@nextcloud",
      "calendardata": "BEGIN:VCALENDAR\r\nVERSION:2.0\r\n...",
      "{http://nextcloud.com/ns}deleted-at": null
    },
    "shares": []
  }
}

Validation:

• ✅ Schema matches Nextcloud documentation
• ✅ Contains complete calendar metadata (calendarData)
• ✅ Contains complete event data (objectData)
• ✅ Includes full iCal data in objectData.calendardata
• ✅ Includes objectData.id for database lookups
• ⚠️ Complex: Much more metadata than file events

Observations:

• Calendar webhooks include significantly more data than file webhooks
• Full iCal content is embedded in objectData.calendardata
• Event ID is in objectData.id (NOT event.id)
• calendarData contains calendar-level metadata
• shares array contains sharing information (empty in this test)

5. CalendarObjectUpdatedEvent (Calendar Event Update)

Test Action: Updated calendar event via CalDAV PUT
Trigger Time: 2025-11-11 08:53:28

Payload (partial):

{
  "user": {
    "uid": "admin",
    "displayName": "admin"
  },
  "time": 1762851207,
  "event": {
    "calendarId": 1,
    "class": "OCP\\Calendar\\Events\\CalendarObjectUpdatedEvent",
    "calendarData": { /* same structure as creation */ },
    "objectData": {
      "id": 3,
      "uri": "webhook-test-event-001.ics",
      "lastmodified": 1762851207,
      "etag": "\"2695a18013e0991e4212b07b61d5e1e2\"",
      "calendarid": 1,
      "size": 315,
      "component": "vevent",
      "classification": 0,
      "uid": "webhook-test-event-001@nextcloud",
      "calendardata": "BEGIN:VCALENDAR\r\nVERSION:2.0\r\n...",
      "{http://nextcloud.com/ns}deleted-at": null
    },
    "shares": []
  }
}

Validation:

• ✅ Schema identical to CalendarObjectCreatedEvent except event.class
• ✅ Same event ID (3) as creation
• ✅ Updated lastmodified timestamp
• ✅ Different etag (changed from creation)
• ✅ Larger size (315 vs 297 bytes)

Observations:

• Update events contain full new state (not delta)
• ETag changes on updates (useful for conflict detection)
• Size field reflects actual iCal size

6. CalendarObjectDeletedEvent (Calendar Event Deletion)

Test Action: Deleted calendar event via CalDAV DELETE
Trigger Time: 2025-11-11 08:54:47
Status: ❌ WEBHOOK DID NOT FIRE

Expected Payload (from Nextcloud docs):

{
  "user": {
    "uid": "admin",
    "displayName": "admin"
  },
  "time": <timestamp>,
  "event": {
    "calendarId": 1,
    "class": "OCP\\Calendar\\Events\\CalendarObjectDeletedEvent",
    "calendarData": { /* calendar metadata */ },
    "objectData": {
      "id": 3,
      "uri": "webhook-test-event-001.ics",
      /* ... other fields ... */
    },
    "shares": []
  }
}

Issue:

• Calendar event was successfully deleted (verified via CalDAV PROPFIND)
• Webhook registration confirmed (ID Dependency Dashboard #6 in webhook_listeners:list)
• Background worker running and processing other events
• No webhook notification received after 2+ minutes

Possible Causes:

1. Known Nextcloud bug with calendar deletion webhooks
2. CalDAV DELETE may not trigger event system properly
3. Deletion event may require trash bin enabled
4. Background job may have silently failed

Recommended Actions:

• File Nextcloud issue report
• Test with trash bin enabled (CalendarObjectMovedToTrashEvent)
• Check Nextcloud error logs for webhook failures
• Verify with Nextcloud 31+ if issue persists

Schema Comparison: Expected vs Actual

File Events

Field	Expected (ADR-010)	Actual	Match
user.uid	string	string	✅
user.displayName	string	string	✅
time	int	int	✅
event.class	string	string	✅
event.node.id	string	int	⚠️ Type mismatch
event.node.path	string	string	✅

Type Discrepancy: node.id is documented as string but returns as int (437 instead of "437")

Calendar Events

Field	Expected (Nextcloud docs)	Actual	Match
user.uid	string	string	✅
user.displayName	string	string	✅
time	int	int	✅
event.class	string	string	✅
event.calendarId	int	int	✅
event.calendarData.*	object	object	✅
event.objectData.id	int	int	✅
event.objectData.uri	string	string	✅
event.objectData.calendardata	string	string	✅
event.objectData.lastmodified	int	int	✅
event.objectData.etag	string	string	✅
event.objectData.component	string|null	string	✅
event.shares	array	array	✅

All calendar event fields match expected schemas.

Key Findings for ADR-010 Implementation

1. Deletion Events Have Different Schema

• File Deletions: No node.id field, only node.path
• Calendar Deletions: Not tested (webhook didn't fire)
• Impact: Webhook handler must check for node.id existence before using it

2. Multiple Webhooks Per Operation

• Creating a note triggers 3-5 webhook events
• Deleting a note triggers 2 events (file + folder)
• Impact: Deduplication logic needed in webhook handler

3. Event-Specific ID Fields

• File events: event.node.id
• Calendar events: event.objectData.id
• Impact: Event parser must handle different ID field locations

4. Full State vs Delta

• All webhooks contain complete current state (not delta)
• Impact: No need for "previous state" tracking in webhook handler

5. Calendar Data Richness

• Calendar webhooks include full iCal content
• Impact: Can extract all event metadata without additional API calls

Recommendations for ADR-010 Implementation

1. Webhook Event Parser (webhook_parser.py)

def extract_document_task(event_class: str, payload: dict) -> DocumentTask | None:
    """Extract DocumentTask from webhook event payload."""
    user_id = payload["user"]["uid"]
    event_data = payload["event"]

    # File/Note events
    if "NodeCreatedEvent" in event_class or "NodeWrittenEvent" in event_class:
        path = event_data["node"]["path"]

        # Only process markdown files for notes
        if not path.endswith(".md"):
            return None

        # IMPORTANT: Check if 'id' exists (missing in deletion events)
        doc_id = str(event_data["node"].get("id", ""))
        if not doc_id:
            # For missing ID, use path-based identifier
            doc_id = f"path:{path}"

        return DocumentTask(
            user_id=user_id,
            doc_id=doc_id,
            doc_type="note",
            operation="index",
            modified_at=payload["time"],
        )

    # File deletion events
    elif "NodeDeletedEvent" in event_class:
        path = event_data["node"]["path"]

        if not path.endswith(".md"):
            return None

        # Deletion events DON'T have node.id - use path
        return DocumentTask(
            user_id=user_id,
            doc_id=f"path:{path}",  # Path-based since ID unavailable
            doc_type="note",
            operation="delete",
            modified_at=payload["time"],
        )

    # Calendar creation/update events
    elif "CalendarObjectCreatedEvent" in event_class or \
         "CalendarObjectUpdatedEvent" in event_class:
        return DocumentTask(
            user_id=user_id,
            doc_id=str(event_data["objectData"]["id"]),
            doc_type="calendar_event",
            operation="index",
            modified_at=event_data["objectData"]["lastmodified"],
        )

    # Calendar deletion events
    elif "CalendarObjectDeletedEvent" in event_class:
        return DocumentTask(
            user_id=user_id,
            doc_id=str(event_data["objectData"]["id"]),
            doc_type="calendar_event",
            operation="delete",
            modified_at=payload["time"],
        )

    return None  # Unsupported event type

2. Deduplication Strategy

Problem: Creating a note triggers 3-5 webhooks
Solution: Idempotent processing + task deduplication

# In webhook handler
async def handle_nextcloud_webhook(request: Request) -> JSONResponse:
    payload = await request.json()

    task = extract_document_task(
        payload["event"]["class"],
        payload
    )

    if task:
        # Idempotent: Queue will only process latest version
        await document_queue.send(task)

    return JSONResponse({"status": "received"}, status_code=200)

3. Path-Based Fallback for Deletions

Since deletion events lack node.id, use path-based identification:

# In Qdrant delete logic
async def delete_document(user_id: str, doc_id: str, doc_type: str):
    if doc_id.startswith("path:"):
        # Path-based deletion
        path = doc_id.removeprefix("path:")
        # Search Qdrant for document with matching path in metadata
        points = await qdrant.scroll(
            collection_name=collection,
            scroll_filter=Filter(must=[
                FieldCondition(
                    key="user_id",
                    match=MatchValue(value=user_id),
                ),
                FieldCondition(
                    key="metadata.path",
                    match=MatchValue(value=path),
                ),
            ]),
        )
        # Delete found points
    else:
        # ID-based deletion (normal case)
        ...

4. Webhook Registration Filters

To reduce webhook volume, add filters:

{
  "httpMethod": "POST",
  "uri": "http://mcp:8000/webhooks/nextcloud",
  "event": "OCP\\Files\\Events\\Node\\NodeCreatedEvent",
  "eventFilter": {
    "event.node.path": "/^.*\\.md$/"
  }
}

This filters to only .md files at the webhook registration level (not handler level).

5. Monitoring and Metrics

Add webhook-specific metrics:

webhook_notifications_received_total{event_type="note_created"} 42
webhook_processing_duration_seconds{event_type="note_created"} 0.023
webhook_errors_total{error_type="parse_error"} 2
webhook_duplicates_filtered_total{doc_type="note"} 15

Testing Checklist for Implementation

• File creation webhook triggers document indexing
• File update webhook triggers reindexing
• File deletion webhook triggers document removal
• File deletion without ID successfully removes document (path-based)
• Calendar creation webhook triggers event indexing
• Calendar update webhook triggers event reindexing
• Calendar deletion webhook triggers event removal (NOT TESTED - webhook didn't fire)
• Duplicate webhooks are deduplicated
• Non-markdown file webhooks are ignored
• Malformed webhook payloads return 400 error
• Webhook authentication validates shared secret
• Webhook processing completes within 50ms

Appendix: Raw Webhook Logs

Complete webhook logs with full payloads are available in MCP container logs:

docker compose logs mcp | grep -A 30 "🔔 Webhook received"

Conclusion

Nextcloud webhooks work as documented with minor exceptions:

1. ✅ File/Note Events: Fully functional and match expected schemas
2. ✅ Calendar Creation/Update: Fully functional with rich metadata
3. ❌ Calendar Deletion: Webhook did not fire (requires investigation)
4. ⚠️ Schema Discrepancy: node.id is integer (not string as documented)
5. ⚠️ Deletion Schema: Missing node.id field (only path provided)

Overall Status: Ready for ADR-010 implementation with noted caveats. Calendar deletion webhook issue should be reported to Nextcloud and may require alternative approach (polling or trash bin events).

[claude]

test body for approval check

[claude]

Code Review - Part 1: Bugs / Correctness

1. Unhandled UnicodeDecodeError in collectives_get_page (server/collectives.py): The except clause catches (HTTPStatusError, OSError) but not UnicodeDecodeError. If a file contains non-UTF-8 bytes, this bubbles up as an unhandled exception rather than gracefully falling back to content = None. Add UnicodeDecodeError to the caught exceptions.

2. Non-standard error code in _handle_collectives_error (server/collectives.py): Using code=-1 in McpError is non-standard. Other tools use standard JSON-RPC codes (e.g. -32603 for INTERNAL_ERROR). Also, OCSError.status_code (403, 404, etc.) is silently dropped - exposing it in the message or mapping to a standard code would help callers diagnose problems.

[claude]

Code Review - Part 2: Unrelated Changes, Annotation Concern

3. pytest-otel added to dev dependencies (pyproject.toml + uv.lock): This OpenTelemetry pytest exporter appears unrelated to Collectives support and pulls in OTLP/gRPC transitive deps. If intentional, it deserves its own PR.

4. deck.py: destructiveHint=True removed from Remove Label from Deck Card: Correct fix (label removal is reversible), but unannounced in the PR description. Worth a brief mention.

5. collectives_update_collective naming: idempotentHint=True is correct for emoji-only updates. But consider renaming to collectives_set_collective_emoji - the Collectives API only exposes emoji as an updatable field, and 'Update Collective' implies broader scope.

[claude]

Code Review - Part 3: Test Concerns and Minor Notes

6. Brittle content assertions: assert 'Welcome' in data['content'] in both test_collectives_get_landing_page_content and test_collectives_search depends on the app default landing page template. Could break across Collectives app versions. Consider len(data['content']) > 0 instead.

7. Session-scoped fixture mutated by tests: test_collectives_update_emoji changes the emoji of the shared temporary_collective fixture. Matches patterns elsewhere in the test suite, but makes tests order-dependent.

Minor notes:

• CollectiveTag.validate_hex_color: Rejects hash-prefixed colors. Unit tests use bare hex ('FF0000') matching the API docs. If any Collectives API version returns '#FF0000' format this would cause Pydantic validation errors on real responses - worth verifying against the actual API.
• test_annotations.py change: Excluding 'remove' from destructive keywords is a global change affecting all tools. The comment explains the rationale well, but a per-tool allowlist would be safer long-term.
• No delete_page tool: If permanent page deletion is unsupported by the API, a note in the collectives_trash_page docstring explaining the page lifecycle would help users.

---

Overall: architecture and approach are clean. The UnicodeDecodeError gap and unrelated pytest-otel dependency are the most concrete things to fix before merging.

[claude]

Review: Nextcloud Collectives Support

Good addition overall — the code follows the project's existing patterns well (Pydantic response models, @require_scopes, @instrument_tool, _make_request + error handling). The OCS envelope helper is clean and the test separation (unit vs integration) is correct. A few issues worth addressing before merge:

---

Bugs / Correctness

Inconsistent error code in collectives_set_collective_emoji (server/collectives.py ~line 827):

# Bug: uses HTTP status code 400 instead of JSON-RPC error code
raise McpError(ErrorData(code=400, message=str(e))) from e

The _handle_collectives_error helper consistently uses -32603. Using 400 here is inconsistent and non-standard for JSON-RPC (valid range is -32768 to -32000). Should be:

raise McpError(ErrorData(code=-32603, message=str(e))) from e

collectives_delete_collective idempotency claim: Marked idempotentHint=True, but calling it twice on the same ID will return an OCS error on the second call (resource no longer exists). The end state is the same but the return value differs — arguably idempotentHint=False is more accurate, per the pattern used by notes/calendar delete tools. Worth checking how other delete tools in this codebase annotate this.

---

Missing Feature

No restore_collective tool: You can trash a collective via collectives_trash_collective but there's no corresponding restore. An agent that accidentally trashes a collective has no way to recover it without manual UI intervention. The client-side pattern for pages shows this is feasible — GET /collectives/trash likely lists trashed collectives and PATCH /collectives/trash/{id} would restore. Worth adding or explicitly documenting as a known gap.

---

Annotation Concern

collectives_trash_collective and collectives_trash_page are both marked destructiveHint=True. These are soft deletes (reversible — the pages/collective can be restored). The PR correctly does NOT mark collectives_remove_tag as destructive for the same reason. For consistency, soft-delete trash operations should arguably not carry destructiveHint=True either. MCP clients use this hint to warn users; triggering that warning for a recoverable operation may cause unnecessary friction.

---

Minor Issues

collectives_set_collective_emoji can't clear the emoji: The tool signature takes emoji: str (required), but the underlying CollectivesClient.update_collective() accepts emoji: str | None. This means there's no way to clear a collective's emoji via MCP. collectives_set_page_emoji correctly accepts emoji: str | None = None. Should be consistent.

pytest-otel added to dev dependencies (pyproject.toml): This dependency doesn't appear related to Collectives. Was this intentionally included in this PR?

WebDAV path construction edge case (server/collectives.py): The path join strips slashes correctly with p.strip("/"), but if collectivePath already has URL-encoded characters or special chars in collective/file names, they might not match what WebDAV expects. Worth a comment noting this assumption, or a test with a collective name containing spaces.

---

Test Coverage Gaps

• No unit test for update_collective raising ValueError when called with no fields (the guard in the client method).
• No test for set_page_emoji(emoji=None) to verify the clear-emoji path.
• collectives_delete_collective is only exercised in fixture cleanup, not a dedicated test. If cleanup fails, the permanent-delete path is never validated.
• Integration test test_collectives_move_page doesn't verify the page has the new title after the move — only checks data["page_id"] and the message string.

---

Positive Notes

• OCS envelope unwrapping with _unwrap_ocs is clean and well-tested (covers missing data key, non-OCS responses, error statuscode in meta).
• The deck.py fix removing destructiveHint=True from Remove Label from Deck Card is correct and the accompanying test_annotations.py change is the right way to enforce this.
• WebDAV content fetch gracefully degrades (returns None and logs a warning) rather than failing the whole tool call — good resilience.
• The temporary_collective fixture correctly handles session-scoped cleanup even if tests fail mid-way.

[claude]

Code Review

Overall this is a well-structured addition that follows existing patterns faithfully (BaseNextcloudClient, Pydantic response models, ADR-017 annotations, scopes, instrument_tool). A few issues to address before merging:

---

Bug: clearing a collective emoji silently raises

In client/collectives.py, update_collective skips emoji from the payload when it is None -- but the collectives_set_collective_emoji tool docstring says "Pass emoji=None to clear the emoji."

Calling collectives_set_collective_emoji(collective_id=X, emoji=None) raises ValueError before hitting the API because update_collective only adds emoji to json_data when emoji is not None. The server layer then converts the ValueError to a McpError, so the caller sees an error instead of a cleared emoji.

Compare set_page_emoji, which correctly sends null unconditionally. Fix: apply the same pattern to update_collective, or update the docstring if clearing is intentionally unsupported.

---

Accidental dependency: pytest-otel

pyproject.toml and uv.lock gain pytest-otel>=2.0.1. Not mentioned in the PR description or changes list, and it pulls in opentelemetry-exporter-otlp, opentelemetry-exporter-otlp-proto-http, and related packages. Looks unintentional -- worth confirming or removing.

---

Minor: collectives_get_trashed_collectives misplaced in Write section

The function carries readOnlyHint=True but is grouped inside the Write Tools comment block in server/collectives.py. It should sit next to collectives_get_trashed_pages in the Read Tools block.

---

Minor: ListTrashedPagesResponse.is_trash is redundant

The type name already conveys that these are trashed pages. The always-True is_trash field adds noise without carrying information.

---

What is working well

• OCS envelope unwrapping is clean and centralised in _unwrap_ocs; error propagation to McpError is consistent across all tools.
• ADR-017 annotations are correct throughout: read-only hints, idempotency choices, and destructiveHint=True on permanent delete.
• WebDAV content fetch in collectives_get_page degrades gracefully (content=None, warning log) without failing the tool call.
• Both unit tests (OCS mock helpers) and integration tests (full lifecycle) are well-structured. The temporary_collective session fixture with cleanup is the right pattern.
• The deck.py fix removing destructiveHint from label removal is correct -- removing an association is reversible, not destructive.
• The non_idempotent_deletes exception in test_annotations.py is well-reasoned and documented.

The emoji-clearing bug and accidental pytest-otel dependency are the items that need resolution before merge.

[claude]

PR Review: Nextcloud Collectives Support

This is a high-quality, well-structured addition that follows the repository's established patterns. Here are my observations:

Strengths

• Consistent architecture: CollectivesClient correctly extends BaseNextcloudClient, Pydantic models inherit from BaseResponse/StatusResponse, all tools use @require_scopes, @instrument_tool, and ToolAnnotations matching project conventions.
• OCS envelope handling: _unwrap_ocs cleanly separates protocol concerns from business logic. OCSError provides clear error categorization.
• Sentinel pattern for optional updates: _UNSET correctly distinguishes 'not provided' from explicit None in update_collective, and is well-tested.
• WebDAV content composition: Path construction in collectives_get_page using strip('/') on each part and falling back to content=None with a warning log is correct and defensive.
• Test coverage: 26 unit tests covering API parsing, error cases, and request validation, plus integration tests covering full CRUD lifecycles including the trash/restore/delete workflow.

Issues and Questions

1. collectives_delete_collective idempotency annotation — The deviation from ADR-017's general guidance (delete = idempotent) is acknowledged by the non_idempotent_deletes exception in test_annotations.py. The reasoning is sound (calling delete twice errors since the resource is already gone). The docstring could be more explicit that this is the reason for idempotentHint=False.

2. _OCS_HEADERS as a mutable class variable — Treated as constants but are mutable dicts on the class. Low-risk since nothing mutates them, but types.MappingProxyType would make the intent clearer.

3. No delete_tag tool — There is create_tag, assign_tag, and remove_tag (removes tag from page), but no way to permanently delete a tag from the collective. Is this a gap, or does the Collectives API not support tag deletion? If the API supports it, this is a natural addition. If not, a comment noting 'API does not support tag deletion' would help future contributors.

4. ValidationError not caught at the MCP tool layer — CollectiveTag.validate_hex_color will raise a ValidationError when CollectiveTag(**t) is called if the API returns an unexpected color format. This bubbles up as an unhandled exception rather than a clean McpError. The same applies to Collective(**c) and PageInfo(**p) throughout server/collectives.py. Since data comes from the Collectives API rather than user input this is low-risk in practice, but wrapping model construction in a try/except ValidationError would be more defensive.

5. collectives_restore_collective marked idempotentHint=True — If you call restore on a collective that is already restored (not in trash), the API would likely return an error, making this not truly idempotent. This mirrors the existing pattern for other restore operations in the codebase, but worth verifying the API behavior.

Minor Notes

• Integration tests use session-scoped fixtures with try/except cleanup which is the correct pattern.
• The pytest-otel addition as a dev dependency is fine; intent is explained in the PR description.
• The deck.py annotation fix (removing destructiveHint=True from 'Remove Label') is correct.
• Pydantic field names using camelCase (circleId, canEdit, etc.) directly match the API JSON, consistent with how other models in the codebase handle camelCase APIs.

Overall this is close to ready. The main open questions are item 3 (delete_tag gap) and item 4 (ValidationError handling). Great work on the comprehensive test coverage and clean separation of concerns.