fix(mcp): resolve memory:// in move_note and stop false cross-project rejections#914
Conversation
… rejections Two confirmed bugs from the integration bug hunt in move_note: - #11: move_note did not resolve memory:// URL identifiers even though its docstring advertises them and read_note/edit_note/delete_note all accept them. It passed the raw 'memory://...' string to resolve_entity, which 404s. Now it calls resolve_project_and_path() first (same as the sibling tools) to strip the scheme prefix and normalize the path before resolving the entity. - #12 (regression from #904): the structural Detection 2 heuristic ('<seg>/projects/<seg>/file.md') wrongly rejected legitimate same-project nested moves like 'notes/projects/2025/file.md' as cross-project moves. The heuristic is fundamentally ambiguous, so it has been removed entirely. Cross-project detection now relies only on Detection 1 (leading segment == a known project name) plus the existing MOVE_OUTCOME_MISMATCH backstop. A bare 'other-workspace/projects/x' that matches no known project name is now allowed as a normal nested folder. Tests came from the integration bug hunt: added regression tests under test-int/bughunt_fixes/test_move_note_edge_cases.py and updated the now-incorrect assertions in test-int/mcp/test_move_note_integration.py and the unit test in tests/mcp/test_tool_move_note.py to match the corrected behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
![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: 4017d9aed3
ℹ️ 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".
| _, resolved_identifier, _ = await resolve_project_and_path( | ||
| client, identifier, active_project.name, context | ||
| ) |
There was a problem hiding this comment.
When identifier is a project-prefixed URL like memory://other-project/docs/note and the caller omits project, this new resolution can return other-project, but the returned ProjectItem is discarded after get_project_client() has already selected the default/current project and after KnowledgeClient was bound to active_project.external_id. In that scenario resolve_entity() runs against the wrong project, so the new memory:// support still fails or targets the wrong route for the same URLs that read/edit/delete pre-detect before client selection.
Useful? React with 👍 / 👎.
|
Claude finished @phernandez's task in 1m 46s —— View job PR Review: fix(mcp): resolve memory:// in move_note and stop false cross-project rejections
SummaryThis is a clean, targeted fix for two confirmed Bug Fix AnalysisBug #11 —
|
| Item | Status |
|---|---|
| Follows Basic Memory coding conventions (CLAUDE.md) | ✅ |
| Python 3.12+ type annotations and async patterns | ✅ |
| SQLAlchemy / FastAPI / Typer conventions | ✅ (no ORM/router changes) |
| 100-char line length | ✅ |
| No commented-out code blocks | ✅ (NOTE block is a decision-point comment, not dead code) |
| Unit tests for new functions | ✅ tests/mcp/test_tool_move_note.py updated |
| Integration tests for new behaviour | ✅ test-int/bughunt_fixes/test_move_note_edge_cases.py (5 new tests) |
| Edge-case coverage | ✅ control tests for delete_note/edit_note memory:// acceptance included |
| 100% coverage maintained | ✅ (as confirmed by PR author's test runs) |
| No schema changes → no Alembic migration needed | ✅ |
| Local-first architecture preserved | ✅ |
| No hardcoded secrets or credentials | ✅ |
| Input validation / path traversal protection unchanged | ✅ |
Windows path compatibility (Path / PureWindowsPath) |
✅ (imports at top of file) |
| No emojis/unicode in log or console output | ✅ |
| No tool signature changes | ✅ |
| Semantic PR title | ✅ fix(mcp): ... |
| Commits signed | ✅ (-s) |
Minor Observations (non-blocking)
-
test_move_note_accepts_memory_urluses a same-project URL only (memory://src/memory-url-move). There is no test for a cross-project memory:// URL likememory://other-project/...— but since cross-project moves are explicitly unsupported and the sibling tools have the same gap, this is acceptable. A follow-up tracking issue might be worthwhile for completeness. -
test_detect_cross_project_only_flags_known_project_name(tests/mcp/test_tool_move_note.py:54) now registersother-projectin the mock project list and explicitly asserts it is rejected. This is a net improvement over the previous test which only checked the structural heuristic. -
The NOTE comment block (
move_note.py:59-68) is slightly long for a comment explaining removed code, but it captures exactly why the heuristic was removed and what the fallback is. This is defensible under the CLAUDE.md literate-programming guidelines.
Verdict
Approve. Both bugs are properly fixed, tests are comprehensive, and the implementation is consistent with sibling-tool patterns. Low regression risk. Ready to merge.
Codex review: the memory:// resolution captured the resolved path but discarded the resolved project, so a 'memory://other-project/...' source would be looked up against the active project's client (misroute). move_note is single-project; reject when the source URL resolves to a different project than the active one, consistent with the cross-project destination guard. Same-project memory:// URLs are unaffected (verified by the existing tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> 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: f719ad7e39
ℹ️ 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 other project, looked up in the active one); move_note cannot move across | ||
| # projects. | ||
| # Outcome: reject up front with the cross-project guidance instead of misrouting. | ||
| if source_project.external_id != active_project.external_id: |
There was a problem hiding this comment.
When project/project_id is omitted and the source is a project-prefixed URL like memory://other-project/src/note, get_project_client() has already selected the default/current project before this check runs, so this branch rejects a normal same-project move within other-project unless the caller redundantly supplies project="other-project". The fresh evidence is that the new code now explicitly returns CROSS_PROJECT_MOVE_NOT_SUPPORTED for the resolved source project instead of using that project to select the client first, unlike read_note/edit_note/delete_note which pre-detect the memory URL prefix before routing.
Useful? React with 👍 / 👎.
|
Claude finished @phernandez's task in 2m 38s —— View job PR Review: fix(mcp): resolve memory:// in move_note and stop false cross-project rejections
SummaryBoth bugs are correctly fixed. The memory:// resolution matches the sibling-tool pattern exactly, and the structural heuristic removal is well-justified and documented. One coverage gap is worth flagging before merge. Bug Fix AnalysisBug #11 —
|
| Item | Status |
|---|---|
| Follows Basic Memory coding conventions (CLAUDE.md) | ✅ |
| Python 3.12+ type annotations and async patterns | ✅ |
| SQLAlchemy 2.0 best practices | ✅ (no ORM changes) |
| FastAPI and Typer conventions followed | ✅ |
| 100-character line length limit maintained | ✅ |
| No commented-out code blocks | ✅ (NOTE block is a decision-point comment, not dead code) |
| Unit tests for new functions/methods | ✅ |
| Integration tests for new MCP tools | ✅ test-int/bughunt_fixes/test_move_note_edge_cases.py |
| Test coverage for edge cases | ✅ (happy paths; see coverage note above) |
| 100% test coverage maintained | --no-cov; new branch at line 675 appears untested |
| Documentation updated (docstring, decision comments) | ✅ |
| CLAUDE.md updated if conventions change | N/A |
| MCP tools follow atomic, composable design | ✅ |
| Database changes include Alembic migrations | N/A |
| Preserves local-first architecture principles | ✅ |
| Knowledge graph operations maintain consistency | ✅ |
| Markdown file handling preserves integrity | ✅ |
| No hardcoded secrets or credentials | ✅ |
| Input validation for MCP tools | ✅ |
| Proper error handling and logging | ✅ |
| Performance considerations addressed | ✅ |
| No sensitive data in logs or commits | ✅ |
Windows path compatibility (PureWindowsPath) |
✅ |
| No emojis/unicode in log or console output | ✅ (emojis in text response strings are pre-existing) |
| No tool signature changes | ✅ |
| Semantic PR title | ✅ fix(mcp): ... |
| Commits signed | ✅ (-s) |
Minor Observations (non-blocking)
-
test_move_note_accepts_memory_urlcovers only same-project URLs. There is no test formemory://other-project/...being correctly rejected. The sibling-tool controls (test_delete_note_accepts_memory_url_control,test_edit_note_accepts_memory_url_control) are a good addition for the contract, but don't exercise the new rejection branch. -
_detect_cross_project_move_attemptstill runs after theresolve_project_and_pathcheck. For a memory:// URL that resolves to the active project,resolved_identifieris a bare path. The subsequent_detect_cross_project_move_attemptcall (line 803) checks destination paths — that ordering is correct and unchanged. -
Updated assertions in
test_move_note_integration.pyare accurate. The two renamed tests (test_move_note_unknown_workspace_shaped_path_allowed*) now assertmoved=Trueforother-workspace/projects/x/...destinations, which is the correct post-fix behavior. The assertions are clean and correctly invert the old expectations.
Verdict
Approve with one pre-merge action: verify the new cross-project memory:// rejection branch at move_note.py:675–692 is either covered by an integration test (preferred) or annotated with # pragma: no cover. Everything else — logic, structure, documentation, and test direction — is correct and ready to merge.
1 participant
Summary
Fixes two confirmed
move_notebugs found by the integration bug hunt, both insrc/basic_memory/mcp/tools/move_note.py.Bugs fixed
move_notedid not resolvememory://URL identifiers. Its docstring advertisesmemory://URLs andread_note/edit_note/delete_noteall accept them, butmove_notepassed the rawmemory://...string straight toresolve_entity, which 404s. It now callsresolve_project_and_path()first (the same pattern the sibling tools use) to strip the scheme prefix and normalize the path before resolving the entity.projectsfolders. Detection 2 (if len(path_parts) >= 3 and path_parts[1] == "projects") wrongly rejected legitimate same-project nested moves likenotes/projects/2025/file.md,work/projects/q1/report.md, etc. The structural heuristic is fundamentally ambiguous (indistinguishable from a normal nested folder), so it has been removed entirely. Cross-project detection now relies only on Detection 1 (leading segment matches a known project name) plus the existingMOVE_OUTCOME_MISMATCHpost-move backstop. A bareother-workspace/projects/xthat matches no known project name is now correctly allowed as a normal nested folder.Testing
Commands and results (all green):
uv run pytest test-int/bughunt_fixes/test_move_note_edge_cases.py test-int/mcp/test_move_note_integration.py -q --no-cov→ 25 passeduv run pytest tests/mcp/test_tool_move_note.py tests/mcp/test_tool_contracts.py tests/mcp/test_tool_telemetry.py -q --no-cov→ 55 passeduv run pytest tests/mcp/test_tool_move_note.py test-int/mcp/test_move_note_integration.py test-int/bughunt_fixes/test_move_note_edge_cases.py -q --no-cov→ 74 passedBASIC_MEMORY_TEST_POSTGRES=1 uv run pytest test-int/bughunt_fixes/test_move_note_edge_cases.py test-int/mcp/test_move_note_integration.py -q --no-cov→ 25 passeduv run ruff check . --fix/uv run ruff format .(reverted incidental edits to unrelated files) — clean on touched filesuv run ty check src tests test-int→ All checks passedNew regression tests live in
test-int/bughunt_fixes/test_move_note_edge_cases.py(from the integration bug hunt). The now-incorrect assertions intest-int/mcp/test_move_note_integration.py(the two workspace-shaped-path rejection tests) and the unit testtest_detect_cross_project_only_flags_workspace_shapeintests/mcp/test_tool_move_note.pywere updated to match the corrected behavior — a bareother-workspace/projects/xis now an allowed nested move.Risk
Low. No tool signature changes. The behavior change is narrow: paths whose leading segment is not a known project name but happen to contain a
projectssegment are now allowed as nested moves (previously falsely rejected). True cross-project intent is still caught by the known-project-name check and the post-move outcome backstop.🤖 Generated with Claude Code