feat: add directory move support to move_note tool #518
Conversation
Add `is_directory` parameter to `move_note` tool for moving entire directories. Also rename `folder` → `directory` across the codebase for consistency. Changes: - Add `is_directory` parameter to move_note MCP tool - Add MoveDirectoryRequest and DirectoryMoveResult schemas - Add move_directory method to EntityService - Add POST /move-directory API endpoints (v1 and v2) - Add move_directory client method to KnowledgeClient - Rename Entity.folder → Entity.directory in schemas - Rename sanitize_for_folder → sanitize_for_directory - Update canvas, write_note, and importer tools to use directory - Update all tests to use directory parameter Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 2m 43s —— View job PR Review: Directory Move SupportThis is a well-structured PR that adds directory move functionality to Basic Memory. I've reviewed the implementation against the team checklist. Here are my findings: ✅ StrengthsCode Quality & Standards
Implementation Quality
|
Add comprehensive integration tests for directory move functionality: - Basic directory move with multiple files - Nested directory structure preservation - Empty directory handling - Content preservation (observations, relations) - Search functionality after move - Single file directory move Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 3m 40s —— View job Re-reviewing PR after updates ✅Analyzing the new commit "test: add integration tests for move_note is_directory parameter" to verify if the blocking issues have been resolved. Todo List:
Updated Findings✅ Integration Tests Added (Good Coverage)The new commit added
These tests exercise the full stack: MCP Tool → Client → API → Service → Repository → File System
|
…ples - Update CLAUDE.md tool signature from folder to directory for canvas tool - Fix canvas.py docstring examples to use correct parameter order (project is an optional keyword arg, not the first positional arg) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 3m 2s —— View job PR Review: Directory Move SupportComprehensive review against Basic Memory team checklist completed. This PR adds directory move functionality with extensive refactoring of Todo List:
Executive SummaryThis PR demonstrates excellent engineering quality with clean architecture, comprehensive integration tests, and proper error handling. The latest commit successfully addressed the previous review's Windows path compatibility concerns and added robust integration tests. However, two critical issues block merge:
✅ StrengthsArchitecture & Design
Code Quality
Testing (Partially Complete)
❌ Critical Issues (Blocking Merge)1. Missing API Endpoint TestsLocation: No tests found for Issue: CLAUDE.md requires 100% test coverage, but the new API endpoints have no tests:
Why this matters:
Required tests in @pytest.mark.asyncio
async def test_move_directory_endpoint_success(client, test_project):
"""Test POST /move-directory endpoint."""
response = await client.post(
"/api/v1/move-directory",
json={
"source_directory": "source",
"destination_directory": "dest",
"project": test_project.name,
},
)
assert response.status_code == 200
data = response.json()
assert data["total_files"] >= 0
assert "moved_files" in data
@pytest.mark.asyncio
async def test_move_directory_endpoint_validation_error(client):
"""Test validation error handling."""
response = await client.post(
"/api/v1/move-directory",
json={"source_directory": ""}, # Invalid empty string
)
assert response.status_code == 422 # Validation errorSimilar tests needed for v2 endpoint in Checklist violation: "Unit tests for new functions/methods" ✗ 2. Documentation Not UpdatedLocation: CLAUDE.md:251, 256, 273 Issue: Tool signatures still show old # Current (incorrect):
- `write_note(title, content, folder, tags)` - Create/update markdown notes...
- `move_note(identifier, destination_path)` - Move notes to new locations...
- `canvas(nodes, edges, title, folder)` - Generate Obsidian canvas files...
# Should be:
- `write_note(title, content, directory, tags)` - Create/update markdown notes...
- `move_note(identifier, destination_path, is_directory)` - Move notes or directories to new locations...
- `canvas(nodes, edges, title, directory)` - Generate Obsidian canvas files...Impact: Users reading CLAUDE.md will see incorrect parameter names, causing confusion Checklist violation: "Documentation updated (README, docstrings)" ✗
|
|
Note: After merging, the documentation at https://github.com/basicmachines-co/docs.basicmemory.com also needs to be updated to reflect the |
Adds is_directory parameter to delete_note MCP tool, allowing users to
delete entire directories and all their contents with a single command.
Changes:
- Add DirectoryDeleteResult and DirectoryDeleteError schemas
- Add delete_directory method to EntityService
- Add /delete-directory API endpoint (v2)
- Add delete_directory method to KnowledgeClient
- Update delete_note MCP tool with is_directory parameter
- Add integration tests for directory deletion
- Update CLAUDE.md documentation
Usage:
delete_note("docs", is_directory=True) # Delete entire directory
delete_note("projects/2024", is_directory=True) # Delete nested
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 3m 16s —— View job PR Review: Directory Move & Delete SupportComprehensive review against Basic Memory team checklist completed. Todo List:
Executive SummaryThis PR successfully implements directory move and delete functionality with excellent architecture and comprehensive integration testing. All previous blocking issues have been resolved: ✅ Documentation updated - CLAUDE.md correctly reflects new parameters However, one final issue requires attention before merge: ❌ Missing API endpoint unit tests - New Team Checklist Results✅ Code Quality & Standards
Details:
|
Add comprehensive tests for the new directory operations: API endpoint tests (v1): - test_move_directory_success - test_move_directory_empty_directory - test_move_directory_validation_error - test_move_directory_nested_structure API endpoint tests (v2): - test_move_directory_v2_success/empty/validation - test_delete_directory_v2_success/empty/validation/nested Integration tests for >10 files edge case: - test_move_directory_many_files - test_delete_directory_many_files Add # pragma: no cover for error handlers that require failure injection to test (per CLAUDE.md guidelines). Coverage: 98% (remaining 2% is postgres_search_repository.py which only runs with BASIC_MEMORY_TEST_POSTGRES=1) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 3m 9s —— View job PR Review: Directory Move & Delete Support ✅Comprehensive review against Basic Memory team checklist completed. Todo List:
Executive SummaryThis PR successfully implements directory move and delete functionality with excellent engineering quality. All previous blocking issues have been resolved: ✅ API endpoint tests added - 11 comprehensive tests across v1 and v2 APIs Recommendation: APPROVE ✅ Team Checklist Results✅ Code Quality & Standards
Details:
✅ Testing & Documentation
Test Coverage Breakdown: API Tests (11 total):
Integration Tests (11 total):
✅ Basic Memory Architecture
Architecture Highlights:
✅ Security & Performance
Security Features:
✅ Compatibility
Path Normalization Verified:
Code Quality HighlightsExcellent Implementation Patterns1. Service Layer - Clean Delegation (entity_service.py:872-1038) # move_directory delegates to proven move_entity method (12 existing tests)
await self.move_entity(
identifier=entity.file_path,
destination_path=new_path,
project_config=project_config,
app_config=app_config,
)Strengths:
2. Schema Design (schemas/request.py, schemas/response.py)
3. API Endpoints (knowledge_router.py:235-276, v2/knowledge_router.py:434-581)
4. MCP Tools (move_note.py, delete_note.py)
5. Repository Pattern (entity_repository.py:444-474)
Previous Review Issues - Resolution Status✅ RESOLVED: API Endpoint Tests AddedPrevious: No tests for
✅ RESOLVED: Documentation UpdatedPrevious: CLAUDE.md showed outdated
✅ RESOLVED: Integration Tests AddedPrevious: No integration tests for directory operations
✅ RESOLVED: Windows Path CompatibilityPrevious: Concern about string operations like
Minor Observations (Non-blocking)Line LengthSome string literals in error messages exceed 100 characters (move_note.py:70, 77, 115). These are acceptable per CLAUDE.md style guide as they're user-facing messages. Test CoverageCI tests are still running, but based on code analysis:
SummaryThis PR demonstrates excellent engineering quality across all dimensions: ✅ Clean Architecture - Proper layering from MCP → Client → API → Service → Repository All previous blocking issues resolved. Final RecommendationsReady to merge after CI tests complete successfully. ✅ Post-merge action (per phernandez's note):
Great work on this feature! The directory move/delete functionality is well-designed, thoroughly tested, and maintains the high code quality standards of the Basic Memory project. The consistent rename from |
Summary
is_directoryparameter tomove_noteMCP tool for moving entire directoriesfolder→directoryacross the codebase for consistencyChanges
move_notewithis_directoryparameter, renamedfolder→directoryinwrite_note,canvas, and importersMoveDirectoryRequest,DirectoryMoveResult,DirectoryMoveErrorEntityService.move_directory()methodPOST /move-directoryendpoints (v1 and v2)KnowledgeClient.move_directory()methoddirectoryparameterTest plan
Closes #516
🤖 Generated with Claude Code