fix: coerce string params to list/dict in MCP tools

[phernandez]

Summary

Fixes #656 — MCP list/dict parameters fail with Pydantic validation when clients send strings.

• Added coerce_list and coerce_dict helpers to utils.py
• Applied BeforeValidator coercion to affected parameters in search_notes, write_note, and canvas tools
• Affected params: entity_types, note_types, tags, metadata_filters (search), metadata (write_note), nodes/edges (canvas)

Test plan

• Unit tests for coerce_list/coerce_dict (tests/test_coerce.py — 13 tests)
• Integration tests through full FastMCP Client → validate_call → tool path (test-int/mcp/test_string_params_integration.py — 5 tests)
• All existing tool tests pass (90 tests, no regressions)

🤖 Generated with Claude Code

[github-actions]

Claude finished @phernandez's task in 1m 32s —— View job

---

PR Review: fix: coerce string params to list/dict in MCP tools

• Gather context and read changed files
• Review code quality & standards
• Review testing & documentation
• Review Basic Memory architecture
• Review security & performance
• Review compatibility
• Post final summary

---

Summary

This is a clean, focused fix for a real interoperability problem — MCP clients that serialize list/dict parameters as JSON strings cause Pydantic validation failures. The solution (Pydantic BeforeValidator coercers in utils.py) is well-targeted and non-invasive. The implementation is solid with a few items worth addressing.

---

Checklist

Code Quality & Standards

• Follows Basic Memory coding conventions
• Python 3.12+ type annotations and async patterns
• FastAPI/Pydantic conventions followed correctly (BeforeValidator is the right pattern)
• 100-character line length maintained
• No commented-out code blocks
• Literate programming style followed (comments explain the "why")

Testing & Documentation

• Unit tests for coerce_list/coerce_dict — comprehensive (7 + 6 cases)
• Integration tests through FastMCP client path
• Missing: canvas integration test — nodes/edges coercion added but not integration-tested (see below)
• No regression in existing tool tests per PR description

Basic Memory Architecture

• MCP tools remain atomic and composable
• No database changes — no migration needed
• Preserves local-first architecture
• Correct composition: coercers live in utils.py, applied at tool boundary via BeforeValidator

Security & Performance

• No hardcoded secrets
• Input validation maintained — coerce_dict correctly passes through non-JSON strings for Pydantic to reject
• No sensitive data exposed
• No performance concerns — json.loads is called at most once per parameter per invocation

Compatibility

• No emoji/unicode in output
• No Windows path comparison issues

---

Issues Found

1. Missing integration test for canvas nodes/edges coercion (minor)

canvas.py applies BeforeValidator(coerce_list) to both nodes and edges, but test-int/mcp/test_string_params_integration.py has no test for these parameters. The five integration tests only cover search_notes (4 params) and write_note.metadata. Canvas is the only affected tool without integration coverage.

Fix this →

2. coerce_list JSON-object-as-string wraps the string rather than returning the value (observation, not a bug)

coerce_list('{"key": "val"}') → ['{"key": "val"}'] (string wrapped in list, not the parsed dict). This is the correct behavior for List[...] targets (the test at line 29 confirms intent), but worth noting: if a caller accidentally passes a single dict serialized as a JSON string to a List[Dict] param, they'll get a list containing a string, which Pydantic will then reject. This is acceptable since a List[Dict] param should receive a JSON array, not a JSON object.

3. Empty-string edge case in coerce_list (very minor)

coerce_list("") returns [""] — an empty string gets wrapped in a list rather than being passed through as-is or returning None. This is unlikely to be exercised in practice, but if it is, the downstream Pydantic List[str] type will accept it (a list with one empty string). No action required unless this becomes a real issue.

4. write_note.tags uses parse_tags (not coerce_list) — intentional and correct

write_note already handles tags: list[str] | str | None via parse_tags, which supports comma-separated strings. This is more permissive than coerce_list and the existing behavior is correct. The inconsistency is intentional and appropriate.

---

Positive Notes

• Minimal diff — the fix is precisely scoped to the affected parameters. No unnecessary refactoring.
• coerce_dict fail-open design is correct — non-JSON strings pass through and let Pydantic reject them downstream, rather than trying to guess intent.
• BeforeValidator is the right Pydantic v2 hook — runs before type coercion, which is exactly what's needed here.
• coerce_list single-string wrapping — the fallback return [v] is a reasonable default for tools where a caller passes "entity" instead of ["entity"].

---

Verdict: Approve with the suggestion to add a canvas integration test for completeness. The core fix is correct and well-tested for the primary use cases.

[github-actions]

Claude finished @phernandez's task in 55s —— View job

---

PR Review: fix: coerce string params to list/dict in MCP tools

• Gather context and read changed files
• Review canvas integration test addition
• Verify checklist items
• Post final summary

---

Summary

The canvas integration test (test_canvas_nodes_edges_as_string) has been added in commit d76a5ae, addressing the only significant gap from the initial review. The PR is complete and ready to merge.

---

Checklist

Code Quality & Standards

• Follows Basic Memory coding conventions
• Python 3.12+ type annotations used throughout
• FastAPI/Pydantic v2 conventions followed — BeforeValidator is the correct hook
• 100-character line length maintained
• No commented-out code blocks
• Literate programming style: coerce_list/coerce_dict docstrings explain the "why"

Testing & Documentation

• Unit tests for coerce_list/coerce_dict — 7 + 6 cases covering all edge paths (tests/test_coerce.py)
• Integration tests through full FastMCP Client → validate_call → tool path (test-int/mcp/test_string_params_integration.py)
• Canvas integration test added — test_canvas_nodes_edges_as_string passes both nodes and edges as json.dumps(...) strings through the MCP protocol path (test-int/mcp/test_string_params_integration.py:135)
• All 6 integration tests cover all affected parameters

Basic Memory Architecture

• MCP tools remain atomic and composable
• No database changes — no migration needed
• Preserves local-first architecture
• Coercers correctly placed in utils.py, applied at tool boundary via BeforeValidator

Security & Performance

• No hardcoded secrets
• coerce_dict correctly passes non-JSON strings through to Pydantic for rejection (fail-open on invalid input, not fail-silent)
• json.loads called at most once per parameter per invocation — no performance concern

Compatibility

• No emoji/unicode in tool output
• No Windows path comparison issues

---

Remaining Observations (no action required)

• coerce_list("") → [""]: An empty string is wrapped in a list rather than returning None. Unlikely to be exercised in practice; downstream Pydantic validation would accept a list[str] with one empty string. Not a bug.
• write_note.tags uses parse_tags (not coerce_list): This is intentional — parse_tags is more permissive (supports comma-separated strings) and the existing behavior is correct.

---

Verdict: Approved. The fix is minimal, correct, and fully tested across all affected tool parameters including the newly added canvas coverage.