Skip to content

Commit 9809b46

Browse files
phernandezclaude
andauthored
fix: enforce strict entity resolution in destructive MCP tools (#650)
Signed-off-by: phernandez <paul@basicmachines.co> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 76ac880 commit 9809b46

11 files changed

Lines changed: 309 additions & 17 deletions

src/basic_memory/mcp/tools/delete_note.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@ async def delete_note(
318318
note_file_path = None
319319
try:
320320
# Resolve identifier to entity ID
321-
entity_id = await knowledge_client.resolve_entity(identifier)
321+
entity_id = await knowledge_client.resolve_entity(identifier, strict=True)
322322
if output_format == "json":
323323
entity = await knowledge_client.get_entity(entity_id)
324324
note_title = entity.title

src/basic_memory/mcp/tools/edit_note.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,7 @@ async def edit_note(
293293

294294
# Try to resolve the entity; for append/prepend, create it if not found
295295
try:
296-
entity_id = await knowledge_client.resolve_entity(identifier)
296+
entity_id = await knowledge_client.resolve_entity(identifier, strict=True)
297297
except Exception as resolve_error:
298298
# Trigger: entity does not exist yet
299299
# Why: append/prepend can meaningfully create a new note from the content,

src/basic_memory/mcp/tools/move_note.py

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
from loguru import logger
88
from fastmcp import Context
9+
from mcp.server.fastmcp.exceptions import ToolError
910

1011
from basic_memory.mcp.server import mcp
1112
from basic_memory.mcp.project_context import get_project_client
@@ -637,16 +638,34 @@ async def _ensure_resolved_entity_id() -> str:
637638
"""Resolve and cache the source entity ID for the duration of this move."""
638639
nonlocal resolved_entity_id
639640
if resolved_entity_id is None:
640-
resolved_entity_id = await knowledge_client.resolve_entity(identifier)
641+
resolved_entity_id = await knowledge_client.resolve_entity(identifier, strict=True)
641642
return resolved_entity_id
642643

643644
try:
644645
resolved_entity_id = await _ensure_resolved_entity_id()
645646
source_entity = await knowledge_client.get_entity(resolved_entity_id)
646647
if "." in source_entity.file_path:
647648
source_ext = source_entity.file_path.split(".")[-1]
649+
except ToolError as e:
650+
# Trigger: strict=True resolve_entity raised because the entity was not found.
651+
# Why: fail fast with a formatted error instead of silently falling through
652+
# to extension defaults and failing later with a confusing message.
653+
# Outcome: move_note returns a user-facing not-found error immediately.
654+
logger.error(f"Move failed for '{identifier}' to '{destination_path}': {e}")
655+
if output_format == "json":
656+
return {
657+
"moved": False,
658+
"title": None,
659+
"permalink": None,
660+
"file_path": None,
661+
"source": identifier,
662+
"destination": destination_path,
663+
"error": str(e),
664+
}
665+
return _format_move_error_response(str(e), identifier, destination_path)
648666
except Exception as e:
649-
# If we can't fetch source metadata, continue with extension defaults.
667+
# If we can't fetch source metadata (e.g. get_entity or file_path parsing fails),
668+
# continue with extension defaults — the entity was at least resolved.
650669
logger.debug(f"Could not fetch source entity for extension check: {e}")
651670

652671
# --- Resolve destination_folder into destination_path ---

src/basic_memory/schemas/base.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,12 @@ def validate_timeframe(timeframe: str) -> str:
140140
if parsed > now:
141141
raise ValueError("Timeframe cannot be in the future") # pragma: no cover
142142

143-
# Could format the duration back to our standard format
144-
days = (now - parsed).days
143+
# Round to nearest day to handle DST transitions where an hour shift
144+
# can cause e.g. "7d" to compute as 6 days + 23 hours
145+
total_seconds = (now - parsed).total_seconds()
146+
days = round(total_seconds / 86400)
145147

146-
# Could enforce reasonable limits
148+
# Enforce reasonable limits
147149
if days > 365:
148150
raise ValueError("Timeframe should be <= 1 year")
149151

test-int/mcp/test_delete_note_integration.py

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -307,8 +307,13 @@ async def test_delete_note_by_file_path(mcp_server, app, test_project):
307307

308308

309309
@pytest.mark.asyncio
310-
async def test_delete_note_case_insensitive(mcp_server, app, test_project):
311-
"""Test that note deletion is case insensitive for titles."""
310+
async def test_delete_note_rejects_case_mismatch(mcp_server, app, test_project):
311+
"""Test that delete_note with wrong case does not fuzzy-match to an existing note.
312+
313+
Strict resolution (#649) prevents destructive operations from silently
314+
resolving to a different note via fuzzy search. Case-mismatched titles
315+
should be rejected, not resolved to the nearest match.
316+
"""
312317

313318
async with Client(mcp_server) as client:
314319
# Create a note with mixed case
@@ -323,7 +328,7 @@ async def test_delete_note_case_insensitive(mcp_server, app, test_project):
323328
},
324329
)
325330

326-
# Try to delete with different case
331+
# Try to delete with different case — should NOT find the note
327332
delete_result = await client.call_tool(
328333
"delete_note",
329334
{
@@ -332,8 +337,28 @@ async def test_delete_note_case_insensitive(mcp_server, app, test_project):
332337
},
333338
)
334339

335-
# Should return True for successful deletion
336-
assert "true" in delete_result.content[0].text.lower()
340+
# Should return False (not found) — strict mode rejects fuzzy matches
341+
assert "false" in delete_result.content[0].text.lower()
342+
343+
# Verify the note still exists using the exact title
344+
read_result = await client.call_tool(
345+
"read_note",
346+
{
347+
"project": test_project.name,
348+
"identifier": "CamelCase Note Title",
349+
},
350+
)
351+
assert "Testing case sensitivity" in read_result.content[0].text
352+
353+
# Delete with exact title should succeed
354+
delete_result2 = await client.call_tool(
355+
"delete_note",
356+
{
357+
"project": test_project.name,
358+
"identifier": "CamelCase Note Title",
359+
},
360+
)
361+
assert "true" in delete_result2.content[0].text.lower()
337362

338363

339364
@pytest.mark.asyncio

test-int/mcp/test_edit_note_integration.py

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -710,3 +710,81 @@ async def test_edit_note_using_different_identifiers(mcp_server, app, test_proje
710710
assert "Edited by title." in content
711711
assert "Edited by permalink." in content
712712
assert "Edited by folder/title." in content
713+
714+
715+
@pytest.mark.asyncio
716+
async def test_edit_note_append_autocreate_does_not_fuzzy_match(mcp_server, app, test_project):
717+
"""Reproduces #649: edit_note append must auto-create, not fuzzy-match to an existing note.
718+
719+
Creates two notes, then attempts to append to a nonexistent identifier.
720+
The tool should create a new note, and neither existing note should be modified.
721+
"""
722+
723+
async with Client(mcp_server) as client:
724+
# Create two notes that could be fuzzy-matched
725+
await client.call_tool(
726+
"write_note",
727+
{
728+
"project": test_project.name,
729+
"title": "Routing Test A",
730+
"directory": "test",
731+
"content": "# Routing Test A\n\nContent A.",
732+
},
733+
)
734+
await client.call_tool(
735+
"write_note",
736+
{
737+
"project": test_project.name,
738+
"title": "Routing Test B",
739+
"directory": "test",
740+
"content": "# Routing Test B\n\nContent B.",
741+
},
742+
)
743+
744+
# Attempt to edit a nonexistent note — should error, not silently edit A or B
745+
edit_result = await client.call_tool(
746+
"edit_note",
747+
{
748+
"project": test_project.name,
749+
"identifier": "Routing Test NONEXISTENT",
750+
"operation": "append",
751+
"content": "\n\nThis should NOT appear in any note.",
752+
},
753+
)
754+
755+
edit_text = edit_result.content[0].text
756+
# append to nonexistent creates a new note — verify it did NOT edit A or B
757+
assert "Created note (append)" in edit_text
758+
assert "fileCreated: true" in edit_text
759+
760+
# Verify neither A nor B was modified
761+
read_a = await client.call_tool(
762+
"read_note",
763+
{"project": test_project.name, "identifier": "Routing Test A"},
764+
)
765+
content_a = read_a.content[0].text
766+
assert "Content A" in content_a
767+
assert "This should NOT appear" not in content_a
768+
769+
read_b = await client.call_tool(
770+
"read_note",
771+
{"project": test_project.name, "identifier": "Routing Test B"},
772+
)
773+
content_b = read_b.content[0].text
774+
assert "Content B" in content_b
775+
assert "This should NOT appear" not in content_b
776+
777+
# Now test find_replace on nonexistent — should error
778+
edit_result2 = await client.call_tool(
779+
"edit_note",
780+
{
781+
"project": test_project.name,
782+
"identifier": "Routing Test NONEXISTENT AGAIN",
783+
"operation": "find_replace",
784+
"content": "replaced",
785+
"find_text": "Content",
786+
},
787+
)
788+
789+
error_text = edit_result2.content[0].text
790+
assert "Edit Failed" in error_text

test-int/mcp/test_move_note_integration.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -716,3 +716,56 @@ async def test_move_note_destination_folder_mutually_exclusive(mcp_server, app,
716716
error_text = move_result.content[0].text
717717
assert "# Move Failed - Invalid Parameters" in error_text
718718
assert "Cannot specify both" in error_text
719+
720+
721+
@pytest.mark.asyncio
722+
async def test_move_note_strict_resolution_rejects_fuzzy_match(mcp_server, app, test_project):
723+
"""move_note must not fuzzy-match a nonexistent identifier to an existing note (#649)."""
724+
725+
async with Client(mcp_server) as client:
726+
# Create two notes that could be fuzzy-matched
727+
await client.call_tool(
728+
"write_note",
729+
{
730+
"project": test_project.name,
731+
"title": "Move Strict Test A",
732+
"directory": "test",
733+
"content": "# Move Strict Test A\n\nContent A.",
734+
},
735+
)
736+
await client.call_tool(
737+
"write_note",
738+
{
739+
"project": test_project.name,
740+
"title": "Move Strict Test B",
741+
"directory": "test",
742+
"content": "# Move Strict Test B\n\nContent B.",
743+
},
744+
)
745+
746+
# Attempt to move a nonexistent note — should error, not move A or B
747+
move_result = await client.call_tool(
748+
"move_note",
749+
{
750+
"project": test_project.name,
751+
"identifier": "Move Strict Test NONEXISTENT",
752+
"destination_path": "archive/Moved.md",
753+
},
754+
)
755+
756+
assert len(move_result.content) == 1
757+
error_text = move_result.content[0].text
758+
assert "# Move Failed" in error_text
759+
760+
# Verify neither A nor B was moved
761+
read_a = await client.call_tool(
762+
"read_note",
763+
{"project": test_project.name, "identifier": "Move Strict Test A"},
764+
)
765+
assert "Content A" in read_a.content[0].text
766+
767+
read_b = await client.call_tool(
768+
"read_note",
769+
{"project": test_project.name, "identifier": "Move Strict Test B"},
770+
)
771+
assert "Content B" in read_b.content[0].text

tests/mcp/test_tool_delete_note.py

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
"""Tests for delete_note MCP tool."""
22

3-
from basic_memory.mcp.tools.delete_note import _format_delete_error_response
3+
import pytest
4+
5+
from basic_memory.mcp.tools.delete_note import delete_note, _format_delete_error_response
6+
from basic_memory.mcp.tools.read_note import read_note
7+
from basic_memory.mcp.tools.write_note import write_note
48

59

610
class TestDeleteNoteErrorFormatting:
@@ -94,5 +98,25 @@ def test_format_delete_error_with_complex_identifier(self, test_project):
9498
assert "folder/note-title" in result # Permalink format
9599

96100

97-
# Integration tests removed to focus on error formatting coverage
98-
# The error formatting tests above provide the necessary coverage for MCP tool error messaging
101+
@pytest.mark.asyncio
102+
async def test_delete_note_rejects_fuzzy_match(client, test_project):
103+
"""delete_note must reject nonexistent identifiers, not fuzzy-match to a similar note."""
104+
await write_note(
105+
project=test_project.name,
106+
title="Delete Target Note",
107+
directory="test",
108+
content="# Delete Target Note\nShould not be deleted.",
109+
)
110+
111+
# Attempt to delete a nonexistent note — should return False, not silently delete the existing note
112+
result = await delete_note(
113+
project=test_project.name,
114+
identifier="Delete Target NONEXISTENT",
115+
)
116+
117+
# Should indicate not found (False or error string)
118+
assert result is False or (isinstance(result, str) and "not found" in result.lower())
119+
120+
# Verify the existing note was NOT deleted
121+
content = await read_note("Delete Target Note", project=test_project.name)
122+
assert "Should not be deleted" in content

tests/mcp/test_tool_edit_note.py

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
"""Tests for the edit_note MCP tool."""
22

3+
34
import pytest
45

56
from basic_memory.mcp.tools.edit_note import edit_note
7+
from basic_memory.mcp.tools.read_note import read_note
68
from basic_memory.mcp.tools.write_note import write_note
79

810

@@ -613,6 +615,70 @@ async def test_edit_note_preserves_permalink_when_frontmatter_missing(client, te
613615
# The edit should succeed without validation errors
614616

615617

618+
@pytest.mark.asyncio
619+
async def test_edit_note_find_replace_rejects_fuzzy_match(client, test_project):
620+
"""find_replace must reject nonexistent identifiers, not fuzzy-match to a similar note."""
621+
# Create two notes that could be fuzzy-matched
622+
await write_note(
623+
project=test_project.name,
624+
title="Routing Test A",
625+
directory="test",
626+
content="# Routing Test A\nContent A.",
627+
)
628+
await write_note(
629+
project=test_project.name,
630+
title="Routing Test B",
631+
directory="test",
632+
content="# Routing Test B\nContent B.",
633+
)
634+
635+
# Attempt to edit a nonexistent note — should error, not silently edit A or B
636+
result = await edit_note(
637+
project=test_project.name,
638+
identifier="Routing Test NONEXISTENT",
639+
operation="find_replace",
640+
content="replaced",
641+
find_text="Content",
642+
)
643+
644+
assert isinstance(result, str)
645+
assert "# Edit Failed" in result
646+
647+
# Verify neither A nor B was modified
648+
content_a = await read_note("Routing Test A", project=test_project.name)
649+
assert "Content A" in content_a
650+
content_b = await read_note("Routing Test B", project=test_project.name)
651+
assert "Content B" in content_b
652+
653+
654+
@pytest.mark.asyncio
655+
async def test_edit_note_append_autocreate_not_fuzzy_match(client, test_project):
656+
"""append to a nonexistent note should auto-create it, not fuzzy-match an existing note."""
657+
await write_note(
658+
project=test_project.name,
659+
title="Existing Note Alpha",
660+
directory="test",
661+
content="# Existing Note Alpha\nOriginal content.",
662+
)
663+
664+
# Append to a nonexistent note — should create a new note, not edit "Existing Note Alpha"
665+
result = await edit_note(
666+
project=test_project.name,
667+
identifier="Existing Note ZZZZZ",
668+
operation="append",
669+
content="# New Note\nBrand new content.",
670+
)
671+
672+
assert isinstance(result, str)
673+
assert "Created note (append)" in result
674+
assert "fileCreated: true" in result
675+
676+
# Verify original note was NOT modified
677+
content = await read_note("Existing Note Alpha", project=test_project.name)
678+
assert "Original content" in content
679+
assert "Brand new content" not in content
680+
681+
616682
@pytest.mark.asyncio
617683
async def test_edit_note_insert_before_section_operation(client, test_project):
618684
"""Test inserting content before a section heading."""

0 commit comments

Comments
 (0)