mirror of
https://github.com/basicmachines-co/basic-memory
synced 2026-06-21 13:47:35 +00:00
fix(mcp): resolve write_note overwrite conflicts by file_path strictly
`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 <uhg.dennnis@gmail.com>
This commit is contained in:
@@ -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()
|
||||
)
|
||||
|
||||
@@ -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"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user