diff --git a/src/basic_memory/mcp/clients/knowledge.py b/src/basic_memory/mcp/clients/knowledge.py index df001add2..d4a8c1d0a 100644 --- a/src/basic_memory/mcp/clients/knowledge.py +++ b/src/basic_memory/mcp/clients/knowledge.py @@ -19,6 +19,7 @@ DirectoryDeleteResult, ) from basic_memory.schemas.v2.graph import GraphNode, OrphanEntitiesResponse +from basic_memory.schemas.v2.entity import EntityResolveResponse class KnowledgeClient: @@ -348,19 +349,13 @@ async def get_orphans(self) -> list[GraphNode]: # --- Resolution --- - async def resolve_entity(self, identifier: str, *, strict: bool = False) -> str: - """Resolve a string identifier to an entity external_id. - - Args: - identifier: The identifier to resolve (permalink, title, or path) - strict: If True, require exact matching (no fuzzy fallback) - - Returns: - The resolved entity external_id (UUID) - - Raises: - ToolError: If the identifier cannot be resolved - """ + async def _resolve_entity_data( + self, + identifier: str, + *, + strict: bool = False, + ) -> dict[str, Any]: + """Request the complete entity-resolution payload.""" from basic_memory.mcp.tools.utils import call_post with logfire.span( @@ -376,5 +371,31 @@ async def resolve_entity(self, identifier: str, *, strict: bool = False) -> str: operation="resolve_entity", path_template="/v2/projects/{project_id}/knowledge/resolve", ) - data = response.json() + data: dict[str, Any] = response.json() + return data + + async def resolve_entity_response( + self, + identifier: str, + *, + strict: bool = False, + ) -> EntityResolveResponse: + """Resolve an identifier while preserving owning-project metadata. + + Args: + identifier: The identifier to resolve (permalink, title, or path) + strict: If True, require exact matching (no fuzzy fallback) + + Returns: + The complete validated resolution response + + Raises: + ToolError: If the identifier cannot be resolved + """ + data = await self._resolve_entity_data(identifier, strict=strict) + return EntityResolveResponse.model_validate(data) + + async def resolve_entity(self, identifier: str, *, strict: bool = False) -> str: + """Resolve a string identifier to an entity external_id.""" + data = await self._resolve_entity_data(identifier, strict=strict) return data["external_id"] diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index 13ab3d4ca..717629f29 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -77,6 +77,18 @@ class WorkspaceProjectLookupMiss(ValueError): """ +class UnresolvedProjectRouteError(ValueError): + """A mutating memory URL named a project prefix that could not be resolved.""" + + def __init__(self, identifier: str, project_prefix: str): + self.identifier = identifier + self.project_prefix = project_prefix + super().__init__( + f"Memory URL project route '{project_prefix}' could not be resolved; " + "refusing to treat the URL as a path in the active project." + ) + + @dataclass(frozen=True) class WorkspaceProjectEntry: """A cloud project resolved together with the workspace that owns it.""" @@ -1278,11 +1290,22 @@ async def resolve_project_and_path( project: Optional[str] = None, context: Optional[Context] = None, headers: HeaderTypes | None = None, + *, + strict_project_routing: bool = False, ) -> tuple[ProjectItem, str, bool]: """Resolve project and normalized path for memory:// identifiers. + Args: + strict_project_routing: Reject a memory URL whose leading project-like + segment cannot be resolved. Mutating tools use this to prevent a + failed route from falling back to the active project. + Returns: Tuple of (active_project, normalized_path, is_memory_url) + + Raises: + UnresolvedProjectRouteError: If strict routing is enabled and the + memory URL's leading project segment does not resolve. """ is_memory_url = identifier.strip().startswith("memory://") config = ConfigManager().config @@ -1380,6 +1403,14 @@ async def resolve_project_and_path( except ToolError as exc: if "project not found" not in str(exc).lower(): raise + if strict_project_routing: + # Trigger: a mutating tool supplied a memory URL whose leading + # project-like segment did not resolve. + # Why: falling back would reinterpret the full route as a path + # in the active project, allowing append/prepend to create a + # phantom note in the wrong project (#1066). + # Outcome: stop before entity resolution or file creation. + raise UnresolvedProjectRouteError(identifier, project_prefix) from exc else: resolved_project = await resolve_project_parameter(project_prefix, context=context) if resolved_project and generate_permalink(resolved_project) != generate_permalink( diff --git a/src/basic_memory/mcp/tools/edit_note.py b/src/basic_memory/mcp/tools/edit_note.py index 08c29deef..1a08107fa 100644 --- a/src/basic_memory/mcp/tools/edit_note.py +++ b/src/basic_memory/mcp/tools/edit_note.py @@ -15,6 +15,7 @@ from basic_memory.config import ConfigManager from basic_memory.ignore_utils import IGNORED_PATH_REJECTION_DETAIL from basic_memory.mcp.project_context import ( + UnresolvedProjectRouteError, _workspace_identifier_discovery_available, detect_project_from_memory_url_prefix, get_project_client, @@ -193,6 +194,38 @@ def _format_ambiguous_workspace_identifier_response( - `edit_note(identifier="{note_identifier}", project_id="", operation=..., content=...)`""" +def _format_unresolved_project_route_response( + *, + error: UnresolvedProjectRouteError, + active_project: str, +) -> str: + """Format a safe stop when a mutating memory URL cannot be routed.""" + return f"""# Edit Failed - Unresolved Project Route + +The memory URL `{error.identifier}` starts with the project route `{error.project_prefix}`, but that project could not be resolved. + +No note was edited or created. Basic Memory did not fall back to the active project `{active_project}`. + +## How to retry +1. Use `list_memory_projects()` to confirm the workspace, project, and project ID. +2. Correct the `memory://` URL so its project route exists, or pass the note path with an explicit `project` or `project_id`. +3. If `{error.project_prefix}` is a directory in `{active_project}`, remove the `memory://` prefix and retry with `project="{active_project}"`.""" + + +def _format_cross_project_entity_response( + *, + identifier: str, + active_project: str, + target_project_id: str, +) -> str: + """Format a safe stop when resolution finds a note in another project.""" + return f"""# Edit Failed - Note Not Found In This Project + +The identifier `{identifier}` resolved to a note outside the selected project `{active_project}`, so no changes were made. + +Retry with `project_id="{target_project_id}"`, or use `list_memory_projects()` to confirm the intended project before editing.""" + + def _format_error_response( error_message: str, operation: str, @@ -554,12 +587,30 @@ async def edit_note( # Use typed KnowledgeClient for API calls knowledge_client = KnowledgeClient(client, active_project.external_id) - _, entity_identifier, _ = await resolve_project_and_path( - client, - identifier, - active_project.name, - context, - ) + unresolved_project_route: UnresolvedProjectRouteError | None = None + try: + _, entity_identifier, _ = await resolve_project_and_path( + client, + identifier, + active_project.name, + context, + strict_project_routing=True, + ) + except UnresolvedProjectRouteError as route_error: + # Trigger: a memory URL's first segment is not a project, which + # can also describe a valid active-project path such as + # memory://src/existing-note. + # Why: existing indexed notes must remain editable, but a miss + # must never reach append/prepend auto-create. + # Outcome: resolve once with the read-compatible fallback, then + # raise the saved route error if normal recovery still misses. + unresolved_project_route = route_error + _, entity_identifier, _ = await resolve_project_and_path( + client, + identifier, + active_project.name, + context, + ) file_created = False entity_id = "" @@ -567,10 +618,33 @@ async def edit_note( # Try to resolve the entity; for append/prepend, create it if not found try: - entity_id = await knowledge_client.resolve_entity( + resolved_entity = await knowledge_client.resolve_entity_response( entity_identifier, strict=True, ) + if resolved_entity.project_external_id != active_project.external_id: + # Trigger: the link resolver found a note owned by another project. + # Why: patching through the active project's endpoint would leak + # an internal entity ID in a misleading 404 and cannot succeed. + # Outcome: stop before mutation and provide the owning project ID. + if output_format == "json": + return { + "title": None, + "permalink": None, + "file_path": None, + "checksum": None, + "operation": operation, + "fileCreated": False, + "error": "CROSS_PROJECT_ENTITY", + "project": active_project.name, + "targetProjectId": resolved_entity.project_external_id, + } + return _format_cross_project_entity_response( + identifier=identifier, + active_project=active_project.name, + target_project_id=resolved_entity.project_external_id, + ) + entity_id = resolved_entity.external_id except Exception as resolve_error: error_msg = str(resolve_error).lower() is_not_found = "entity not found" in error_msg or "not found" in error_msg @@ -587,6 +661,8 @@ async def edit_note( if recovered_entity_id is not None: entity_id = recovered_entity_id + elif is_not_found and unresolved_project_route is not None: + raise unresolved_project_route elif is_not_found and operation in ("append", "prepend"): # Trigger: entity does not exist yet (on disk or in the index) # Why: append/prepend can meaningfully create a new note from the @@ -741,6 +817,23 @@ async def edit_note( except Exception as e: logger.error(f"Error editing note: {e}") + if isinstance(e, UnresolvedProjectRouteError): + if output_format == "json": + return { + "title": None, + "permalink": None, + "file_path": None, + "checksum": None, + "operation": operation, + "fileCreated": False, + "error": "UNRESOLVED_PROJECT_ROUTE", + "project": active_project.name, + "projectRoute": e.project_prefix, + } + return _format_unresolved_project_route_response( + error=e, + active_project=active_project.name, + ) if output_format == "json": return { "title": None, diff --git a/tests/mcp/clients/test_clients.py b/tests/mcp/clients/test_clients.py index 5028f249a..f1663eae0 100644 --- a/tests/mcp/clients/test_clients.py +++ b/tests/mcp/clients/test_clients.py @@ -130,6 +130,33 @@ async def mock_call_post(client, url, **kwargs): result = await client.resolve_entity("my-note") assert result == "entity-uuid-123" + @pytest.mark.asyncio + async def test_resolve_entity_response_preserves_project_metadata(self, monkeypatch): + """Complete resolution responses retain the owning project external ID.""" + mock_response = MagicMock() + mock_response.json.return_value = { + "external_id": "entity-uuid-123", + "entity_id": 42, + "project_external_id": "project-uuid-456", + "permalink": "other-project/notes/my-note", + "file_path": "notes/My Note.md", + "title": "My Note", + "resolution_method": "permalink", + } + + async def mock_call_post(client, url, **kwargs): + assert "/v2/projects/proj-123/knowledge/resolve" in url + assert kwargs["json"] == {"identifier": "my-note", "strict": True} + return mock_response + + monkeypatch.setattr("basic_memory.mcp.tools.utils.call_post", mock_call_post) + + client = KnowledgeClient(MagicMock(), "proj-123") + result = await client.resolve_entity_response("my-note", strict=True) + + assert result.external_id == "entity-uuid-123" + assert result.project_external_id == "project-uuid-456" + @pytest.mark.asyncio async def test_index_file(self, monkeypatch): """Test index_file posts the file path to the index-file endpoint.""" diff --git a/tests/mcp/test_tool_edit_note.py b/tests/mcp/test_tool_edit_note.py index d16512d7b..bfa1069dc 100644 --- a/tests/mcp/test_tool_edit_note.py +++ b/tests/mcp/test_tool_edit_note.py @@ -11,6 +11,7 @@ from basic_memory.mcp.tools.edit_note import _resolve_after_disk_recovery, edit_note from basic_memory.mcp.tools.read_note import read_note from basic_memory.mcp.tools.write_note import write_note +from basic_memory.schemas.v2.entity import EntityResolveResponse def test_edit_note_workspace_project_route_helper(): @@ -378,6 +379,96 @@ async def test_edit_note_append_creates_json_format(client, test_project): assert result["operation"] == "append" +@pytest.mark.asyncio +async def test_edit_note_memory_url_unresolved_project_never_autocreates(client, test_project): + """A failed memory URL route must not create a phantom note in the active project.""" + result = await edit_note( + project=test_project.name, + identifier="memory://missing-project/notes/phantom-note", + operation="append", + content="# Phantom\n\nThis must not be created.", + ) + + assert isinstance(result, str) + assert "# Edit Failed - Unresolved Project Route" in result + assert "No note was edited or created" in result + assert f"active project `{test_project.name}`" in result + assert not (Path(test_project.path) / "missing-project" / "notes" / "phantom-note.md").exists() + + +@pytest.mark.asyncio +async def test_edit_note_memory_url_unresolved_project_json_error(client, test_project): + """JSON mode reports an unresolved route without creating a file.""" + result = await edit_note( + project=test_project.name, + identifier="memory://missing-project/notes/phantom-json-note", + operation="prepend", + content="# Phantom JSON", + output_format="json", + ) + + assert isinstance(result, dict) + assert result["error"] == "UNRESOLVED_PROJECT_ROUTE" + assert result["fileCreated"] is False + assert result["projectRoute"] == "missing-project" + assert not ( + Path(test_project.path) / "missing-project" / "notes" / "phantom-json-note.md" + ).exists() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("output_format", ["text", "json"]) +async def test_edit_note_cross_project_resolution_stops_before_patch( + monkeypatch, + client, + test_project, + output_format, +): + """A sibling-project match should return a clean retry instead of leaking an entity ID.""" + target_project_id = "22222222-2222-2222-2222-222222222222" + target_entity_id = "33333333-3333-3333-3333-333333333333" + + async def resolve_in_sibling(self, identifier, *, strict=False): + assert strict is True + return EntityResolveResponse( + external_id=target_entity_id, + entity_id=42, + project_external_id=target_project_id, + permalink="sibling-project/notes/cross-project-note", + file_path="notes/Cross Project Note.md", + title="Cross Project Note", + resolution_method="search", + ) + + async def fail_patch(*args, **kwargs): # pragma: no cover + raise AssertionError("cross-project matches must stop before patching") + + monkeypatch.setattr(KnowledgeClient, "resolve_entity_response", resolve_in_sibling) + monkeypatch.setattr(KnowledgeClient, "patch_entity", fail_patch) + + result = await edit_note( + project=test_project.name, + identifier="sibling-project::Cross Project Note", + operation="append", + content="\nMust not be appended.", + output_format=output_format, + ) + + if output_format == "json": + assert isinstance(result, dict) + assert result["error"] == "CROSS_PROJECT_ENTITY" + assert result["fileCreated"] is False + assert result["project"] == test_project.name + assert result["targetProjectId"] == target_project_id + assert target_entity_id not in result.values() + else: + assert isinstance(result, str) + assert "# Edit Failed - Note Not Found In This Project" in result + assert f"selected project `{test_project.name}`" in result + assert f'project_id="{target_project_id}"' in result + assert target_entity_id not in result + + @pytest.mark.asyncio async def test_edit_note_existing_note_json_includes_file_created_false(client, test_project): """JSON output for editing an existing note should include fileCreated: false."""