From 0811c48252877dda11fff9b3b2fbd48cda92529c Mon Sep 17 00:00:00 2001 From: Drew Cain Date: Thu, 11 Jun 2026 09:38:36 -0500 Subject: [PATCH] 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 Signed-off-by: Drew Cain --- src/basic_memory/mcp/tools/search.py | 11 ++ src/basic_memory/utils.py | 9 +- tests/mcp/test_tool_search.py | 158 +++++++++++++++++++++++++++ tests/test_coerce.py | 16 +++ 4 files changed, 193 insertions(+), 1 deletion(-) diff --git a/src/basic_memory/mcp/tools/search.py b/src/basic_memory/mcp/tools/search.py index c6e8778d..b8072f96 100644 --- a/src/basic_memory/mcp/tools/search.py +++ b/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 [] diff --git a/src/basic_memory/utils.py b/src/basic_memory/utils.py index 43ed9490..5df2b1ee 100644 --- a/src/basic_memory/utils.py +++ b/src/basic_memory/utils.py @@ -619,6 +619,13 @@ 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. @@ -626,7 +633,7 @@ def parse_str_list(v: Any) -> List[str]: 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() ] diff --git a/tests/mcp/test_tool_search.py b/tests/mcp/test_tool_search.py index d178a5f0..36a96716 100644 --- a/tests/mcp/test_tool_search.py +++ b/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"] diff --git a/tests/test_coerce.py b/tests/test_coerce.py index 1c826058..91cd42cc 100644 --- a/tests/test_coerce.py +++ b/tests/test_coerce.py @@ -176,3 +176,19 @@ class TestParseStrList: 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]