Repository navigation
fix(mcp): create missing edit_note notes at the path the identifier names - #1782
Merged
Merged
Conversation
When append/prepend auto-creates a missing note from a project-qualified identifier (permalink or memory:// URL), drop the active project slug before deriving the on-disk path so files are not nested under an extra project-named directory. Fixes #1775 Signed-off-by: ningyixi <ningyixi@users.noreply.github.com>
A plain identifier that starts with the project's prefix is ambiguous when it names a missing note: 'demo/scratch/note' is the note 'scratch/note' spelled with its routing prefix, or a note in a folder named 'demo'. The first commit stripped a literal permalink prefix before create; this replaces that with one model of the readings. - note_path_readings() lists every project-relative path the identifier names: one for a memory URL (its leading segments routed), two for a plain path below this project's prefix. Prefixes match by permalink and include the workspace-qualified form, as routing does. - Disk recovery probes every reading, so an unindexed file below the prefix is edited instead of colliding with a create (Codex P1). - choose_note_create_path() lets existing folders decide; the project root is no evidence. Both folders holding notes refuses with AMBIGUOUS_NOTE_PATH and the spelling that picks each. With no evidence, permalinks_include_project decides, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KjsDUcHANBa17M84sKxFG3 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. |
![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: 87cd8cf203
ℹ️ 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".
The previous commit read identifier paths in edit_note and judged folders through /directory/list, which is built from entity rows. A folder whose notes were on disk but not yet indexed read as empty, so a note could land below the prefix and never be relocated. And the path rules now lived in three places: the link resolver, MCP routing, and edit_note. - services/note_placement.py owns the rules. note_path_readings() lists every project-relative path an identifier names; the link resolver and placement both use it, with the request's workspace from workspace_slug_for_canonical_permalinks(). - place_missing_note() judges folders through FileService.directory_has_files(), which reads storage, so unindexed files count. - QUERY /knowledge/placement returns the readings and either the create folder and title or the ambiguity. edit_note probes the returned readings for unindexed files and creates where the server says; it keeps no path logic. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KjsDUcHANBa17M84sKxFG3 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: f4eddfb4f7
ℹ️ 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".
Review follow-up for #1782. An identifier like 'demo/../../other/secrets' or 'demo//tmp/x' produced readings outside the project, and /placement walked those directories before edit_note's path check ran: an existence oracle across project boundaries, and a blocking scan on a large tree. - place_missing_note() checks every reading against the project before probing any folder and returns OutsideProjectPlacement for the first one that escapes. edit_note maps it to SECURITY_VALIDATION_ERROR before disk recovery; its own directory check is gone. - FileService.directory_has_files() refuses paths outside the project and walks off the event loop. Tests cover the prefixed-traversal and extra-slash cases at the placement function, FileService, the endpoint called directly, and edit_note end to end. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KjsDUcHANBa17M84sKxFG3 Signed-off-by: phernandez <paul@basicmachines.co>
Review follow-up for #1782. Production accepts a write before it materializes the file, so a folder holding only a just-accepted note is not on storage yet. Placement judged by storage alone sent the next note in that folder below the prefix, and materialization never moves it back. Folder evidence is now canonical note state: accepted notes, through EntityRepository.directory_has_entities() (an exact prefix compare, not LIKE), then files on storage. The index alone missed unindexed files; storage alone missed accepted ones. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KjsDUcHANBa17M84sKxFG3 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: fd46474ef9
ℹ️ 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".
phernandez
added a commit
that referenced
this pull request
Oct 18, 2026
Review follow-up for #1782. An identifier like 'demo/../../other/secrets' or 'demo//tmp/x' produced readings outside the project, and /placement walked those directories before edit_note's path check ran: an existence oracle across project boundaries, and a blocking scan on a large tree. - place_missing_note() checks every reading against the project before probing any folder and returns OutsideProjectPlacement for the first one that escapes. edit_note maps it to SECURITY_VALIDATION_ERROR before disk recovery; its own directory check is gone. - FileService.directory_has_files() refuses paths outside the project and walks off the event loop. Tests cover the prefixed-traversal and extra-slash cases at the placement function, FileService, the endpoint called directly, and edit_note end to end. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KjsDUcHANBa17M84sKxFG3 Signed-off-by: phernandez <paul@basicmachines.co>
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1775. Supersedes #1776 by @ningyixi, whose commit is cherry-picked here with authorship kept, so the full CI runs on an in-repo branch.
The problem
edit_noteappend/prepend creates a missing note. Resolving an existing note already understood an identifier spelled with the project's prefix (demo/scratch/note,memory://demo/scratch/note), but the create path used the raw identifier, so it made ademo/folder inside projectdemo.For a plain path the create is genuinely ambiguous:
demo/scratch/notecan mean the notescratch/notespelled with its routing prefix, or a note inside a real folder nameddemo.What changes
The path rules live in one place, server side, and folders are judged from storage.
services/note_placement.pyowns the rules.note_path_readings()lists every project-relative path an identifier names. A memory URL has one reading, since its leading segments routed to the project. A plain path below this project's prefix has two. Prefixes match by permalink and include the workspace-qualified form.place_missing_note()first checks that every reading stays inside the project, and refuses with anoutside_projectoutcome before probing any folder;edit_notereports it asSECURITY_VALIDATION_ERROR. It then lets folders decide, judged from canonical note state: accepted notes (EntityRepository.directory_has_entities(), an exact prefix match, since the entity row exists at accept time) and files on storage (FileService.directory_has_files()). So a folder counts whether its notes are accepted but not yet materialized, or on disk but not yet indexed. The project root is no evidence, since it always exists.AMBIGUOUS_NOTE_PATH, naming the spelling that picks each.permalinks_include_projectdecides.QUERY /knowledge/placementreturns the readings and either the create folder and title or the ambiguity.edit_notekeeps no path logic. It probes the returned readings for unindexed files (fixing Codex's P1 on fix(mcp): strip active project prefix on edit_note autocreate #1776), then creates where the server says.Commits: @ningyixi's original fix; a first follow-up that read paths client side; and the server-side placement, which replaces it after the Codex review here showed the client's folder check read the index rather than storage.
Cloud follow-up
basicmachines-co/basic-memory-cloud#2565 adds
S3FileService.directory_has_files()(a one-object prefix listing). Until Cloud has it, storage evidence on Cloud comes from the inherited local check, which finds no folders; accepted notes still count, and otherwise placement falls back to the permalink convention.Tests
tests/services/test_note_placement.py.directory_has_files()against storage: a nested file counts, an empty folder does not, and paths outside the project are refused.directory_has_entities(): exact prefix match, so no sibling folder and noLIKEwildcard counts; also run on Postgres.<project>/../../other/...) and extra-slash (<project>//tmp/...) identifiers are refused without probing, at the placement function,FileService, the/placementendpoint called directly, andedit_noteend to end.edit_notetests: an unindexed file below the prefix is appended; a folder holding only an unindexed file wins; both folders existing refuses without creating anything. @ningyixi's two end-to-end tests pass unchanged.just fast-check, andjust fast-test(311 passed).🤖 Generated with Claude Code
https://claude.ai/code/session_01KjsDUcHANBa17M84sKxFG3