Repository navigation
fix(core): move a directory's notes as accepted note moves - #1670
Conversation
The directory-move endpoint moved every file through the legacy storage-first EntityService.move_entity. For a Markdown note that moved the stored object and the entity row but never note_content, which kept the old path and revision; a later materialization could write the note back at the old path, and the move published no per-note move event, vacate marker or embeddings. Each Markdown note now moves through NoteContentMutationService.move_note and the runtime's materialization hand-off, exactly like the single-move endpoint. Regular files have no accepted content, so their stored bytes still move directly. Per-file partial failure is reported as before. The service-level EntityService.move_directory is gone. Closes #1667 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8wjd6HrtSA2LN9ssC4NzF Signed-off-by: phernandez <paul@basicmachines.co>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4ebec0bf4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| schedule_followups(entity.id) | ||
| moved_files.append(destination_path) |
There was a problem hiding this comment.
When the requested destination differs only in case from an existing directory (for example, moving to archive when Archive/ exists), move_note canonicalizes the destination to the existing casing, but this code still appends the pre-normalized destination_path. The API and MCP tool therefore report archive/Plan.md even though the accepted note and, on case-sensitive storage, the file are at Archive/Plan.md, causing follow-up operations using the reported path to miss the note. Use the file path from the returned accepted/materialized payload for Markdown entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 04e3a34: Markdown entries report the file path from the accepted payload (a test moves into archive while Archive/ exists and expects Archive/Plan.md).
| for entity in entities: | ||
| destination_path = entity.file_path.replace(f"{source}/", f"{destination}/", 1) |
There was a problem hiding this comment.
On the default SQLite backend, find_by_directory_prefix uses case-insensitive LIKE, so requesting source drafts can return an entity at Drafts/Plan.md. This case-sensitive replacement then leaves the path unchanged, and the accepted-note runner rejects it as a same-path move; the previous implementation handled a nonmatching prefix by constructing a destination under the requested directory. Canonicalize the source casing or explicitly derive the suffix from the matched entity so case-variant source requests do not turn every note into a failed move.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 04e3a34: the destination keeps the entity's own path below the matched prefix, and the project root keeps the whole path. A SQLite test moves drafts when the folder is Drafts/sub and expects archive/sub/Plan.md.
…ectory moves A note's accepted move can adopt an existing folder's casing, so the result now reports the path from the accepted payload. SQLite matches the source folder in any casing, so the destination keeps the entity's own path below the matched prefix instead of a case-sensitive replace that left it unchanged; the project root keeps the whole path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8wjd6HrtSA2LN9ssC4NzF Signed-off-by: phernandez <paul@basicmachines.co>
No route moves a Markdown note through EntityService.move_entity anymore: single moves and directory moves both use the accepted note move. Its permalink and frontmatter branches were dead, and a direct call would have left note_content behind. It now refuses Markdown notes, takes only the identifier and destination, and moves a regular file's bytes and index row. The twelve service tests that drove Markdown notes through it are gone; the behavior that still applies (missing source, taken destination, unsafe path, new destination folder, rollback after a failed row update) is covered for regular files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8wjd6HrtSA2LN9ssC4NzF Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efba9efd08
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| moved_path = ( | ||
| await move_regular_file( | ||
| file_path=entity.file_path, | ||
| destination_path=destination_path, |
There was a problem hiding this comment.
When a directory contains both Markdown notes and regular files and the requested destination differs only by case from an existing directory (for example, archive while Archive/ exists), accepted note moves canonicalize to Archive/..., but this branch passes the raw archive/... path to the regular-file mover. On case-sensitive local storage this creates a second directory and splits a single directory move across Archive and archive; it can also bypass conflicts with regular files already under the canonical directory. Resolve the destination directory casing once for the entire batch and use it for both branches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 929f43b: the destination's casing is resolved once per directory move with the same rule accepted note moves use, and both branches build paths from it. A test moves a note and a PNG into archive beside an existing Archive/ and expects both under Archive/; it fails without the fix.
…ry file Accepted note moves adopt an existing folder's casing (#1326), but regular files took the requested casing, so moving into 'archive' beside an existing 'Archive/' split one directory move across two folders on case-sensitive storage. The batch now resolves the destination by the same rule and uses it for notes and regular files alike. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8wjd6HrtSA2LN9ssC4NzF Signed-off-by: phernandez <paul@basicmachines.co>
Summary
Closes #1667.
The
move-directoryendpoint moved every file through the legacy storage-firstEntityService.move_entity. For a Markdown note that moved the stored object and the entity row but nevernote_content, which kept the old path and revision. In Basic Memory Cloud a later materialization could then write the note back at its old path, and a directory move published no per-notenote.moved, vacate marker or embeddings refresh.services/directory_moves.py: each Markdown note moves throughNoteContentMutationService.move_noteplus the runtime'smaterialize_write_change, exactly like the single-move endpoint. Regular files (no accepted content) still move their stored bytes viamove_regular_file. Per-file partial failure is reported as before.EntityService.move_directoryis removed.Tests
test-int/test_directory_moves.py(HTTP, real DB): a note and a regular file move together;note_contentfollows the note to a new revision at the new path, bytes land at the new paths and nothing remains at the old ones. Fails on the old path with('drafts/Plan.md', 1) == ('archive/drafts/Plan.md', 2). A refused note (destination conflict) is reported while the rest move.move_regular_file; the directory-move invalidation test observes accepted state (reads serve accepted content) and asserts each note's invalidations land before the next note is accepted.Follow-up in Basic Memory Cloud: drop the request-time vector scheduler (basic-memory-cloud#2371) now that every scheduling route materializes.
🤖 Generated with Claude Code
https://claude.ai/code/session_01T8wjd6HrtSA2LN9ssC4NzF