Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/basic_memory/mcp/tools/search.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
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
from basic_memory.utils import build_canonical_permalink, coerce_dict, coerce_list, parse_tags
from basic_memory.mcp.async_client import (
_explicit_routing,
_force_local_mode,
Expand Down Expand Up @@ -676,9 +676,13 @@ 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.
tags: Annotated[
List[str] | None,
BeforeValidator(coerce_list),
BeforeValidator(parse_tags),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject malformed tag values before parsing

When an MCP client sends a malformed tags value such as a JSON number or object, this validator now accepts it because parse_tags stringifies unsupported types (for example 42 becomes ["42"]) before Pydantic validates List[str]. With the previous coerce_list path those inputs were left as non-lists and rejected, so this weakens the tool boundary and turns caller mistakes into confusing no-result searches with bogus tag filters. Consider wrapping parse_tags here so only str, list, or None are accepted, or tightening parse_tags without breaking its frontmatter callers.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in follow-up PR #941: the tags BeforeValidator now uses a new strict_search_tags wrapper (utils.py) that only normalizes str/list/None via parse_tags and passes any other type through unchanged, so Pydantic rejects tags=42 / tags={"a": 1} with a clear validation error instead of stringifying them into silent no-result searches. parse_tags itself is unchanged for its frontmatter/write_note callers.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Normalize tags for direct tool callers too

Because the comma splitting lives only in the BeforeValidator, it is skipped by direct callers of search_notes; I checked src/basic_memory/cli/commands/tool.py, and the basic-memory tool search-notes command imports this function as mcp_search and calls it directly with Typer's tags list. In that path, --tag alpha,beta is still forwarded as ["alpha,beta"], so the search looks for a literal comma tag and returns no matches even though the newly documented MCP behavior says comma-separated tags should match tag: shorthand/write-note conventions. Consider normalizing tags inside search_notes before it is merged into SearchQuery, not only at the MCP validation boundary.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in follow-up PR #941: search_notes now also runs parse_tags(tags) in the function body (alongside the other filter-param normalization, before the tag: shorthand merge and SearchQuery construction), so direct callers like bm tool search-notes --tag alpha,beta (cli/commands/tool.py) get the same comma-split semantics as the MCP path. The BeforeValidator is retained for boundary strictness, and parse_tags is idempotent so MCP-validated input passes through unchanged. Covered by a direct-call regression test.

] = None,
status: Optional[str] = None,
min_similarity: Annotated[
Expand Down Expand Up @@ -795,7 +799,9 @@ async def search_notes(
observations whose category matches exactly.
after_date: Optional date filter for recent content (e.g., "1 week", "2d", "2024-01-01")
metadata_filters: Optional structured frontmatter filters (e.g., {"status": "in-progress"})
tags: Optional tag filter (frontmatter tags); shorthand for metadata_filters["tags"]
tags: Optional tag filter (frontmatter tags); shorthand for metadata_filters["tags"].
Accepts a list (["a", "b"]) or a comma-separated string ("a,b"), matching the
write_note tags convention and the tag: query shorthand.
status: Optional status filter (frontmatter status); shorthand for metadata_filters["status"]
min_similarity: Optional float to override the global semantic_min_similarity threshold
for this query. E.g., 0.0 to see all vector results, or 0.8 for high precision.
Expand Down
72 changes: 72 additions & 0 deletions tests/mcp/test_tool_search.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
"""Tests for search MCP tools."""

import inspect

import pytest
from contextlib import asynccontextmanager
from datetime import datetime, timedelta
from types import SimpleNamespace
from typing import cast

from pydantic import TypeAdapter

from basic_memory.mcp.tools import write_note
from basic_memory.mcp.tools.search import (
search_notes,
Expand Down Expand Up @@ -1622,6 +1626,74 @@ async def fake_resolve(client, query, project, context):
assert captured_payload["text"] == "authentication"


# --- Tests for comma-separated tags parameter (#910) ----------------------------


def test_search_notes_tags_annotation_splits_comma_strings():
"""The tags parameter annotation must parse every documented input form (#910).

Direct function calls bypass the BeforeValidator, so validate through the same
Annotated metadata pydantic applies on the MCP path. coerce_list wrapped a bare
comma string as the single literal tag ["a,b"]; parse_tags splits it like the
tag: query shorthand and write_note's tags convention.
"""
annotation = inspect.signature(search_notes).parameters["tags"].annotation
adapter = TypeAdapter(annotation)

real_list = adapter.validate_python(["a", "b"])
comma_string = adapter.validate_python("a,b")
json_string = adapter.validate_python('["a", "b"]')
single_string = adapter.validate_python("a")

assert real_list == ["a", "b"]
# The comma string and the real list must behave identically (the #910 bug).
assert comma_string == real_list
assert json_string == real_list
assert single_string == ["a"]


@pytest.mark.asyncio
async def test_search_notes_tags_comma_string_filters_via_mcp(mcp, client, test_project):
"""tags="alpha,beta" through the real MCP layer must match like a real list (#910)."""
from fastmcp import Client

async with Client(mcp) as mcp_client:
await mcp_client.call_tool(
"write_note",
{
"project": test_project.name,
"title": "Tag Split Note",
"directory": "test",
"content": "# Tag Split Note\nTagSplitToken body",
"tags": ["alpha", "beta"],
},
)

async def found(tags_value: object) -> bool:
result = await mcp_client.call_tool(
"search_notes",
{
"project": test_project.name,
"query": "TagSplitToken",
"search_type": "text",
"tags": tags_value,
},
)
return "Tag Split Note" in result.content[0].text

as_list = await found(["alpha", "beta"])
as_comma_string = await found("alpha,beta")
as_json_string = await found('["alpha", "beta"]')
as_single_string = await found("alpha")

assert as_list, "real-list tags must match (sanity)"
assert as_comma_string == as_list, "comma string must behave like the real list"
assert as_json_string == as_list
assert as_single_string == as_list
# Negative control: the filter is actually applied, not silently dropped.
assert not await found("gamma")


# --- Tests for text output format (#641) -----------------------------------


Expand Down
Loading