fix(core): let edit_note metadata set a note's type - #1417
Conversation
_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 <sammy@wachtel.us>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e301d130ad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # 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"}) |
There was a problem hiding this comment.
When append or prepend targets a missing note, edit_note bypasses this merge and constructs an Entity with entity_metadata=metadata but the default note_type="note" (edit_note.py:718-724). schema_to_markdown() then explicitly removes type from entity metadata and writes schema.note_type (markdown/utils.py:108-117), so metadata={"type": "decision"} still silently creates a note even though the same request changes the type when the target already exists. Pass the metadata type as the auto-created entity's note_type, or apply the standard edit after creation.
Useful? React with 👍 / 👎.
| Identity fields (title/permalink) are dropped from the merge; every other key, | ||
| ``type`` included, overwrites the existing frontmatter value or is added new. The |
There was a problem hiding this comment.
When an existing note receives metadata={"type": ""} or a whitespace-only value, this newly allowed key is written without passing through the NoteType boundary validation, so the accepted Markdown and Entity.note_type contain an invalid empty classification. This is not merely cosmetic: WikiSourceNote.__post_init__ rejects blank note types (wiki_projector.py:97-98), allowing one edit request to break projection snapshot construction. Validate and normalize this special metadata key before merging it, just as the regular note_type request field is validated.
AGENTS.md reference: AGENTS.md:L128-L129
Useful? React with 👍 / 👎.
1 participant
What this changes
_METADATA_IDENTITY_FIELDSinsrc/basic_memory/services/note_preparation.pydropstitle,typeandpermalinkfrom anedit_notemetadata merge. This removestypefrom that set and leaves the other two alone.
Why
The comment on the constant explains the exclusion:
That is exactly right for two of the three, and I want to be clear that I am not
proposing to weaken it:
titleis read out of frontmatter and then handed toreconcile_prepared_edit_title_from_h1(...), which can replace it with the note's H1.A merged title really can be overwritten.
permalinkis read and then passed throughresolve_permalink(...), whichsuffixes on collision. A merged permalink really can come back as something else.
typedoes not share that property. Its entire handling inprepare_edit_entity_contentis:A plain read, with nothing downstream that can disagree. The
note_typethat reachesthe entity row comes from
_build_entity_fields, which re-parses the merged markdown(
entity_markdown.frontmatter.type) — so writingtypeinto the frontmatter is howyou feed that path, not a race against it. The one other place the local
note_typetravels is
_build_frontmatter_markdown(...)on the permalink branch, whereresolve_permalinkonly ever looks atfrontmatter.permalinkand ignores the typeentirely.
So the guard is correct for title and permalink and over-broad by one member.
The behavior before this change
edit_note(..., metadata={"type": "decision"})returned a successful edit and left thenote's type as it was. Nothing in the response distinguished that from a type change
that worked, so the only way to notice is to read the note back afterwards and compare.
Setting the type through
metadatais the natural thing to reach for, since every otherfrontmatter field works that way.
Tests
test_merge_metadata_into_markdown_writes_type— the merge writestypewhile stilldropping
titleandpermalinkfrom the same payload.test_prepare_edit_entity_content_metadata_sets_note_type— the prepared write carriesthe new type in both the markdown frontmatter and
entity_fields.note_type, with titleand permalink unmoved.
test_edit_note_metadata_sets_note_type(integration) — end to end through the MCPtool: the type lands in the file, and
search_notes(note_types=["decision"])finds thenote, so the index agrees with the file.
..._metadata_ignores_identity_fieldstests, unit and integration, keeptheir
titleandpermalinkhijack attempts and keep asserting those are refused. Idropped only the
typekey from their payloads and added an assertion that anunsupplied type is still left alone.
All three new tests fail against the current
mainbehavior and pass with the change.Docs updated in the same commit: the
edit_notetool docstring, theEditEntityRequestcomment in
schemas/request.py, and themetadatabullet inman3/edit-note(3).md.How I tested
just test-unit-sqlite— 5735 passed, 43 skipped.just test-int-sqlite— 501 passed, 16 skipped.tests/services/test_entity_service_prepare.pyandtest-int/mcp/test_edit_note_integration.pywithBASIC_MEMORY_TEST_POSTGRES=1, allpassing. This change touches only frontmatter merging, so I ran the affected suites
against Postgres rather than the whole matrix.
just fix,just format,just typecheck— clean, apart from four pre-existingunresolved-importdiagnostics forpymilvus, which lives in the optionalmilvusextra that a plain
uv syncdoes not install. They are present on an unmodifiedcheckout too.
Three failures in
tests/cli/test_cloud_promo.pyon my machine are environmental: thosetests construct a real
ConfigManager()with no isolation, so they read~/.basic-memory/config.json, and mine already records the promo as shown. They failidentically on a clean checkout of
mainhere. Nothing to do with this change, and Ileft them alone.