diff --git a/src/basic_memory/api/v2/routers/memory_router.py b/src/basic_memory/api/v2/routers/memory_router.py index 77ef32ab6..ca8bae5bb 100644 --- a/src/basic_memory/api/v2/routers/memory_router.py +++ b/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 diff --git a/tests/api/v2/test_memory_router.py b/tests/api/v2/test_memory_router.py index bd19914d7..c2655344d 100644 --- a/tests/api/v2/test_memory_router.py +++ b/tests/api/v2/test_memory_router.py @@ -1,10 +1,17 @@ """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, @@ -12,6 +19,78 @@ 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) diff --git a/tests/mcp/test_tool_bm_context.py b/tests/mcp/test_tool_bm_context.py index 3cc86a64f..e73cf66c9 100644 --- a/tests/mcp/test_tool_bm_context.py +++ b/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