From 785cb55fcdf80d229bc910b803829d7a75a73abb Mon Sep 17 00:00:00 2001 From: Steven Huang <134844631+remote-controlled-man@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:49:38 +0800 Subject: [PATCH 1/2] fix(api): return 400 for malformed memory-context locators Malformed memory-context locators raised ValueError from normalization and escaped the v2 route as HTTP 500. Reject them at the API boundary with a bounded 400 while preserving the normalizer reason and keeping genuine context-service failures visible. Add API and MCP boundary regressions. Refs #1879. Signed-off-by: Steven Huang <134844631+remote-controlled-man@users.noreply.github.com> --- .../api/v2/routers/memory_router.py | 121 +++++++---- tests/api/v2/test_memory_router.py | 199 +++++++++++++++++- tests/mcp/test_tool_bm_context.py | 20 ++ 3 files changed, 294 insertions(+), 46 deletions(-) diff --git a/src/basic_memory/api/v2/routers/memory_router.py b/src/basic_memory/api/v2/routers/memory_router.py index 77ef32ab6..6bf5f988f 100644 --- a/src/basic_memory/api/v2/routers/memory_router.py +++ b/src/basic_memory/api/v2/routers/memory_router.py @@ -187,49 +187,82 @@ async def get_memory_context( 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) + ) 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. + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=str(exc), + ) from exc - since = parse_timeframe(timeframe) if timeframe else None - limit = page_size - offset = (page - 1) * page_size + since = parse_timeframe(timeframe) if timeframe else None + limit = page_size + offset = (page - 1) * page_size - with logfire.span( - "api.memory.build_context.build_context", - domain="memory", - action="build_context", - phase="build_context", - page=page, - page_size=page_size, - ): - try: - context = await context_service.build_context( - memory_url, - depth=depth, - since=since, - limit=limit, - offset=offset, - max_related=max_related, - include_observations=include_observations, - ) - 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 - with logfire.span( - "api.memory.build_context.shape_response", - domain="memory", - action="build_context", - phase="shape_response", - result_count=len(context.results), - ): - async with db.scoped_session(session_maker) as session: - return await to_graph_context( - context, - entity_repository=entity_repository, - session=session, - page=page, - page_size=page_size, - ) + with logfire.span( + "api.memory.build_context.build_context", + domain="memory", + action="build_context", + phase="build_context", + page=page, + page_size=page_size, + ): + try: + context = await context_service.build_context( + memory_url, + depth=depth, + since=since, + limit=limit, + offset=offset, + max_related=max_related, + include_observations=include_observations, + ) + 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 + with logfire.span( + "api.memory.build_context.shape_response", + domain="memory", + action="build_context", + phase="shape_response", + result_count=len(context.results), + ): + async with db.scoped_session(session_maker) as session: + result = await to_graph_context( + context, + entity_repository=entity_repository, + session=session, + page=page, + page_size=page_size, + ) + except HTTPException as exc: + # Malformed input (400) and ambiguous titles (409) are both caller-visible + # outcomes; only a server-side status would be a failure. + 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", "success") + return result diff --git a/tests/api/v2/test_memory_router.py b/tests/api/v2/test_memory_router.py index bd19914d7..e67c4a708 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,71 @@ 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 the attributes the memory router ships on its root span.""" + + def __init__(self, name: str, attributes: dict[str, Any]) -> None: + self.name = name + self.attributes = attributes + + 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_info: object) -> None: + 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 +521,126 @@ 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) + + +@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 + assert _root_memory_span(spans).attributes["outcome"] == "success" + + +@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 + assert _root_memory_span(spans).attributes["outcome"] == "failed" + + +@pytest.mark.asyncio +async def test_get_memory_context_preserves_ambiguous_identifier_conflict( + app: FastAPI, + client: AsyncClient, + v2_project_url: str, +): + """Ambiguous identifiers keep their existing 409 conflict response.""" + app.dependency_overrides[get_context_service_v2_external] = lambda: _AmbiguousContextService() + try: + 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"] 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 From b875a06c397451178eb5428b70c0eef464a47a8c Mon Sep 17 00:00:00 2001 From: Steven Huang <134844631+remote-controlled-man@users.noreply.github.com> Date: Wed, 28 Oct 2026 15:44:27 +0800 Subject: [PATCH 2/2] fix(api): defer client errors past request span Caller-visible 400/409 responses no longer exit the Logfire request span as exceptions, keeping locator content out of span exception telemetry. Unexpected 5xx failures remain in-span failures. Signed-off-by: Steven Huang <134844631+remote-controlled-man@users.noreply.github.com> --- .../api/v2/routers/memory_router.py | 117 ++++++++++-------- tests/api/v2/test_memory_router.py | 27 +++- 2 files changed, 87 insertions(+), 57 deletions(-) diff --git a/src/basic_memory/api/v2/routers/memory_router.py b/src/basic_memory/api/v2/routers/memory_router.py index 6bf5f988f..ca8bae5bb 100644 --- a/src/basic_memory/api/v2/routers/memory_router.py +++ b/src/basic_memory/api/v2/routers/memory_router.py @@ -180,6 +180,56 @@ async def get_memory_context( Returns: GraphContext with the entity and its related context """ + 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 + + with logfire.span( + "api.memory.build_context.build_context", + domain="memory", + action="build_context", + phase="build_context", + page=page, + page_size=page_size, + ): + try: + context = await context_service.build_context( + memory_url, + depth=depth, + since=since, + limit=limit, + offset=offset, + max_related=max_related, + include_observations=include_observations, + ) + except AmbiguousIdentifierError as exc: + # A title URL that names several notes has no single context to build. + # 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", + action="build_context", + phase="shape_response", + result_count=len(context.results), + ): + async with db.scoped_session(session_maker) as session: + return await to_graph_context( + context, + entity_repository=entity_repository, + session=session, + page=page, + page_size=page_size, + ) + with logfire.span( "api.request.memory.build_context", entrypoint="api", @@ -200,57 +250,12 @@ async def get_memory_context( # 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. - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=str(exc), - ) from exc - - since = parse_timeframe(timeframe) if timeframe else None - limit = page_size - offset = (page - 1) * page_size - - with logfire.span( - "api.memory.build_context.build_context", - domain="memory", - action="build_context", - phase="build_context", - page=page, - page_size=page_size, - ): - try: - context = await context_service.build_context( - memory_url, - depth=depth, - since=since, - limit=limit, - offset=offset, - max_related=max_related, - include_observations=include_observations, - ) - 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 - with logfire.span( - "api.memory.build_context.shape_response", - domain="memory", - action="build_context", - phase="shape_response", - result_count=len(context.results), - ): - async with db.scoped_session(session_maker) as session: - result = await to_graph_context( - context, - entity_repository=entity_repository, - session=session, - page=page, - page_size=page_size, - ) + # 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: - # Malformed input (400) and ambiguous titles (409) are both caller-visible - # outcomes; only a server-side status would be a failure. + # Unexpected HTTP errors keep the original in-span outcome handling. span.set_attribute( "outcome", "input_rejected" if exc.status_code < 500 else "failed", @@ -264,5 +269,13 @@ async def get_memory_context( span.set_attribute("outcome", "failed") raise - span.set_attribute("outcome", "success") - return result + 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 e67c4a708..c2655344d 100644 --- a/tests/api/v2/test_memory_router.py +++ b/tests/api/v2/test_memory_router.py @@ -25,11 +25,12 @@ class _RecordingSpan: - """Capture the attributes the memory router ships on its root span.""" + """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 @@ -40,7 +41,13 @@ def set_attributes(self, values: dict[str, Any]) -> None: def __enter__(self) -> "_RecordingSpan": return self - def __exit__(self, *exc_info: object) -> None: + def __exit__( + self, + _exc_type: type[BaseException] | None, + exc: BaseException | None, + _traceback: object, + ) -> None: + self.exited_with = exc return None @@ -591,6 +598,7 @@ async def test_get_memory_context_records_input_rejected_outcome( 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 @@ -604,7 +612,9 @@ async def test_get_memory_context_records_success_outcome( response = await client.get(f"{v2_project_url}/memory/test/root") assert response.status_code == 200 - assert _root_memory_span(spans).attributes["outcome"] == "success" + root_span = _root_memory_span(spans) + assert root_span.attributes["outcome"] == "success" + assert root_span.exited_with is None @pytest.mark.asyncio @@ -626,7 +636,9 @@ async def test_get_memory_context_keeps_service_failures_as_server_errors( app.dependency_overrides.pop(get_context_service_v2_external, None) assert response.status_code == 500 - assert _root_memory_span(spans).attributes["outcome"] == "failed" + root_span = _root_memory_span(spans) + assert root_span.attributes["outcome"] == "failed" + assert isinstance(root_span.exited_with, RuntimeError) @pytest.mark.asyncio @@ -634,13 +646,18 @@ 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: - response = await client.get(f"{v2_project_url}/memory/Bean%20Origins") + 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)