From df485aa5a461d6b0ad53623b012bf9121136624f Mon Sep 17 00:00:00 2001 From: Paul Hernandez <60959+phernandez@users.noreply.github.com> Date: Wed, 10 Jun 2026 14:04:56 -0500 Subject: [PATCH] fix(mcp): tighten search_notes tags input and normalize for direct callers (#941) Refs #910. Follow-up to #932. Signed-off-by: phernandez --- src/basic_memory/mcp/tools/search.py | 29 ++++- src/basic_memory/utils.py | 32 +++++ tests/mcp/test_tool_search.py | 169 +++++++++++++++++++++++++++ tests/test_coerce.py | 62 +++++++++- 4 files changed, 284 insertions(+), 8 deletions(-) diff --git a/src/basic_memory/mcp/tools/search.py b/src/basic_memory/mcp/tools/search.py index 562ed411..08f87bc1 100644 --- a/src/basic_memory/mcp/tools/search.py +++ b/src/basic_memory/mcp/tools/search.py @@ -11,7 +11,13 @@ from fastmcp import Context 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: 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, diff --git a/src/basic_memory/utils.py b/src/basic_memory/utils.py index 33db10e1..05249498 100644 --- a/src/basic_memory/utils.py +++ b/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: diff --git a/tests/mcp/test_tool_search.py b/tests/mcp/test_tool_search.py index 9acecafa..d178a5f0 100644 --- a/tests/mcp/test_tool_search.py +++ b/tests/mcp/test_tool_search.py @@ -1694,6 +1694,175 @@ async def test_search_notes_tags_comma_string_filters_via_mcp(mcp, client, test_ 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) ----------------------------------- diff --git a/tests/test_coerce.py b/tests/test_coerce.py index d8d972b6..a3aebc82 100644 --- a/tests/test_coerce.py +++ b/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 @@ class TestCoerceList: 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."""