fix(mcp): normalize note_types/entity_types/categories on direct call path and reject non-string list elements

Codex review of PR #962 identified two real issues:

1. CLI bypass: the BeforeValidator(parse_str_list) on note_types, entity_types, and
   categories only fires through MCP/Pydantic validation. The CLI path in
   cli/commands/tool.py calls search_notes() directly, so `bm tool search-notes
   --type note,task` arrived as note_types=["note,task"] and matched nothing.
   Fix: add in-body parse_str_list() normalization for all three params (mirroring
   the existing parse_tags() call for tags on the same code path).

2. Silent stringify: parse_str_list used str(raw) in the list branch, so [42] became
   ["42"] before Pydantic saw it, accepting invalid input as a no-result search
   instead of rejecting it. Fix: guard against non-string list elements and return
   the original value unchanged so Pydantic rejects it with a clear error.

Tests added: annotation-level split tests for note_types/entity_types/categories,
non-string-element rejection tests, async direct-call regression for note_types,
and unit-level parse_str_list non-string list tests in test_coerce.py.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>

Author: groksrc

src/basic_memory/mcp/tools/search.py

@@ -899,6 +899,17 @@ async def search_notes(
     if page_size < 1:
         raise ValueError(f"page_size must be >= 1, got {page_size}")
 
+    # Trigger: list params arrived via a direct function call instead of the MCP layer.
+    # Why: the BeforeValidator annotations only run through MCP/Pydantic validation; direct
+    #      callers (e.g. `bm tool search-notes --type note,task` in cli/commands/tool.py,
+    #      which Typer collects as the one-element list ["note,task"]) would otherwise
+    #      forward the comma string as one literal type that matches nothing (#930).
+    # Outcome: comma-split/list normalization applies on every path; parse_str_list is
+    #          idempotent, so MCP-validated input passes through unchanged.
+    note_types = parse_str_list(note_types) if note_types is not None else []
+    entity_types = parse_str_list(entity_types) if entity_types is not None else []
+    categories = parse_str_list(categories) if categories is not None else []
+
     # Avoid mutable-default-argument footguns. Treat None as "no filter".
     # Lowercase note_types so "Chapter" matches the stored "chapter".
     note_types = [t.lower() for t in note_types] if note_types else []

src/basic_memory/utils.py

@@ -619,14 +619,21 @@ def parse_str_list(v: Any) -> List[str]:
         return []
 
     if isinstance(v, list):
+        # Trigger: a list element is not a string (e.g. [42] or ["note", 42]).
+        # Why: str(raw) would silently convert 42 → "42" and let invalid caller data pass
+        #   Pydantic validation as a junk filter, producing a silent no-result search.
+        # Outcome: return the list unchanged so Pydantic rejects it with a clear error.
+        if not all(isinstance(raw, str) for raw in v if raw is not None):
+            return v  # type: ignore[return-value]
+
         # Trigger: a list element may itself be a comma-separated string (e.g. some MCP clients
         #   serialise `["note,task"]` when the caller passed `note_types="note,task"`).
         # Outcome: flatten each element by splitting on commas and stripping whitespace.
         return [
             item.strip()
             for raw in v
             if raw is not None
-            for item in str(raw).split(",")
+            for item in raw.split(",")
             if item and item.strip()
         ]
 

tests/mcp/test_tool_search.py

@@ -2115,3 +2115,161 @@ def test_default_search_type_falls_back_to_text_when_semantic_disabled():
 
     with patch.object(search_module, "get_container", return_value=mock_container):
         assert search_module._default_search_type() == "text"
+
+
+# --- Tests for note_types/entity_types/categories comma-split fix (#930, Codex review) ---
+
+
+def test_search_notes_note_types_annotation_splits_comma_strings():
+    """The note_types parameter annotation must parse every documented input form (#930).
+
+    Direct function calls bypass the BeforeValidator; validate through the same
+    Annotated metadata pydantic applies on the MCP path. The old coerce_list wrapped a
+    bare comma string as the single literal type ["note,task"]; parse_str_list splits it.
+    """
+    annotation = inspect.signature(search_notes).parameters["note_types"].annotation
+    adapter = TypeAdapter(annotation)
+
+    real_list = adapter.validate_python(["note", "task"])
+    comma_string = adapter.validate_python("note,task")
+    json_string = adapter.validate_python('["note", "task"]')
+    single_string = adapter.validate_python("note")
+    comma_in_list = adapter.validate_python(["note,task"])
+
+    assert real_list == ["note", "task"]
+    assert comma_string == real_list, "comma string must behave like the real list"
+    assert json_string == real_list
+    assert single_string == ["note"]
+    assert comma_in_list == real_list, "list with comma element must be flattened"
+
+
+def test_search_notes_entity_types_annotation_splits_comma_strings():
+    """The entity_types parameter annotation must parse every documented input form (#930)."""
+    annotation = inspect.signature(search_notes).parameters["entity_types"].annotation
+    adapter = TypeAdapter(annotation)
+
+    real_list = adapter.validate_python(["entity", "observation"])
+    comma_string = adapter.validate_python("entity,observation")
+    comma_in_list = adapter.validate_python(["entity,observation"])
+
+    assert real_list == ["entity", "observation"]
+    assert comma_string == real_list
+    assert comma_in_list == real_list
+
+
+def test_search_notes_categories_annotation_splits_comma_strings():
+    """The categories parameter annotation must parse every documented input form (#930)."""
+    annotation = inspect.signature(search_notes).parameters["categories"].annotation
+    adapter = TypeAdapter(annotation)
+
+    real_list = adapter.validate_python(["requirement", "decision"])
+    comma_string = adapter.validate_python("requirement,decision")
+    comma_in_list = adapter.validate_python(["requirement,decision"])
+
+    assert real_list == ["requirement", "decision"]
+    assert comma_string == real_list
+    assert comma_in_list == real_list
+
+
+def test_search_notes_note_types_annotation_rejects_non_string_list_elements():
+    """note_types=[42] must fail Pydantic validation, not be stringified to ['42'].
+
+    parse_str_list used str(raw) to coerce list elements, silently accepting [42] as
+    ["42"]. The fix guards against non-string list elements and returns the original
+    value so Pydantic rejects it with a clear error.
+    """
+    from pydantic import ValidationError
+
+    annotation = inspect.signature(search_notes).parameters["note_types"].annotation
+    adapter = TypeAdapter(annotation)
+
+    with pytest.raises(ValidationError):
+        adapter.validate_python([42])
+    with pytest.raises(ValidationError):
+        adapter.validate_python(["note", 42])
+
+    # All-string lists remain valid.
+    assert adapter.validate_python(["note", "task"]) == ["note", "task"]
+
+
+def test_search_notes_entity_types_annotation_rejects_non_string_list_elements():
+    """entity_types=[42] must fail Pydantic validation, not be stringified."""
+    from pydantic import ValidationError
+
+    annotation = inspect.signature(search_notes).parameters["entity_types"].annotation
+    adapter = TypeAdapter(annotation)
+
+    with pytest.raises(ValidationError):
+        adapter.validate_python([42])
+    with pytest.raises(ValidationError):
+        adapter.validate_python(["entity", 42])
+
+    assert adapter.validate_python(["entity", "observation"]) == ["entity", "observation"]
+
+
+def test_search_notes_categories_annotation_rejects_non_string_list_elements():
+    """categories=[42] must fail Pydantic validation, not be stringified."""
+    from pydantic import ValidationError
+
+    annotation = inspect.signature(search_notes).parameters["categories"].annotation
+    adapter = TypeAdapter(annotation)
+
+    with pytest.raises(ValidationError):
+        adapter.validate_python([42])
+    with pytest.raises(ValidationError):
+        adapter.validate_python(["requirement", 42])
+
+    assert adapter.validate_python(["requirement", "decision"]) == ["requirement", "decision"]
+
+
+@pytest.mark.asyncio
+async def test_search_notes_direct_call_splits_comma_note_types(client, test_project):
+    """Direct callers bypass the BeforeValidator, so the body must normalize note_types.
+
+    Regression for the CLI path: `bm tool search-notes --type note,task` calls this
+    function directly with Typer's collected list ["note,task"], which must split into
+    ["note", "task"] and match the note correctly (#930, Codex review follow-up).
+    """
+    await write_note(
+        project=test_project.name,
+        title="Direct NoteType Split Note",
+        directory="test",
+        content="# Direct NoteType Split Note\nNoteTypeSplitToken body",
+    )
+
+    async def found(note_types_value: list[str] | None) -> bool:
+        result = await search_notes(
+            project=test_project.name,
+            query="NoteTypeSplitToken",
+            search_type="text",
+            output_format="json",
+            note_types=note_types_value,
+        )
+        assert isinstance(result, dict), f"search failed: {result}"
+        return any(r["title"] == "Direct NoteType Split Note" for r in result["results"])
+
+    assert await found(None), "no filter must match (sanity)"
+    assert await found(["note"]), "plain single-type list must match (sanity)"
+    # The CLI regression: Typer collects --type note,task as the single element "note,task".
+    assert await found(["note,task"]), "comma list element must be flattened and match 'note'"
+    # Negative control: a specific nonexistent type must not match.
+    assert not await found(["nonexistent_type"])
+
+
+def test_search_notes_parse_str_list_rejects_non_string_list_elements_in_place():
+    """parse_str_list must return non-str list elements unchanged for Pydantic rejection.
+
+    The old implementation used str(raw) which silently coerced [42] -> ['42'],
+    causing bad caller data to become silent no-result searches instead of a
+    clear Pydantic validation error.
+    """
+    from basic_memory.utils import parse_str_list
+
+    # Non-string list elements pass through unchanged.
+    assert parse_str_list([42]) == [42]  # type: ignore[arg-type]
+    assert parse_str_list(["ok", 42]) == ["ok", 42]  # type: ignore[arg-type]
+    assert parse_str_list([{"a": 1}]) == [{"a": 1}]  # type: ignore[arg-type]
+
+    # All-string lists still work correctly.
+    assert parse_str_list(["note", "task"]) == ["note", "task"]
+    assert parse_str_list(["note,task"]) == ["note", "task"]

tests/test_coerce.py

@@ -176,3 +176,19 @@ def test_int_passthrough_for_pydantic_rejection(self):
     def test_dict_passthrough_for_pydantic_rejection(self):
         value = {"a": 1}
         assert parse_str_list(value) is value  # type: ignore[arg-type]
+
+    # --- Non-string list elements pass through unchanged (Codex review fix) ---
+
+    def test_int_list_passthrough_for_pydantic_rejection(self):
+        """Lists with non-string elements must not be stringified ([42] → ['42'])."""
+        value = [42]
+        assert parse_str_list(value) is value  # type: ignore[arg-type]
+
+    def test_mixed_list_passthrough_for_pydantic_rejection(self):
+        """One non-string element poisons the whole list — no partial coercion."""
+        value = ["note", 42]
+        assert parse_str_list(value) is value  # type: ignore[arg-type]
+
+    def test_dict_list_passthrough_for_pydantic_rejection(self):
+        value = [{"a": 1}]
+        assert parse_str_list(value) is value  # type: ignore[arg-type]