From 476239d8788fc0cb589b487efe0565f7ba3af193 Mon Sep 17 00:00:00 2001 From: Adit Karode <83557805+AKarode@users.noreply.github.com> Date: Sat, 6 Jun 2026 22:41:46 -0400 Subject: [PATCH] feat(cli): add delete-note tool command --- src/basic_memory/cli/commands/tool.py | 81 +++++ src/basic_memory/mcp/tools/delete_note.py | 10 +- .../test_cli_tool_delete_note_integration.py | 277 ++++++++++++++++++ tests/cli/test_cli_tool_json_output.py | 147 ++++++++++ tests/mcp/test_tool_json_output_modes.py | 35 +++ 5 files changed, 549 insertions(+), 1 deletion(-) create mode 100644 test-int/cli/test_cli_tool_delete_note_integration.py diff --git a/src/basic_memory/cli/commands/tool.py b/src/basic_memory/cli/commands/tool.py index 0eb96aa4..db901ef6 100644 --- a/src/basic_memory/cli/commands/tool.py +++ b/src/basic_memory/cli/commands/tool.py @@ -15,6 +15,7 @@ from basic_memory.cli.app import app from basic_memory.cli.commands.command_utils import run_with_cleanup from basic_memory.cli.commands.routing import force_routing, validate_routing_flags from basic_memory.mcp.tools import build_context as mcp_build_context +from basic_memory.mcp.tools import delete_note as mcp_delete_note from basic_memory.mcp.tools import edit_note as mcp_edit_note from basic_memory.mcp.tools import list_memory_projects as mcp_list_projects from basic_memory.mcp.tools import list_workspaces as mcp_list_workspaces @@ -40,6 +41,26 @@ def _print_json(result: Any) -> None: print(json.dumps(result, indent=2, ensure_ascii=True, default=str)) +def _delete_note_failure_message(result: dict[str, Any]) -> str | None: + """Return the CLI failure message for delete-note JSON results, if any.""" + error = result.get("error") + if error: + return str(error) + + failed_deletes = result.get("failed_deletes") + # Trigger: directory deletion can partially fail without raising from the service. + # Why: cleanup scripts need a non-zero exit when files remain undeleted. + # Outcome: the CLI fails even if older MCP JSON did not include an error field. + if ( + result.get("is_directory") is True + and isinstance(failed_deletes, int) + and failed_deletes > 0 + ): + return f"Directory delete incomplete: {failed_deletes} file(s) failed" + + return None + + # --- Commands --- @@ -178,6 +199,66 @@ def read_note( raise +@tool_app.command("delete-note") +def delete_note( + identifier: str, + is_directory: bool = typer.Option( + False, "--is-directory", help="Delete a directory instead of a single note" + ), + project: Annotated[ + Optional[str], + typer.Option(help="The project to use. If not provided, the default project will be used."), + ] = None, + project_id: Annotated[ + Optional[str], + typer.Option( + "--project-id", + help="Project external_id (UUID). Takes precedence over --project; use to disambiguate same-named projects across cloud workspaces.", + ), + ] = None, + local: bool = typer.Option( + False, "--local", help="Force local API routing (ignore cloud mode)" + ), + cloud: bool = typer.Option(False, "--cloud", help="Force cloud API routing"), +) -> None: + """Delete a note or directory from the knowledge base. + + Examples: + + bm tool delete-note notes/old-draft + bm tool delete-note docs/archive --is-directory + """ + try: + validate_routing_flags(local, cloud) + + with force_routing(local=local, cloud=cloud): + result = run_with_cleanup( + mcp_delete_note( + identifier=identifier, + is_directory=is_directory, + project=project, + project_id=project_id, + output_format="json", + ) + ) + + if isinstance(result, dict): + failure_message = _delete_note_failure_message(result) + if failure_message: + typer.echo(f"Error: {failure_message}", err=True) + raise typer.Exit(1) + + _print_json(result) + except ValueError as e: + typer.echo(f"Error: {e}", err=True) + raise typer.Exit(1) + except Exception as e: # pragma: no cover + if not isinstance(e, typer.Exit): + typer.echo(f"Error during delete_note: {e}", err=True) + raise typer.Exit(1) + raise + + @tool_app.command() def edit_note( identifier: str, diff --git a/src/basic_memory/mcp/tools/delete_note.py b/src/basic_memory/mcp/tools/delete_note.py index b7be9354..348d4957 100644 --- a/src/basic_memory/mcp/tools/delete_note.py +++ b/src/basic_memory/mcp/tools/delete_note.py @@ -315,14 +315,22 @@ async def delete_note( ) result = await knowledge_client.delete_directory(directory_identifier) if output_format == "json": - return { + response = { "deleted": result.failed_deletes == 0, "is_directory": True, "identifier": identifier, "total_files": result.total_files, "successful_deletes": result.successful_deletes, "failed_deletes": result.failed_deletes, + "deleted_files": result.deleted_files, + "errors": [error.model_dump() for error in result.errors], } + if result.failed_deletes > 0: + response["error"] = ( + "Directory delete incomplete: " + f"{result.failed_deletes} of {result.total_files} file(s) failed" + ) + return response # Build success message for directory delete result_lines = [ diff --git a/test-int/cli/test_cli_tool_delete_note_integration.py b/test-int/cli/test_cli_tool_delete_note_integration.py new file mode 100644 index 00000000..4778c71e --- /dev/null +++ b/test-int/cli/test_cli_tool_delete_note_integration.py @@ -0,0 +1,277 @@ +"""Integration tests for `basic-memory tool delete-note`.""" + +import json +from pathlib import Path +from typing import Any + +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, + *, + project: str | None = None, +) -> dict[str, Any]: + args = [ + "tool", + "write-note", + "--title", + title, + "--folder", + folder, + "--content", + content, + ] + if project is not None: + args.extend(["--project", project]) + + result = runner.invoke(cli_app, args) + assert result.exit_code == 0, result.output + return json.loads(result.stdout) + + +def _read_note(identifier: str, *, project: str | None = None) -> dict[str, Any]: + args = ["tool", "read-note", identifier] + if project is not None: + args.extend(["--project", project]) + + result = runner.invoke(cli_app, args) + assert result.exit_code == 0, result.output + return json.loads(result.stdout) + + +def _delete_note( + identifier: str, + *, + is_directory: bool = False, + project: str | None = None, + project_id: str | None = None, + local: bool = False, +) -> tuple[int, dict[str, Any], str]: + args = ["tool", "delete-note", identifier] + if is_directory: + args.append("--is-directory") + if project is not None: + args.extend(["--project", project]) + if project_id is not None: + args.extend(["--project-id", project_id]) + if local: + args.append("--local") + + result = runner.invoke(cli_app, args) + payload = json.loads(result.stdout) if result.stdout else {} + return result.exit_code, payload, result.output + + +def _search_notes( + query: str, + *, + mode_flag: str | None = None, + page_size: int = 20, +) -> dict[str, Any]: + args = ["tool", "search-notes", query, "--page-size", str(page_size)] + if mode_flag is not None: + args.append(mode_flag) + + result = runner.invoke( + cli_app, + args, + ) + assert result.exit_code == 0, result.output + return json.loads(result.stdout) + + +def _project_file(test_project, file_path: str) -> Path: + return Path(test_project.path) / file_path + + +def test_delete_note_removes_file_database_record_and_search_result( + app, app_config, test_project, config_manager +) -> None: + """Single-note deletion removes the note from every user-visible surface.""" + note = _write_note( + "CLI Delete Single Note", + "delete-cli", + "# CLI Delete Single Note\n\nUniqueSingleDeleteToken\n\n- [status] ready to delete", + ) + note_path = _project_file(test_project, note["file_path"]) + assert note_path.exists() + + exit_code, payload, output = _delete_note(note["permalink"]) + + assert exit_code == 0, output + assert payload == { + "deleted": True, + "title": "CLI Delete Single Note", + "permalink": note["permalink"], + "file_path": note["file_path"], + } + assert not note_path.exists() + + missing = _read_note(note["permalink"]) + assert missing["title"] is None + assert missing["permalink"] is None + assert missing["content"] is None + + search = _search_notes("CLI Delete Single Note", mode_flag="--title") + assert search["total"] == 0 + assert search["results"] == [] + + +def test_delete_note_not_found_returns_json_without_error( + app, app_config, test_project, config_manager +) -> None: + """A missing note is machine-readable and does not produce a CLI failure.""" + exit_code, payload, output = _delete_note("delete-cli/missing-note") + + assert exit_code == 0, output + assert payload == { + "deleted": False, + "title": None, + "permalink": None, + "file_path": None, + } + + +def test_delete_note_case_mismatch_does_not_delete_exact_note( + app, app_config, test_project, config_manager +) -> None: + """Strict CLI deletes must not fuzzy-match a differently cased title.""" + note = _write_note( + "CLI CamelCase Delete Note", + "delete-cli", + "# CLI CamelCase Delete Note\n\nCaseSensitiveDeleteToken", + ) + + exit_code, payload, output = _delete_note("cli camelcase delete note") + + assert exit_code == 0, output + assert payload["deleted"] is False + still_there = _read_note(note["permalink"]) + assert still_there["title"] == "CLI CamelCase Delete Note" + assert "CaseSensitiveDeleteToken" in still_there["content"] + + +def test_delete_note_project_id_takes_precedence_over_wrong_project_name( + app, app_config, test_project, config_manager +) -> None: + """CLI `--project-id` routes destructive operations to the exact project.""" + note = _write_note( + "CLI Delete By Project ID", + "delete-cli", + "# CLI Delete By Project ID\n\nProjectIdDeleteToken", + ) + + exit_code, payload, output = _delete_note( + note["file_path"], + project="not-the-test-project", + project_id=test_project.external_id, + ) + + assert exit_code == 0, output + assert payload["deleted"] is True + assert payload["title"] == "CLI Delete By Project ID" + assert _read_note(note["permalink"])["title"] is None + + +def test_delete_note_memory_url_detects_project_from_identifier( + app, app_config, test_project, config_manager +) -> None: + """A memory:// URL can select the project without a separate --project flag.""" + note = _write_note( + "CLI Delete Memory URL", + "delete-cli", + "# CLI Delete Memory URL\n\nMemoryUrlDeleteToken", + project=test_project.name, + ) + memory_url = f"memory://{test_project.name}/{note['permalink']}" + + exit_code, payload, output = _delete_note(memory_url) + + assert exit_code == 0, output + assert payload["deleted"] is True + assert payload["permalink"] == note["permalink"] + assert _read_note(note["permalink"], project=test_project.name)["title"] is None + + +def test_delete_directory_removes_nested_files_database_records_and_search_results( + app, app_config, test_project, config_manager +) -> None: + """Directory deletion removes nested notes and reports a complete JSON summary.""" + notes = [ + _write_note( + "CLI Delete Directory Root", + "delete-cli-dir", + "# CLI Delete Directory Root\n\nDirectoryDeleteTokenRoot", + ), + _write_note( + "CLI Delete Directory Child", + "delete-cli-dir/child", + "# CLI Delete Directory Child\n\nDirectoryDeleteTokenChild", + ), + _write_note( + "CLI Delete Directory Deep Child", + "delete-cli-dir/child/deep", + "# CLI Delete Directory Deep Child\n\nDirectoryDeleteTokenDeep", + ), + ] + note_paths = [_project_file(test_project, note["file_path"]) for note in notes] + assert all(path.exists() for path in note_paths) + + exit_code, payload, output = _delete_note("delete-cli-dir", is_directory=True, local=True) + + assert exit_code == 0, output + assert payload["deleted"] is True + assert payload["is_directory"] is True + assert payload["identifier"] == "delete-cli-dir" + assert payload["total_files"] == 3 + assert payload["successful_deletes"] == 3 + assert payload["failed_deletes"] == 0 + assert payload["errors"] == [] + assert set(payload["deleted_files"]) == {note["file_path"] for note in notes} + assert not any(path.exists() for path in note_paths) + + for note in notes: + assert _read_note(note["permalink"])["title"] is None + + search = _search_notes("CLI Delete Directory", mode_flag="--title") + assert search["total"] == 0 + assert search["results"] == [] + + +def test_delete_directory_without_flag_does_not_delete_child_notes( + app, app_config, test_project, config_manager +) -> None: + """The CLI must not treat a directory path as destructive without --is-directory.""" + note = _write_note( + "CLI Delete Directory Safety", + "delete-cli-safety", + "# CLI Delete Directory Safety\n\nDirectorySafetyToken", + ) + + exit_code, payload, output = _delete_note("delete-cli-safety") + + assert exit_code == 0, output + assert payload["deleted"] is False + still_there = _read_note(note["permalink"]) + assert still_there["title"] == "CLI Delete Directory Safety" + assert _project_file(test_project, note["file_path"]).exists() + + +def test_delete_note_rejects_conflicting_routing_flags( + app, app_config, test_project, config_manager +) -> None: + """delete-note validates the same --local/--cloud conflict as other tool commands.""" + result = runner.invoke( + cli_app, + ["tool", "delete-note", "delete-cli/missing-note", "--local", "--cloud"], + ) + + assert result.exit_code != 0 + assert "Cannot specify both --local and --cloud" in result.output diff --git a/tests/cli/test_cli_tool_json_output.py b/tests/cli/test_cli_tool_json_output.py index d5e5e183..f32c7dfc 100644 --- a/tests/cli/test_cli_tool_json_output.py +++ b/tests/cli/test_cli_tool_json_output.py @@ -39,6 +39,21 @@ EDIT_NOTE_RESULT = { "operation": "append", } +DELETE_NOTE_RESULT = { + "deleted": True, + "is_directory": False, + "title": "Test Note", + "permalink": "notes/test-note", + "file_path": "notes/Test Note.md", +} + +DELETE_DIRECTORY_RESULT = { + "deleted": True, + "is_directory": True, + "directory": "notes/archive", + "deleted_count": 3, +} + BUILD_CONTEXT_RESULT = { "results": [], "metadata": {"uri": "test/topic", "depth": 1}, @@ -215,6 +230,138 @@ def test_read_note_include_frontmatter(mock_mcp_read): assert mock_mcp_read.call_args.kwargs["include_frontmatter"] is True +# --- delete-note --- + + +@patch( + "basic_memory.cli.commands.tool.mcp_delete_note", + new_callable=AsyncMock, + return_value=DELETE_NOTE_RESULT, +) +def test_delete_note_json_output(mock_mcp_delete: AsyncMock) -> None: + """delete-note outputs valid JSON from MCP tool.""" + result = runner.invoke( + cli_app, + ["tool", "delete-note", "test-note"], + ) + + assert result.exit_code == 0, f"CLI failed: {result.output}" + data = json.loads(result.output) + assert data["deleted"] is True + assert data["permalink"] == "notes/test-note" + mock_mcp_delete.assert_called_once() + assert mock_mcp_delete.call_args.kwargs["output_format"] == "json" + assert mock_mcp_delete.call_args.kwargs["is_directory"] is False + + +@patch( + "basic_memory.cli.commands.tool.mcp_delete_note", + new_callable=AsyncMock, + return_value=DELETE_DIRECTORY_RESULT, +) +def test_delete_note_directory_flag(mock_mcp_delete: AsyncMock) -> None: + """delete-note --is-directory passes directory mode to MCP.""" + result = runner.invoke( + cli_app, + ["tool", "delete-note", "notes/archive", "--is-directory"], + ) + + assert result.exit_code == 0, f"CLI failed: {result.output}" + data = json.loads(result.output) + assert data["is_directory"] is True + assert mock_mcp_delete.call_args.kwargs["is_directory"] is True + + +@patch( + "basic_memory.cli.commands.tool.mcp_delete_note", + new_callable=AsyncMock, + return_value={ + "deleted": False, + "is_directory": False, + "identifier": "missing-note", + "error": None, + }, +) +def test_delete_note_not_found_outputs_json(mock_mcp_delete: AsyncMock) -> None: + """delete-note treats not-found JSON without an error as a successful command.""" + result = runner.invoke( + cli_app, + ["tool", "delete-note", "missing-note"], + ) + + assert result.exit_code == 0, f"CLI failed: {result.output}" + data = json.loads(result.output) + assert data["deleted"] is False + assert data["identifier"] == "missing-note" + assert mock_mcp_delete.call_args.kwargs["output_format"] == "json" + + +@patch( + "basic_memory.cli.commands.tool.mcp_delete_note", + new_callable=AsyncMock, + return_value={ + "deleted": False, + "is_directory": False, + "identifier": "test-note", + "error": "Delete failed", + }, +) +def test_delete_note_error_response(mock_mcp_delete: AsyncMock) -> None: + """delete-note exits with code 1 when MCP tool returns an error field.""" + result = runner.invoke( + cli_app, + ["tool", "delete-note", "test-note"], + ) + + assert result.exit_code == 1 + assert "Error: Delete failed" in result.output + assert mock_mcp_delete.call_args.kwargs["output_format"] == "json" + + +@patch( + "basic_memory.cli.commands.tool.mcp_delete_note", + new_callable=AsyncMock, + return_value={ + "deleted": False, + "is_directory": True, + "identifier": "notes/archive", + "total_files": 3, + "successful_deletes": 2, + "failed_deletes": 1, + "errors": [{"path": "notes/archive/locked.md", "error": "permission denied"}], + }, +) +def test_delete_note_directory_partial_failure_exits_nonzero( + mock_mcp_delete: AsyncMock, +) -> None: + """delete-note --is-directory exits 1 when any directory file remains undeleted.""" + result = runner.invoke( + cli_app, + ["tool", "delete-note", "notes/archive", "--is-directory"], + ) + + assert result.exit_code == 1 + assert "Error: Directory delete incomplete: 1 file(s) failed" in result.output + assert mock_mcp_delete.call_args.kwargs["output_format"] == "json" + + +@patch( + "basic_memory.cli.commands.tool.mcp_delete_note", + new_callable=AsyncMock, + return_value=DELETE_NOTE_RESULT, +) +def test_delete_note_project_id_passthrough(mock_mcp_delete: AsyncMock) -> None: + """--project-id forwards to the MCP tool's project_id parameter.""" + uuid = "11111111-1111-1111-1111-111111111111" + result = runner.invoke( + cli_app, + ["tool", "delete-note", "test-note", "--project-id", uuid], + ) + + assert result.exit_code == 0, f"CLI failed: {result.output}" + assert mock_mcp_delete.call_args.kwargs["project_id"] == uuid + + # --- edit-note --- diff --git a/tests/mcp/test_tool_json_output_modes.py b/tests/mcp/test_tool_json_output_modes.py index 069534b5..06960aac 100644 --- a/tests/mcp/test_tool_json_output_modes.py +++ b/tests/mcp/test_tool_json_output_modes.py @@ -18,6 +18,7 @@ from basic_memory.mcp.tools import ( recent_activity, write_note, ) +from basic_memory.schemas.response import DirectoryDeleteError, DirectoryDeleteResult @pytest.mark.asyncio @@ -326,6 +327,40 @@ async def test_delete_directory_json_mode_returns_structured_error_on_failure( assert "simulated directory delete failure" in json_delete["error"] +@pytest.mark.asyncio +async def test_delete_directory_json_mode_reports_partial_delete_failure( + app, test_project, monkeypatch +): + async def mock_delete_directory(self, directory: str): + return DirectoryDeleteResult( + total_files=2, + successful_deletes=1, + failed_deletes=1, + deleted_files=["mode-tests/deleted.md"], + errors=[ + DirectoryDeleteError( + path="mode-tests/locked.md", + error="permission denied", + ) + ], + ) + + monkeypatch.setattr(KnowledgeClient, "delete_directory", mock_delete_directory) + + json_delete = await delete_note( + identifier="mode-tests", + is_directory=True, + project=test_project.name, + output_format="json", + ) + assert isinstance(json_delete, dict) + assert json_delete["deleted"] is False + assert json_delete["failed_deletes"] == 1 + assert json_delete["deleted_files"] == ["mode-tests/deleted.md"] + assert json_delete["errors"] == [{"path": "mode-tests/locked.md", "error": "permission denied"}] + assert "Directory delete incomplete" in json_delete["error"] + + @pytest.mark.asyncio async def test_move_note_text_and_json_modes(app, test_project): await write_note(