From 4fd9cae2935a5b9eb8339e7c5d52fb31dbd4ffcd Mon Sep 17 00:00:00 2001 From: Dennis Hempel Date: Thu, 28 May 2026 09:26:38 +0200 Subject: [PATCH] fix(mcp): resolve write_note overwrite conflicts by file_path strictly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `write_note(overwrite=True)` caught the 409 from create_entity and then called `knowledge_client.resolve_entity(entity.permalink)` with the default `strict=False`. In workspace-prefixed palaces the client-built permalink omits the workspace slug, so exact permalink lookup misses and the fuzzy fallback could pick an orphan row that shares tokens with the canonical permalink. The update then wrote to the orphan, leaving the canonical row stale. On the next overwrite the permalink uniqueness check in `_resolve_schema_permalink` found duplicate rows and minted `-1`/`-2` suffixes on the canonical entity, accumulating orphans on every re-synthesis run. The 409 came from a `file_service.exists(file_path)` check in `prepare_create_entity_content`, so the file_path is the authoritative key for the canonical row — no fuzzy matching needed. Resolve by file_path with `strict=True`, POSIX-normalized so Windows clients send the form the server stores. Adds a regression test that spies on `resolve_entity` and asserts the identifier and `strict` flag, plus checks that no `-1`/`-2` suffix is minted under the canonical permalink. Reported in basic-memory-bug-report Issue 1. Signed-off-by: Dennis Hempel --- src/basic_memory/mcp/tools/write_note.py | 15 ++++- tests/mcp/test_tool_write_note.py | 73 ++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/basic_memory/mcp/tools/write_note.py b/src/basic_memory/mcp/tools/write_note.py index 6866c370..0007e721 100644 --- a/src/basic_memory/mcp/tools/write_note.py +++ b/src/basic_memory/mcp/tools/write_note.py @@ -1,6 +1,7 @@ """Write note tool for Basic Memory MCP server.""" import textwrap +from pathlib import Path from typing import Annotated, List, Union, Optional, Literal import logfire @@ -269,7 +270,19 @@ async def write_note( raise ValueError( "Entity permalink is required for updates" ) # pragma: no cover - entity_id = await knowledge_client.resolve_entity(entity.permalink) + # Resolve the conflicting entity by file_path with strict=True. + # The 409 came from a file_service.exists(file_path) check, so this + # file_path is the authoritative key for the canonical row. Resolving + # by permalink with fuzzy fallback (the previous behavior) could pick + # an orphan with a similar permalink — especially in workspace-prefixed + # palaces where the client-built permalink omits the workspace slug — + # causing the update to write to the wrong row and the next call to + # mint a -1/-2 suffix on the canonical entity. + # POSIX-normalize so Windows clients send the same form the server stores. + file_path_identifier = Path(entity.file_path).as_posix() + entity_id = await knowledge_client.resolve_entity( + file_path_identifier, strict=True + ) result = await knowledge_client.update_entity( entity_id, entity.model_dump() ) diff --git a/tests/mcp/test_tool_write_note.py b/tests/mcp/test_tool_write_note.py index 85dbb383..1198608e 100644 --- a/tests/mcp/test_tool_write_note.py +++ b/tests/mcp/test_tool_write_note.py @@ -1311,3 +1311,76 @@ class TestWriteNoteOverwriteGuard: assert "# Created note" in result assert f"project: {test_project.name}" in result assert "file_path: guard/Brand New Note.md" in result + + @pytest.mark.asyncio + async def test_write_note_overwrite_resolves_by_file_path_strictly( + self, app, test_project, entity_repository, monkeypatch + ): + """Regression: overwrite=True must resolve the conflicting entity by + file_path with strict=True, not by permalink with fuzzy fallback. + + Bug shape: in workspace-prefixed palaces the client-built permalink + omits the workspace slug, so resolve_entity(permalink) with the default + strict=False would fall through to fuzzy search and could pick an + orphan row sharing tokens with the canonical permalink. The update + then wrote to the orphan, the canonical row stayed stale, and the + next overwrite minted a -1/-2 suffix because the permalink uniqueness + check found duplicate rows. + + The 409 we catch came from a file_service.exists(file_path) check, + so file_path is the authoritative key — strict resolution against it + is safe even when permalinks are workspace-prefixed elsewhere. + """ + # Spy on the resolve_entity call to assert the identifier and strict flag. + from basic_memory.mcp.clients import knowledge as knowledge_mod + + original_resolve = knowledge_mod.KnowledgeClient.resolve_entity + captured: dict[str, Any] = {} + + async def spy_resolve(self, identifier, *, strict=False): + captured["identifier"] = identifier + captured["strict"] = strict + return await original_resolve(self, identifier, strict=strict) + + monkeypatch.setattr(knowledge_mod.KnowledgeClient, "resolve_entity", spy_resolve) + + # Create then overwrite the canonical note. + await write_note( + project=test_project.name, + title="Overview", + directory="features/foo", + content="# Overview\n\nVersion A", + ) + canonical_permalink = f"{test_project.name}/features/foo/overview" + canonical = await entity_repository.get_by_permalink(canonical_permalink) + assert canonical is not None + canonical_id = canonical.id + + result = await write_note( + project=test_project.name, + title="Overview", + directory="features/foo", + content="# Overview\n\nVersion B", + overwrite=True, + ) + assert "# Updated note" in result + + # The overwrite path resolved by file_path with strict=True — not by + # permalink with the default fuzzy fallback. + assert captured.get("identifier") == "features/foo/Overview.md" + assert captured.get("strict") is True + + # And the canonical row was updated in place — no duplicate -1/-2 row. + canonical_after = await entity_repository.get_by_permalink(canonical_permalink) + assert canonical_after is not None + assert canonical_after.id == canonical_id + + content = await read_note(canonical_permalink, project=test_project.name) + assert "Version B" in content + assert "Version A" not in content + + for suffix in ("-1", "-2"): + stray = await entity_repository.get_by_permalink(f"{canonical_permalink}{suffix}") + assert stray is None, ( + f"overwrite=True minted a stray '{suffix}' suffix on the canonical permalink" + )