From 9734fd1727181c0e12749f5e0bc3b1f1921c7bd6 Mon Sep 17 00:00:00 2001 From: phernandez Date: Mon, 20 Jul 2026 21:23:02 -0500 Subject: [PATCH 1/2] feat(mcp): add metadata param to edit_note for frontmatter updates Rebuilt against main after the note-preparation extraction: the metadata merge now lives in services/note_preparation.py and threads through the entity-service wrapper, the accepted-note write/mutation runners, the EditEntityRequest schema, and the edit_note MCP tool. Addresses all three review findings: - Frontmatter-only rewrites no longer reflow the note body. The merge extracts the body with the new remove_frontmatter(strip=False) and drops exactly one separator newline, so leading blank lines, trailing hard-break spaces, and a missing final newline round-trip byte-exact. - Null metadata values are rejected with a ValueError (surfaced as a bad request) instead of silently dropping the field after the normalize step filtered the YAML null out of indexed metadata. - The documented metadata-only pattern (empty append/prepend plus metadata) skips the content operation entirely, so it no longer appends "\n" to bodies without a trailing newline. Issue #1011. Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01A2bgrGfWiL4izcjj66u9R8 Signed-off-by: phernandez --- src/basic_memory/file_utils.py | 10 +- .../indexing/accepted_note_mutation_runner.py | 1 + .../indexing/accepted_note_write_runner.py | 5 +- src/basic_memory/mcp/tools/edit_note.py | 22 +- src/basic_memory/schemas/request.py | 6 +- src/basic_memory/services/entity_service.py | 8 +- src/basic_memory/services/note_preparation.py | 82 +++++- test-int/mcp/test_edit_note_integration.py | 94 +++++++ tests/api/v2/test_knowledge_router.py | 51 ++++ .../test_accepted_note_mutation_runner.py | 80 +++++- .../test_accepted_note_write_runner.py | 65 ++++- tests/mcp/test_tool_contracts.py | 1 + tests/mcp/test_tool_telemetry.py | 1 + tests/services/test_entity_service_prepare.py | 248 ++++++++++++++++++ tests/utils/test_file_utils.py | 11 + 15 files changed, 665 insertions(+), 20 deletions(-) diff --git a/src/basic_memory/file_utils.py b/src/basic_memory/file_utils.py index 9286ddb20..f526c470f 100644 --- a/src/basic_memory/file_utils.py +++ b/src/basic_memory/file_utils.py @@ -431,12 +431,16 @@ def parse_frontmatter(content: str) -> Dict[str, Any]: raise -def remove_frontmatter(content: str) -> str: +def remove_frontmatter(content: str, *, strip: bool = True) -> str: """ Remove YAML frontmatter from content. Args: content: Content with frontmatter + strip: When True (default), strip surrounding whitespace from the returned + body — the historical behavior. Pass False to get the body exactly as + it appears after the closing fence; frontmatter-only rewrites must not + reflow the note body (issue #1090 review). Returns: Content with frontmatter removed, or original content if no frontmatter @@ -452,10 +456,10 @@ def remove_frontmatter(content: str) -> str: # Why: inline `---` is ordinary content, not frontmatter (issue #972) # Outcome: return the content untouched (stripped to preserve prior behavior) if split is None: - return content.strip() + return content.strip() if strip else content _, body = split - return body.strip() + return body.strip() if strip else body def dump_frontmatter(post: frontmatter.Post) -> str: diff --git a/src/basic_memory/indexing/accepted_note_mutation_runner.py b/src/basic_memory/indexing/accepted_note_mutation_runner.py index 0552de812..82c8763e5 100644 --- a/src/basic_memory/indexing/accepted_note_mutation_runner.py +++ b/src/basic_memory/indexing/accepted_note_mutation_runner.py @@ -676,6 +676,7 @@ async def _run_accepted_note_edit( expected_replacements=request.data.expected_replacements, replace_subsections=request.data.replace_subsections, user_profile_value=user_profile_value, + metadata=request.data.metadata, ) except (ParseError, ValueError) as error: reject_accepted_note_mutation(AcceptedNoteMutationRejectKind.bad_request, str(error)) diff --git a/src/basic_memory/indexing/accepted_note_write_runner.py b/src/basic_memory/indexing/accepted_note_write_runner.py index b73594fb1..6e15316bb 100644 --- a/src/basic_memory/indexing/accepted_note_write_runner.py +++ b/src/basic_memory/indexing/accepted_note_write_runner.py @@ -5,7 +5,7 @@ from collections.abc import Sequence from dataclasses import dataclass from datetime import datetime -from typing import Protocol +from typing import Any, Protocol from sqlalchemy.ext.asyncio import AsyncSession @@ -88,6 +88,7 @@ async def prepare_edit_entity_content( find_text: str | None = ..., expected_replacements: int = ..., replace_subsections: bool = ..., + metadata: dict[str, Any] | None = ..., session: AsyncSession | None = ..., ) -> PreparedEntityWrite: ... @@ -310,6 +311,7 @@ async def prepare_accepted_note_edit( expected_replacements: int, replace_subsections: bool, user_profile_value: str | None, + metadata: dict[str, Any] | None = None, ) -> AcceptedPreparedNoteWrite: """Prepare a partial accepted edit and apply its entity fields.""" prepared = await preparer.prepare_edit_entity_content( @@ -321,6 +323,7 @@ async def prepare_accepted_note_edit( find_text=find_text, expected_replacements=expected_replacements, replace_subsections=replace_subsections, + metadata=metadata, session=session, ) result = AcceptedPreparedNoteWrite( diff --git a/src/basic_memory/mcp/tools/edit_note.py b/src/basic_memory/mcp/tools/edit_note.py index 1a08107fa..15fcd0b51 100644 --- a/src/basic_memory/mcp/tools/edit_note.py +++ b/src/basic_memory/mcp/tools/edit_note.py @@ -7,7 +7,7 @@ from loguru import logger from fastmcp import Context from mcp.server.fastmcp.exceptions import ToolError -from pydantic import AliasChoices, Field +from pydantic import AliasChoices, BeforeValidator, Field if TYPE_CHECKING: # pragma: no cover from basic_memory.mcp.clients import KnowledgeClient @@ -30,7 +30,7 @@ detect_project_from_workspace_identifier_prefix, is_workspace_qualified_plain_identifier, ) -from basic_memory.utils import normalize_project_reference, validate_project_path +from basic_memory.utils import coerce_dict, normalize_project_reference, validate_project_path EDIT_OPERATIONS = ( "append", @@ -347,7 +347,7 @@ def _format_error_response( @mcp.tool( title="Edit Note", - description="Edit an existing markdown note using various operations like append, prepend, find_replace, replace_section, insert_before_section, or insert_after_section.", + description="Edit an existing markdown note using various operations like append, prepend, find_replace, replace_section, insert_before_section, or insert_after_section. Pass metadata to merge YAML frontmatter fields independent of the operation.", tags={"notes"}, annotations={ "title": "Edit Note", @@ -391,6 +391,7 @@ async def edit_note( ] = None, expected_replacements: Optional[int] = None, replace_subsections: Optional[bool] = None, + metadata: Annotated[Optional[dict], BeforeValidator(coerce_dict)] = None, output_format: Literal["text", "json"] = "text", context: Context | None = None, ) -> str | dict: @@ -433,6 +434,12 @@ async def edit_note( the replacement content may freely introduce new headings. Set to false to replace only the immediate content under the header, stopping at the next heading of any level and preserving subsections. + metadata: Optional dict of frontmatter fields to merge, independent of `operation`. + Provided keys overwrite existing frontmatter values (or are added if new); + unrelated frontmatter keys and the note body are left untouched. Can be + combined with any operation in the same call. `title`, `type`, and `permalink` + are ignored since those have their own dedicated handling. Key deletion is + not supported. output_format: "text" returns the existing markdown summary. "json" returns machine-readable edit metadata. context: Optional FastMCP context for performance caching. @@ -480,6 +487,11 @@ async def edit_note( # Update status across document (expecting exactly 2 occurrences) edit_note("reports", "status-report", "find_replace", "In Progress", find_text="Not Started", expected_replacements=2) + # Update frontmatter fields without touching the body (any operation works; + # append with empty content is a no-op on the body itself) + edit_note("support", "tickets/2026-06-18-printer-offline", "append", "", + metadata={"status": "resolved", "closed_at": "2026-06-18T10:42:00Z"}) + Raises: HTTPError: If project doesn't exist or is inaccessible ValueError: If operation is invalid or required parameters are missing @@ -557,6 +569,7 @@ async def edit_note( has_find_text=bool(find_text), expected_replacements=effective_replacements, replace_subsections=effective_replace_subsections, + has_metadata=bool(metadata), ): async with get_project_client(project, context=context, project_id=project_id) as ( client, @@ -696,6 +709,7 @@ async def edit_note( directory=directory, content_type="text/markdown", content=content, + entity_metadata=metadata, ) logger.info( @@ -727,6 +741,8 @@ async def edit_note( edit_data["expected_replacements"] = str(effective_replacements) if not effective_replace_subsections: # Only send if different from default edit_data["replace_subsections"] = False + if metadata: + edit_data["metadata"] = metadata # Call the PATCH endpoint result = await knowledge_client.patch_entity(entity_id, edit_data) diff --git a/src/basic_memory/schemas/request.py b/src/basic_memory/schemas/request.py index 154dc870d..1f19c4120 100644 --- a/src/basic_memory/schemas/request.py +++ b/src/basic_memory/schemas/request.py @@ -1,6 +1,6 @@ """Request schemas for interacting with the knowledge graph.""" -from typing import List, Optional, Annotated, Literal +from typing import Any, List, Optional, Annotated, Literal from annotated_types import MaxLen, MinLen from pydantic import BaseModel, field_validator @@ -81,6 +81,10 @@ class EditEntityRequest(BaseModel): # level-aware section including subsections; False stops at the first heading # of any level, preserving them. replace_subsections: bool = True + # Frontmatter fields to merge, independent of `operation` (issue #1011). Set/overwrite + # semantics: provided keys overwrite existing values, unrelated keys and the body are + # untouched. title/type/permalink are ignored — they have their own resolution paths. + metadata: Optional[dict[str, Any]] = None @field_validator("section") @classmethod diff --git a/src/basic_memory/services/entity_service.py b/src/basic_memory/services/entity_service.py index 27bc673fa..610d4647a 100644 --- a/src/basic_memory/services/entity_service.py +++ b/src/basic_memory/services/entity_service.py @@ -3,7 +3,7 @@ from collections.abc import Callable from dataclasses import dataclass from pathlib import Path -from typing import List, Optional, Sequence, Tuple, Union +from typing import Any, List, Optional, Sequence, Tuple, Union from loguru import logger from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker @@ -264,6 +264,7 @@ async def prepare_edit_entity_content( find_text: Optional[str] = None, expected_replacements: int = 1, replace_subsections: bool = True, + metadata: Optional[dict[str, Any]] = None, skip_conflict_check: bool = False, session: AsyncSession | None = None, ) -> PreparedEntityWrite: @@ -273,6 +274,10 @@ async def prepare_edit_entity_content( does not read files, write files, or mutate database rows. That makes the edit base explicit so higher layers can reject stale content instead of silently editing whichever storage copy happens to be newest. + + ``metadata``, when provided, is merged into the note's YAML frontmatter + independent of ``operation``. Null values are rejected — key deletion is + not supported. """ return await self._note_preparation.prepare_edit_entity_content( entity, @@ -283,6 +288,7 @@ async def prepare_edit_entity_content( find_text=find_text, expected_replacements=expected_replacements, replace_subsections=replace_subsections, + metadata=metadata, skip_conflict_check=skip_conflict_check, session=session, ) diff --git a/src/basic_memory/services/note_preparation.py b/src/basic_memory/services/note_preparation.py index 662038b21..970f23795 100644 --- a/src/basic_memory/services/note_preparation.py +++ b/src/basic_memory/services/note_preparation.py @@ -6,6 +6,7 @@ from dataclasses import dataclass from datetime import datetime from pathlib import Path +from typing import Any import frontmatter import yaml @@ -648,6 +649,54 @@ def apply_edit_operation( raise ValueError(f"Unsupported operation: {operation}") +# title/type/permalink already have dedicated resolution paths in +# prepare_edit_entity_content (H1 title reconciliation, permalink resolver). Letting a +# metadata merge touch them would race with those paths and could be silently reverted. +_METADATA_IDENTITY_FIELDS = frozenset({"title", "type", "permalink"}) + + +def _merge_metadata_into_markdown(markdown_content: str, metadata: dict[str, Any]) -> str: + """Merge caller-supplied fields into a markdown string's YAML frontmatter. + + Identity fields (title/type/permalink) are dropped from the merge; every other key + overwrites the existing frontmatter value or is added new. The note body, and any + frontmatter keys not present in ``metadata``, are left untouched. + """ + null_keys = sorted(key for key, value in metadata.items() if value is None) + if null_keys: + # A null value would be filtered out of the indexed entity metadata right after + # this merge, silently losing the field even though key deletion is unsupported. + raise ValueError( + "metadata values cannot be null (key deletion is not supported): " + + ", ".join(null_keys) + ) + sanitized = {k: v for k, v in metadata.items() if k not in _METADATA_IDENTITY_FIELDS} + if not sanitized: + return markdown_content + + if has_frontmatter(markdown_content): + current_metadata = parse_frontmatter(markdown_content) + # strip=False: a frontmatter-only rewrite must not reflow the body (leading + # blank lines, trailing hard-break spaces). dump_frontmatter re-inserts the + # single blank separator line after the closing fence, so drop exactly one + # leading newline here to round-trip the body unchanged. + body = remove_frontmatter(markdown_content, strip=False) + if body.startswith("\r\n"): + body = body[2:] + elif body.startswith("\n"): + body = body[1:] + else: + current_metadata = {} + body = markdown_content + + merged_metadata = deepcopy(current_metadata) + merged_metadata.update(sanitized) + + post = frontmatter.Post(body) + post.metadata.update(merged_metadata) + return dump_frontmatter(post) + + async def prepare_edit_entity_content( dependencies: NotePreparationDependencies, entity: Entity, @@ -659,19 +708,32 @@ async def prepare_edit_entity_content( find_text: str | None = None, expected_replacements: int = 1, replace_subsections: bool = True, + metadata: dict[str, Any] | None = None, skip_conflict_check: bool = False, session: AsyncSession | None = None, ) -> PreparedEntityWrite: file_path = Path(entity.file_path) - markdown_content = apply_edit_operation( - current_content, - operation, - content, - section, - find_text, - expected_replacements, - replace_subsections, - ) + # Trigger: the documented metadata-only pattern is empty append/prepend plus metadata. + # Why: an empty append still appends "\n" to a body without a trailing newline, so a + # request advertised as frontmatter-only would mutate the body it promised to keep. + # Outcome: skip the content operation entirely and merge into the current text. + metadata_only_edit = bool(metadata) and operation in ("append", "prepend") and not content + if metadata_only_edit: + markdown_content = current_content + else: + markdown_content = apply_edit_operation( + current_content, + operation, + content, + section, + find_text, + expected_replacements, + replace_subsections, + ) + # Merge before frontmatter-derived resolution below so merged keys land in the + # indexed entity metadata in the same pass — see _merge_metadata_into_markdown. + if metadata: + markdown_content = _merge_metadata_into_markdown(markdown_content, metadata) title = entity.title note_type = entity.note_type permalink = entity.permalink @@ -876,6 +938,7 @@ async def prepare_edit_entity_content( find_text: str | None = None, expected_replacements: int = 1, replace_subsections: bool = True, + metadata: dict[str, Any] | None = None, skip_conflict_check: bool = False, session: AsyncSession | None = None, ) -> PreparedEntityWrite: @@ -889,6 +952,7 @@ async def prepare_edit_entity_content( find_text=find_text, expected_replacements=expected_replacements, replace_subsections=replace_subsections, + metadata=metadata, skip_conflict_check=skip_conflict_check, session=session, ) diff --git a/test-int/mcp/test_edit_note_integration.py b/test-int/mcp/test_edit_note_integration.py index 5a355fbe0..0f07cdb6d 100644 --- a/test-int/mcp/test_edit_note_integration.py +++ b/test-int/mcp/test_edit_note_integration.py @@ -9,6 +9,8 @@ import pytest from fastmcp import Client +from basic_memory.file_utils import parse_frontmatter + @pytest.mark.asyncio async def test_edit_note_append_operation(mcp_server, app, test_project): @@ -825,4 +827,96 @@ async def test_edit_note_recovers_file_on_disk_not_indexed(mcp_server, app, test ) content = read_result.content[0].text assert "status: final" in content + + +@pytest.mark.asyncio +async def test_edit_note_metadata_merges_frontmatter(mcp_server, app, test_project): + """metadata merges frontmatter fields independent of `operation` (issue #1011).""" + + async with Client(mcp_server) as client: + await client.call_tool( + "write_note", + { + "project": test_project.name, + "title": "Metadata Merge Ticket", + "directory": "tickets", + "content": "# Metadata Merge Ticket\n\nTicket body.", + "metadata": {"status": "open", "opened_at": "2026-06-18T09:14:00Z"}, + }, + ) + + edit_result = await client.call_tool( + "edit_note", + { + "project": test_project.name, + "identifier": "tickets/metadata-merge-ticket", + "operation": "append", + "content": "\n\nResolution notes.", + "metadata": {"status": "resolved", "closed_at": "2026-06-18T10:42:00Z"}, + }, + ) + + edit_text = edit_result.content[0].text + assert "Edited note (append)" in edit_text + + read_result = await client.call_tool( + "read_note", + {"project": test_project.name, "identifier": "tickets/metadata-merge-ticket"}, + ) + content = read_result.content[0].text + + # New key added, existing key overwritten, unrelated frontmatter and body preserved. + frontmatter = parse_frontmatter(content) + assert frontmatter["status"] == "resolved" + assert frontmatter["closed_at"] == "2026-06-18T10:42:00Z" + assert frontmatter["opened_at"] == "2026-06-18T09:14:00Z" + assert "Ticket body." in content + assert "Resolution notes." in content + + +@pytest.mark.asyncio +async def test_edit_note_metadata_ignores_identity_fields(mcp_server, app, test_project): + """title/type/permalink in `metadata` are ignored rather than hijacking the note's identity.""" + + async with Client(mcp_server) as client: + await client.call_tool( + "write_note", + { + "project": test_project.name, + "title": "Identity Guard Note", + "directory": "tickets", + "content": "# Identity Guard Note\n\nBody.", + }, + ) + + await client.call_tool( + "edit_note", + { + "project": test_project.name, + "identifier": "tickets/identity-guard-note", + "operation": "append", + "content": "", + "metadata": { + "title": "Hijacked Title", + "type": "hijacked", + "permalink": "hijacked/permalink", + "status": "resolved", + }, + }, + ) + + read_result = await client.call_tool( + "read_note", + {"project": test_project.name, "identifier": "tickets/identity-guard-note"}, + ) + content = read_result.content[0].text + frontmatter = parse_frontmatter(content) + + assert frontmatter["status"] == "resolved" + assert frontmatter["title"] == "Identity Guard Note" + # permalink is preserved, not hijacked to the metadata value. Assert on the note's + # own slug rather than the exact string, since the harness prefixes the project name. + assert frontmatter["permalink"] != "hijacked/permalink" + assert frontmatter["permalink"].endswith("tickets/identity-guard-note") + assert frontmatter["type"] == "note" assert "status: draft" not in content diff --git a/tests/api/v2/test_knowledge_router.py b/tests/api/v2/test_knowledge_router.py index fc91757a7..b9e9bca04 100644 --- a/tests/api/v2/test_knowledge_router.py +++ b/tests/api/v2/test_knowledge_router.py @@ -1070,6 +1070,57 @@ async def test_edit_entity_by_id_find_replace( assert note_content.file_write_status == "synced" +@pytest.mark.asyncio +async def test_edit_entity_by_id_metadata_merges_frontmatter( + client: AsyncClient, + file_service, + test_project: Project, + v2_project_url, + entity_repository, + session_maker, +): + """PATCH with `metadata` merges frontmatter fields independent of `operation` (issue #1011).""" + create_data = { + "title": "TestMetadataMerge", + "directory": "test", + "content": "---\nstatus: open\nowner: alice\n---\n# TestMetadataMerge\n\nBody content", + } + response = await client.post(f"{v2_project_url}/knowledge/entities", json=create_data) + assert response.status_code == 202 + created_entity = EntityResponseV2.model_validate(response.json()) + original_external_id = created_entity.external_id + assert original_external_id is not None + + edit_data = { + "operation": "append", + "content": "\n\nResolution notes.", + "metadata": {"status": "resolved", "closed_at": "2026-06-18T10:42:00Z"}, + } + response = await client.patch( + f"{v2_project_url}/knowledge/entities/{original_external_id}", + json=edit_data, + ) + + assert response.status_code == 202 + edited_entity = EntityResponseV2.model_validate(response.json()) + assert edited_entity.external_id is not None + + file_path = file_service.get_entity_path(edited_entity) + file_content, _ = await file_service.read_file(file_path) + frontmatter = parse_frontmatter(file_content) + + # New key added, existing key overwritten, unrelated key and body preserved. + assert frontmatter["closed_at"] == "2026-06-18T10:42:00Z" + assert frontmatter["status"] == "resolved" + assert frontmatter["owner"] == "alice" + assert "Body content" in file_content + assert "Resolution notes." in file_content + + note_content = await _get_note_content(session_maker, test_project.id, edited_entity.id) + assert note_content is not None + assert parse_frontmatter(note_content.markdown_content)["status"] == "resolved" + + @pytest.mark.asyncio async def test_delete_entity_by_id( client: AsyncClient, diff --git a/tests/indexing/test_accepted_note_mutation_runner.py b/tests/indexing/test_accepted_note_mutation_runner.py index dac1f9ca9..b7884241f 100644 --- a/tests/indexing/test_accepted_note_mutation_runner.py +++ b/tests/indexing/test_accepted_note_mutation_runner.py @@ -162,7 +162,18 @@ def __init__( self.conflict_calls: list[tuple[str, bool, AsyncSession | None]] = [] self.replace_calls: list[tuple[Entity, EntitySchema, str, AsyncSession | None]] = [] self.edit_calls: list[ - tuple[Entity, str, str, str, str | None, str | None, int, bool, AsyncSession | None] + tuple[ + Entity, + str, + str, + str, + str | None, + str | None, + int, + bool, + dict | None, + AsyncSession | None, + ] ] = [] self.move_calls: list[tuple[Entity, str, str, AsyncSession | None]] = [] self.self_relation_calls: list[tuple[str, Entity, AsyncSession | None]] = [] @@ -210,6 +221,7 @@ async def prepare_edit_entity_content( find_text: str | None = None, expected_replacements: int = 1, replace_subsections: bool = True, + metadata: dict | None = None, session: AsyncSession | None = None, ) -> PreparedEntityWrite: self.edit_calls.append( @@ -222,6 +234,7 @@ async def prepare_edit_entity_content( find_text, expected_replacements, replace_subsections, + metadata, session, ) ) @@ -1169,6 +1182,7 @@ async def test_run_accepted_note_edit_applies_patch_against_db_content( "# Old", 1, True, + None, cast(AsyncSession, session), ) ] @@ -1182,6 +1196,70 @@ async def test_run_accepted_note_edit_applies_patch_against_db_content( assert persistence_calls[1].await_count == 0 +@pytest.mark.asyncio +async def test_run_accepted_note_edit_threads_metadata_into_preparer() -> None: + """The `metadata` field on EditEntityRequest must reach the edit preparer. + + Regression guard for issue #1011: `metadata` merges frontmatter fields + independent of `operation`, so the accepted-note-edit runner must pass it + through unchanged instead of silently dropping it. + """ + session = _MutationSession() + project = _project() + prepared = _prepared_replacement() + entity = _entity(file_path="notes/accepted.md") + note_content = _note_content(entity) + project_repository = _ProjectRepository(project) + entity_lookup_repository = _EntityLookupRepository(by_external_id=entity) + note_content_lookup_repository = _NoteContentLookupRepository(note_content) + preparer = _CreatePreparer(prepared) + preparer_factory = _PreparerFactory(preparer) + pending_entity_repository = _PendingEntityRepository(entity) + note_content_accept_repository = _NoteContentAcceptRepository(note_content) + search_repository = _SearchRepository() + + await run_accepted_note_edit( + cast(AsyncSession, session), + request=AcceptedNoteEditMutation( + project_external_id="project-123", + entity_external_id="note-123", + data=EditEntityRequest( + operation="find_replace", + content="# Replacement", + find_text="# Old", + expected_replacements=1, + metadata={"status": "resolved"}, + ), + actor=AcceptedNoteMutationActor(user_profile_id=None), + source="mcp", + ), + dependencies=_dependencies( + project_repository=project_repository, + entity_lookup_repository=entity_lookup_repository, + note_content_lookup_repository=note_content_lookup_repository, + preparer_factory=preparer_factory, + pending_entity_repository=pending_entity_repository, + note_content_accept_repository=note_content_accept_repository, + search_repository=search_repository, + ), + ) + + assert preparer.edit_calls == [ + ( + entity, + "# Old\n", + "find_replace", + "# Replacement", + None, + "# Old", + 1, + True, + {"status": "resolved"}, + cast(AsyncSession, session), + ) + ] + + @pytest.mark.asyncio @pytest.mark.parametrize("file_checksum", ["file-checksum", None]) async def test_run_accepted_note_move_carries_previous_path_and_materialized_cleanup( diff --git a/tests/indexing/test_accepted_note_write_runner.py b/tests/indexing/test_accepted_note_write_runner.py index d824d105f..803f0ac6b 100644 --- a/tests/indexing/test_accepted_note_write_runner.py +++ b/tests/indexing/test_accepted_note_write_runner.py @@ -255,7 +255,18 @@ class _EditPreparer: def __init__(self, prepared: PreparedEntityWrite) -> None: self.prepared = prepared self.calls: list[ - tuple[Entity, str, str, str, str | None, str | None, int, bool, AsyncSession | None] + tuple[ + Entity, + str, + str, + str, + str | None, + str | None, + int, + bool, + dict | None, + AsyncSession | None, + ] ] = [] async def prepare_edit_entity_content( @@ -269,6 +280,7 @@ async def prepare_edit_entity_content( find_text: str | None = None, expected_replacements: int = 1, replace_subsections: bool = True, + metadata: dict | None = None, session: AsyncSession | None = None, ) -> PreparedEntityWrite: self.calls.append( @@ -281,6 +293,7 @@ async def prepare_edit_entity_content( find_text, expected_replacements, replace_subsections, + metadata, session, ) ) @@ -582,6 +595,7 @@ async def test_prepare_accepted_note_edit_applies_entity_fields() -> None: "# Accepted", 1, True, + None, cast(AsyncSession, session), ) ] @@ -594,6 +608,55 @@ async def test_prepare_accepted_note_edit_applies_entity_fields() -> None: assert session.flush_count == 1 +@pytest.mark.asyncio +async def test_prepare_accepted_note_edit_threads_metadata_to_preparer() -> None: + """`metadata` must reach the preparer so frontmatter merges apply independent of `operation`.""" + session = _FlushSession() + entity = _entity() + fields = _PreparedFields( + title="Edited", + note_type="note", + entity_metadata={"status": "resolved"}, + content_type="text/markdown", + permalink="edited", + file_path="notes/edited.md", + created_at=_PREPARED_CREATED_AT, + updated_at=_PREPARED_UPDATED_AT, + ) + prepared = _prepared(markdown_content="# Edited\n", fields=fields) + preparer = _EditPreparer(prepared) + + await prepare_accepted_note_edit( + preparer, + cast(AsyncSession, session), + entity=entity, + current_note_content=_note_content(), + operation="find_replace", + content="# Edited", + section=None, + find_text="# Accepted", + expected_replacements=1, + replace_subsections=True, + metadata={"status": "resolved"}, + user_profile_value=None, + ) + + assert preparer.calls == [ + ( + entity, + "# Accepted\n", + "find_replace", + "# Edited", + None, + "# Accepted", + 1, + True, + {"status": "resolved"}, + cast(AsyncSession, session), + ) + ] + + def test_apply_accepted_prepared_entity_fields_updates_mutable_entity() -> None: entity = _entity() diff --git a/tests/mcp/test_tool_contracts.py b/tests/mcp/test_tool_contracts.py index 4197ceeaf..dbb10d1d0 100644 --- a/tests/mcp/test_tool_contracts.py +++ b/tests/mcp/test_tool_contracts.py @@ -46,6 +46,7 @@ "find_text", "expected_replacements", "replace_subsections", + "metadata", "output_format", ], "fetch": ["id"], diff --git a/tests/mcp/test_tool_telemetry.py b/tests/mcp/test_tool_telemetry.py index 39aa73683..d50139cd9 100644 --- a/tests/mcp/test_tool_telemetry.py +++ b/tests/mcp/test_tool_telemetry.py @@ -220,6 +220,7 @@ async def test_edit_note_emits_root_operation_and_project_context( "has_find_text": False, "expected_replacements": 1, "replace_subsections": True, + "has_metadata": False, }, ) span_names = [name for name, _ in spans] diff --git a/tests/services/test_entity_service_prepare.py b/tests/services/test_entity_service_prepare.py index 900f7d9b5..bda0f83b3 100644 --- a/tests/services/test_entity_service_prepare.py +++ b/tests/services/test_entity_service_prepare.py @@ -12,6 +12,7 @@ from basic_memory.schemas import Entity as EntitySchema from basic_memory.services.exceptions import EntityAlreadyExistsError from basic_memory.services.entity_service import PreparedEntityFields +from basic_memory.services.note_preparation import _merge_metadata_into_markdown @pytest.mark.asyncio @@ -580,3 +581,250 @@ async def test_prepare_edit_entity_content_prepend_without_frontmatter_uses_simp ) assert prepared.markdown_content == "Prepended line\nOriginal body" + + +@pytest.mark.asyncio +async def test_prepare_edit_entity_content_metadata_adds_new_key( + entity_service, + file_service, +) -> None: + created = await entity_service.create_entity( + EntitySchema( + title="Metadata New Key", + directory="notes", + note_type="note", + content="---\nstatus: draft\n---\nBody", + ) + ) + + current_content = await file_service.read_file_content(created.file_path) + prepared = await entity_service.prepare_edit_entity_content( + created, + current_content, + operation="append", + content="", + metadata={"closed_at": "2026-06-18T10:42:00Z"}, + ) + + prepared_frontmatter = parse_frontmatter(prepared.markdown_content) + assert prepared_frontmatter["closed_at"] == "2026-06-18T10:42:00Z" + assert prepared_frontmatter["status"] == "draft" + + +@pytest.mark.asyncio +async def test_prepare_edit_entity_content_metadata_overwrites_existing_key( + entity_service, + file_service, +) -> None: + created = await entity_service.create_entity( + EntitySchema( + title="Metadata Overwrite", + directory="notes", + note_type="note", + content="---\nstatus: draft\n---\nBody", + ) + ) + + current_content = await file_service.read_file_content(created.file_path) + prepared = await entity_service.prepare_edit_entity_content( + created, + current_content, + operation="append", + content="", + metadata={"status": "resolved"}, + ) + + assert parse_frontmatter(prepared.markdown_content)["status"] == "resolved" + + +@pytest.mark.asyncio +async def test_prepare_edit_entity_content_metadata_preserves_unrelated_keys_and_body( + entity_service, + file_service, +) -> None: + created = await entity_service.create_entity( + EntitySchema( + title="Metadata Preserve", + directory="notes", + note_type="note", + content="---\nstatus: draft\nowner: alice\n---\nOriginal body", + ) + ) + + current_content = await file_service.read_file_content(created.file_path) + prepared = await entity_service.prepare_edit_entity_content( + created, + current_content, + operation="append", + content="\nMore body", + metadata={"status": "resolved"}, + ) + + prepared_frontmatter = parse_frontmatter(prepared.markdown_content) + assert prepared_frontmatter["owner"] == "alice" + assert prepared_frontmatter["status"] == "resolved" + assert "Original body" in remove_frontmatter(prepared.markdown_content) + assert "More body" in remove_frontmatter(prepared.markdown_content) + + +@pytest.mark.asyncio +async def test_prepare_edit_entity_content_metadata_ignores_identity_fields( + entity_service, + file_service, +) -> None: + created = await entity_service.create_entity( + EntitySchema( + title="Metadata Identity Guard", + directory="notes", + note_type="note", + content="Original body", + ) + ) + + current_content = await file_service.read_file_content(created.file_path) + prepared = await entity_service.prepare_edit_entity_content( + created, + current_content, + operation="append", + content="", + metadata={ + "title": "Hijacked Title", + "type": "hijacked", + "permalink": "hijacked/permalink", + "status": "resolved", + }, + ) + + prepared_frontmatter = parse_frontmatter(prepared.markdown_content) + assert prepared_frontmatter["status"] == "resolved" + assert prepared.entity_fields.title == "Metadata Identity Guard" + assert prepared.entity_fields.permalink == created.permalink + assert prepared_frontmatter["title"] == "Metadata Identity Guard" + assert prepared_frontmatter["permalink"] == created.permalink + + +@pytest.mark.asyncio +async def test_prepare_edit_entity_content_metadata_without_existing_frontmatter( + entity_service, +) -> None: + created = await entity_service.create_entity( + EntitySchema( + title="Metadata No Frontmatter", + directory="notes", + note_type="note", + content="Plain body", + ) + ) + + prepared = await entity_service.prepare_edit_entity_content( + created, + "Plain body", + operation="append", + content="", + metadata={"status": "resolved"}, + ) + + prepared_frontmatter = parse_frontmatter(prepared.markdown_content) + assert prepared_frontmatter["status"] == "resolved" + assert "Plain body" in remove_frontmatter(prepared.markdown_content) + + +@pytest.mark.asyncio +async def test_prepare_edit_entity_content_metadata_only_edit_preserves_body_exactly( + entity_service, +) -> None: + """A frontmatter-only request must not reflow the body (PR #1090 review). + + Trailing hard-break spaces, extra blank lines, and a missing final newline + are all meaningful markdown; the merge must round-trip them byte-exact. + """ + created = await entity_service.create_entity( + EntitySchema( + title="Metadata Body Fidelity", + directory="notes", + note_type="note", + content="Plain body", + ) + ) + body = "Line with hard break \n\n\n indented tail without trailing newline" + current_content = f"---\nstatus: draft\n---\n\n{body}" + + prepared = await entity_service.prepare_edit_entity_content( + created, + current_content, + operation="append", + content="", + metadata={"status": "resolved"}, + ) + + assert parse_frontmatter(prepared.markdown_content)["status"] == "resolved" + assert prepared.markdown_content.endswith(f"---\n\n{body}") + + +@pytest.mark.asyncio +async def test_prepare_edit_entity_content_metadata_only_edit_skips_append_newline( + entity_service, +) -> None: + """The documented metadata-only pattern must be a true body no-op (PR #1090 review). + + An empty append normally appends "\\n" to content without a trailing + newline; combined with metadata that mutated a body the caller asked to + leave untouched. + """ + created = await entity_service.create_entity( + EntitySchema( + title="Metadata NoOp Append", + directory="notes", + note_type="note", + content="Plain body", + ) + ) + + prepared = await entity_service.prepare_edit_entity_content( + created, + "Plain body without trailing newline", + operation="append", + content="", + metadata={"status": "resolved"}, + ) + + assert prepared.markdown_content.endswith("Plain body without trailing newline") + + +@pytest.mark.asyncio +async def test_prepare_edit_entity_content_metadata_rejects_null_values( + entity_service, +) -> None: + """Null metadata values are rejected instead of silently dropping the field (PR #1090 review).""" + created = await entity_service.create_entity( + EntitySchema( + title="Metadata Null Guard", + directory="notes", + note_type="note", + content="---\nstatus: draft\n---\nBody", + ) + ) + + with pytest.raises(ValueError, match="key deletion is not supported.*status"): + await entity_service.prepare_edit_entity_content( + created, + "---\nstatus: draft\n---\nBody", + operation="append", + content="", + metadata={"status": None}, + ) + + +def test_merge_metadata_into_markdown_identity_only_metadata_is_noop(): + """A merge holding only identity fields must leave the markdown byte-identical.""" + markdown = "---\nstatus: draft\n---\n\nBody \n" + merged = _merge_metadata_into_markdown(markdown, {"title": "X", "type": "y", "permalink": "z"}) + assert merged == markdown + + +def test_merge_metadata_into_markdown_preserves_crlf_body(): + """CRLF notes keep their body when the separator line is dropped for re-dumping.""" + markdown = "---\r\nstatus: draft\r\n---\r\n\r\nBody line\r\n" + merged = _merge_metadata_into_markdown(markdown, {"status": "resolved"}) + assert merged.endswith("Body line\r\n") + assert parse_frontmatter(merged)["status"] == "resolved" diff --git a/tests/utils/test_file_utils.py b/tests/utils/test_file_utils.py index 7af48fbdd..c18ad8e9b 100644 --- a/tests/utils/test_file_utils.py +++ b/tests/utils/test_file_utils.py @@ -606,3 +606,14 @@ def test_remove_frontmatter_with_bom(self): content_with_bom = "\ufeff---\ntitle: Test\n---\nContent here" result = remove_frontmatter(content_with_bom) assert result == "Content here" + + +def test_remove_frontmatter_strip_false_preserves_body_exactly(): + """strip=False returns the body verbatim so frontmatter-only rewrites don't reflow it.""" + content = "---\nstatus: draft\n---\n\nBody with hard break \n" + assert remove_frontmatter(content, strip=False) == "\nBody with hard break \n" + + +def test_remove_frontmatter_strip_false_without_frontmatter_returns_content_unchanged(): + text = " leading spaces and trailing newline\n" + assert remove_frontmatter(text, strip=False) == text From 72dd8aa285e8516b8306f6c962310f1dccc6fddf Mon Sep 17 00:00:00 2001 From: phernandez Date: Mon, 20 Jul 2026 22:32:13 -0500 Subject: [PATCH 2/2] fix(mcp): preserve separatorless bodies and guard null metadata on auto-create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review follow-ups on the metadata merge: - A note whose body starts immediately after the closing fence no longer gains a blank line from dump_frontmatter's canonical separator — the frontmatter is serialized alone and the body reattached verbatim. - edit_note rejects null metadata values before dispatch, so the append/prepend auto-create fallback behaves identically to the existing-note path instead of writing a YAML null that indexing silently drops. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01A2bgrGfWiL4izcjj66u9R8 Signed-off-by: phernandez --- src/basic_memory/mcp/tools/edit_note.py | 11 ++++++++ src/basic_memory/services/note_preparation.py | 10 +++++++ test-int/mcp/test_edit_note_integration.py | 27 +++++++++++++++++++ tests/services/test_entity_service_prepare.py | 9 +++++++ 4 files changed, 57 insertions(+) diff --git a/src/basic_memory/mcp/tools/edit_note.py b/src/basic_memory/mcp/tools/edit_note.py index 15fcd0b51..d4153c622 100644 --- a/src/basic_memory/mcp/tools/edit_note.py +++ b/src/basic_memory/mcp/tools/edit_note.py @@ -592,6 +592,17 @@ async def edit_note( section_ops = ("replace_section", "insert_before_section", "insert_after_section") if operation in section_ops and not section: raise ValueError("section parameter is required for section-based operations") + # Reject null metadata values before dispatch so both the edit path and the + # append/prepend auto-create fallback behave identically — the service-side + # guard only covers existing notes, and an auto-created note would otherwise + # be written with a YAML null that indexing silently filters out. + if metadata: + null_keys = sorted(k for k, v in metadata.items() if v is None) + if null_keys: + raise ValueError( + "metadata values cannot be null (key deletion is not supported): " + + ", ".join(null_keys) + ) # Use the PATCH endpoint to edit the entity try: diff --git a/src/basic_memory/services/note_preparation.py b/src/basic_memory/services/note_preparation.py index 970f23795..d5616b504 100644 --- a/src/basic_memory/services/note_preparation.py +++ b/src/basic_memory/services/note_preparation.py @@ -674,6 +674,7 @@ def _merge_metadata_into_markdown(markdown_content: str, metadata: dict[str, Any if not sanitized: return markdown_content + had_separator = True if has_frontmatter(markdown_content): current_metadata = parse_frontmatter(markdown_content) # strip=False: a frontmatter-only rewrite must not reflow the body (leading @@ -685,6 +686,8 @@ def _merge_metadata_into_markdown(markdown_content: str, metadata: dict[str, Any body = body[2:] elif body.startswith("\n"): body = body[1:] + else: + had_separator = False else: current_metadata = {} body = markdown_content @@ -694,6 +697,13 @@ def _merge_metadata_into_markdown(markdown_content: str, metadata: dict[str, Any post = frontmatter.Post(body) post.metadata.update(merged_metadata) + if not had_separator and body: + # Trigger: the original note had no blank line between the closing fence and + # the body ("---\nBody"), but dump_frontmatter always emits one. + # Outcome: serialize the frontmatter alone and reattach the body verbatim so + # a frontmatter-only merge cannot insert a blank line into the body. + post.content = "" + return dump_frontmatter(post) + body return dump_frontmatter(post) diff --git a/test-int/mcp/test_edit_note_integration.py b/test-int/mcp/test_edit_note_integration.py index 0f07cdb6d..0f5c0783c 100644 --- a/test-int/mcp/test_edit_note_integration.py +++ b/test-int/mcp/test_edit_note_integration.py @@ -920,3 +920,30 @@ async def test_edit_note_metadata_ignores_identity_fields(mcp_server, app, test_ assert frontmatter["permalink"].endswith("tickets/identity-guard-note") assert frontmatter["type"] == "note" assert "status: draft" not in content + + +@pytest.mark.asyncio +async def test_edit_note_metadata_null_values_rejected_before_auto_create( + mcp_server, app, test_project +): + """Null metadata values fail up front — including on the append auto-create path. + + Without the tool-level guard an auto-created note would be written with a + YAML null that indexing silently filters out of entity_metadata. + """ + async with Client(mcp_server) as client: + with pytest.raises(Exception) as exc_info: + await client.call_tool( + "edit_note", + { + "project": test_project.name, + "identifier": "conversations/never-created", + "operation": "append", + "content": "", + "metadata": {"status": None}, + }, + ) + + error_message = str(exc_info.value) + assert "key deletion is not supported" in error_message + assert "status" in error_message diff --git a/tests/services/test_entity_service_prepare.py b/tests/services/test_entity_service_prepare.py index bda0f83b3..16564671a 100644 --- a/tests/services/test_entity_service_prepare.py +++ b/tests/services/test_entity_service_prepare.py @@ -828,3 +828,12 @@ def test_merge_metadata_into_markdown_preserves_crlf_body(): merged = _merge_metadata_into_markdown(markdown, {"status": "resolved"}) assert merged.endswith("Body line\r\n") assert parse_frontmatter(merged)["status"] == "resolved" + + +def test_merge_metadata_into_markdown_preserves_separatorless_body(): + """A note whose body starts right after the closing fence must not gain a blank line.""" + markdown = "---\nstatus: draft\n---\nBody line\n" + merged = _merge_metadata_into_markdown(markdown, {"status": "resolved"}) + assert merged.endswith("---\nBody line\n") + assert "\n\nBody line" not in merged + assert parse_frontmatter(merged)["status"] == "resolved"