Skip to content

Conversation

@phernandez
Copy link
Member

Summary

Resolves #392 - Simplified the search_notes MCP tool schema for better Claude Desktop compatibility.

Changes

  • Changed types: Optional[List[str]] = Nonetypes: List[str] = []
  • Changed entity_types: Optional[List[str]] = Noneentity_types: List[str] = []
  • Updated validation comment for clarity

Benefits

  • Cleaner MCP schema without Optional wrappers
  • Better compatibility with Claude Desktop
  • Backward compatible - empty lists behave like None
  • No test changes required

Testing

Reviewed existing tests - all already use list format and are compatible with the new signature.

🤖 Generated with Claude Code

Changed types and entity_types parameters from Optional[List[str]]
to List[str] with empty list defaults. This simplifies the MCP tool
schema for better compatibility with Claude Desktop while maintaining
backward compatibility through empty list validation.

Resolves #392

Co-authored-by: Paul Hernandez <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 22, 2025

Claude finished @phernandez's task —— View job


Code Review Complete ✅

Review Checklist

Code Quality & Standards

  • ✅ Follows Basic Memory's coding conventions in CLAUDE.md
  • ✅ Python 3.12+ type annotations and async patterns
  • ✅ SQLAlchemy 2.0 best practices (N/A - no DB changes)
  • ✅ FastAPI and Typer conventions followed (N/A - MCP tool only)
  • ✅ 100-character line length limit maintained
  • ✅ No commented-out code blocks

Testing & Documentation

  • ✅ Unit tests for new functions/methods (N/A - signature change only, existing tests cover it)
  • ✅ Integration tests for new MCP tools (N/A - modification to existing tool)
  • ✅ Test coverage for edge cases (Verified - all existing tests use list format and remain compatible)
  • ⚠️ Documentation updated (docstring updated, but MCP tool documentation may need updates elsewhere)
  • ✅ CLAUDE.md updated if conventions change (N/A - no convention changes)

Basic Memory Architecture

  • ✅ MCP tools follow atomic, composable design
  • ✅ Database changes include Alembic migrations (N/A - no DB schema changes)
  • ✅ Preserves local-first architecture principles
  • ✅ Knowledge graph operations maintain consistency
  • ✅ Markdown file handling preserves integrity (N/A)
  • ✅ AI-human collaboration patterns followed

Security & Performance

  • ✅ No hardcoded secrets or credentials
  • ✅ Input validation for MCP tools (Empty list validation preserved)
  • ✅ Proper error handling and logging
  • ✅ Performance considerations addressed
  • ✅ No sensitive data in logs or commits

Detailed Review

Summary

This PR successfully simplifies the search_notes MCP tool schema by removing Optional wrappers from list parameters. The change is well-designed, backward compatible, and addresses a real Claude Desktop compatibility issue.

Changes Analysis

File: src/basic_memory/mcp/tools/search.py

Lines 208-209 (Before):

types: Optional[List[str]] = None,
entity_types: Optional[List[str]] = None,

Lines 208-209 (After):

types: List[str] = [],
entity_types: List[str] = [],

Line 348 (Comment Update):

# Add optional filters if provided (empty lists are treated as no filter)

Strengths

  1. ✅ Cleaner Schema: Removing Optional wrappers makes the MCP tool schema more straightforward for Claude Desktop and other MCP clients to consume.

  2. ✅ Backward Compatible: The change maintains backward compatibility because:

    • Empty lists are falsy in Python, just like None
    • The validation logic if entity_types: and if types: works identically for both [] and None
    • All existing tests already use list format (verified in both unit and integration tests)
  3. ✅ Proper Issue Resolution: Addresses the root cause identified in issue [BUG] User had a problem calling search_notes tool #392 - Claude Desktop had trouble with Optional[List[str]] schema complexity.

  4. ✅ Test Compatibility: I reviewed all test files:

    • tests/mcp/test_tool_search.py: All tests pass lists directly (e.g., types=["note"], entity_types=["entity"])
    • test-int/mcp/test_search_integration.py: All integration tests use list format (e.g., entity_types: ["entity"])
    • No test changes needed
  5. ✅ Type Safety: Maintains Python 3.12+ type annotations correctly with List[str] defaults.

  6. ✅ Clear Documentation: Updated inline comment explains the behavior clearly.

Minor Concerns

  1. ⚠️ Documentation Elsewhere: While the docstring is updated, there may be other documentation that references these parameters. Consider checking:

    • README.md MCP tool examples
    • Any API documentation
    • User guides that show search_notes usage
  2. ℹ️ SearchQuery Schema Mismatch: The SearchQuery Pydantic model in src/basic_memory/schemas/search.py:52-53 still uses Optional[List[str]] and Optional[List[SearchItemType]]. This is not a problem because:

    • The MCP tool normalizes empty lists before passing to SearchQuery
    • SearchQuery accepts both None and empty lists equally
    • However, for consistency, you might consider updating SearchQuery in a future PR

Code Quality Assessment

Line Length: ✅ All lines under 100 characters
Type Annotations: ✅ Proper Python 3.12+ types
Async Patterns: ✅ Uses async context manager correctly
Error Handling: ✅ Maintained (comprehensive error formatting)
Logging: ✅ Appropriate logger usage
Code Style: ✅ Follows ruff formatting standards

Tested Scenarios (Verified in Test Suite)

All these scenarios are covered by existing tests:

  • ✅ Basic search without filters
  • ✅ Search with types=["note"] filter
  • ✅ Search with entity_types=["entity"] filter
  • ✅ Search with entity_types=["observation"] filter
  • ✅ Search with date filters
  • ✅ Pagination
  • ✅ All search types (text, title, permalink, permalink_match)
  • ✅ Boolean operators (AND, OR, NOT)
  • ✅ Empty result handling

Recommendations

  1. ✅ Approve and Merge: This PR is ready to merge. The change is minimal, well-tested, and solves a real user problem.

  2. Future Enhancement: Consider a follow-up PR to update SearchQuery schema for consistency (optional, not blocking).

  3. Documentation Check: Before or after merge, verify that any external documentation reflects the simplified parameter types.

Verdict

APPROVED

This is a clean, minimal fix that:

  • Solves the Claude Desktop compatibility issue reported in [BUG] User had a problem calling search_notes tool #392
  • Maintains full backward compatibility
  • Requires no test changes (all tests already compatible)
  • Follows Basic Memory coding standards
  • Improves schema clarity and simplicity

Great work on choosing the simpler solution over the more complex Union[str, List[str]] approach! This is exactly the right fix for the problem.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] User had a problem calling search_notes tool

2 participants