From e301d130ad6e8f66ec3c5419fa26903dff02a2eb Mon Sep 17 00:00:00 2001 From: samwachtel Date: Mon, 31 Aug 2026 15:39:03 -0400 Subject: [PATCH] fix(core): let edit_note metadata set a note's type _METADATA_IDENTITY_FIELDS drops title, type and permalink from a metadata merge, on the grounds that all three have dedicated resolution paths in prepare_edit_entity_content that a merge would race. That holds for two of the three. A merged title can be overwritten by reconcile_prepared_edit_title_from_h1, and a merged permalink can come back suffixed from resolve_permalink, so in both cases the caller's value can vanish without a word. type has no such second opinion. Its whole "resolution path" is one line reading the frontmatter back out, and the note_type that reaches the database comes from re-parsing the merged markdown - so writing type into frontmatter feeds that path instead of fighting it. Net effect before this change: edit_note(..., metadata={"type": "decision"}) returned success and changed nothing, with no way for the caller to tell. Drop type from the exclusion set and leave title and permalink where they are. Tests cover both halves: type now lands in the file's frontmatter and in the indexed entity, and title and permalink are still refused. Signed-off-by: samwachtel --- src/basic_memory/man/man3/edit-note(3).md | 3 +- src/basic_memory/mcp/tools/edit_note.py | 6 +-- src/basic_memory/schemas/request.py | 2 +- src/basic_memory/services/note_preparation.py | 16 +++--- test-int/mcp/test_edit_note_integration.py | 49 +++++++++++++++++- tests/services/test_entity_service_prepare.py | 51 ++++++++++++++++++- 6 files changed, 111 insertions(+), 16 deletions(-) diff --git a/src/basic_memory/man/man3/edit-note(3).md b/src/basic_memory/man/man3/edit-note(3).md index 644edbe00..d179667f7 100644 --- a/src/basic_memory/man/man3/edit-note(3).md +++ b/src/basic_memory/man/man3/edit-note(3).md @@ -68,7 +68,8 @@ permalink, or memory:// URL — there is no fuzzy fallback for edits. heading of any level and preserves subsections - **metadata** — dict of frontmatter fields merged in alongside any operation; given keys overwrite or add, other keys and the body are untouched. - `title`, `type`, and `permalink` are ignored; keys cannot be deleted + `title` and `permalink` are ignored; `type` is applied like any other + frontmatter field; keys cannot be deleted - **project** / **project_id** / **workspace** — routing; same semantics as [[write-note(3)]] diff --git a/src/basic_memory/mcp/tools/edit_note.py b/src/basic_memory/mcp/tools/edit_note.py index 736b6f5db..3e7faa7be 100644 --- a/src/basic_memory/mcp/tools/edit_note.py +++ b/src/basic_memory/mcp/tools/edit_note.py @@ -437,9 +437,9 @@ async def edit_note( 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. + combined with any operation in the same call. `title` and `permalink` are + ignored since those have their own dedicated handling; `type` is applied like + any other frontmatter field. 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. diff --git a/src/basic_memory/schemas/request.py b/src/basic_memory/schemas/request.py index 1f19c4120..37239826f 100644 --- a/src/basic_memory/schemas/request.py +++ b/src/basic_memory/schemas/request.py @@ -83,7 +83,7 @@ class EditEntityRequest(BaseModel): 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. + # untouched. title/permalink are ignored — they have their own resolution paths. metadata: Optional[dict[str, Any]] = None @field_validator("section") diff --git a/src/basic_memory/services/note_preparation.py b/src/basic_memory/services/note_preparation.py index 23ed69e2a..65f244ffc 100644 --- a/src/basic_memory/services/note_preparation.py +++ b/src/basic_memory/services/note_preparation.py @@ -689,18 +689,20 @@ 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"}) +# title and permalink get reworked after the merge in prepare_edit_entity_content — +# title by H1 reconciliation, permalink by the collision-suffixing resolver. Either can +# hand back a value the caller did not ask for, so a metadata merge that set them would +# be silently reverted. `type` has no such second opinion: prepare_edit_entity_content +# just reads it back out of the frontmatter, so writing it there is how you set it. +_METADATA_IDENTITY_FIELDS = frozenset({"title", "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. + Identity fields (title/permalink) are dropped from the merge; every other key, + ``type`` included, 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: diff --git a/test-int/mcp/test_edit_note_integration.py b/test-int/mcp/test_edit_note_integration.py index 0f5c0783c..b00a8ab61 100644 --- a/test-int/mcp/test_edit_note_integration.py +++ b/test-int/mcp/test_edit_note_integration.py @@ -876,7 +876,7 @@ async def test_edit_note_metadata_merges_frontmatter(mcp_server, app, test_proje @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.""" + """title/permalink in `metadata` are ignored rather than hijacking the note's identity.""" async with Client(mcp_server) as client: await client.call_tool( @@ -898,7 +898,6 @@ async def test_edit_note_metadata_ignores_identity_fields(mcp_server, app, test_ "content": "", "metadata": { "title": "Hijacked Title", - "type": "hijacked", "permalink": "hijacked/permalink", "status": "resolved", }, @@ -922,6 +921,52 @@ async def test_edit_note_metadata_ignores_identity_fields(mcp_server, app, test_ assert "status: draft" not in content +@pytest.mark.asyncio +async def test_edit_note_metadata_sets_note_type(mcp_server, app, test_project): + """`type` in `metadata` reaches both the file's frontmatter and the indexed entity.""" + + async with Client(mcp_server) as client: + await client.call_tool( + "write_note", + { + "project": test_project.name, + "title": "Type Change Note", + "directory": "tickets", + "content": "# Type Change Note\n\nBody.", + }, + ) + + await client.call_tool( + "edit_note", + { + "project": test_project.name, + "identifier": "tickets/type-change-note", + "operation": "append", + "content": "", + "metadata": {"type": "decision"}, + }, + ) + + read_result = await client.call_tool( + "read_note", + {"project": test_project.name, "identifier": "tickets/type-change-note"}, + ) + content = read_result.content[0].text + assert parse_frontmatter(content)["type"] == "decision" + assert "Body." in content + + # The index has to agree with the file, otherwise the note reverts on the next sync. + search_result = await client.call_tool( + "search_notes", + { + "project": test_project.name, + "query": "Type Change Note", + "note_types": ["decision"], + }, + ) + assert "tickets/type-change-note" in search_result.content[0].text + + @pytest.mark.asyncio async def test_edit_note_metadata_null_values_rejected_before_auto_create( mcp_server, app, test_project diff --git a/tests/services/test_entity_service_prepare.py b/tests/services/test_entity_service_prepare.py index 58b19613a..fe058e98b 100644 --- a/tests/services/test_entity_service_prepare.py +++ b/tests/services/test_entity_service_prepare.py @@ -680,6 +680,7 @@ async def test_prepare_edit_entity_content_metadata_ignores_identity_fields( entity_service, file_service, ) -> None: + """title and permalink stay under their own resolvers, whatever `metadata` says.""" created = await entity_service.create_entity( EntitySchema( title="Metadata Identity Guard", @@ -697,7 +698,6 @@ async def test_prepare_edit_entity_content_metadata_ignores_identity_fields( content="", metadata={ "title": "Hijacked Title", - "type": "hijacked", "permalink": "hijacked/permalink", "status": "resolved", }, @@ -709,6 +709,41 @@ async def test_prepare_edit_entity_content_metadata_ignores_identity_fields( assert prepared.entity_fields.permalink == created.permalink assert prepared_frontmatter["title"] == "Metadata Identity Guard" assert prepared_frontmatter["permalink"] == created.permalink + # An untouched note type is not collateral damage of the guard above. + assert prepared.entity_fields.note_type == "note" + + +@pytest.mark.asyncio +async def test_prepare_edit_entity_content_metadata_sets_note_type( + entity_service, + file_service, +) -> None: + """`type` is a plain frontmatter field: the merge writes it and the read-back keeps it.""" + created = await entity_service.create_entity( + EntitySchema( + title="Metadata Type Change", + 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={"type": "decision"}, + ) + + prepared_frontmatter = parse_frontmatter(prepared.markdown_content) + assert prepared_frontmatter["type"] == "decision" + assert prepared.entity_fields.note_type == "decision" + # Setting the type is not a license to move the note. + assert prepared.entity_fields.title == "Metadata Type Change" + assert prepared.entity_fields.permalink == created.permalink + assert "Original body" in remove_frontmatter(prepared.markdown_content) @pytest.mark.asyncio @@ -826,10 +861,22 @@ async def test_prepare_edit_entity_content_metadata_rejects_null_values( 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"}) + merged = _merge_metadata_into_markdown(markdown, {"title": "X", "permalink": "z"}) assert merged == markdown +def test_merge_metadata_into_markdown_writes_type(): + """`type` is merged like any other field, while title and permalink are still dropped.""" + markdown = "---\ntitle: Keep Me\ntype: note\npermalink: notes/keep-me\n---\n\nBody\n" + merged = _merge_metadata_into_markdown( + markdown, {"title": "X", "type": "decision", "permalink": "z"} + ) + merged_frontmatter = parse_frontmatter(merged) + assert merged_frontmatter["type"] == "decision" + assert merged_frontmatter["title"] == "Keep Me" + assert merged_frontmatter["permalink"] == "notes/keep-me" + + 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"