diff --git a/CLAUDE.md b/CLAUDE.md index 7c17cc1..38d4b37 100644 --- a/CLAUDE.md +++ b/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 diff --git a/docs/ADR-017-mcp-tool-annotations.md b/docs/ADR-017-mcp-tool-annotations.md index ada5f7f..f785829 100644 --- a/docs/ADR-017-mcp-tool-annotations.md +++ b/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 -- **Accepted**: TBD -- **Implemented**: TBD +- **Reviewed**: 2025-12-11 (Self-review during implementation) +- **Accepted**: 2025-12-11 +- **Implemented**: 2025-12-11 (Phase 1 & 2 complete) diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index bbe3637..2586ab3 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/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") diff --git a/nextcloud_mcp_server/server/webdav.py b/nextcloud_mcp_server/server/webdav.py index 51f2479..71337ee 100644 --- a/nextcloud_mcp_server/server/webdav.py +++ b/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, ), ) diff --git a/tests/server/test_annotations.py b/tests/server/test_annotations.py new file mode 100644 index 0000000..2a387ff --- /dev/null +++ b/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" + )