fix(api): return 400 for malformed memory-context locators

src/basic_memory/api/v2/routers/memory_router.py

@@ -180,19 +180,11 @@ async def get_memory_context(
     Returns:
         GraphContext with the entity and its related context
     """
-    with logfire.span(
-        "api.request.memory.build_context",
-        entrypoint="api",
-        domain="memory",
-        action="build_context",
-        page=page,
-        page_size=page_size,
-    ):
-        logger.debug(
-            f"V2 Getting context for project {project_id}, URI: `{uri}` depth: `{depth}` timeframe: `{timeframe}` page: `{page}` page_size: `{page_size}` max_related: `{max_related}`"
-        )
-        memory_url = normalize_memory_url(uri)
+    deferred_http_error: tuple[int, str, BaseException] | None = None
+    result: GraphContext | None = None
 
+    async def _build_context() -> GraphContext | None:
+        nonlocal deferred_http_error
         since = parse_timeframe(timeframe) if timeframe else None
         limit = page_size
         offset = (page - 1) * page_size
@@ -217,7 +209,11 @@ async def get_memory_context(
                 )
             except AmbiguousIdentifierError as exc:
                 # A title URL that names several notes has no single context to build.
-                raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(exc)) from exc
+                # Defer the caller-visible 409 past this span so Logfire does not
+                # record the locator content as an exception.
+                deferred_http_error = (status.HTTP_409_CONFLICT, str(exc), exc)
+                return None
+
         with logfire.span(
             "api.memory.build_context.shape_response",
             domain="memory",
@@ -233,3 +229,53 @@ async def get_memory_context(
                     page=page,
                     page_size=page_size,
                 )
+
+    with logfire.span(
+        "api.request.memory.build_context",
+        entrypoint="api",
+        domain="memory",
+        action="build_context",
+        page=page,
+        page_size=page_size,
+    ) as span:
+        try:
+            logger.debug(
+                f"V2 Getting context for project {project_id}, URI: `{uri}` depth: `{depth}` timeframe: `{timeframe}` page: `{page}` page_size: `{page_size}` max_related: `{max_related}`"
+            )
+            try:
+                memory_url = normalize_memory_url(uri)
+            except ValueError as exc:
+                # Trigger: the path is not a valid memory locator (embedded scheme,
+                #   doubled separator, invalid character, empty/whitespace).
+                # Why: normalize_memory_url raises ValueError for caller input; letting
+                #   it escape reported a client mistake as an internal server error (#1879).
+                # Outcome: a bounded 400 with the normalizer's specific reason before
+                #   any context traversal or span exception.
+                deferred_http_error = (status.HTTP_400_BAD_REQUEST, str(exc), exc)
+            else:
+                result = await _build_context()
+        except HTTPException as exc:
+            # Unexpected HTTP errors keep the original in-span outcome handling.
+            span.set_attribute(
+                "outcome",
+                "input_rejected" if exc.status_code < 500 else "failed",
+            )
+            raise
+        except Exception:
+            # Trigger: the context service or response shaping raised.
+            # Why: genuine failures must stay visible instead of being flattened into
+            #   a validation response.
+            # Outcome: the 500 path records a failed terminal outcome.
+            span.set_attribute("outcome", "failed")
+            raise
+
+        span.set_attribute(
+            "outcome",
+            "input_rejected" if deferred_http_error is not None else "success",
+        )
+
+    if deferred_http_error is not None:
+        status_code, detail, cause = deferred_http_error
+        raise HTTPException(status_code=status_code, detail=detail) from cause
+    assert result is not None
+    return result

tests/api/v2/test_memory_router.py

@@ -1,17 +1,96 @@
 """Tests for v2 memory router endpoints."""
 
-import pytest
-from httpx import AsyncClient
+import importlib
+from contextlib import contextmanager
 from pathlib import Path
+from types import SimpleNamespace
+from typing import Any
+
+import pytest
+from fastapi import FastAPI
+from httpx import ASGITransport, AsyncClient
 
 from basic_memory import db
+from basic_memory.deps.services import get_context_service_v2_external
 from basic_memory.models import Project
 from basic_memory.schemas.memory import (
     MAX_CONTEXT_DEPTH,
     MAX_CONTEXT_PAGE_SIZE,
     MAX_CONTEXT_RELATED_RESULTS,
     MAX_RECENT_ACTIVITY_PAGE_SIZE,
 )
+from basic_memory.services.exceptions import AmbiguousIdentifierError
+
+memory_router_module = importlib.import_module("basic_memory.api.v2.routers.memory_router")
+
+
+class _RecordingSpan:
+    """Capture span attributes and whether the span exited with an exception."""
+
+    def __init__(self, name: str, attributes: dict[str, Any]) -> None:
+        self.name = name
+        self.attributes = attributes
+        self.exited_with: BaseException | None = None
+
+    def set_attribute(self, key: str, value: Any) -> None:
+        self.attributes[key] = value
+
+    def set_attributes(self, values: dict[str, Any]) -> None:
+        self.attributes.update(values)
+
+    def __enter__(self) -> "_RecordingSpan":
+        return self
+
+    def __exit__(
+        self,
+        _exc_type: type[BaseException] | None,
+        exc: BaseException | None,
+        _traceback: object,
+    ) -> None:
+        self.exited_with = exc
+        return None
+
+
+@contextmanager
+def _record_memory_router_spans(monkeypatch):
+    spans: list[_RecordingSpan] = []
+
+    def span(name: str, **attributes: Any) -> _RecordingSpan:
+        recorded = _RecordingSpan(name, dict(attributes))
+        spans.append(recorded)
+        return recorded
+
+    monkeypatch.setattr(memory_router_module, "logfire", SimpleNamespace(span=span))
+    yield spans
+
+
+def _root_memory_span(spans: list[_RecordingSpan]) -> _RecordingSpan:
+    return next(span for span in spans if span.name == "api.request.memory.build_context")
+
+
+class _TraversalSpy:
+    def __init__(self) -> None:
+        self.calls = 0
+
+    async def build_context(self, *args: Any, **kwargs: Any) -> None:
+        self.calls += 1
+        raise AssertionError("context traversal must not run for a rejected locator")
+
+
+class _ExplodingContextService:
+    async def build_context(self, *args: Any, **kwargs: Any) -> None:
+        raise RuntimeError("context backend exploded")
+
+
+class _AmbiguousContextService:
+    async def build_context(self, *args: Any, **kwargs: Any) -> None:
+        raise AmbiguousIdentifierError(
+            "Bean Origins",
+            [
+                ("bean-origins", "Bean Origins.md"),
+                ("archive/bean-origins", "archive/Bean Origins.md"),
+            ],
+        )
 
 
 async def create_test_entity(
@@ -449,3 +528,136 @@ async def test_get_memory_context_skips_observations_when_asked(
         == full_data["results"][0]["primary_result"]["permalink"]
     )
     assert shape_data["results"][0]["related_results"] == full_data["results"][0]["related_results"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+    ("locator", "expected_detail"),
+    [
+        # The production locator shape: an arbitrary HTTPS URL embedded in a path.
+        ("workspace/project/https://example.invalid/notes", "contains protocol scheme"),
+        ("", "cannot be empty"),
+        ("%20%20%20", "cannot be empty or whitespace"),
+        ("notes%3Cbrackets%3E", "contains invalid characters"),
+        ("notes//meeting", "contains double slashes"),
+    ],
+)
+async def test_get_memory_context_rejects_malformed_locators(
+    app: FastAPI,
+    client: AsyncClient,
+    v2_project_url: str,
+    locator: str,
+    expected_detail: str,
+):
+    """Malformed locators are caller input, not an internal server error (#1879)."""
+    traversal = _TraversalSpy()
+    app.dependency_overrides[get_context_service_v2_external] = lambda: traversal
+    try:
+        response = await client.get(f"{v2_project_url}/memory/{locator}")
+    finally:
+        app.dependency_overrides.pop(get_context_service_v2_external, None)
+
+    assert response.status_code == 400
+    assert expected_detail in response.json()["detail"]
+    # The boundary rejects before the context service is touched.
+    assert traversal.calls == 0
+
+
+@pytest.mark.asyncio
+async def test_get_memory_context_accepts_relative_and_memory_url_locators(
+    client: AsyncClient,
+    test_graph,
+    v2_project_url: str,
+):
+    """Both accepted locator spellings still resolve to the same context."""
+    relative = await client.get(f"{v2_project_url}/memory/test/root")
+    scheme = await client.get(f"{v2_project_url}/memory/memory://test/root")
+
+    assert relative.status_code == 200
+    assert scheme.status_code == 200
+    assert (
+        relative.json()["results"][0]["primary_result"]["permalink"]
+        == scheme.json()["results"][0]["primary_result"]["permalink"]
+    )
+
+
+@pytest.mark.asyncio
+async def test_get_memory_context_records_input_rejected_outcome(
+    client: AsyncClient,
+    v2_project_url: str,
+    monkeypatch,
+):
+    """The root span records a bounded terminal outcome for caller input rejection."""
+    with _record_memory_router_spans(monkeypatch) as spans:
+        response = await client.get(
+            f"{v2_project_url}/memory/workspace/project/https://example.invalid/notes"
+        )
+
+    assert response.status_code == 400
+    root_span = _root_memory_span(spans)
+    assert root_span.attributes["outcome"] == "input_rejected"
+    # The span carries a bounded outcome, not the caller's uploaded locator content.
+    assert "example.invalid" not in repr(root_span.attributes)
+    assert root_span.exited_with is None
+
+
+@pytest.mark.asyncio
+async def test_get_memory_context_records_success_outcome(
+    client: AsyncClient,
+    test_graph,
+    v2_project_url: str,
+    monkeypatch,
+):
+    with _record_memory_router_spans(monkeypatch) as spans:
+        response = await client.get(f"{v2_project_url}/memory/test/root")
+
+    assert response.status_code == 200
+    root_span = _root_memory_span(spans)
+    assert root_span.attributes["outcome"] == "success"
+    assert root_span.exited_with is None
+
+
+@pytest.mark.asyncio
+async def test_get_memory_context_keeps_service_failures_as_server_errors(
+    app: FastAPI,
+    v2_project_url: str,
+    monkeypatch,
+):
+    """A genuine context-service failure stays visible as a 500 and a failed outcome."""
+    app.dependency_overrides[get_context_service_v2_external] = lambda: _ExplodingContextService()
+    try:
+        with _record_memory_router_spans(monkeypatch) as spans:
+            async with AsyncClient(
+                transport=ASGITransport(app=app, raise_app_exceptions=False),
+                base_url="http://test",
+            ) as failing_client:
+                response = await failing_client.get(f"{v2_project_url}/memory/test/root")
+    finally:
+        app.dependency_overrides.pop(get_context_service_v2_external, None)
+
+    assert response.status_code == 500
+    root_span = _root_memory_span(spans)
+    assert root_span.attributes["outcome"] == "failed"
+    assert isinstance(root_span.exited_with, RuntimeError)
+
+
+@pytest.mark.asyncio
+async def test_get_memory_context_preserves_ambiguous_identifier_conflict(
+    app: FastAPI,
+    client: AsyncClient,
+    v2_project_url: str,
+    monkeypatch,
+):
+    """Ambiguous identifiers keep their existing 409 conflict response."""
+    app.dependency_overrides[get_context_service_v2_external] = lambda: _AmbiguousContextService()
+    try:
+        with _record_memory_router_spans(monkeypatch) as spans:
+            response = await client.get(f"{v2_project_url}/memory/Bean%20Origins")
+    finally:
+        app.dependency_overrides.pop(get_context_service_v2_external, None)
+
+    assert response.status_code == 409
+    assert "Ambiguous identifier 'Bean Origins'" in response.json()["detail"]
+    root_span = _root_memory_span(spans)
+    assert root_span.attributes["outcome"] == "input_rejected"
+    assert all(span.exited_with is None for span in spans)

tests/mcp/test_tool_bm_context.py

@@ -6,7 +6,10 @@
 from typing import Any
 
 import pytest
+from fastmcp import Client
+from fastmcp.exceptions import ToolError
 
+from basic_memory.mcp.server import mcp
 from basic_memory.mcp.tools import bm_context, write_note
 
 
@@ -133,3 +136,20 @@ async def test_text_output_names_the_next_page(client, test_project):
     assert isinstance(text, str)
     assert "More results available. Call bm_context(identifier='graph/*'" in text
     assert "page=2, page_size=1)" in text
+
+
+@pytest.mark.asyncio
+async def test_malformed_locator_is_actionable_input_feedback(client, test_project):
+    """The production-shaped locator must not surface as an opaque server failure (#1879)."""
+    locator = "workspace/project/https://example.invalid/notes"
+
+    async with Client(mcp) as mcp_client:
+        with pytest.raises(ToolError) as exc_info:
+            await mcp_client.call_tool(
+                "bm_context",
+                {"identifier": locator, "project": test_project.name},
+            )
+
+    message = str(exc_info.value)
+    assert "protocol scheme" in message
+    assert "Internal server error" not in message