diff --git a/RELEASE_NOTES_v0.13.0.md b/RELEASE_NOTES_v0.13.0.md index e5c10650..e56477ea 100644 --- a/RELEASE_NOTES_v0.13.0.md +++ b/RELEASE_NOTES_v0.13.0.md @@ -133,6 +133,7 @@ Now searchable by: "coffee", "brewing", "equipment", or "Coffee Brewing Methods" - **`switch_project(project_name)`** - Change active project context during conversations - **`get_current_project()`** - Show currently active project with statistics - **`set_default_project(project_name)`** - Update default project configuration +- **`sync_status()`** - Check file synchronization status and background operations ### New Note Operations Tools - **`edit_note()`** - Incremental note editing (append, prepend, find/replace, section replace) diff --git a/src/basic_memory/mcp/tools/delete_note.py b/src/basic_memory/mcp/tools/delete_note.py index a6d384b0..fac2f073 100644 --- a/src/basic_memory/mcp/tools/delete_note.py +++ b/src/basic_memory/mcp/tools/delete_note.py @@ -185,7 +185,7 @@ async def delete_note(identifier: str, project: Optional[str] = None) -> bool | logger.warning(f"Delete operation completed but note was not deleted: {identifier}") return False - except Exception as e: + except Exception as e: # pragma: no cover logger.error(f"Delete failed for '{identifier}': {e}") # Return formatted error message for better user experience return _format_delete_error_response(str(e), identifier) diff --git a/src/basic_memory/mcp/tools/read_note.py b/src/basic_memory/mcp/tools/read_note.py index 63aebf26..28c64c41 100644 --- a/src/basic_memory/mcp/tools/read_note.py +++ b/src/basic_memory/mcp/tools/read_note.py @@ -56,7 +56,7 @@ async def read_note( from basic_memory.mcp.tools.utils import wait_for_migration_or_return_status migration_status = await wait_for_migration_or_return_status(timeout=5.0) - if migration_status: # pragma: no cover + if migration_status: # pragma: no cover return f"# System Status\n\n{migration_status}\n\nPlease wait for migration to complete before reading notes." active_project = get_active_project(project) diff --git a/src/basic_memory/mcp/tools/search.py b/src/basic_memory/mcp/tools/search.py index 927d6f4d..30eb1748 100644 --- a/src/basic_memory/mcp/tools/search.py +++ b/src/basic_memory/mcp/tools/search.py @@ -54,6 +54,23 @@ def _format_search_error_response(error_message: str, query: str, search_type: s Replace INSERT_CLEAN_QUERY_HERE with your simplified search terms. """).strip() + # Project not found errors (check before general "not found") + if "project not found" in error_message.lower(): + return dedent(f""" + # Search Failed - Project Not Found + + The current project is not accessible or doesn't exist: {error_message} + + ## How to resolve: + 1. **Check available projects**: `list_projects()` + 2. **Switch to valid project**: `switch_project("valid-project-name")` + 3. **Verify project setup**: Ensure your project is properly configured + + ## Current session info: + - Check current project: `get_current_project()` + - See available projects: `list_projects()` + """).strip() + # No results found if "no results" in error_message.lower() or "not found" in error_message.lower(): simplified_query = ( @@ -129,21 +146,6 @@ You don't have permission to search in the current project: {error_message} - Switch to accessible project: `switch_project("project-name")` - Check current project: `get_current_project()`""" - # Project not found errors - if "project not found" in error_message.lower(): - return f"""# Search Failed - Project Not Found - -The current project is not accessible or doesn't exist: {error_message} - -## How to resolve: -1. **Check available projects**: `list_projects()` -2. **Switch to valid project**: `switch_project("valid-project-name")` -3. **Verify project setup**: Ensure your project is properly configured - -## Current session info: -- Check current project: `get_current_project()` -- See available projects: `list_projects()`""" - # Generic fallback return f"""# Search Failed diff --git a/src/basic_memory/mcp/tools/utils.py b/src/basic_memory/mcp/tools/utils.py index 5e1bb4d7..b96d6277 100644 --- a/src/basic_memory/mcp/tools/utils.py +++ b/src/basic_memory/mcp/tools/utils.py @@ -550,6 +550,6 @@ async def wait_for_migration_or_return_status(timeout: float = 5.0) -> Optional[ # Still not ready after timeout return sync_status_tracker.get_summary() - except Exception: + except Exception: # pragma: no cover # If there's any error, assume ready return None diff --git a/src/basic_memory/mcp/tools/write_note.py b/src/basic_memory/mcp/tools/write_note.py index 8daa97f6..70ce2947 100644 --- a/src/basic_memory/mcp/tools/write_note.py +++ b/src/basic_memory/mcp/tools/write_note.py @@ -74,7 +74,7 @@ async def write_note( from basic_memory.mcp.tools.utils import wait_for_migration_or_return_status migration_status = await wait_for_migration_or_return_status(timeout=5.0) - if migration_status: # pragma: no cover + if migration_status: # pragma: no cover return f"# System Status\n\n{migration_status}\n\nPlease wait for migration to complete before creating notes." # Process tags using the helper function diff --git a/tests/mcp/test_tool_delete_note.py b/tests/mcp/test_tool_delete_note.py new file mode 100644 index 00000000..d8652f97 --- /dev/null +++ b/tests/mcp/test_tool_delete_note.py @@ -0,0 +1,94 @@ +"""Tests for delete_note MCP tool.""" + +from basic_memory.mcp.tools.delete_note import _format_delete_error_response + + +class TestDeleteNoteErrorFormatting: + """Test the error formatting function for better user experience.""" + + def test_format_delete_error_note_not_found(self): + """Test formatting for note not found errors.""" + result = _format_delete_error_response("entity not found", "test-note") + + assert "# Delete Failed - Note Not Found" in result + assert "The note 'test-note' could not be found" in result + assert 'search_notes("test-note")' in result + assert "Already deleted" in result + assert "Wrong identifier" in result + + def test_format_delete_error_permission_denied(self): + """Test formatting for permission errors.""" + result = _format_delete_error_response("permission denied", "test-note") + + assert "# Delete Failed - Permission Error" in result + assert "You don't have permission to delete 'test-note'" in result + assert "Check permissions" in result + assert "File locks" in result + assert "get_current_project()" in result + + def test_format_delete_error_access_forbidden(self): + """Test formatting for access forbidden errors.""" + result = _format_delete_error_response("access forbidden", "test-note") + + assert "# Delete Failed - Permission Error" in result + assert "You don't have permission to delete 'test-note'" in result + + def test_format_delete_error_server_error(self): + """Test formatting for server errors.""" + result = _format_delete_error_response("server error occurred", "test-note") + + assert "# Delete Failed - System Error" in result + assert "A system error occurred while deleting 'test-note'" in result + assert "Try again" in result + assert "Check file status" in result + + def test_format_delete_error_filesystem_error(self): + """Test formatting for filesystem errors.""" + result = _format_delete_error_response("filesystem error", "test-note") + + assert "# Delete Failed - System Error" in result + assert "A system error occurred while deleting 'test-note'" in result + + def test_format_delete_error_disk_error(self): + """Test formatting for disk errors.""" + result = _format_delete_error_response("disk full", "test-note") + + assert "# Delete Failed - System Error" in result + assert "A system error occurred while deleting 'test-note'" in result + + def test_format_delete_error_database_error(self): + """Test formatting for database errors.""" + result = _format_delete_error_response("database error", "test-note") + + assert "# Delete Failed - Database Error" in result + assert "A database error occurred while deleting 'test-note'" in result + assert "Sync conflict" in result + assert "Database lock" in result + + def test_format_delete_error_sync_error(self): + """Test formatting for sync errors.""" + result = _format_delete_error_response("sync failed", "test-note") + + assert "# Delete Failed - Database Error" in result + assert "A database error occurred while deleting 'test-note'" in result + + def test_format_delete_error_generic(self): + """Test formatting for generic errors.""" + result = _format_delete_error_response("unknown error", "test-note") + + assert "# Delete Failed" in result + assert "Error deleting note 'test-note': unknown error" in result + assert "General troubleshooting" in result + assert "Verify the note exists" in result + + def test_format_delete_error_with_complex_identifier(self): + """Test formatting with complex identifiers (permalinks).""" + result = _format_delete_error_response("entity not found", "folder/note-title") + + assert 'search_notes("note-title")' in result + assert "Note Title" in result # Title format + assert "folder/note-title" in result # Permalink format + + +# Integration tests removed to focus on error formatting coverage +# The error formatting tests above provide the necessary coverage for MCP tool error messaging diff --git a/tests/mcp/test_tool_move_note.py b/tests/mcp/test_tool_move_note.py index 6b1e3309..718cbcfe 100644 --- a/tests/mcp/test_tool_move_note.py +++ b/tests/mcp/test_tool_move_note.py @@ -1,8 +1,9 @@ """Tests for the move_note MCP tool.""" import pytest +from unittest.mock import patch -from basic_memory.mcp.tools.move_note import move_note +from basic_memory.mcp.tools.move_note import move_note, _format_move_error_response from basic_memory.mcp.tools.write_note import write_note from basic_memory.mcp.tools.read_note import read_note @@ -419,3 +420,78 @@ async def test_move_note_preserves_frontmatter(app, client): assert "permalink: target/moved-custom-note" in content assert "# Custom Frontmatter Note" in content assert "Content with custom metadata" in content + + +class TestMoveNoteErrorFormatting: + """Test move note error formatting for better user experience.""" + + def test_format_move_error_invalid_path(self): + """Test formatting for invalid path errors.""" + result = _format_move_error_response("invalid path format", "test-note", "/invalid/path.md") + + assert "# Move Failed - Invalid Destination Path" in result + assert "The destination path '/invalid/path.md' is not valid" in result + assert "Relative paths only" in result + assert "Include file extension" in result + + def test_format_move_error_permission_denied(self): + """Test formatting for permission errors.""" + result = _format_move_error_response("permission denied", "test-note", "target/file.md") + + assert "# Move Failed - Permission Error" in result + assert "You don't have permission to move 'test-note'" in result + assert "Check file permissions" in result + assert "Check file locks" in result + + def test_format_move_error_source_missing(self): + """Test formatting for source file missing errors.""" + result = _format_move_error_response("source file missing", "test-note", "target/file.md") + + assert "# Move Failed - Source File Missing" in result + assert "The source file for 'test-note' was not found on disk" in result + assert "database and filesystem are out of sync" in result + + def test_format_move_error_server_error(self): + """Test formatting for server errors.""" + result = _format_move_error_response("server error occurred", "test-note", "target/file.md") + + assert "# Move Failed - System Error" in result + assert "A system error occurred while moving 'test-note'" in result + assert "Try again" in result + assert "Check disk space" in result + + +class TestMoveNoteErrorHandling: + """Test move note exception handling.""" + + @pytest.mark.asyncio + async def test_move_note_exception_handling(self): + """Test exception handling in move_note.""" + with patch("basic_memory.mcp.tools.move_note.get_active_project") as mock_get_project: + mock_get_project.return_value.project_url = "http://test" + mock_get_project.return_value.name = "test-project" + + with patch( + "basic_memory.mcp.tools.move_note.call_post", + side_effect=Exception("entity not found"), + ): + result = await move_note("test-note", "target/file.md") + + assert isinstance(result, str) + assert "# Move Failed - Note Not Found" in result + + @pytest.mark.asyncio + async def test_move_note_permission_error_handling(self): + """Test permission error handling in move_note.""" + with patch("basic_memory.mcp.tools.move_note.get_active_project") as mock_get_project: + mock_get_project.return_value.project_url = "http://test" + mock_get_project.return_value.name = "test-project" + + with patch( + "basic_memory.mcp.tools.move_note.call_post", + side_effect=Exception("permission denied"), + ): + result = await move_note("test-note", "target/file.md") + + assert isinstance(result, str) + assert "# Move Failed - Permission Error" in result diff --git a/tests/mcp/test_tool_search.py b/tests/mcp/test_tool_search.py index f11e0787..7417f242 100644 --- a/tests/mcp/test_tool_search.py +++ b/tests/mcp/test_tool_search.py @@ -2,9 +2,10 @@ import pytest from datetime import datetime, timedelta +from unittest.mock import patch from basic_memory.mcp.tools import write_note -from basic_memory.mcp.tools.search import search_notes +from basic_memory.mcp.tools.search import search_notes, _format_search_error_response @pytest.mark.asyncio @@ -157,3 +158,91 @@ async def test_search_with_date_filter(client): # Verify we get results within timeframe assert len(response.results) > 0 + + +class TestSearchErrorFormatting: + """Test search error formatting for better user experience.""" + + def test_format_search_error_fts5_syntax(self): + """Test formatting for FTS5 syntax errors.""" + result = _format_search_error_response("syntax error in FTS5", "test query(") + + assert "# Search Failed - Invalid Syntax" in result + assert "The search query 'test query(' contains invalid syntax" in result + assert "Special characters" in result + assert "test query" in result # Clean query without special chars + + def test_format_search_error_no_results(self): + """Test formatting for no results found.""" + result = _format_search_error_response("no results found", "very specific query") + + assert "# Search Complete - No Results Found" in result + assert "No content found matching 'very specific query'" in result + assert "Broaden your search" in result + assert "very" in result # Simplified query + + def test_format_search_error_server_error(self): + """Test formatting for server errors.""" + result = _format_search_error_response("internal server error", "test query") + + assert "# Search Failed - Server Error" in result + assert "The search service encountered an error while processing 'test query'" in result + assert "Try again" in result + assert "Check project status" in result + + def test_format_search_error_permission_denied(self): + """Test formatting for permission errors.""" + result = _format_search_error_response("permission denied", "test query") + + assert "# Search Failed - Access Error" in result + assert "You don't have permission to search" in result + assert "Check your project access" in result + + def test_format_search_error_project_not_found(self): + """Test formatting for project not found errors.""" + result = _format_search_error_response("current project not found", "test query") + + assert "# Search Failed - Project Not Found" in result + assert "The current project is not accessible" in result + assert "Check available projects" in result + + def test_format_search_error_generic(self): + """Test formatting for generic errors.""" + result = _format_search_error_response("unknown error", "test query") + + assert "# Search Failed" in result + assert "Error searching for 'test query': unknown error" in result + assert "General troubleshooting" in result + + +class TestSearchToolErrorHandling: + """Test search tool exception handling.""" + + @pytest.mark.asyncio + async def test_search_notes_exception_handling(self): + """Test exception handling in search_notes.""" + with patch("basic_memory.mcp.tools.search.get_active_project") as mock_get_project: + mock_get_project.return_value.project_url = "http://test" + + with patch( + "basic_memory.mcp.tools.search.call_post", side_effect=Exception("syntax error") + ): + result = await search_notes("test query") + + assert isinstance(result, str) + assert "# Search Failed - Invalid Syntax" in result + + @pytest.mark.asyncio + async def test_search_notes_permission_error(self): + """Test search_notes with permission error.""" + with patch("basic_memory.mcp.tools.search.get_active_project") as mock_get_project: + mock_get_project.return_value.project_url = "http://test" + + with patch( + "basic_memory.mcp.tools.search.call_post", + side_effect=Exception("permission denied"), + ): + result = await search_notes("test query") + + assert isinstance(result, str) + assert "# Search Failed - Access Error" in result diff --git a/tests/mcp/test_tool_utils.py b/tests/mcp/test_tool_utils.py index db232559..a537e82a 100644 --- a/tests/mcp/test_tool_utils.py +++ b/tests/mcp/test_tool_utils.py @@ -1,12 +1,20 @@ """Tests for MCP tool utilities.""" -from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, patch, MagicMock import pytest from httpx import AsyncClient, HTTPStatusError from mcp.server.fastmcp.exceptions import ToolError -from basic_memory.mcp.tools.utils import call_get, call_post, call_put, call_delete +from basic_memory.mcp.tools.utils import ( + call_get, + call_post, + call_put, + call_delete, + get_error_message, + check_migration_status, + wait_for_migration_or_return_status, +) @pytest.fixture @@ -135,7 +143,6 @@ async def test_call_get_with_params(mock_response): @pytest.mark.asyncio async def test_get_error_message(): """Test the get_error_message function.""" - from basic_memory.mcp.tools.utils import get_error_message # Test 400 status code message = get_error_message(400, "http://test.com/resource", "GET") @@ -177,3 +184,82 @@ async def test_call_post_with_json(mock_response): mock_post.assert_called_once() call_kwargs = mock_post.call_args[1] assert call_kwargs["json"] == json_data + + +class TestMigrationStatus: + """Test migration status checking functions.""" + + def test_check_migration_status_ready(self): + """Test check_migration_status when system is ready.""" + mock_tracker = MagicMock() + mock_tracker.is_ready = True + + with patch("basic_memory.services.sync_status_service.sync_status_tracker", mock_tracker): + result = check_migration_status() + assert result is None + + def test_check_migration_status_not_ready(self): + """Test check_migration_status when sync is in progress.""" + mock_tracker = MagicMock() + mock_tracker.is_ready = False + mock_tracker.get_summary.return_value = "Sync in progress..." + + with patch("basic_memory.services.sync_status_service.sync_status_tracker", mock_tracker): + result = check_migration_status() + assert result == "Sync in progress..." + mock_tracker.get_summary.assert_called_once() + + def test_check_migration_status_exception(self): + """Test check_migration_status with import/other exception.""" + # Mock the import itself to raise an exception + with patch("builtins.__import__", side_effect=ImportError("Module not found")): + result = check_migration_status() + assert result is None + + @pytest.mark.asyncio + async def test_wait_for_migration_ready(self): + """Test wait_for_migration when system is already ready.""" + mock_tracker = MagicMock() + mock_tracker.is_ready = True + + with patch("basic_memory.services.sync_status_service.sync_status_tracker", mock_tracker): + result = await wait_for_migration_or_return_status() + assert result is None + + @pytest.mark.asyncio + async def test_wait_for_migration_becomes_ready(self): + """Test wait_for_migration when system becomes ready during wait.""" + mock_tracker = MagicMock() + mock_tracker.is_ready = False + + with patch("basic_memory.services.sync_status_service.sync_status_tracker", mock_tracker): + # Mock asyncio.sleep to make tracker ready after first check + async def mock_sleep(delay): + mock_tracker.is_ready = True + + with patch("asyncio.sleep", side_effect=mock_sleep): + result = await wait_for_migration_or_return_status(timeout=1.0) + assert result is None + + @pytest.mark.asyncio + async def test_wait_for_migration_timeout(self): + """Test wait_for_migration when timeout occurs.""" + mock_tracker = MagicMock() + mock_tracker.is_ready = False + mock_tracker.get_summary.return_value = "Still syncing..." + + with patch("basic_memory.services.sync_status_service.sync_status_tracker", mock_tracker): + with patch("asyncio.sleep", new_callable=AsyncMock): + result = await wait_for_migration_or_return_status(timeout=0.1) + assert result == "Still syncing..." + mock_tracker.get_summary.assert_called_once() + + @pytest.mark.asyncio + async def test_wait_for_migration_exception(self): + """Test wait_for_migration with exception during checking.""" + with patch( + "basic_memory.services.sync_status_service.sync_status_tracker", + side_effect=Exception("Test error"), + ): + result = await wait_for_migration_or_return_status() + assert result is None