fix(mcp): tighten search_notes tags input and normalize for direct callers (#941)

Refs #910. Follow-up to #932.

Signed-off-by: phernandez <paul@basicmemory.com>

Author: phernandez

src/basic_memory/mcp/tools/search.py

@@ -11,7 +11,13 @@
 from pydantic import AliasChoices, BeforeValidator, Field
 
 from basic_memory.config import ConfigManager, has_cloud_credentials
-from basic_memory.utils import build_canonical_permalink, coerce_dict, coerce_list, parse_tags
+from basic_memory.utils import (
+    build_canonical_permalink,
+    coerce_dict,
+    coerce_list,
+    parse_tags,
+    strict_search_tags,
+)
 from basic_memory.mcp.async_client import (
     _explicit_routing,
     _force_local_mode,
@@ -676,13 +682,15 @@ async def search_notes(
         Dict[str, Any] | None,
         BeforeValidator(coerce_dict),
     ] = None,
-    # parse_tags, not coerce_list: tags="a,b" must split into ["a", "b"] to match the
-    # tag: query shorthand below and write_note's documented tags convention (#910).
-    # coerce_list would wrap the comma string as the single literal tag ["a,b"],
-    # which matches nothing.
+    # strict_search_tags, not coerce_list: tags="a,b" must split into ["a", "b"] to
+    # match the tag: query shorthand below and write_note's documented tags convention
+    # (#910). coerce_list would wrap the comma string as the single literal tag
+    # ["a,b"], which matches nothing. Unlike bare parse_tags, the strict wrapper only
+    # splits str/list/None and lets Pydantic reject other types (42, {"a": 1}) with a
+    # clear validation error instead of stringifying them into junk tags.
     tags: Annotated[
         List[str] | None,
-        BeforeValidator(parse_tags),
+        BeforeValidator(strict_search_tags),
     ] = None,
     status: Optional[str] = None,
     min_similarity: Annotated[
@@ -893,6 +901,15 @@ async def search_notes(
     # so preserve their original casing (unlike the lowercased note_types).
     categories = categories or []
 
+    # Trigger: tags arrived via a direct function call instead of the MCP layer.
+    # Why: the BeforeValidator above only runs through MCP/Pydantic validation; direct
+    #      callers (e.g. `bm tool search-notes --tag a,b` in cli/commands/tool.py, which
+    #      Typer collects as the one-element list ["a,b"]) would otherwise forward the
+    #      comma string as one literal tag that matches nothing (#910).
+    # Outcome: comma-split/list normalization applies on every path; parse_tags is
+    #          idempotent, so MCP-validated input passes through unchanged.
+    tags = parse_tags(tags) or None
+
     # Parse tag:<value> shorthand at tool level so it works with all search modes.
     # Handles "tag:security", "tag:coffee tag:brewing", "tag:coffee AND tag:brewing".
     # Without this, hybrid/vector modes fail because they require non-empty text,

src/basic_memory/utils.py

@@ -568,6 +568,38 @@ def parse_tags(tags: Union[List[str], str, None]) -> List[str]:
         return []
 
 
+def strict_search_tags(v: Any) -> Any:
+    """Strictly coerce tag input at the search_notes tool boundary.
+
+    parse_tags stringifies anything (42 -> ["42"], {"a": 1} -> junk tags), which would
+    turn caller type mistakes into silent no-result searches. At the tool boundary only
+    str, all-string lists, and None are valid tag inputs; everything else — including
+    lists with non-string elements like [42] — passes through unchanged so Pydantic
+    rejects it with a clear validation error.
+
+    JSON array strings (the MCP clients-serialize-arrays-as-strings path) get the same
+    all-string check: '[42]' or '["ok", 42]' would otherwise be stringified by
+    parse_tags' recursive JSON handling before Pydantic ever sees the bad elements.
+    """
+    if isinstance(v, list) and not all(isinstance(item, str) for item in v):
+        return v
+    # Trigger: a str that looks like a JSON array, mirroring parse_tags' detection.
+    # Why: parse_tags recursively parses JSON arrays, stringifying non-string elements
+    #   ('[42]' -> ["42"]) and hiding the type error from Pydantic.
+    # Outcome: malformed arrays pass through unchanged so Pydantic rejects them; valid
+    #   all-string arrays and plain comma strings still delegate to parse_tags.
+    if isinstance(v, str) and v.strip().startswith("[") and v.strip().endswith("]"):
+        try:
+            parsed = json.loads(v)
+        except json.JSONDecodeError:
+            parsed = None
+        if isinstance(parsed, list) and not all(isinstance(item, str) for item in parsed):
+            return v
+    if v is None or isinstance(v, (str, list)):
+        return parse_tags(v)
+    return v
+
+
 def coerce_list(v: Any) -> Any:
     """Coerce string input to list for MCP clients that serialize lists as strings."""
     if v is None:

tests/mcp/test_tool_search.py

@@ -1694,6 +1694,175 @@ async def found(tags_value: object) -> bool:
         assert not await found("gamma")
 
 
+def test_search_notes_tags_annotation_rejects_non_string_types():
+    """Unsupported tag types must fail validation, not be stringified (#932 follow-up).
+
+    Bare parse_tags coerces anything to strings (42 -> ["42"], {"a": 1} -> junk tags),
+    silently turning caller mistakes into no-result searches. The strict_search_tags
+    wrapper only normalizes str/list/None and lets Pydantic reject everything else.
+    """
+    from pydantic import ValidationError
+
+    annotation = inspect.signature(search_notes).parameters["tags"].annotation
+    adapter = TypeAdapter(annotation)
+
+    with pytest.raises(ValidationError):
+        adapter.validate_python(42)
+    with pytest.raises(ValidationError):
+        adapter.validate_python({"a": 1})
+
+    # Lists with non-string elements must also fail, not be stringified ([42] -> ["42"]).
+    with pytest.raises(ValidationError):
+        adapter.validate_python([42])
+    with pytest.raises(ValidationError):
+        adapter.validate_python([{"a": 1}])
+    with pytest.raises(ValidationError):
+        adapter.validate_python(["ok", 42])
+
+    # JSON-array strings with non-string elements must fail the same way — parse_tags
+    # would otherwise recursively stringify them before Pydantic validates List[str].
+    with pytest.raises(ValidationError):
+        adapter.validate_python("[42]")
+    with pytest.raises(ValidationError):
+        adapter.validate_python('[{"a": 1}]')
+    with pytest.raises(ValidationError):
+        adapter.validate_python('["ok", 42]')
+
+    # All-string lists and all-string JSON-array strings remain valid.
+    assert adapter.validate_python(["a", "b"]) == ["a", "b"]
+    assert adapter.validate_python('["a","b"]') == ["a", "b"]
+
+    # None stays a valid "no filter" input.
+    assert adapter.validate_python(None) in (None, [])
+
+
+@pytest.mark.asyncio
+async def test_search_notes_tags_invalid_type_rejected_via_mcp(mcp, client, test_project):
+    """tags=42 through the real MCP layer must raise a validation error (#932 follow-up)."""
+    from fastmcp import Client
+    from fastmcp.exceptions import ToolError
+
+    async with Client(mcp) as mcp_client:
+        with pytest.raises(ToolError):
+            await mcp_client.call_tool(
+                "search_notes",
+                {
+                    "project": test_project.name,
+                    "query": "anything",
+                    "tags": 42,
+                },
+            )
+        with pytest.raises(ToolError):
+            await mcp_client.call_tool(
+                "search_notes",
+                {
+                    "project": test_project.name,
+                    "query": "anything",
+                    "tags": {"a": 1},
+                },
+            )
+        # Lists with non-string elements must be rejected too, not stringified.
+        with pytest.raises(ToolError):
+            await mcp_client.call_tool(
+                "search_notes",
+                {
+                    "project": test_project.name,
+                    "query": "anything",
+                    "tags": [42],
+                },
+            )
+        with pytest.raises(ToolError):
+            await mcp_client.call_tool(
+                "search_notes",
+                {
+                    "project": test_project.name,
+                    "query": "anything",
+                    "tags": [{"a": 1}],
+                },
+            )
+        with pytest.raises(ToolError):
+            await mcp_client.call_tool(
+                "search_notes",
+                {
+                    "project": test_project.name,
+                    "query": "anything",
+                    "tags": ["ok", 42],
+                },
+            )
+        # JSON-array strings with non-string elements (clients that serialize arrays as
+        # strings) must be rejected too, not recursively stringified by parse_tags.
+        with pytest.raises(ToolError):
+            await mcp_client.call_tool(
+                "search_notes",
+                {
+                    "project": test_project.name,
+                    "query": "anything",
+                    "tags": "[42]",
+                },
+            )
+        with pytest.raises(ToolError):
+            await mcp_client.call_tool(
+                "search_notes",
+                {
+                    "project": test_project.name,
+                    "query": "anything",
+                    "tags": '[{"a": 1}]',
+                },
+            )
+        with pytest.raises(ToolError):
+            await mcp_client.call_tool(
+                "search_notes",
+                {
+                    "project": test_project.name,
+                    "query": "anything",
+                    "tags": '["ok", 42]',
+                },
+            )
+        # Sanity: a valid all-string JSON-array string is still accepted.
+        await mcp_client.call_tool(
+            "search_notes",
+            {
+                "project": test_project.name,
+                "query": "anything",
+                "tags": '["a","b"]',
+            },
+        )
+
+
+@pytest.mark.asyncio
+async def test_search_notes_direct_call_splits_comma_tags(client, test_project):
+    """Direct callers bypass the BeforeValidator, so the body must normalize tags.
+
+    Regression for the CLI path: `bm tool search-notes --tag alpha,beta` calls this
+    function directly with Typer's collected list ["alpha,beta"], which must split
+    into ["alpha", "beta"] instead of matching nothing (#910, #932 follow-up).
+    """
+    await write_note(
+        project=test_project.name,
+        title="Direct Tag Split Note",
+        directory="test",
+        content="# Direct Tag Split Note\nDirectTagToken body",
+        tags=["alpha", "beta"],
+    )
+
+    async def found(tags_value: list[str] | None) -> bool:
+        result = await search_notes(
+            project=test_project.name,
+            query="DirectTagToken",
+            search_type="text",
+            output_format="json",
+            tags=tags_value,
+        )
+        assert isinstance(result, dict), f"search failed: {result}"
+        return any(r["title"] == "Direct Tag Split Note" for r in result["results"])
+
+    assert await found(["alpha"]), "plain tag list must match (sanity)"
+    # The CLI regression: Typer collects --tag alpha,beta as the single element "alpha,beta".
+    assert await found(["alpha,beta"])
+    # Negative control: the filter is still applied.
+    assert not await found(["gamma"])
+
+
 # --- Tests for text output format (#641) -----------------------------------
 
 

tests/test_coerce.py

@@ -1,9 +1,9 @@
-"""Tests for coerce_list and coerce_dict utility functions.
+"""Tests for coerce_list, coerce_dict, and strict_search_tags utility functions.
 
 These must fail until the helpers are implemented in utils.py.
 """
 
-from basic_memory.utils import coerce_list, coerce_dict
+from basic_memory.utils import coerce_list, coerce_dict, strict_search_tags
 
 
 class TestCoerceList:
@@ -33,6 +33,64 @@ def test_int_passthrough(self):
         assert coerce_list(42) == 42
 
 
+class TestStrictSearchTags:
+    """Tests for strict_search_tags (the search_notes tags boundary coercer)."""
+
+    def test_none_parses_to_empty_list(self):
+        assert strict_search_tags(None) == []
+
+    def test_comma_string_splits(self):
+        assert strict_search_tags("a,b") == ["a", "b"]
+
+    def test_list_with_comma_element_splits(self):
+        assert strict_search_tags(["alpha,beta"]) == ["alpha", "beta"]
+
+    def test_plain_list_passthrough(self):
+        assert strict_search_tags(["a", "b"]) == ["a", "b"]
+
+    def test_json_array_string(self):
+        assert strict_search_tags('["a", "b"]') == ["a", "b"]
+
+    def test_int_passthrough_for_pydantic_rejection(self):
+        """Unsupported types pass through unchanged so Pydantic rejects them."""
+        assert strict_search_tags(42) == 42
+
+    def test_dict_passthrough_for_pydantic_rejection(self):
+        value = {"a": 1}
+        assert strict_search_tags(value) is value
+
+    def test_int_list_passthrough_for_pydantic_rejection(self):
+        """Lists with non-string elements pass through unchanged so Pydantic rejects them."""
+        value = [42]
+        assert strict_search_tags(value) is value
+
+    def test_dict_list_passthrough_for_pydantic_rejection(self):
+        value = [{"a": 1}]
+        assert strict_search_tags(value) is value
+
+    def test_mixed_list_passthrough_for_pydantic_rejection(self):
+        """One bad element poisons the whole list — no partial stringification."""
+        value = ["ok", 42]
+        assert strict_search_tags(value) is value
+
+    def test_json_array_string_with_int_passthrough_for_pydantic_rejection(self):
+        """A JSON-array string with non-string elements must not be stringified."""
+        value = "[42]"
+        assert strict_search_tags(value) is value
+
+    def test_json_array_string_with_dict_passthrough_for_pydantic_rejection(self):
+        value = '[{"a": 1}]'
+        assert strict_search_tags(value) is value
+
+    def test_json_array_string_mixed_passthrough_for_pydantic_rejection(self):
+        """One bad element poisons the whole JSON-array string — no partial parse."""
+        value = '["ok", 42]'
+        assert strict_search_tags(value) is value
+
+    def test_json_array_string_all_strings_still_parses(self):
+        assert strict_search_tags('["a","b"]') == ["a", "b"]
+
+
 class TestCoerceDict:
     """Tests for coerce_dict."""