Skip to content

fix(mcp): add project detection from memory:// URLs in edit_note and delete_note#668

Open
phernandez wants to merge 1 commit intomainfrom
fix/664-detect-project-url-prefix
Open

fix(mcp): add project detection from memory:// URLs in edit_note and delete_note#668
phernandez wants to merge 1 commit intomainfrom
fix/664-detect-project-url-prefix

Conversation

@phernandez
Copy link
Member

Summary

  • edit_note and delete_note were missing detect_project_from_url_prefix() that other tools (read_note, build_context, search_notes, read_content) already use
  • This caused incorrect cloud routing when using memory:// URLs without an explicit project parameter in multi-project setups
  • Added the same 4-line detection pattern before get_project_client() in both tools

Fixes #664

Test plan

  • just fast-check passes (lint, typecheck, impacted tests)
  • pytest tests/mcp/test_tool_edit_note.py tests/mcp/test_tool_delete_note.py -v passes
  • Verify multi-project routing works with memory://project-name/note URLs in edit_note and delete_note

🤖 Generated with Claude Code

…delete_note

edit_note and delete_note were missing detect_project_from_url_prefix()
which other tools (read_note, build_context, search_notes, read_content)
already use. This caused incorrect cloud routing when using memory://
URLs without an explicit project parameter in multi-project setups.

Fixes #664

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 14, 2026

Claude finished @phernandez's task in 1m 42s —— View job


PR Review

  • Read changed files
  • Review against team checklist
  • Post final review

Summary

This is a clean, minimal fix that closes a real routing gap. edit_note and delete_note were the only write-path tools accepting memory:// URLs that lacked project auto-detection — now they're consistent with all read-path tools.


Code Quality & Standards ✅

The implementation is correct and follows the established pattern exactly:

# delete_note.py:227-230 / edit_note.py:264-267
if project is None:
    detected = detect_project_from_url_prefix(identifier, ConfigManager().config)
    if detected:
        project = detected

Placement (before get_project_client()) is correct. Imports are correct. The 4-line block is byte-for-byte identical to read_note, build_context, search_notes, and read_content. No dead code, no commented blocks, line length fine.


Testing & Documentation ⚠️ One Gap

Missing tests for the new code paths. CLAUDE.md is explicit: "Coverage must stay at 100%". The new if project is None / detect / if detected block in both tools adds uncovered lines unless existing tests happen to call these tools with a memory://project-name/... URL and project=None. A quick grep confirms no such tests exist yet in test_tool_edit_note.py or test_tool_delete_note.py.

For consistency with test_project_context.py and the existing coverage standard, two small tests are needed — one per tool:

# tests/mcp/test_tool_edit_note.py
async def test_edit_note_detects_project_from_memory_url(...):
    result = await edit_note(
        identifier="memory://test-project/some-note",
        operation="append",
        content="extra",
        project=None,   # must be None to trigger detection
    )
    # assert it routed to test-project, not the default

# tests/mcp/test_tool_delete_note.py
async def test_delete_note_detects_project_from_memory_url(...):
    result = await delete_note(
        identifier="memory://test-project/some-note",
        project=None,
    )
    # assert routing was correct (False is fine if note doesn't exist)

Fix this →


Basic Memory Architecture ⚠️ move_note Also Affected

move_note.py accepts memory:// URLs in its identifier parameter (per its own docstring) but also lacks detect_project_from_url_prefix. This PR doesn't address it, which is fine for scope — but worth a follow-up issue so the routing gap is fully closed across all write tools.


Checklist

Item Status
Follows Basic Memory coding conventions
Python 3.12+ type annotations and async patterns
FastAPI/Typer conventions followed
100-character line length maintained
No commented-out code
Unit tests for new code paths ⚠️ Missing
100% test coverage maintained ⚠️ At risk
No hardcoded secrets or credentials
Input validation for MCP tools
Proper error handling and logging
No emoji/unicode in console output

Verdict: Logically correct and architecturally sound. The only ask before merge is adding tests for the two new detection branches to maintain the 100% coverage requirement. The move_note gap is a good candidate for a follow-up issue.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7c12ff39e1

ℹ️ 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".

Comment on lines +227 to +228
if project is None:
detected = detect_project_from_url_prefix(identifier, ConfigManager().config)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Limit auto project routing to memory:// identifiers

This new routing block runs for every identifier, but detect_project_from_url_prefix() also matches plain path/permalink inputs (not just memory://...), so non-URL deletes now get silently re-routed when their first segment equals another project name. In a multi-project setup, a call like delete_note("research/archive") with project=None can now delete from project research instead of the default project where that path was intended, which is a destructive misroute; please gate this override to actual memory:// identifiers before setting project (and apply the same fix to the identical block in edit_note).

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]

1 participant