fix: address PR review feedback

Address all reviewer comments from PR #387:

1. ✅ Add unit tests for annotations (tests/server/test_annotations.py)
   - 10 comprehensive test functions validating all annotation patterns
   - Tests for titles, read-only, destructive, idempotent operations
   - Validates specific ADR-017 decisions (webdav write, semantic search)
   - Cross-category consistency checks

2. ✅ Fix nc_webdav_write_file idempotency classification
   - Changed from idempotentHint=False to idempotentHint=True
   - Rationale: Uses HTTP PUT without version control
   - Writing same content to same path = same end state (idempotent)

3. ✅ Fix semantic search openWorldHint inconsistency
   - Changed from openWorldHint=False to openWorldHint=True
   - Rationale: Consistent with other Nextcloud tools
   - Nextcloud is external to MCP server (indexed data is implementation detail)

4. ✅ Update ADR-017 with resolved decisions
   - Converted Open Questions to Resolved Questions
   - Added detailed rationale for webdav write and semantic search
   - Updated status from Proposed to Implemented
   - Added decision timeline with dates

5. ✅ Add MCP Tool Annotations guidelines to CLAUDE.md
   - Comprehensive section with code examples for all patterns
   - Key principles documented (idempotency, destructive, open world)
   - References ADR-017 for detailed rationale

All OAuth tools verified to have proper annotations (oauth_tools.py lines 686-751).

CLAUDE.md

@@ -56,6 +56,68 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co
   - Pass-through (default): Simple, stateless (ENABLE_TOKEN_EXCHANGE=false)
   - Token exchange (opt-in): RFC 8693 delegation (ENABLE_TOKEN_EXCHANGE=true)
 
+### MCP Tool Annotations (ADR-017)
+
+**All tools MUST include annotations** following these patterns:
+
+```python
+from mcp.types import ToolAnnotations
+
+# Read-only tools (list, search, get)
+@mcp.tool(
+    title="Human Readable Name",
+    annotations=ToolAnnotations(
+        readOnlyHint=True,
+        openWorldHint=True,  # Nextcloud is external to MCP server
+    ),
+)
+
+# Create operations
+@mcp.tool(
+    title="Create Resource",
+    annotations=ToolAnnotations(
+        idempotentHint=False,  # Creates new resources each time
+        openWorldHint=True,
+    ),
+)
+
+# Update operations (with etag/version control)
+@mcp.tool(
+    title="Update Resource",
+    annotations=ToolAnnotations(
+        idempotentHint=False,  # ETag changes = different inputs
+        openWorldHint=True,
+    ),
+)
+
+# Delete operations
+@mcp.tool(
+    title="Delete Resource",
+    annotations=ToolAnnotations(
+        destructiveHint=True,   # Permanently deletes data
+        idempotentHint=True,    # Same end state if called repeatedly
+        openWorldHint=True,
+    ),
+)
+
+# HTTP PUT without version control (special case)
+@mcp.tool(
+    title="Write File",
+    annotations=ToolAnnotations(
+        idempotentHint=True,  # Same content = same end state
+        openWorldHint=True,
+    ),
+)
+```
+
+**Key Principles**:
+- **Idempotency**: Same inputs → same result. ETags change after updates, making them non-idempotent
+- **Destructive**: Operations that permanently delete/overwrite data
+- **Open World**: All Nextcloud tools access external service (openWorldHint=True)
+- **Titles**: Use human-readable names, not snake_case function names
+
+**See**: `docs/ADR-017-mcp-tool-annotations.md` for detailed rationale and examples
+
 ### Project Structure
 - `nextcloud_mcp_server/client/` - HTTP clients for Nextcloud APIs
 - `nextcloud_mcp_server/server/` - MCP tool/resource definitions

docs/ADR-017-mcp-tool-annotations.md

@@ -2,7 +2,7 @@
 
 ## Status
 
-Proposed
+Implemented
 
 ## Context
 
@@ -67,10 +67,12 @@ async def tool(
 - **Deletes**: `delete_note(id=1)` → note deleted
   - Second call → 404 or success (same end state: note doesn't exist)
   - Note: May return different status code, but end state is identical
-- **Full resource PUT without version control**: Would be idempotent (we don't have these)
+- **Full resource PUT without version control**: `write_file(path="/test.txt", content="Hello")` → file has "Hello"
+  - Second call → file still has "Hello" (same end state)
+  - Example: `nc_webdav_write_file` uses HTTP PUT without etags/version control
 - **Set operations**: `set_property(id=1, value="X")` → property = X
   - Second call → property still = X (same result)
-  - Note: Nextcloud updates use etags, so not applicable
+  - Note: Nextcloud updates with etags use version control, so not idempotent
 
 **Read-Only** (always idempotent, never destructive):
 - All list, search, get operations
@@ -471,11 +473,23 @@ def test_notes_tools_have_annotations():
 - Add annotation guidelines to CLAUDE.md
 - Include examples in developer documentation
 
-## Open Questions
+## Resolved Questions
+
+1. **WebDAV write_file idempotency** (Resolved: 2025-12-11)
+   - **Decision**: Mark as `idempotentHint=True`
+   - **Rationale**: Uses HTTP PUT without version control. Writing same content to same path repeatedly produces identical end state, which is the definition of idempotency in HTTP semantics.
+
+2. **Semantic search openWorldHint** (Resolved: 2025-12-11)
+   - **Decision**: Mark as `openWorldHint=True`
+   - **Rationale**: For consistency with other Nextcloud tools. While the data being searched is "indexed/internal", Nextcloud itself is external to the MCP server. The fact that data is indexed is an implementation detail, not a fundamental difference from other Nextcloud queries.
 
-1. **Icons**: Should we add visual icons for tools? (Requires design work)
-2. **Semantic search openWorldHint**: False (internal data only) or True (Nextcloud is external)?
 3. **Read-only with side effects**: Should tools that log analytics still be readOnlyHint=true?
+   - **Decision**: Yes. Logging/analytics are non-visible side effects that don't change user-observable state. Read-only refers to data modifications that affect the user's content.
+
+## Future Considerations
+
+1. **Icons**: Visual icons for tools (requires design work, deferred to future ADR)
+2. **Parameter descriptions**: Add Pydantic `Field(description=...)` for better auto-completion (Phase 3, future work)
 
 ## References
 
@@ -487,6 +501,6 @@ def test_notes_tools_have_annotations():
 ## Decision Timeline
 
 - **Proposed**: 2025-12-11
-- **Reviewed**: TBD
+- **Reviewed**: 2025-12-11 (Self-review during implementation)
-- **Accepted**: TBD
+- **Accepted**: 2025-12-11
-- **Implemented**: TBD
+- **Implemented**: 2025-12-11 (Phase 1 & 2 complete)

nextcloud_mcp_server/server/semantic.py

@@ -39,7 +39,7 @@ def configure_semantic_tools(mcp: FastMCP):
         title="Semantic Search",
         annotations=ToolAnnotations(
             readOnlyHint=True,  # Search doesn't modify data
-            openWorldHint=False,  # Searches only indexed Nextcloud data
+            openWorldHint=True,  # Queries external Nextcloud service
         ),
     )
     @require_scopes("semantic:read")

nextcloud_mcp_server/server/webdav.py

@@ -133,7 +133,7 @@ def configure_webdav_tools(mcp: FastMCP):
     @mcp.tool(
         title="Write File",
         annotations=ToolAnnotations(
-            idempotentHint=False,  # No etag/version control = not truly idempotent
+            idempotentHint=True,  # HTTP PUT without version control is idempotent
             openWorldHint=True,
         ),
     )

tests/server/test_annotations.py

@@ -0,0 +1,193 @@
+"""Tests for MCP tool annotations (ADR-017)."""
+
+import pytest
+from mcp import ClientSession
+
+pytestmark = pytest.mark.integration
+
+
+async def test_all_tools_have_titles(nc_mcp_client: ClientSession):
+    """Verify all tools have human-readable titles (Phase 1 of ADR-017)."""
+    tools = await nc_mcp_client.list_tools()
+
+    # Every tool should have a title (not None)
+    for tool in tools.tools:
+        assert tool.title is not None, f"Tool {tool.name} is missing a title"
+        # Title should not be empty
+        assert tool.title.strip() != "", f"Tool {tool.name} has an empty title"
+        # Title should be human-readable (not snake_case function name)
+        assert tool.title != tool.name, (
+            f"Tool {tool.name} title is same as function name"
+        )
+
+
+async def test_all_tools_have_annotations(nc_mcp_client: ClientSession):
+    """Verify all tools have ToolAnnotations (Phase 2 of ADR-017)."""
+    tools = await nc_mcp_client.list_tools()
+
+    for tool in tools.tools:
+        # Every tool should have annotations
+        assert tool.annotations is not None, f"Tool {tool.name} is missing annotations"
+
+
+async def test_read_only_tools_have_correct_annotations(nc_mcp_client: ClientSession):
+    """Verify read-only tools are marked correctly."""
+    tools = await nc_mcp_client.list_tools()
+
+    # Known read-only tools (list, search, get operations)
+    read_only_prefixes = ["list", "search", "get"]
+    read_only_patterns = ["_get_", "_list_", "_search_"]
+
+    for tool in tools.tools:
+        # Check if tool name suggests it's read-only
+        is_likely_readonly = tool.name.startswith(tuple(read_only_prefixes)) or any(
+            pattern in tool.name for pattern in read_only_patterns
+        )
+
+        if is_likely_readonly:
+            assert tool.annotations is not None, f"Tool {tool.name} missing annotations"
+            assert tool.annotations.readOnlyHint is True, (
+                f"Read-only tool {tool.name} should have readOnlyHint=True"
+            )
+            assert tool.annotations.destructiveHint is not True, (
+                f"Read-only tool {tool.name} should not have destructiveHint=True"
+            )
+
+
+async def test_destructive_tools_have_correct_annotations(nc_mcp_client: ClientSession):
+    """Verify destructive operations are marked correctly."""
+    tools = await nc_mcp_client.list_tools()
+
+    # Known destructive operations
+    destructive_keywords = ["delete", "remove", "revoke"]
+
+    for tool in tools.tools:
+        has_destructive_keyword = any(
+            keyword in tool.name.lower() for keyword in destructive_keywords
+        )
+
+        if has_destructive_keyword:
+            assert tool.annotations is not None, f"Tool {tool.name} missing annotations"
+            assert tool.annotations.destructiveHint is True, (
+                f"Destructive tool {tool.name} should have destructiveHint=True"
+            )
+
+
+async def test_delete_operations_are_idempotent(nc_mcp_client: ClientSession):
+    """Verify delete operations are marked as idempotent (ADR-017 decision)."""
+    tools = await nc_mcp_client.list_tools()
+
+    for tool in tools.tools:
+        if "delete" in tool.name.lower():
+            assert tool.annotations is not None, f"Tool {tool.name} missing annotations"
+            assert tool.annotations.idempotentHint is True, (
+                f"Delete tool {tool.name} should be idempotent (same end state)"
+            )
+
+
+async def test_create_operations_not_idempotent(nc_mcp_client: ClientSession):
+    """Verify create operations are marked as non-idempotent."""
+    tools = await nc_mcp_client.list_tools()
+
+    for tool in tools.tools:
+        if "create" in tool.name.lower() and "calendar_create_meeting" not in tool.name:
+            assert tool.annotations is not None, f"Tool {tool.name} missing annotations"
+            assert tool.annotations.idempotentHint is not True, (
+                f"Create tool {tool.name} should not be idempotent (creates new resources)"
+            )
+
+
+async def test_update_operations_not_idempotent(nc_mcp_client: ClientSession):
+    """Verify update operations are marked as non-idempotent (due to etag requirements)."""
+    tools = await nc_mcp_client.list_tools()
+
+    for tool in tools.tools:
+        if "update" in tool.name.lower():
+            assert tool.annotations is not None, f"Tool {tool.name} missing annotations"
+            # Most updates use etags which change each time, making them non-idempotent
+            # Exception: calendar_update_event might be different
+            assert tool.annotations.idempotentHint is not True, (
+                f"Update tool {tool.name} should not be idempotent (etag changes)"
+            )
+
+
+async def test_webdav_write_is_idempotent(nc_mcp_client: ClientSession):
+    """Verify nc_webdav_write_file is marked as idempotent (ADR-017 decision).
+
+    WebDAV write uses HTTP PUT without version control, making it idempotent.
+    Writing same content to same path repeatedly produces same end state.
+    """
+    tools = await nc_mcp_client.list_tools()
+
+    write_tool = next(
+        (tool for tool in tools.tools if tool.name == "nc_webdav_write_file"), None
+    )
+    assert write_tool is not None, "nc_webdav_write_file tool not found"
+    assert write_tool.annotations is not None, "write_file missing annotations"
+    assert write_tool.annotations.idempotentHint is True, (
+        "nc_webdav_write_file should be idempotent (HTTP PUT without version control)"
+    )
+
+
+async def test_semantic_search_open_world(nc_mcp_client: ClientSession):
+    """Verify semantic search has openWorldHint=True (ADR-017 decision).
+
+    Semantic search queries external Nextcloud service, consistent with other tools.
+    """
+    tools = await nc_mcp_client.list_tools()
+
+    semantic_tool = next(
+        (tool for tool in tools.tools if tool.name == "nc_semantic_search"), None
+    )
+    if semantic_tool:  # Only if semantic search is enabled
+        assert semantic_tool.annotations is not None, (
+            "semantic_search missing annotations"
+        )
+        assert semantic_tool.annotations.openWorldHint is True, (
+            "nc_semantic_search should have openWorldHint=True (queries external service)"
+        )
+
+
+async def test_annotation_consistency(nc_mcp_client: ClientSession):
+    """Verify annotation consistency across similar tools."""
+    tools = await nc_mcp_client.list_tools()
+
+    # Group tools by category
+    categories = {
+        "notes": [],
+        "calendar": [],
+        "contacts": [],
+        "webdav": [],
+        "tables": [],
+        "deck": [],
+        "cookbook": [],
+        "sharing": [],
+    }
+
+    for tool in tools.tools:
+        for category in categories:
+            if tool.name.startswith(f"nc_{category}_"):
+                categories[category].append(tool)
+
+    # Within each category, similar operations should have similar annotations
+    for category, category_tools in categories.items():
+        # All list/search/get operations should be read-only
+        read_ops = [
+            t
+            for t in category_tools
+            if any(op in t.name for op in ["list", "search", "get"])
+        ]
+        for tool in read_ops:
+            assert tool.annotations.readOnlyHint is True, (
+                f"{tool.name} is a read operation but not marked read-only"
+            )
+
+        # All delete operations should be destructive and idempotent
+        delete_ops = [t for t in category_tools if "delete" in t.name]
+        for tool in delete_ops:
+            assert tool.annotations.destructiveHint is True, (
+                f"{tool.name} is a delete operation but not marked destructive"
+            )
+            assert tool.annotations.idempotentHint is True, (
+                f"{tool.name} is a delete operation but not marked idempotent"
+            )