feat: Add JSON output mode for BM MCP tools (default text)

[phernandez]

Summary

• add output_format support ("text" | "json", default "text") across BM MCP tools used by openclaw-basic-memory
• preserve existing human-readable responses when output_format="text"
• return structured payloads when output_format="json" for:
  • read_note, write_note, edit_note, recent_activity
  • list_memory_projects, create_memory_project
  • delete_note, move_note
• add output_format to build_context for interface consistency
• implement idempotent create_memory_project behavior with already_exists=true on repeated calls
• update docs/README and tool contract expectations for the new parameter and JSON behavior

Tests

• added unit tests for text/json mode behavior across updated tools
• added MCP integration tests validating JSON-structured responses for updated tools
• replaced old ASCII integration coverage with JSON output integration coverage
• verified type checks: just typecheck
• verified sqlite suite: just test-sqlite

Notes

• created from a clean branch (codex/mcp-output-format-json-pr) based on origin/main to keep unrelated commits out of this PR.

[github-actions]

Claude finished @phernandez's task in 3m 20s —— View job

---

PR Review: Add JSON output mode for BM MCP tools

Tasks:

• Gather context and understand the PR
• Review changed files against team checklist
• Check code quality and standards
• Verify testing and documentation
• Check architecture patterns
• Post final review

---

Summary

This PR adds output_format: Literal["text", "json"] = "text" to the primary MCP tools used by openclaw-basic-memory. The approach is sound: existing text behavior is preserved as the default, structured JSON payloads are returned on request. The tool contracts are locked by the updated test_tool_contracts.py, tests are thorough, and the idempotent create_memory_project behavior is a clean addition. Overall this is a solid PR with a few issues that warrant attention before merge.

---

Code Quality & Standards Checklist

• Follows Basic Memory's coding conventions in CLAUDE.md
• Python 3.12+ type annotations and async patterns (Literal["text", "json"], str | dict returns)
• SQLAlchemy 2.0 best practices (not directly affected)
• FastAPI and Typer conventions followed
• 100-character line length limit maintained
• No commented-out code blocks

Testing & Documentation

• Unit tests for new functions/methods (test_tool_json_output_modes.py)
• Integration tests for new MCP tools (test_output_format_json_integration.py)
• Test coverage for edge cases (missing notes, security errors, idempotent create)
• Documentation updated (README, docstrings, tool descriptions)
• 100% test coverage — some # pragma: no cover concerns (see below)

Basic Memory Architecture

• MCP tools follow atomic, composable design
• No database schema changes
• Preserves local-first architecture principles
• Text mode unchanged — no behavior regression for existing callers
• Tool contracts locked in test_tool_contracts.py

Security & Performance

• No hardcoded secrets or credentials
• Security validation errors return structured JSON error payloads in json mode
• Performance: resolve_entity called 3x in move_note (see below)

Compatibility

• build_context — format="markdown" renamed to output_format="text", default still "json" (unchanged)
• Unicode characters in new create_memory_project text output (see below)

---

Issues

1. Hardcoded placeholder fields in list_memory_projects JSON output

src/basic_memory/mcp/tools/project_management.py:46-47

"is_private": False,
"display_name": None,

These fields don't exist on ProjectItem. They appear to be stubs for a future API contract. If callers begin depending on is_private=False always being accurate, this will become a problem when cloud/private projects are introduced. Either remove these fields or document clearly that they are reserved placeholders. Fix this →

2. Escaped newline bug in create_memory_project text response

src/basic_memory/mcp/tools/project_management.py:157

f"{'• Set as default project\\n' if is_default else ''}"

The \\n inside the f-string conditional evaluates to the literal two-character string \n, not a newline. When is_default=True, this prints • Set as default project\n (with a visible backslash-n) instead of starting a new line. Should be \n inside the string, e.g.:

("• Set as default project\n" if is_default else "")

Fix this →

3. Triple resolve_entity calls for single-file move in move_note

src/basic_memory/mcp/tools/move_note.py:575, 618, 666

resolve_entity(identifier) is called three times for the same identifier within a single single-file move operation:

• Line 575: first extension check attempt (inside try/except)
• Line 618: second extension check (after the earlier one)
• Line 666: actual move operation

The entity_id resolved at line 618 could be passed directly to line 666 instead of re-resolving. This adds two unnecessary API round-trips per move call. Fix this →

4. Dead code in read_note helper functions

src/basic_memory/mcp/tools/read_note.py:195-221

_search_results(), _result_title(), _result_permalink(), _result_file_path() each have both isinstance(item, dict) and hasattr(item, ...) branches. Since they're only ever called with search_notes.fn(output_format="json") results (which are always dict via model_dump()), the hasattr branches are unreachable dead code. This adds complexity without benefit. Fix this →

5. build_context: format="markdown" is a silent breaking change for existing callers

src/basic_memory/mcp/tools/build_context.py

The old parameter was format: str = "json" and the markdown mode was format="markdown". This PR renames it to output_format: Literal["json", "text"] = "json". Any existing caller passing the kwarg format="markdown" will now receive JSON silently (Python ignores extra kwargs if the tool framework doesn't enforce them, or may raise). This is a behavioral compatibility concern for any non-test callers. The in-repo tests are all updated correctly.

---

Minor Notes

• project_management.py:153: New ✓, • chars in the idempotent "already exists" text response — consistent with pre-existing text format in the same file.
• move_note.py:682-686: Emojis (✅📁🔗📊) in success text are pre-existing, not introduced here.
• delete_note.py:146: Pre-existing inconsistent support email (support@basicmemory.com vs support@basicmachines.co).
• edit_note.py:291,299: Pre-existing \\n escape in summary.append("\\n## Observations") — this was present before this PR and is a separate bug.
• chatgpt_tools.py fetch(): Correctly calls read_note.fn() without output_format, getting default "text" — no regression.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d3e44e6151

ℹ️ 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".

[chatgpt-codex-connector]

Return JSON for directory-delete failures

When output_format="json" is requested for directory deletes, only the success path returns a dict; if knowledge_client.delete_directory() raises (for example, missing directory or permission errors), the handler below still returns a markdown string. That mixed return type will break callers that unconditionally parse JSON in error scenarios, which is exactly when machine-readable output is most needed.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve requested activity types in JSON mode

recent_activity accepts type filters for entity, relation, and observation, but the new JSON flattening helper drops every non-entity item. As a result, calls like recent_activity(type="relation", output_format="json") can return empty/incomplete data even when matching relations exist, so downstream tools get incorrect results.

Useful? React with 👍 / 👎.

[github-actions]

Claude finished @phernandez's task in 2m 33s —— View job

---

PR Review: Add JSON output mode for BM MCP tools

Tasks:

• Gather context and understand the PR
• Read changed files
• Check code quality and standards
• Verify testing and documentation
• Check architecture patterns
• Check compatibility (Windows paths, unicode/emoji in output)
• Post final review

---

Summary

This PR adds output_format: Literal["text", "json"] = "text" to the primary MCP tools used by openclaw-basic-memory. The approach is sound: existing text behavior is preserved as the default, structured JSON payloads are returned on request. The tool contracts are locked by the updated test_tool_contracts.py, tests are thorough, and the idempotent create_memory_project behavior is a clean addition.

There are several issues that warrant attention before merge, including one real bug in JSON mode for recent_activity.

---

Code Quality & Standards Checklist

• Follows Basic Memory's coding conventions in CLAUDE.md
• Python 3.12+ type annotations and async patterns (Literal["text", "json"], str | dict returns)
• SQLAlchemy 2.0 best practices (not directly affected)
• FastAPI and Typer conventions followed
• 100-character line length limit maintained
• No commented-out code blocks

Testing & Documentation

• Unit tests for new functions/methods (test_tool_json_output_modes.py)
• Integration tests for new MCP tools (test_output_format_json_integration.py)
• Test coverage for edge cases (missing notes, security errors, idempotent create)
• Documentation updated (README, docstrings, tool descriptions)
• 100% test coverage — some # pragma: no cover concerns (see issue Create/update db on startup  #4 below)

Basic Memory Architecture

• MCP tools follow atomic, composable design
• No database schema changes
• Preserves local-first architecture principles
• Text mode unchanged — no behavior regression for existing callers
• Tool contracts locked in test_tool_contracts.py

Security & Performance

• No hardcoded secrets or credentials
• Security validation errors return structured JSON error payloads in json mode
• Performance: resolve_entity called 3x in move_note (see issue fix: fix permalink uniqueness violations on create/update/sync #3 below)

Compatibility

• Unicode characters in new text output (see issue Alembic #6 below)
• recent_activity JSON mode silently drops non-entity types (see issue Pre release #1 below — bug)
• build_context: format="markdown" rename is a silent breaking change (see issue Pre release fixups #5 below)

---

Issues

1. Bug: recent_activity JSON mode silently returns empty results for non-entity types

src/basic_memory/mcp/tools/recent_activity.py:337

_extract_recent_entity_rows() filters out anything that isn't type == "entity":

def _extract_recent_entity_rows(activity_data: GraphContext, ...) -> list[dict]:
    for result in activity_data.results:
        primary = result.primary_result
        if primary.type != "entity":
            continue   # ← relations and observations are silently dropped
        ...

A caller passing type="relation" or type="observation" with output_format="json" will always receive an empty list, even when matching results exist. This is the same issue flagged by Codex in the review comment. The function name is also misleading (_extract_recent_entity_rows — callers expect the filter to be applied by the type parameter they passed, not hardcoded inside this helper). Fix this →

2. Bug: Escaped newline in create_memory_project text response

src/basic_memory/mcp/tools/project_management.py:157

f"{'• Set as default project\\n' if is_default else ''}"

The \\n inside the single-quoted string inside the f-string is a literal two-character \n, not a newline. When is_default=True, the output prints • Set as default project\n (visible backslash-n). Should be:

("• Set as default project\n" if is_default else "")

Fix this →

3. Performance: Triple resolve_entity calls for a single-file move in move_note

src/basic_memory/mcp/tools/move_note.py:575, 618, 666

resolve_entity(identifier) is called three times within a single-file move:

• Line 575: first attempt to get source extension (inside try/except)
• Line 618: second attempt to get source extension for mismatch check (comment even says "might already be cached from above")
• Line 666: final call for the actual move

The entity_id resolved at line 618 could be directly reused at line 666, eliminating two unnecessary API round-trips. Fix this →

4. Dead code in read_note helper functions

src/basic_memory/mcp/tools/read_note.py:199-221

_search_results(), _result_title(), _result_permalink(), _result_file_path() each have both isinstance(item, dict) and hasattr(item, ...) branches. Since they are only ever called with search_notes.fn(output_format="json") results (which are always dict), the hasattr branches are unreachable dead code that adds complexity and likely needs # pragma: no cover or should be removed. Fix this →

5. build_context: format="markdown" rename is a silent breaking change

src/basic_memory/mcp/tools/build_context.py:205

The old parameter was format: str = "json" with format="markdown" for text mode. This PR renames it to output_format: Literal["json", "text"] = "json". Any existing callers passing format="markdown" will silently receive JSON (the framework ignores extra kwargs or raises, depending on FastMCP version). In-repo tests are all updated correctly, but external callers — including any openclaw integration — passing the old kwarg will get incorrect behavior silently. Worth documenting in the PR notes or adding a deprecation warning. Investigate this →

6. Unicode characters in new text output (compatibility concern)

src/basic_memory/mcp/tools/project_management.py:153

CLAUDE.md custom instructions specify: "Avoid using emojis and unicode characters in console and log output". The new create_memory_project text response introduces ✓ and • characters. These are fine in MCP responses consumed by LLMs, but if the response text also appears in CLI output (via basic-memory tool commands), they may render incorrectly on Windows or non-UTF-8 terminals. The pre-existing emoji usage in move_note.py (✅, 📁, 🔗, 📊) has the same concern but that predates this PR. Since this PR is introducing new instances, worth flagging.

7. Hardcoded placeholder fields in list_memory_projects JSON output

src/basic_memory/mcp/tools/project_management.py:46-47

"is_private": False,
"display_name": None,

These fields don't exist on ProjectItem. If callers begin depending on is_private=False always being accurate, this becomes a problem when cloud/private projects are introduced. Either remove these fields or add a comment clearly marking them as reserved stubs with a schema evolution caveat. Fix this →

---

Minor Notes

• edit_note.py:291,299: Pre-existing \\n escape in summary.append("\\n## Observations") — this was present before this PR and is a separate bug (same pattern as issue Memory json import #2 above).
• delete_note.py: The delete_directory exception handler (pre-existing, # pragma: no cover) correctly returns JSON for exceptions. No regression introduced here.
• chatgpt_tools.py fetch(): Correctly calls read_note.fn() without output_format, getting default "text" — no regression.
• move_note.py:682-686: Emojis (✅📁🔗📊) in success text are pre-existing, not introduced here.
• delete_note.py:146: Pre-existing inconsistent support email (support@basicmemory.com vs support@basicmachines.co).

---

Priority for merge-blocking fixes: Issue #1 (JSON mode returning empty for non-entity type filters) and Issue #2 (escaped newline rendering as literal \n) are the most important to address before merge.

[github-actions]

Claude finished @phernandez's task in 3m 37s —— View job

---

PR Review: Add JSON output mode for BM MCP tools

Tasks:

• Gather context and understand the PR
• Review new commits since last review (01c4892, fc54382)
• Check if previous review issues were addressed
• Check code quality and standards
• Verify testing and documentation
• Check architecture patterns
• Post final review

---

Summary

This PR is in good shape. The latest commit (01c4892) directly addressed the two highest-priority bugs from the previous reviews:

1. Fixed: recent_activity JSON mode now includes all result types — _extract_recent_entity_rows was renamed to _extract_recent_rows and the hardcoded if primary.type != "entity": continue guard was removed. New integration tests in test_recent_activity_json_output_for_relation_and_observation_types validate this.

2. Fixed: Directory-delete JSON error handling — delete_note now returns a structured JSON error dict when knowledge_client.delete_directory() raises an exception in JSON mode, matching the Codex P2 review comment.

There are 3 remaining issues that should be addressed before merge: one bug (escaped newline), one dead-code concern, and one hardcoded placeholder schema concern.

---

Code Quality & Standards Checklist

• Follows Basic Memory's coding conventions in CLAUDE.md
• Python 3.12+ type annotations (Literal["text", "json"], str | dict returns)
• SQLAlchemy 2.0 best practices (not affected)
• FastAPI and Typer conventions followed
• 100-character line length limit maintained
• No commented-out code blocks

Testing & Documentation

• Unit tests for new functions/methods (test_tool_json_output_modes.py)
• Integration tests for new MCP tools (test_output_format_json_integration.py)
• Test coverage for edge cases (missing notes, security errors, idempotent create, relation/observation types)
• Documentation updated (README, docstrings, tool descriptions)
• 100% test coverage concern — hasattr branches in read_note.py are unreachable but not annotated with # pragma: no cover (see Issue Memory json import #2 below)

Basic Memory Architecture

• MCP tools follow atomic, composable design
• No database schema changes
• Preserves local-first architecture principles
• Text mode unchanged — no behavior regression for existing callers
• Tool contracts locked in test_tool_contracts.py
• recent_activity JSON mode now correctly includes all result types (fixed in latest commit)

Security & Performance

• No hardcoded secrets or credentials
• Security validation errors return structured JSON error payloads in json mode
• Performance: triple resolve_entity calls in move_note (see Issue fix: fix permalink uniqueness violations on create/update/sync #3 below — pre-existing pattern, not blocking)

Compatibility

• fc54382 normalizes Windows path assertions in tests
• build_context: output_format default is still "json" — no regression for in-repo callers (all tests updated). External callers using the old format="markdown" kwarg would fail silently, but that's a pre-existing concern for any external integrations.
• Unicode in new text output (see Issue Pre release #1 below — minor, but CLAUDE.md flags it)

---

Remaining Issues

1. Bug: Escaped newline in create_memory_project text response (still present)

src/basic_memory/mcp/tools/project_management.py:157

f"{'• Set as default project\\n' if is_default else ''}"

The \\n inside the single-quoted string within the f-string is a literal two-character \n, not a newline. When is_default=True, output reads • Set as default project\n with a visible backslash-n. This is confirmed — the same file has the correct form on line 183: result += "• Set as default project\n". Fix:

("• Set as default project\n" if is_default else "")

Fix this →

2. Dead code in read_note helper functions (unpragma'd)

src/basic_memory/mcp/tools/read_note.py:199-221

_search_results, _result_title, _result_permalink, _result_file_path each have dual isinstance(item, dict) / hasattr(item, ...) branches. Since they're only ever called with search_notes.fn(output_format="json") results (always dict), the hasattr branches are unreachable. They don't carry # pragma: no cover annotations, which means they're silently dragging down coverage OR the coverage tool happens not to catch them. Either remove the hasattr branches (simpler) or add # pragma: no cover (acceptable if you want defensive code). Fix this →

3. Hardcoded placeholder fields in list_memory_projects JSON output

src/basic_memory/mcp/tools/project_management.py:46-47

"is_private": False,
"display_name": None,

These don't come from ProjectItem. If external callers begin depending on is_private=False always being accurate, this will become a problem when private/cloud projects are introduced. At minimum, add a comment marking them as reserved stubs. Fix this →

4. _extract_recent_rows JSON output omits type field

src/basic_memory/mcp/tools/recent_activity.py:337-347

The new _extract_recent_rows correctly includes all result types (entity/relation/observation), but the emitted dict doesn't include a "type" key. A JSON caller requesting type="relation" gets back a list where each item has title, permalink, file_path, created_at — but no field to identify what type each row is. The API already filters upstream, so the list will contain only the requested type, but the caller can't confirm this from the payload. Consider adding "type": primary.type to the row dict. Fix this →

---

Minor Notes (not blocking)

• move_note.py:575,618,666: Triple resolve_entity calls for a single-file move — the comment at line 617 acknowledges this ("might already be cached from above"). Not introduced by this PR, but worth a follow-up refactor.
• edit_note.py:291,299: Pre-existing \\n escape bug in summary.append("\\n## Observations") — same pattern as Issue Pre release #1 above, predates this PR.
• delete_note.py:146: Pre-existing inconsistent support email (support@basicmemory.com vs support@basicmachines.co).
• chatgpt_tools.py: Correctly calls read_note.fn() without output_format, getting default "text" — no regression.
• Unicode/emoji in new text output (✓, • in create_memory_project) — CLAUDE.md says to avoid these in console/log output. Since these responses go to LLMs via MCP (not CLI terminals) this is low risk, but worth noting for consistency.

---

Priority: Issue #1 (escaped newline) is a straightforward bug fix. Issues #2-4 are improvements worth addressing before merge. Issue #3 (triple resolve_entity) can be deferred to a follow-up.

[github-actions]

Claude finished @phernandez's task in 2m 54s —— View job

---

PR Review: Add JSON output mode for BM MCP tools

Tasks:

• Gather context and understand the PR
• Review latest commit (c85d096) changes
• Check if previous review issues were addressed
• Verify code quality and standards
• Post final review

---

Summary

The latest commit (c85d096 Fix remaining MCP JSON output review issues) addresses all four issues flagged in the previous review. This PR is ready for merge pending one minor coverage concern noted below.

---

Status of Previous Review Issues

✅ Fixed: Escaped newline in create_memory_project text response

src/basic_memory/mcp/tools/project_management.py:159

Corrected from \\n to \n. A corresponding regression test was added that explicitly asserts "Set as default project\\n" not in result and "Set as default project\n" in result. Clean fix.

✅ Fixed: _extract_recent_rows omits type field

src/basic_memory/mcp/tools/recent_activity.py:338

"type": primary.type is now included in the row dict. Both unit tests (test_tool_json_output_modes.py) and integration tests (test_output_format_json_integration.py) updated to assert type is present and correct for entity, relation, and observation results.

✅ Addressed: Hardcoded placeholder fields in list_memory_projects

src/basic_memory/mcp/tools/project_management.py:46-49

A clear comment was added marking is_private and display_name as reserved stubs for future cloud schema evolution. Acceptable approach.

✅ Fixed: Triple resolve_entity calls in move_note

src/basic_memory/mcp/tools/move_note.py:576-581

Refactored to use an _ensure_resolved_entity_id() inner async function with a nonlocal cached variable. The entity ID is now resolved at most once per call (or twice only if the first try block raises an exception and recovery is attempted in the fallback check). This is a clean solution that eliminates the unnecessary round-trips.

✅ Bonus fixes in this commit

• edit_note.py:291,299: Pre-existing \\n escape bugs in summary.append() calls — both fixed.
• delete_note.py:143: Support email corrected from support@basicmemory.com to support@basicmachines.co.

---

Code Quality & Standards Checklist

• Follows Basic Memory's coding conventions in CLAUDE.md
• Python 3.12+ type annotations (Literal["text", "json"], str | dict returns, str | None)
• SQLAlchemy 2.0 best practices (not affected)
• FastAPI and Typer conventions followed
• 100-character line length limit maintained
• No commented-out code blocks

Testing & Documentation

• Unit tests for new functions/methods (test_tool_json_output_modes.py)
• Integration tests for new MCP tools (test_output_format_json_integration.py)
• Test coverage for edge cases (missing notes, security errors, idempotent create, relation/observation types)
• Documentation updated (README, docstrings, tool descriptions)
• Minor coverage concern — see issue Pre release #1 below

Basic Memory Architecture

• MCP tools follow atomic, composable design
• No database schema changes
• Preserves local-first architecture principles
• Text mode unchanged — no behavior regression for existing callers
• Tool contracts locked in test_tool_contracts.py
• recent_activity JSON mode correctly includes all result types with type field
• move_note resolve_entity now cached — no redundant API round-trips

Security & Performance

• No hardcoded secrets or credentials
• Security validation errors return structured JSON error payloads in json mode
• Triple resolve_entity calls eliminated via _ensure_resolved_entity_id caching

Compatibility

• Windows path assertions normalized (fc54382)
• build_context output_format default is still "json" — in-repo callers all updated
• create_memory_project idempotent behavior works correctly in both text and json modes
• Pre-existing emoji usage in move_note success text not changed by this PR

---

Remaining Minor Issue

1. _search_results still has an unreachable model_dump branch with no # pragma: no cover

src/basic_memory/mcp/tools/read_note.py:199-205

def _search_results(payload: object) -> list[dict]:
    if isinstance(payload, dict):
        results = payload.get("results")
        return results if isinstance(results, list) else []
    model_dump = getattr(payload, "model_dump", None)  # ← unreachable with current callers
    if callable(model_dump):
        dumped = model_dump()
        if isinstance(dumped, dict):
            results = dumped.get("results")
            return results if isinstance(results, list) else []
    return []

The previous review flagged the hasattr branches on _result_title, _result_permalink, _result_file_path — those were correctly removed. However, _search_results was updated to use a model_dump fallback instead, which is still unreachable since search_notes.fn(output_format="json") always returns a dict. The function is only ever called from a single code path. Given CLAUDE.md's 100% coverage requirement, this branch either needs a test or a # pragma: no cover annotation, or should be simplified to the dict-only path (which would also align with the CLAUDE.md guideline against fallback logic). Fix this →

---

Minor Notes (not blocking)

• move_note.py: _ensure_resolved_entity_id is a nested async function with a nonlocal — this is idiomatic Python but slightly unusual. The implementation is correct. The exception handling for the fallback source_entity is None path (lines 624-629) is now the only way a second resolution could occur, which is appropriate.
• list_memory_projects placeholder stubs (is_private, display_name) are now clearly documented as reserved — callers should be aware these values are not authoritative until cloud project support ships.

---

Conclusion: All merge-blocking issues from previous reviews have been addressed. The PR is in good shape. The one remaining item (_search_results coverage) is minor — a one-line # pragma: no cover on the model_dump branch or simplification to dict-only would resolve it.

[github-actions]

Claude finished @phernandez's task in 2m 26s —— View job

---

PR Review: Add JSON output mode for BM MCP tools

Tasks:

• Gather context and understand the PR
• Review latest commit (5b090c2) changes
• Check if previous review issues were addressed
• Verify code quality and standards
• Post final review

---

Summary

The latest commit (5b090c2 Simplify read_note fallback to JSON dict contract) resolves the final merge-blocking issue from the previous review: the unreachable model_dump fallback in _search_results. The function is now dict-only, which matches the established JSON contract and removes the dead code.

Additionally, the tests were updated to align with the JSON dict contract — mocks now return plain dict structures instead of SearchResponse/SearchResult objects, which correctly reflects what search_notes.fn(output_format="json") actually returns.

This PR is nearly ready for merge. One minor coverage concern remains.

---

Status of Final Remaining Issue

✅ Fixed: _search_results unreachable model_dump branch

src/basic_memory/mcp/tools/read_note.py:195-199

The model_dump fallback was removed entirely. _search_results is now:

def _search_results(payload: object) -> list[dict]:
    if not isinstance(payload, dict):
        return []
    results = payload.get("results")
    return results if isinstance(results, list) else []

Clean, no dead branches. ✅

⚠️ Minor: _result_title, _result_permalink, _result_file_path — unreachable guards without # pragma: no cover

src/basic_memory/mcp/tools/read_note.py:201-216

These three helpers still have if not isinstance(item, dict) guards:

def _result_title(item: object) -> str:
    if not isinstance(item, dict):
        return ""           # ← unreachable: _search_results always returns list[dict]
    return str(item.get("title") or "")

Since _search_results guarantees items are dicts, the early-return branches in these helpers are still unreachable. Given CLAUDE.md's 100% coverage requirement, these branches either need a # pragma: no cover annotation or should be removed (simplest fix would be to drop the isinstance guard entirely since the JSON contract is now locked). The previous review mentioned this pattern — _search_results was fixed, but these three weren't updated.

This is minor and non-blocking if branch coverage isn't enforced, but worth fixing before merge to stay consistent with the 100% coverage rule. Fix this →

---

Checklist

Code Quality & Standards

• Follows Basic Memory's coding conventions in CLAUDE.md
• Python 3.12+ type annotations (Literal["text", "json"], str | dict returns)
• SQLAlchemy 2.0 best practices (not affected)
• FastAPI and Typer conventions followed
• 100-character line length limit maintained
• No commented-out code blocks

Testing & Documentation

• Unit tests for new functions/methods (test_tool_json_output_modes.py)
• Integration tests for new MCP tools (test_output_format_json_integration.py)
• Test coverage for edge cases (missing notes, security errors, idempotent create, relation/observation types)
• Documentation updated (README, docstrings, tool descriptions)
• Tests updated to use dict contract (latest commit)
• 100% coverage — three _result_* unreachable guards without # pragma: no cover (see above)

Basic Memory Architecture

• MCP tools follow atomic, composable design
• No database schema changes
• Preserves local-first architecture principles
• Text mode unchanged — no behavior regression for existing callers
• Tool contracts locked in test_tool_contracts.py
• recent_activity JSON mode includes all types with type field
• move_note resolve_entity cached via _ensure_resolved_entity_id

Security & Performance

• No hardcoded secrets or credentials
• Security validation errors return structured JSON error payloads in json mode
• Triple resolve_entity calls eliminated

Compatibility

• Windows path assertions normalized
• build_context output_format default preserved
• Escaped newline bug in create_memory_project fixed
• _extract_recent_rows type field included in JSON output
• list_memory_projects placeholder fields documented as reserved stubs

---

Conclusion: All merge-blocking issues from all previous reviews have been addressed in the five commits of this PR. The one remaining item — unreachable if not isinstance(item, dict) guards in _result_title, _result_permalink, _result_file_path — is a minor coverage concern. Removing the guards or adding # pragma: no cover would bring the PR to a clean state. This PR is otherwise ready to merge.

[github-actions]

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

---

PR Review: Add JSON output mode for BM MCP tools

Tasks:

• Gather context and understand the PR
• Review latest commit (46416ef) changes
• Check if previous review issues were addressed
• Verify code quality and standards
• Post final review

---

Summary

The latest commit (46416ef Apply suggestion from @phernandez) is a single-line change in delete_note.py: the support email is set to support@basicmemory.com (reverting from support@basicmachines.co applied in a previous commit). This reflects a deliberate team decision about the canonical support address.

Combined with 4fa5861 Remove unreachable read_note result guards (which removed the if not isinstance(item, dict) guards from _result_title, _result_permalink, _result_file_path), all previously flagged issues across all review rounds have now been addressed.

---

Final Status of All Issues

Issue	Status
Bug: recent_activity JSON mode dropped non-entity types	✅ Fixed (01c4892)
Bug: Escaped \\n in create_memory_project text output	✅ Fixed (c85d096)
_extract_recent_rows missing type field in JSON output	✅ Fixed (c85d096)
Triple resolve_entity calls in move_note	✅ Fixed via _ensure_resolved_entity_id (c85d096)
_search_results unreachable model_dump fallback	✅ Removed (5b090c2)
_result_title/permalink/file_path unreachable isinstance guards	✅ Removed (4fa5861)
list_memory_projects hardcoded placeholder fields	✅ Documented as reserved stubs (c85d096)
Delete directory exception not returning JSON in JSON mode	✅ Fixed (01c4892)
Pre-existing \\n escapes in edit_note summary	✅ Fixed (c85d096)
Support email inconsistency	✅ Resolved — support@basicmemory.com per team preference (46416ef)
Windows path assertions in tests	✅ Fixed (fc54382)

---

Code Quality & Standards Checklist

• Follows Basic Memory's coding conventions in CLAUDE.md
• Python 3.12+ type annotations (Literal["text", "json"], str | dict returns)
• SQLAlchemy 2.0 best practices (not affected)
• FastAPI and Typer conventions followed
• 100-character line length limit maintained
• No commented-out code blocks

Testing & Documentation

• Unit tests for new functions/methods (test_tool_json_output_modes.py)
• Integration tests for new MCP tools (test_output_format_json_integration.py)
• Test coverage for edge cases (missing notes, security errors, idempotent create, relation/observation types)
• Documentation updated (README, docstrings, tool descriptions)
• 100% coverage — unreachable branches removed, no dead code paths remain

Basic Memory Architecture

• MCP tools follow atomic, composable design
• No database schema changes
• Preserves local-first architecture principles
• Text mode unchanged — no behavior regression for existing callers
• Tool contracts locked in test_tool_contracts.py
• recent_activity JSON includes all result types with type field
• move_note resolve_entity cached via _ensure_resolved_entity_id

Security & Performance

• No hardcoded secrets or credentials
• Security validation errors return structured JSON payloads in JSON mode
• Triple resolve_entity API round-trips eliminated

Compatibility

• Windows path assertions normalized
• build_context output_format default preserved ("json")
• list_memory_projects placeholder stubs documented as reserved for cloud schema
• chatgpt_tools.fetch() calls read_note.fn() without output_format — defaults to text, no regression

---

Conclusion: This PR is clean and ready to merge. All issues identified across five review rounds have been resolved. The JSON output contract is well-defined, backward compatibility is preserved for text-mode callers, and the test suite fully covers the new behavior.

[github-actions]

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

---

PR Review: Add JSON output mode for BM MCP tools

Tasks:

• Gather context and understand the PR
• Review changed files against team checklist
• Check code quality and standards
• Verify testing and documentation
• Check architecture patterns
• Post final review

---

Summary

This PR is clean and ready to merge. All issues raised across the previous five review rounds have been addressed. The latest commit (46416ef Apply suggestion from @phernandez) confirmed the canonical support email; 4fa5861 removed the last unreachable branches from the read_note helpers. The final state of the code is correct, well-tested, and consistent with project conventions.

---

Final Checklist Status

Issue	Status
recent_activity JSON mode dropped non-entity types	✅ Fixed
Escaped \\n in create_memory_project text output	✅ Fixed (\n in f-string conditional on line 159)
_extract_recent_rows missing type field in JSON output	✅ Fixed
Triple resolve_entity calls in move_note	✅ Fixed via _ensure_resolved_entity_id caching
_search_results unreachable model_dump fallback	✅ Removed
_result_title/_permalink/_file_path unreachable isinstance guards	✅ Removed (4fa5861)
list_memory_projects hardcoded placeholder fields	✅ Documented as reserved stubs
Directory-delete exception not returning JSON in JSON mode	✅ Fixed
Pre-existing \\n escapes in edit_note summary	✅ Fixed
Support email inconsistency	✅ Resolved to support@basicmemory.com (46416ef)
Windows path assertions in tests	✅ Fixed

---

Code Quality & Standards Checklist

• Follows Basic Memory's coding conventions in CLAUDE.md
• Python 3.12+ type annotations (Literal["text", "json"], str | dict returns)
• SQLAlchemy 2.0 best practices (not affected)
• FastAPI and Typer conventions followed
• 100-character line length limit maintained
• No commented-out code blocks

Testing & Documentation

• Unit tests for new functions/methods (test_tool_json_output_modes.py)
• Integration tests for new MCP tools (test_output_format_json_integration.py)
• Test coverage for edge cases (missing notes, security errors, idempotent create, relation/observation types)
• Documentation updated (README, docstrings)
• 100% coverage — all unreachable branches removed, no dead code paths remain
• Tool contracts locked in test_tool_contracts.py

Basic Memory Architecture

• MCP tools follow atomic, composable design
• No database schema changes
• Preserves local-first architecture principles
• Text mode default — no behavior regression for existing callers
• recent_activity JSON includes all result types with type field
• move_note resolve_entity cached via _ensure_resolved_entity_id

Security & Performance

• No hardcoded secrets or credentials
• Security validation errors return structured JSON payloads in JSON mode
• Triple resolve_entity API round-trips eliminated

Compatibility

• Windows path assertions normalized
• build_context output_format default preserved ("json")
• chatgpt_tools.fetch() calls read_note.fn() without output_format — defaults to text, no regression
• list_memory_projects placeholder stubs documented as reserved for cloud schema

---

One Minor Observation (non-blocking)

src/basic_memory/mcp/tools/read_note.py:368-384 — format_related_results (text-mode fallback) still has dual isinstance(result, dict) / getattr branches. Since text_candidates always comes from _search_results() which now guarantees list[dict], the getattr branches are unreachable. This was not flagged in previous reviews and it is a text-mode-only code path, so it does not affect JSON callers. Worth a follow-up cleanup to either drop the getattr branches or add # pragma: no cover, but not blocking.

---

Conclusion: All merge-blocking issues from all previous review rounds have been resolved. This PR is ready to merge.