admin/basicmachines-co-basic-memory
1
0
Fork 0
mirror of https://github.com/basicmachines-co/basic-memory synced 2026-06-21 13:47:35 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(cli): align bm tool commands with MCP (error exits, overwrite, category, default) (#913)

Browse Source 
Signed-off-by: phernandez <paul@basicmachines.co>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Paul Hernandez
2026-06-07 22:56:43 -05:00
committed by GitHub
parent 480a2d9468
commit 8570d96bad
5 changed files with 439 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/basic_memory/cli/commands/tool.py
+47 -1
View File
@@ -100,6 +100,11 @@ def write_note(
help="Project external_id (UUID). Takes precedence over --project; use to disambiguate same-named projects across cloud workspaces.",
),
] = None,
overwrite: bool = typer.Option(
False,
"--overwrite",
help="Replace an existing note on conflict (matches MCP write_note overwrite=True)",
),
local: bool = typer.Option(
False, "--local", help="Force local API routing (ignore cloud mode)"
),
@@ -112,6 +117,7 @@ def write_note(
bm tool write-note --title "My Note" --folder "notes" --content "Note content"
bm tool write-note --title "My Guide" --folder "notes" --content "..." --type guide
echo "content" | bm tool write-note --title "My Note" --folder "notes"
bm tool write-note --title "My Note" --folder "notes" --overwrite
bm tool write-note --title "My Note" --folder "notes" --local
"""
try:
@@ -144,9 +150,22 @@ def write_note(
project_id=project_id,
tags=tags,
note_type=note_type,
overwrite=overwrite,
output_format="json",
)
)
# MCP tool returns an error field on failure in JSON mode (e.g.
# NOTE_ALREADY_EXISTS on a blocked overwrite, SECURITY_VALIDATION_ERROR).
# Trigger: result carries a non-empty `error`.
# Why: parity with delete-note/edit-note/search-notes so exit-code-driven
# scripts detect a failed/blocked write instead of seeing exit 0.
# Outcome: print the error to stderr and exit non-zero.
if isinstance(result, dict) and result.get("error"):
typer.echo(f"Error: {result['error']}", err=True)
_print_json(result)
raise typer.Exit(1)
_print_json(result)
except ValueError as e:
typer.echo(f"Error: {e}", err=True)
@@ -200,6 +219,19 @@ def read_note(
output_format="json",
)
)
# MCP tool returns an error field on failure in JSON mode (e.g.
# SECURITY_VALIDATION_ERROR on a path-traversal identifier). A genuine
# not-found returns null fields with no `error` key, so it still exits 0.
# Trigger: result carries a non-empty `error`.
# Why: parity with edit-note/delete-note/search-notes so a blocked read
# surfaces a non-zero exit instead of looking like success.
# Outcome: print the error to stderr and exit non-zero.
if isinstance(result, dict) and result.get("error"):
typer.echo(f"Error: {result['error']}", err=True)
_print_json(result)
raise typer.Exit(1)
_print_json(result)
except ValueError as e:
typer.echo(f"Error: {e}", err=True)
@@ -417,7 +449,9 @@ def recent_activity(
"7d", "--timeframe", help="Timeframe filter (e.g., '7d', '1 week')"
),
page: int = typer.Option(1, "--page", help="Page number for pagination"),
page_size: int = typer.Option(50, "--page-size", help="Number of results per page"),
# Match the MCP recent_activity default (page_size=10) so identical default
# invocations return the same number of rows from CLI and MCP.
page_size: int = typer.Option(10, "--page-size", help="Number of results per page"),
project: Annotated[
Optional[str],
typer.Option(help="The project to use. If not provided, the default project will be used."),
@@ -502,6 +536,16 @@ def search_notes(
help="Filter by search item type: entity, observation, relation (repeatable)",
),
] = None,
categories: Annotated[
Optional[List[str]],
typer.Option(
"--category",
help=(
"Filter observation results to exact categories (repeatable); "
"pair with --entity-type observation"
),
),
] = None,
meta: Annotated[
Optional[List[str]],
typer.Option("--meta", help="Filter by frontmatter key=value (repeatable)"),
@@ -536,6 +580,7 @@ def search_notes(
bm tool search-notes --permalink "specs/*"
bm tool search-notes --tag python --tag async
bm tool search-notes --meta status=draft
bm tool search-notes "auth" --entity-type observation --category requirement
"""
try:
validate_routing_flags(local, cloud)
@@ -601,6 +646,7 @@ def search_notes(
page_size=page_size,
note_types=note_types,
entity_types=entity_types,
categories=categories,
metadata_filters=metadata_filters,
tags=tags,
status=status,
test-int/bughunt_fixes/test_cli_tool_read_note_security_error.py
+50
View File
@@ -0,0 +1,50 @@
"""Bug hunt regression test (#6): `bm tool read-note` exit code on a
path-traversal SECURITY_VALIDATION_ERROR.
The MCP read_note tool detects path-traversal identifiers and returns
{"error": "SECURITY_VALIDATION_ERROR", ...}. Every other wrapped tool command
exits non-zero on an error payload; read-note used to print the payload and
exit 0. These integration tests assert read-note now matches its siblings.
"""
import pytest
from typer.testing import CliRunner
from basic_memory.cli.main import app as cli_app
from basic_memory.mcp.tools import read_note as mcp_read_note
runner = CliRunner()
TRAVERSAL_IDENTIFIER = "../../../../etc/passwd"
@pytest.mark.asyncio
async def test_read_note_security_error_mcp_emits_error_field(
app, app_config, test_project, config_manager
):
"""MCP read_note JSON flags the path traversal with a SECURITY_VALIDATION_ERROR."""
result = await mcp_read_note(
identifier=TRAVERSAL_IDENTIFIER,
project=test_project.name,
output_format="json",
)
assert isinstance(result, dict)
assert result.get("error") == "SECURITY_VALIDATION_ERROR"
def test_read_note_security_error_cli_exit_code_matches_other_tools(
app, app_config, test_project, config_manager
):
"""CLI read-note must not exit 0 when the MCP payload carries an error."""
result = runner.invoke(
cli_app,
["tool", "read-note", TRAVERSAL_IDENTIFIER, "--project", test_project.name],
)
combined = result.stdout
assert "SECURITY_VALIDATION_ERROR" in combined, combined
assert result.exit_code != 0, (
f"read-note exited {result.exit_code} on a SECURITY_VALIDATION_ERROR; "
"other tool commands exit non-zero on error payloads"
)
test-int/bughunt_fixes/test_cli_tool_recent_activity_page_size.py
+76
View File
@@ -0,0 +1,76 @@
"""Bug hunt regression test (#3): `bm tool recent-activity` page_size default.
The MCP recent_activity tool defaults page_size=10; the CLI wrapper used to
default to 50. Because page_size becomes the SQL LIMIT for the query, identical
default invocations returned a different number of rows from CLI vs MCP. This
integration test proves the CLI default now matches the MCP default of 10.
"""
import json
from typer.testing import CliRunner
from basic_memory.cli.main import app as cli_app
runner = CliRunner()
MCP_DEFAULT_PAGE_SIZE = 10
def _write_note(title: str, folder: str, content: str) -> None:
result = runner.invoke(
cli_app,
[
"tool",
"write-note",
"--title",
title,
"--folder",
folder,
"--content",
content,
],
)
assert result.exit_code == 0, result.output
def test_recent_activity_default_page_size_matches_mcp(
app, app_config, test_project, config_manager, monkeypatch
):
"""CLI recent-activity default page_size must match the MCP tool default (10)."""
monkeypatch.setenv("BASIC_MEMORY_MCP_PROJECT", test_project.name)
for i in range(15):
_write_note(
f"Parity Note {i:02d}",
"parity-recent",
f"# Parity Note {i:02d}\n\nUnique body token PARITY{i:02d}.",
)
mcp_default_result = runner.invoke(
cli_app,
[
"tool",
"recent-activity",
"--project",
test_project.name,
"--page-size",
str(MCP_DEFAULT_PAGE_SIZE),
],
)
assert mcp_default_result.exit_code == 0, mcp_default_result.output
mcp_default_rows = json.loads(mcp_default_result.stdout)
assert len(mcp_default_rows) == MCP_DEFAULT_PAGE_SIZE
cli_default_result = runner.invoke(
cli_app,
["tool", "recent-activity", "--project", test_project.name],
)
assert cli_default_result.exit_code == 0, cli_default_result.output
cli_default_rows = json.loads(cli_default_result.stdout)
assert len(cli_default_rows) == MCP_DEFAULT_PAGE_SIZE, (
f"CLI recent-activity default returned {len(cli_default_rows)} rows but "
f"the MCP tool default (page_size={MCP_DEFAULT_PAGE_SIZE}) returns "
f"{len(mcp_default_rows)}; the CLI and MCP default page_size must match."
)
test-int/bughunt_fixes/test_cli_tool_search_notes_category.py
+73
View File
@@ -0,0 +1,73 @@
"""Bug hunt regression test (#4): `bm tool search-notes` --category filter.
The MCP search_notes tool exposes a `categories` parameter for exact-match
observation-category filtering. The CLI wrapper had no equivalent flag. This
integration test asserts the CLI now exposes `--category` and that it filters
observation results to the requested category exactly.
"""
import json
from typer.testing import CliRunner
from basic_memory.cli.main import app as cli_app
runner = CliRunner()
def _write_note(title: str, folder: str, content: str) -> dict:
result = runner.invoke(
cli_app,
[
"tool",
"write-note",
"--title",
title,
"--folder",
folder,
"--content",
content,
],
)
assert result.exit_code == 0, result.output
return json.loads(result.stdout)
def test_search_notes_exposes_category_filter(app, app_config, test_project, config_manager):
"""CLI search-notes should expose --category like the MCP `categories` param."""
_write_note(
"Category Filter Note",
"parity-category",
"# Category Filter Note\n\n"
"## Observations\n"
"- [requirement] system must authenticate users CATTOKEN\n"
"- [decision] use OAuth for auth CATTOKEN\n",
)
result = runner.invoke(
cli_app,
[
"tool",
"search-notes",
"CATTOKEN",
"--project",
test_project.name,
"--entity-type",
"observation",
"--category",
"requirement",
],
)
assert result.exit_code == 0, (
"`--category` filter is not supported by the CLI search-notes command "
"even though the MCP search_notes tool documents a `categories` param. "
f"exit_code={result.exit_code} output={result.output}"
)
payload = json.loads(result.stdout)
categories = {r.get("category") for r in payload.get("results", []) if r.get("category")}
assert categories == {"requirement"}, (
"--category requirement should return only requirement observations, "
f"got categories={categories}"
)
test-int/bughunt_fixes/test_cli_tool_write_note_parity.py
+193
View File
@@ -0,0 +1,193 @@
"""Bug hunt regression tests: `bm tool write-note` CLI/MCP parity.
Covers three confirmed bugs found by the integration-test bug hunt:
- #1 / #5: write-note exits 0 on a conflict/error JSON result (silent failure,
inconsistent with delete-note/edit-note/search-notes which exit non-zero).
- #2: write-note had no `--overwrite` flag even though the MCP write_note tool
supports overwrite=True to replace an existing note.
These are integration tests: real CliRunner -> CLI command -> MCP tool ->
in-process ASGI API -> real SQLite/Postgres DB and filesystem. No mocks.
"""
import asyncio
import json
from typer.testing import CliRunner
from basic_memory.cli.main import app as cli_app
from basic_memory.mcp.tools import write_note as mcp_write_note
runner = CliRunner()
# --- #1: write-note exits non-zero on a conflict/error result ---
def _write_conflict(content_token: str):
return runner.invoke(
cli_app,
[
"tool",
"write-note",
"--title",
"Conflict Exit Note",
"--folder",
"parity-conflict",
"--content",
f"# Conflict Exit Note\n\n{content_token}",
"--project",
"test-project",
],
)
def test_write_note_nonzero_exit_on_conflict_error(app, app_config, test_project, config_manager):
"""write-note should exit non-zero when the MCP result carries an error."""
first = _write_conflict("FIRST")
assert first.exit_code == 0, first.output
second = _write_conflict("SECOND")
payload = json.loads(second.stdout)
# Confirm the MCP layer reported a conflict error in the JSON.
assert payload.get("error") == "NOTE_ALREADY_EXISTS", payload
assert payload.get("action") == "conflict", payload
# Parity with delete-note / edit-note / search-notes: an error result
# must drive a non-zero exit code so scripts can detect failure.
assert second.exit_code != 0, (
"write-note returned an error JSON payload "
f"({payload.get('error')}) but exited 0. Sibling tool commands "
"(delete-note, edit-note, search-notes) exit non-zero on error."
)
# --- #5: blocked NOTE_ALREADY_EXISTS write must not report success ---
def _cli_write(project_name: str):
return runner.invoke(
cli_app,
[
"tool",
"write-note",
"--title",
"Conflict Note",
"--folder",
"conflict",
"--content",
"# Conflict Note\n\nFirst body.\n",
"--project",
project_name,
],
)
def test_mcp_write_note_conflict_emits_error(app, app_config, test_project, config_manager):
"""Baseline: the MCP tool reports NOTE_ALREADY_EXISTS on a blocked re-write."""
async def _go():
first = await mcp_write_note(
title="Conflict Note",
content="# Conflict Note\n\nFirst body.\n",
directory="conflict",
project=test_project.name,
output_format="json",
)
# output_format="json" returns a dict; narrow for the type checker.
assert isinstance(first, dict)
assert first.get("action") == "created"
assert "error" not in first
second = await mcp_write_note(
title="Conflict Note",
content="# Conflict Note\n\nSecond body (should be blocked).\n",
directory="conflict",
project=test_project.name,
output_format="json",
)
return second
second = asyncio.run(_go())
assert isinstance(second, dict)
assert second.get("error") == "NOTE_ALREADY_EXISTS"
assert second.get("action") == "conflict"
assert second.get("file_path") is None
def test_cli_write_note_conflict_should_exit_nonzero(app, app_config, test_project, config_manager):
"""CLI write-note must NOT exit 0 when the write was blocked by a conflict."""
first = _cli_write(test_project.name)
assert first.exit_code == 0, first.output
first_payload = json.loads(first.stdout)
assert first_payload["action"] == "created"
second = _cli_write(test_project.name)
assert "NOTE_ALREADY_EXISTS" in second.stdout, second.output
assert second.exit_code != 0, (
f"write-note exited {second.exit_code} after a blocked NOTE_ALREADY_EXISTS "
"write; the note was NOT written but the CLI reported success"
)
# --- #2: write-note --overwrite flag (MCP overwrite=True parity) ---
def _write_overwrite(args_extra: list[str]):
return runner.invoke(
cli_app,
[
"tool",
"write-note",
"--title",
"Overwrite Parity Note",
"--folder",
"parity-overwrite",
"--content",
"# Overwrite Parity Note\n\nVERSION_BODY",
*args_extra,
],
)
def test_write_note_cli_can_overwrite_like_mcp(app, app_config, test_project, config_manager):
"""CLI write-note must be able to overwrite an existing note (MCP overwrite=True)."""
first = _write_overwrite(["--project", test_project.name])
assert first.exit_code == 0, first.output
first_data = json.loads(first.stdout)
permalink = first_data["permalink"]
second = runner.invoke(
cli_app,
[
"tool",
"write-note",
"--title",
"Overwrite Parity Note",
"--folder",
"parity-overwrite",
"--content",
"# Overwrite Parity Note\n\nNEW_VERSION_BODY",
"--project",
test_project.name,
"--overwrite",
],
)
assert second.exit_code == 0, (
"CLI write-note has no way to overwrite an existing note even though "
"the MCP write_note tool supports overwrite=True. "
f"exit_code={second.exit_code} output={second.output}"
)
read = runner.invoke(
cli_app,
["tool", "read-note", permalink, "--project", test_project.name],
)
assert read.exit_code == 0, read.output
read_data = json.loads(read.stdout)
assert "NEW_VERSION_BODY" in (read_data.get("content") or "")