Claude finished @phernandez's task in 1m 52s —— View job

Code Review — PR #912: fix(core): make note_types search filter case-insensitive

• Read changed files and gather context
• Review code quality and standards
• Review testing coverage
• Review security and performance
• Post final review

Summary

This is a clean, minimal fix for a real bug. The root cause is correct: the SQLite backend's json_extract(...) IN (...) and the Postgres backend's JSONB @> containment were both exact-match comparisons, while the documented behavior and MCP tool layer both expect case-insensitive matching. Wrapping the stored value in LOWER() and lowercasing the filter parameters fixes the mismatch on both backends.

Checklist

Code Quality & Standards

• Follows Basic Memory's coding conventions — parameter names, loop structure, and style all match the adjacent categories filter block
• Python 3.12+ type annotations and async patterns — no changes to signatures
• SQLAlchemy 2.0 best practices — continues using parameterized text() queries
• 100-character line length limit — longest new line in SQLite file is ~67 chars; Postgres line is within limit
• No commented-out code blocks
• Literate programming style — new comments follow the required Trigger/Why/Outcome pattern

Testing & Documentation

• Integration tests added (test-int/mcp/test_search_note_types_case_insensitive.py)
• Control test verifies the previously-working lowercase path still works (no regression)
• Tests use real DB + real ASGI + MCP client — no mocks
• Missing test case: neither test covers note_types=["chapter"] (lowercase filter) against a note with type: Chapter (uppercase stored). The main test uses note_types=["Chapter"], which the MCP tool lowercases to "chapter" before calling the repo — but the test title "filter is case-insensitive" implies the filter value casing varies. Adding this explicit case removes ambiguity about what "case-insensitive" means.
• No unit test in tests/repository/: the changed code is in _build_fts_query_parts. The integration tests provide full coverage at the MCP level, but a unit test for the repository query-building step (like the existing ones in tests/repository/test_search_repository.py) would be closer to the changed code and easier to read as documentation. Not a blocking concern if 100% coverage is already met by the integration path.

Basic Memory Architecture

• No schema changes (no Alembic migration needed)
• Local-first architecture preserved
• Minimal diff — only the note_types filter clause changed, nothing else

Security & Performance

• SQL injection protection preserved — values are still bound as named parameters (:note_type_0, :note_type_1, etc.)
• No hardcoded secrets or credentials
• The json.dumps removal in Postgres is safe — the old {"note_type": note_type} JSON-document containment pattern is replaced with a scalar extraction, which is simpler and equally correct
• Minor performance note: LOWER(json_extract(...)) and LOWER(metadata->>'note_type') are not index-sargable, so a future index on note_type would not be used by this filter. Since metadata is a JSON/JSONB column with no current index on note_type, this is a non-issue in practice — but worth keeping in mind if note_types filtering becomes a hot path.

Compatibility

• No file path comparisons involved
• No emojis or non-ASCII characters added

Specific Code Notes

SQLite (sqlite_search_repository.py:819–828): The change from

params[param_name] = t
# ...
f"json_extract(search_index.metadata, '$.note_type') IN ({placeholders})"

to

params[param_name] = t.lower()
# ...
"LOWER(json_extract(search_index.metadata, '$.note_type')) IN ({placeholders})"

is correct and matches the pattern of the categories filter immediately above it.

Postgres (postgres_search_repository.py:787–795): The switch from JSONB @> containment (exact-match on a JSON document) to scalar text extraction + LOWER() + IN is the right approach. The previous code required json.dumps({"note_type": note_type}) to construct a containment document — the new pattern is simpler and semantically clearer. The json import is still used elsewhere in the file (json.loads in the result-mapping loop), so no unused import is created.

Behavior symmetry: Both backends now lowercase the stored value at query time via LOWER() and lowercase the filter values at bind time via .lower(). This means any combination of casing in the filter or stored frontmatter will match, which is the documented intent.

Verdict

Approve with minor suggestions. The fix is correct, the SQL injection protection is intact, and the integration tests validate both the fixed case and the regression guard. The two items above (additional test for lowercase-filter-vs-uppercase-stored, and a repository-layer unit test) would improve test documentation but are not blockers for merging.