diff --git a/CLAUDE.md b/CLAUDE.md index ed69773..0ffc880 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -102,13 +102,19 @@ Each Nextcloud app has a corresponding server module that: - **Important**: Integration tests run against live Docker containers. After making code changes to the MCP server, rebuild only the MCP container with `docker-compose up --build -d mcp` before running tests #### Testing Best Practices -- **Always restart MCP server** after code changes with `docker-compose up --build -d mcp` +- **MANDATORY: Always run tests after implementing features or fixing bugs** + - Run tests to completion before considering any task complete + - If tests require modifications to pass, ask for permission before proceeding + - Use `docker-compose up --build -d mcp` to rebuild MCP container after code changes - **Use existing fixtures** from `tests/conftest.py` to avoid duplicate setup work: - `nc_mcp_client` - MCP client session for tool/resource testing - `nc_client` - Direct NextcloudClient for setup/cleanup operations - `temporary_note` - Creates and cleans up test notes automatically - `temporary_addressbook` - Creates and cleans up test address books - `temporary_contact` - Creates and cleans up test contacts +- **Test specific functionality** after changes: + - For Notes changes: `uv run pytest tests/integration/test_mcp.py -k "notes" -v` + - For specific API changes: `uv run pytest tests/integration/test_notes_api.py -v` - **Avoid creating standalone test scripts** - use pytest with proper fixtures instead ### Configuration Files diff --git a/nextcloud_mcp_server/models/__init__.py b/nextcloud_mcp_server/models/__init__.py index 4a06d95..6845df9 100644 --- a/nextcloud_mcp_server/models/__init__.py +++ b/nextcloud_mcp_server/models/__init__.py @@ -3,8 +3,6 @@ # Base models from .base import ( BaseResponse, - ErrorResponse, - SuccessResponse, IdResponse, StatusResponse, ) @@ -82,8 +80,6 @@ from .webdav import ( __all__ = [ # Base models "BaseResponse", - "ErrorResponse", - "SuccessResponse", "IdResponse", "StatusResponse", # Notes models diff --git a/nextcloud_mcp_server/models/base.py b/nextcloud_mcp_server/models/base.py index 40f091f..62bde81 100644 --- a/nextcloud_mcp_server/models/base.py +++ b/nextcloud_mcp_server/models/base.py @@ -1,7 +1,7 @@ """Base Pydantic models for common response patterns.""" from datetime import datetime, timezone -from typing import Any, Dict, Optional, Union +from typing import Optional, Union from pydantic import BaseModel, Field, field_serializer @@ -35,24 +35,6 @@ class BaseResponse(BaseModel): return iso_string -class ErrorResponse(BaseResponse): - """Response model for error cases.""" - - success: bool = Field(default=False, description="Always False for error responses") - error: str = Field(description="Error message") - error_code: Optional[str] = Field(None, description="Optional error code") - details: Optional[Dict[str, Any]] = Field( - None, description="Additional error details" - ) - - -class SuccessResponse(BaseResponse): - """Generic success response.""" - - message: Optional[str] = Field(None, description="Optional success message") - data: Optional[Dict[str, Any]] = Field(None, description="Optional response data") - - class IdResponse(BaseResponse): """Response model for operations that return a new ID.""" diff --git a/nextcloud_mcp_server/models/notes.py b/nextcloud_mcp_server/models/notes.py index 6a60025..9bdc627 100644 --- a/nextcloud_mcp_server/models/notes.py +++ b/nextcloud_mcp_server/models/notes.py @@ -19,7 +19,7 @@ class Note(BaseModel): favorite: bool = Field( default=False, description="Whether note is marked as favorite" ) - etag: Optional[str] = Field(None, description="ETag for versioning") + etag: str = Field(description="ETag for versioning") readonly: bool = Field(default=False, description="Whether note is read-only") @property @@ -50,6 +50,7 @@ class CreateNoteResponse(IdResponse): title: str = Field(description="The created note title") category: str = Field(description="The created note category") + etag: str = Field(description="Current ETag for the created note") class UpdateNoteResponse(BaseResponse): @@ -58,6 +59,7 @@ class UpdateNoteResponse(BaseResponse): id: int = Field(description="The updated note ID") title: str = Field(description="The updated note title") category: str = Field(description="The updated note category") + etag: str = Field(description="Current ETag for the updated note") class DeleteNoteResponse(StatusResponse): @@ -72,6 +74,7 @@ class AppendContentResponse(BaseResponse): id: int = Field(description="The updated note ID") title: str = Field(description="The updated note title") category: str = Field(description="The updated note category") + etag: str = Field(description="Current ETag for the updated note") class SearchNotesResponse(BaseResponse): diff --git a/nextcloud_mcp_server/server/notes.py b/nextcloud_mcp_server/server/notes.py index 2461e7b..cd21d45 100644 --- a/nextcloud_mcp_server/server/notes.py +++ b/nextcloud_mcp_server/server/notes.py @@ -1,10 +1,11 @@ import logging from httpx import HTTPStatusError +from mcp.shared.exceptions import McpError +from mcp.types import ErrorData from mcp.server.fastmcp import Context, FastMCP from nextcloud_mcp_server.client import NextcloudClient -from nextcloud_mcp_server.models.base import ErrorResponse from nextcloud_mcp_server.models.notes import ( Note, NotesSettings, @@ -54,8 +55,6 @@ def configure_notes_tools(mcp: FastMCP): @mcp.resource("nc://Notes/{note_id}") async def nc_get_note(note_id: int): """Get user note using note id""" - from mcp.shared.exceptions import McpError - from mcp.types import ErrorData ctx: Context = mcp.get_context() client: NextcloudClient = ctx.request_context.lifespan_context.client @@ -80,7 +79,7 @@ def configure_notes_tools(mcp: FastMCP): @mcp.tool() async def nc_notes_create_note( title: str, content: str, category: str, ctx: Context - ) -> CreateNoteResponse | ErrorResponse: + ) -> CreateNoteResponse: """Create a new note""" client: NextcloudClient = ctx.request_context.lifespan_context.client try: @@ -91,22 +90,31 @@ def configure_notes_tools(mcp: FastMCP): ) note = Note(**note_data) return CreateNoteResponse( - id=note.id, title=note.title, category=note.category + id=note.id, title=note.title, category=note.category, etag=note.etag ) except HTTPStatusError as e: if e.response.status_code == 403: - return ErrorResponse( - error="Access denied: insufficient permissions to create notes" + raise McpError( + ErrorData( + code=-1, + message="Access denied: insufficient permissions to create notes", + ) ) elif e.response.status_code == 413: - return ErrorResponse(error="Note content too large") + raise McpError(ErrorData(code=-1, message="Note content too large")) elif e.response.status_code == 409: - return ErrorResponse( - error=f"A note with title '{title}' already exists in this category" + raise McpError( + ErrorData( + code=-1, + message=f"A note with title '{title}' already exists in this category", + ) ) else: - return ErrorResponse( - error=f"Failed to create note: server error ({e.response.status_code})" + raise McpError( + ErrorData( + code=-1, + message=f"Failed to create note: server error ({e.response.status_code})", + ) ) @mcp.tool() @@ -117,8 +125,13 @@ def configure_notes_tools(mcp: FastMCP): content: str | None, category: str | None, ctx: Context, - ) -> UpdateNoteResponse | ErrorResponse: - """Update an existing note's title, content, or category""" + ) -> UpdateNoteResponse: + """Update an existing note's title, content, or category. + + REQUIRED: etag parameter must be provided to prevent overwriting concurrent changes. + Get the current ETag by first retrieving the note using nc://Notes/{note_id} resource. + If the note has been modified by someone else since you retrieved it, + the update will fail with a 412 error.""" logger.info("Updating note %s", note_id) client: NextcloudClient = ctx.request_context.lifespan_context.client try: @@ -131,31 +144,44 @@ def configure_notes_tools(mcp: FastMCP): ) note = Note(**note_data) return UpdateNoteResponse( - id=note.id, title=note.title, category=note.category + id=note.id, title=note.title, category=note.category, etag=note.etag ) except HTTPStatusError as e: if e.response.status_code == 404: - return ErrorResponse(error=f"Note {note_id} not found") + raise McpError(ErrorData(code=-1, message=f"Note {note_id} not found")) elif e.response.status_code == 412: - return ErrorResponse( - error=f"Note {note_id} has been modified by someone else. Please refresh and try again." + raise McpError( + ErrorData( + code=-1, + message=f"Note {note_id} has been modified by someone else. Please refresh and try again.", + ) ) elif e.response.status_code == 403: - return ErrorResponse( - error=f"Access denied: insufficient permissions to update note {note_id}" + raise McpError( + ErrorData( + code=-1, + message=f"Access denied: insufficient permissions to update note {note_id}", + ) ) elif e.response.status_code == 413: - return ErrorResponse(error="Updated note content is too large") + raise McpError( + ErrorData(code=-1, message="Updated note content is too large") + ) else: - return ErrorResponse( - error=f"Failed to update note {note_id}: server error ({e.response.status_code})" + raise McpError( + ErrorData( + code=-1, + message=f"Failed to update note {note_id}: server error ({e.response.status_code})", + ) ) @mcp.tool() async def nc_notes_append_content( note_id: int, content: str, ctx: Context - ) -> AppendContentResponse | ErrorResponse: - """Append content to an existing note with a clear separator. The tool automatically adds separators between existing and new content - do not include separators in your content.""" + ) -> AppendContentResponse: + """Append content to an existing note. The tool adds a `\n---\n` + between the note and what will be appended.""" + logger.info("Appending content to note %s", note_id) client: NextcloudClient = ctx.request_context.lifespan_context.client try: @@ -164,28 +190,35 @@ def configure_notes_tools(mcp: FastMCP): ) note = Note(**note_data) return AppendContentResponse( - id=note.id, title=note.title, category=note.category + id=note.id, title=note.title, category=note.category, etag=note.etag ) except HTTPStatusError as e: if e.response.status_code == 404: - return ErrorResponse(error=f"Note {note_id} not found") + raise McpError(ErrorData(code=-1, message=f"Note {note_id} not found")) elif e.response.status_code == 403: - return ErrorResponse( - error=f"Access denied: insufficient permissions to modify note {note_id}" + raise McpError( + ErrorData( + code=-1, + message=f"Access denied: insufficient permissions to modify note {note_id}", + ) ) elif e.response.status_code == 413: - return ErrorResponse( - error="Content to append would make the note too large" + raise McpError( + ErrorData( + code=-1, + message="Content to append would make the note too large", + ) ) else: - return ErrorResponse( - error=f"Failed to append content to note {note_id}: server error ({e.response.status_code})" + raise McpError( + ErrorData( + code=-1, + message=f"Failed to append content to note {note_id}: server error ({e.response.status_code})", + ) ) @mcp.tool() - async def nc_notes_search_notes( - query: str, ctx: Context - ) -> SearchNotesResponse | ErrorResponse: + async def nc_notes_search_notes(query: str, ctx: Context) -> SearchNotesResponse: """Search notes by title or content, returning only id, title, and category.""" client: NextcloudClient = ctx.request_context.lifespan_context.client try: @@ -207,20 +240,26 @@ def configure_notes_tools(mcp: FastMCP): ) except HTTPStatusError as e: if e.response.status_code == 403: - return ErrorResponse( - error="Access denied: insufficient permissions to search notes" + raise McpError( + ErrorData( + code=-1, + message="Access denied: insufficient permissions to search notes", + ) ) elif e.response.status_code == 400: - return ErrorResponse(error="Invalid search query format") + raise McpError( + ErrorData(code=-1, message="Invalid search query format") + ) else: - return ErrorResponse( - error=f"Search failed: server error ({e.response.status_code})" + raise McpError( + ErrorData( + code=-1, + message=f"Search failed: server error ({e.response.status_code})", + ) ) @mcp.tool() - async def nc_notes_delete_note( - note_id: int, ctx: Context - ) -> DeleteNoteResponse | ErrorResponse: + async def nc_notes_delete_note(note_id: int, ctx: Context) -> DeleteNoteResponse: """Delete a note permanently""" logger.info("Deleting note %s", note_id) client: NextcloudClient = ctx.request_context.lifespan_context.client @@ -233,12 +272,18 @@ def configure_notes_tools(mcp: FastMCP): ) except HTTPStatusError as e: if e.response.status_code == 404: - return ErrorResponse(error=f"Note {note_id} not found") + raise McpError(ErrorData(code=-1, message=f"Note {note_id} not found")) elif e.response.status_code == 403: - return ErrorResponse( - error=f"Access denied: insufficient permissions to delete note {note_id}" + raise McpError( + ErrorData( + code=-1, + message=f"Access denied: insufficient permissions to delete note {note_id}", + ) ) else: - return ErrorResponse( - error=f"Failed to delete note {note_id}: server error ({e.response.status_code})" + raise McpError( + ErrorData( + code=-1, + message=f"Failed to delete note {note_id}: server error ({e.response.status_code})", + ) ) diff --git a/nextcloud_mcp_server/server/webdav.py b/nextcloud_mcp_server/server/webdav.py index ae4040a..678ea46 100644 --- a/nextcloud_mcp_server/server/webdav.py +++ b/nextcloud_mcp_server/server/webdav.py @@ -43,11 +43,11 @@ def configure_webdav_tools(mcp: FastMCP): Examples: # Read a text file result = await nc_webdav_read_file("Documents/readme.txt") - print(result['content']) # Decoded text content + logger.info(result['content']) # Decoded text content # Read a binary file result = await nc_webdav_read_file("Images/photo.jpg") - print(result['encoding']) # 'base64' + logger.info(result['encoding']) # 'base64' """ client: NextcloudClient = ctx.request_context.lifespan_context.client content, content_type = await client.webdav.read_file(path) diff --git a/tests/integration/test_error_propagation.py b/tests/integration/test_error_propagation.py index 45d5e33..623d5ab 100644 --- a/tests/integration/test_error_propagation.py +++ b/tests/integration/test_error_propagation.py @@ -20,23 +20,15 @@ async def test_missing_note_resource_error(nc_mcp_client: ClientSession): @pytest.mark.integration async def test_delete_missing_note_tool_error(nc_mcp_client: ClientSession): """Test that deleting a non-existent note returns proper error.""" - # Try to delete a non-existent note + # Try to delete a non-existent note - should return error response response = await nc_mcp_client.call_tool( "nc_notes_delete_note", {"note_id": 999999} ) - logger.info(f"Delete missing note response: {response}") - - # Should return structured error response with improved message + # Should return error response (not raise exception) for tools assert response is not None - assert ( - response.isError is False - ) # Tools now return structured responses, not MCP errors - - # Check structured content for error - assert "success" in response.structuredContent["result"] - assert response.structuredContent["result"]["success"] is False - assert "Note 999999 not found" in response.structuredContent["result"]["error"] + assert response.isError is True + assert "Note 999999 not found" in response.content[0].text @pytest.mark.integration @@ -81,7 +73,7 @@ async def test_update_note_with_invalid_etag(nc_mcp_client: ClientSession, nc_cl note_id = note_data["id"] try: - # Try to update with invalid ETag + # Try to update with invalid ETag - should return error response response = await nc_mcp_client.call_tool( "nc_notes_update_note", { @@ -93,16 +85,10 @@ async def test_update_note_with_invalid_etag(nc_mcp_client: ClientSession, nc_cl }, ) - logger.info(f"Invalid ETag response: {response}") - - # Should return structured error response with improved message + # Should return error response (not raise exception) for tools assert response is not None - assert response.isError is False # Tools now return structured responses - assert "success" in response.structuredContent["result"] - assert response.structuredContent["result"]["success"] is False - assert ( - "modified by someone else" in response.structuredContent["result"]["error"] - ) + assert response.isError is True + assert "modified by someone else" in response.content[0].text finally: # Clean up diff --git a/tests/integration/test_mcp.py b/tests/integration/test_mcp.py index 06b89ff..d4b30cc 100644 --- a/tests/integration/test_mcp.py +++ b/tests/integration/test_mcp.py @@ -123,8 +123,11 @@ async def test_mcp_notes_crud_workflow( created_note = create_result.content[0].text note_data = json.loads(created_note) # Parse the returned JSON note_id = note_data["id"] + create_etag = note_data["etag"] # Verify create response includes ETag - logger.info(f"Note created via MCP with ID: {note_id}") + logger.info(f"Note created via MCP with ID: {note_id}, ETag: {create_etag}") + assert "etag" in note_data, "Create response should include ETag" + assert create_etag, "Create ETag should not be empty" # 2. Verify creation via direct NextcloudClient direct_note = await nc_client.notes.get_note(note_id) @@ -165,6 +168,15 @@ async def test_mcp_notes_crud_workflow( f"MCP note update failed: {update_result.content}" ) + # Verify update response includes new ETag + updated_note_data = json.loads(update_result.content[0].text) + update_etag = updated_note_data["etag"] + + logger.info(f"Note updated via MCP, new ETag: {update_etag}") + assert "etag" in updated_note_data, "Update response should include ETag" + assert update_etag, "Update ETag should not be empty" + assert update_etag != etag, "ETag should change after update" + # 5. Verify update via direct NextcloudClient updated_direct_note = await nc_client.notes.get_note(note_id) assert updated_direct_note["title"] == updated_title @@ -181,6 +193,15 @@ async def test_mcp_notes_crud_workflow( f"MCP note append failed: {append_result.content}" ) + # Verify append response includes new ETag + appended_note_data = json.loads(append_result.content[0].text) + append_etag = appended_note_data["etag"] + + logger.info(f"Content appended via MCP, new ETag: {append_etag}") + assert "etag" in appended_note_data, "Append response should include ETag" + assert append_etag, "Append ETag should not be empty" + assert append_etag != update_etag, "ETag should change after append" + # 7. Verify append via direct NextcloudClient appended_direct_note = await nc_client.notes.get_note(note_id) assert append_content in appended_direct_note["content"] @@ -256,6 +277,82 @@ async def test_mcp_notes_crud_workflow( logger.warning(f"Failed to cleanup note: {e}") +async def test_mcp_notes_etag_conflict( + nc_mcp_client: ClientSession, nc_client: NextcloudClient +): + """Test that MCP note updates fail when using stale ETags.""" + + unique_suffix = uuid.uuid4().hex[:8] + test_title = f"ETag Test Note {unique_suffix}" + test_content = f"This is test content for ETag testing {unique_suffix}" + test_category = "ETTesting" + + created_note = None + + try: + # 1. Create note via MCP + logger.info(f"Creating note for ETag conflict test: {test_title}") + create_result = await nc_mcp_client.call_tool( + "nc_notes_create_note", + {"title": test_title, "content": test_content, "category": test_category}, + ) + + assert create_result.isError is False + note_data = json.loads(create_result.content[0].text) + note_id = note_data["id"] + original_etag = note_data["etag"] + created_note = note_data + + # 2. Update note to change ETag + first_update_result = await nc_mcp_client.call_tool( + "nc_notes_update_note", + { + "note_id": note_id, + "etag": original_etag, + "title": f"First Update {test_title}", + "content": test_content, + "category": test_category, + }, + ) + + assert first_update_result.isError is False + updated_data = json.loads(first_update_result.content[0].text) + new_etag = updated_data["etag"] + assert new_etag != original_etag, "ETag should have changed after update" + + # 3. Try to update with the stale (original) ETag - this should fail + logger.info(f"Attempting update with stale ETag: {original_etag}") + conflict_result = await nc_mcp_client.call_tool( + "nc_notes_update_note", + { + "note_id": note_id, + "etag": original_etag, # Use stale ETag + "title": "This should fail", + "content": "This update should be rejected", + "category": test_category, + }, + ) + + # 4. Verify the update was rejected with a 412 error + # With McpError, tools now return proper error responses + assert conflict_result.isError is True, "Update with stale ETag should fail" + response_content = conflict_result.content[0].text + assert "modified by someone else" in response_content, ( + f"Expected conflict error message, got: {response_content}" + ) + + logger.info("Successfully verified ETag conflict handling via MCP") + + finally: + # Cleanup + if created_note is not None: + try: + await nc_client.notes.delete_note(created_note["id"]) + logger.info(f"Cleaned up test note {created_note['id']}") + except Exception as e: + logger.warning(f"Failed to cleanup test note: {e}") + + async def test_mcp_webdav_workflow( nc_mcp_client: ClientSession, nc_client: NextcloudClient ): diff --git a/tests/test_models.py b/tests/test_models.py index 304f916..0f7bf0d 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1,11 +1,13 @@ """Unit tests for Pydantic models and serialization.""" -import json -import re from datetime import datetime, timezone +import json +import logging +import re +from nextcloud_mcp_server.models.base import BaseResponse -from nextcloud_mcp_server.models.base import BaseResponse, SuccessResponse +logger = logging.getLogger(__name__) def test_timestamp_format_validation(): @@ -15,7 +17,7 @@ def test_timestamp_format_validation(): seen in MCP inspector. MCP expects RFC3339 format with timezone information. """ # Create a response object - response = SuccessResponse(message="Test message") + response = BaseResponse() # Serialize to JSON (mimics what MCP inspector sees) json_str = response.model_dump_json() @@ -76,7 +78,7 @@ def test_current_broken_format(): assert "+" not in current_format assert "-" not in current_format[-6:] # Check last 6 chars for timezone - print(f"Current broken format: {current_format}") - print( + logger.info(f"Current broken format: {current_format}") + logger.info( "This format causes MCP validation errors because it lacks timezone information" )