Skip to content

Commit 6d202cf

Browse files
fix(api): return 400 for malformed memory-context locators (#1885)
Fixes #1879. 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 before traversal, while preserving the normalizer's reason and keeping genuine context-service failures visible. Add API and MCP boundary regressions. Tests: - full SQLite unit suite: 9022 passed, 82 skipped - test-int-sqlite: 678 passed, 91 skipped - typecheck, ruff check/format: passed Postgres/testcontainers: not run locally (Docker unavailable); CI will cover. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Malformed memory locators now return a clear client error rather than appearing as an internal server error. * Ambiguous memory identifiers return a conflict response, while service failures remain server errors. * **Improvements** * Context requests accept valid relative paths and equivalent `memory://` locators, providing consistent results across both locator formats. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Steven Huang <[email protected]>
1 parent d957980 commit 6d202cf

3 files changed

Lines changed: 293 additions & 15 deletions

File tree

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

Lines changed: 59 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -180,19 +180,11 @@ async def get_memory_context(
180180
Returns:
181181
GraphContext with the entity and its related context
182182
"""
183-
with logfire.span(
184-
"api.request.memory.build_context",
185-
entrypoint="api",
186-
domain="memory",
187-
action="build_context",
188-
page=page,
189-
page_size=page_size,
190-
):
191-
logger.debug(
192-
f"V2 Getting context for project {project_id}, URI: `{uri}` depth: `{depth}` timeframe: `{timeframe}` page: `{page}` page_size: `{page_size}` max_related: `{max_related}`"
193-
)
194-
memory_url = normalize_memory_url(uri)
183+
deferred_http_error: tuple[int, str, BaseException] | None = None
184+
result: GraphContext | None = None
195185

186+
async def _build_context() -> GraphContext | None:
187+
nonlocal deferred_http_error
196188
since = parse_timeframe(timeframe) if timeframe else None
197189
limit = page_size
198190
offset = (page - 1) * page_size
@@ -217,7 +209,11 @@ async def get_memory_context(
217209
)
218210
except AmbiguousIdentifierError as exc:
219211
# A title URL that names several notes has no single context to build.
220-
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(exc)) from exc
212+
# Defer the caller-visible 409 past this span so Logfire does not
213+
# record the locator content as an exception.
214+
deferred_http_error = (status.HTTP_409_CONFLICT, str(exc), exc)
215+
return None
216+
221217
with logfire.span(
222218
"api.memory.build_context.shape_response",
223219
domain="memory",
@@ -233,3 +229,53 @@ async def get_memory_context(
233229
page=page,
234230
page_size=page_size,
235231
)
232+
233+
with logfire.span(
234+
"api.request.memory.build_context",
235+
entrypoint="api",
236+
domain="memory",
237+
action="build_context",
238+
page=page,
239+
page_size=page_size,
240+
) as span:
241+
try:
242+
logger.debug(
243+
f"V2 Getting context for project {project_id}, URI: `{uri}` depth: `{depth}` timeframe: `{timeframe}` page: `{page}` page_size: `{page_size}` max_related: `{max_related}`"
244+
)
245+
try:
246+
memory_url = normalize_memory_url(uri)
247+
except ValueError as exc:
248+
# Trigger: the path is not a valid memory locator (embedded scheme,
249+
# doubled separator, invalid character, empty/whitespace).
250+
# Why: normalize_memory_url raises ValueError for caller input; letting
251+
# it escape reported a client mistake as an internal server error (#1879).
252+
# Outcome: a bounded 400 with the normalizer's specific reason before
253+
# any context traversal or span exception.
254+
deferred_http_error = (status.HTTP_400_BAD_REQUEST, str(exc), exc)
255+
else:
256+
result = await _build_context()
257+
except HTTPException as exc:
258+
# Unexpected HTTP errors keep the original in-span outcome handling.
259+
span.set_attribute(
260+
"outcome",
261+
"input_rejected" if exc.status_code < 500 else "failed",
262+
)
263+
raise
264+
except Exception:
265+
# Trigger: the context service or response shaping raised.
266+
# Why: genuine failures must stay visible instead of being flattened into
267+
# a validation response.
268+
# Outcome: the 500 path records a failed terminal outcome.
269+
span.set_attribute("outcome", "failed")
270+
raise
271+
272+
span.set_attribute(
273+
"outcome",
274+
"input_rejected" if deferred_http_error is not None else "success",
275+
)
276+
277+
if deferred_http_error is not None:
278+
status_code, detail, cause = deferred_http_error
279+
raise HTTPException(status_code=status_code, detail=detail) from cause
280+
assert result is not None
281+
return result

‎tests/api/v2/test_memory_router.py‎

Lines changed: 214 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,96 @@
11
"""Tests for v2 memory router endpoints."""
22

3-
import pytest
4-
from httpx import AsyncClient
3+
import importlib
4+
from contextlib import contextmanager
55
from pathlib import Path
6+
from types import SimpleNamespace
7+
from typing import Any
8+
9+
import pytest
10+
from fastapi import FastAPI
11+
from httpx import ASGITransport, AsyncClient
612

713
from basic_memory import db
14+
from basic_memory.deps.services import get_context_service_v2_external
815
from basic_memory.models import Project
916
from basic_memory.schemas.memory import (
1017
MAX_CONTEXT_DEPTH,
1118
MAX_CONTEXT_PAGE_SIZE,
1219
MAX_CONTEXT_RELATED_RESULTS,
1320
MAX_RECENT_ACTIVITY_PAGE_SIZE,
1421
)
22+
from basic_memory.services.exceptions import AmbiguousIdentifierError
23+
24+
memory_router_module = importlib.import_module("basic_memory.api.v2.routers.memory_router")
25+
26+
27+
class _RecordingSpan:
28+
"""Capture span attributes and whether the span exited with an exception."""
29+
30+
def __init__(self, name: str, attributes: dict[str, Any]) -> None:
31+
self.name = name
32+
self.attributes = attributes
33+
self.exited_with: BaseException | None = None
34+
35+
def set_attribute(self, key: str, value: Any) -> None:
36+
self.attributes[key] = value
37+
38+
def set_attributes(self, values: dict[str, Any]) -> None:
39+
self.attributes.update(values)
40+
41+
def __enter__(self) -> "_RecordingSpan":
42+
return self
43+
44+
def __exit__(
45+
self,
46+
_exc_type: type[BaseException] | None,
47+
exc: BaseException | None,
48+
_traceback: object,
49+
) -> None:
50+
self.exited_with = exc
51+
return None
52+
53+
54+
@contextmanager
55+
def _record_memory_router_spans(monkeypatch):
56+
spans: list[_RecordingSpan] = []
57+
58+
def span(name: str, **attributes: Any) -> _RecordingSpan:
59+
recorded = _RecordingSpan(name, dict(attributes))
60+
spans.append(recorded)
61+
return recorded
62+
63+
monkeypatch.setattr(memory_router_module, "logfire", SimpleNamespace(span=span))
64+
yield spans
65+
66+
67+
def _root_memory_span(spans: list[_RecordingSpan]) -> _RecordingSpan:
68+
return next(span for span in spans if span.name == "api.request.memory.build_context")
69+
70+
71+
class _TraversalSpy:
72+
def __init__(self) -> None:
73+
self.calls = 0
74+
75+
async def build_context(self, *args: Any, **kwargs: Any) -> None:
76+
self.calls += 1
77+
raise AssertionError("context traversal must not run for a rejected locator")
78+
79+
80+
class _ExplodingContextService:
81+
async def build_context(self, *args: Any, **kwargs: Any) -> None:
82+
raise RuntimeError("context backend exploded")
83+
84+
85+
class _AmbiguousContextService:
86+
async def build_context(self, *args: Any, **kwargs: Any) -> None:
87+
raise AmbiguousIdentifierError(
88+
"Bean Origins",
89+
[
90+
("bean-origins", "Bean Origins.md"),
91+
("archive/bean-origins", "archive/Bean Origins.md"),
92+
],
93+
)
1594

1695

1796
async def create_test_entity(
@@ -449,3 +528,136 @@ async def test_get_memory_context_skips_observations_when_asked(
449528
== full_data["results"][0]["primary_result"]["permalink"]
450529
)
451530
assert shape_data["results"][0]["related_results"] == full_data["results"][0]["related_results"]
531+
532+
533+
@pytest.mark.asyncio
534+
@pytest.mark.parametrize(
535+
("locator", "expected_detail"),
536+
[
537+
# The production locator shape: an arbitrary HTTPS URL embedded in a path.
538+
("workspace/project/https://example.invalid/notes", "contains protocol scheme"),
539+
("", "cannot be empty"),
540+
("%20%20%20", "cannot be empty or whitespace"),
541+
("notes%3Cbrackets%3E", "contains invalid characters"),
542+
("notes//meeting", "contains double slashes"),
543+
],
544+
)
545+
async def test_get_memory_context_rejects_malformed_locators(
546+
app: FastAPI,
547+
client: AsyncClient,
548+
v2_project_url: str,
549+
locator: str,
550+
expected_detail: str,
551+
):
552+
"""Malformed locators are caller input, not an internal server error (#1879)."""
553+
traversal = _TraversalSpy()
554+
app.dependency_overrides[get_context_service_v2_external] = lambda: traversal
555+
try:
556+
response = await client.get(f"{v2_project_url}/memory/{locator}")
557+
finally:
558+
app.dependency_overrides.pop(get_context_service_v2_external, None)
559+
560+
assert response.status_code == 400
561+
assert expected_detail in response.json()["detail"]
562+
# The boundary rejects before the context service is touched.
563+
assert traversal.calls == 0
564+
565+
566+
@pytest.mark.asyncio
567+
async def test_get_memory_context_accepts_relative_and_memory_url_locators(
568+
client: AsyncClient,
569+
test_graph,
570+
v2_project_url: str,
571+
):
572+
"""Both accepted locator spellings still resolve to the same context."""
573+
relative = await client.get(f"{v2_project_url}/memory/test/root")
574+
scheme = await client.get(f"{v2_project_url}/memory/memory://test/root")
575+
576+
assert relative.status_code == 200
577+
assert scheme.status_code == 200
578+
assert (
579+
relative.json()["results"][0]["primary_result"]["permalink"]
580+
== scheme.json()["results"][0]["primary_result"]["permalink"]
581+
)
582+
583+
584+
@pytest.mark.asyncio
585+
async def test_get_memory_context_records_input_rejected_outcome(
586+
client: AsyncClient,
587+
v2_project_url: str,
588+
monkeypatch,
589+
):
590+
"""The root span records a bounded terminal outcome for caller input rejection."""
591+
with _record_memory_router_spans(monkeypatch) as spans:
592+
response = await client.get(
593+
f"{v2_project_url}/memory/workspace/project/https://example.invalid/notes"
594+
)
595+
596+
assert response.status_code == 400
597+
root_span = _root_memory_span(spans)
598+
assert root_span.attributes["outcome"] == "input_rejected"
599+
# The span carries a bounded outcome, not the caller's uploaded locator content.
600+
assert "example.invalid" not in repr(root_span.attributes)
601+
assert root_span.exited_with is None
602+
603+
604+
@pytest.mark.asyncio
605+
async def test_get_memory_context_records_success_outcome(
606+
client: AsyncClient,
607+
test_graph,
608+
v2_project_url: str,
609+
monkeypatch,
610+
):
611+
with _record_memory_router_spans(monkeypatch) as spans:
612+
response = await client.get(f"{v2_project_url}/memory/test/root")
613+
614+
assert response.status_code == 200
615+
root_span = _root_memory_span(spans)
616+
assert root_span.attributes["outcome"] == "success"
617+
assert root_span.exited_with is None
618+
619+
620+
@pytest.mark.asyncio
621+
async def test_get_memory_context_keeps_service_failures_as_server_errors(
622+
app: FastAPI,
623+
v2_project_url: str,
624+
monkeypatch,
625+
):
626+
"""A genuine context-service failure stays visible as a 500 and a failed outcome."""
627+
app.dependency_overrides[get_context_service_v2_external] = lambda: _ExplodingContextService()
628+
try:
629+
with _record_memory_router_spans(monkeypatch) as spans:
630+
async with AsyncClient(
631+
transport=ASGITransport(app=app, raise_app_exceptions=False),
632+
base_url="http://test",
633+
) as failing_client:
634+
response = await failing_client.get(f"{v2_project_url}/memory/test/root")
635+
finally:
636+
app.dependency_overrides.pop(get_context_service_v2_external, None)
637+
638+
assert response.status_code == 500
639+
root_span = _root_memory_span(spans)
640+
assert root_span.attributes["outcome"] == "failed"
641+
assert isinstance(root_span.exited_with, RuntimeError)
642+
643+
644+
@pytest.mark.asyncio
645+
async def test_get_memory_context_preserves_ambiguous_identifier_conflict(
646+
app: FastAPI,
647+
client: AsyncClient,
648+
v2_project_url: str,
649+
monkeypatch,
650+
):
651+
"""Ambiguous identifiers keep their existing 409 conflict response."""
652+
app.dependency_overrides[get_context_service_v2_external] = lambda: _AmbiguousContextService()
653+
try:
654+
with _record_memory_router_spans(monkeypatch) as spans:
655+
response = await client.get(f"{v2_project_url}/memory/Bean%20Origins")
656+
finally:
657+
app.dependency_overrides.pop(get_context_service_v2_external, None)
658+
659+
assert response.status_code == 409
660+
assert "Ambiguous identifier 'Bean Origins'" in response.json()["detail"]
661+
root_span = _root_memory_span(spans)
662+
assert root_span.attributes["outcome"] == "input_rejected"
663+
assert all(span.exited_with is None for span in spans)

‎tests/mcp/test_tool_bm_context.py‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,10 @@
66
from typing import Any
77

88
import pytest
9+
from fastmcp import Client
10+
from fastmcp.exceptions import ToolError
911

12+
from basic_memory.mcp.server import mcp
1013
from basic_memory.mcp.tools import bm_context, write_note
1114

1215

@@ -133,3 +136,20 @@ async def test_text_output_names_the_next_page(client, test_project):
133136
assert isinstance(text, str)
134137
assert "More results available. Call bm_context(identifier='graph/*'" in text
135138
assert "page=2, page_size=1)" in text
139+
140+
141+
@pytest.mark.asyncio
142+
async def test_malformed_locator_is_actionable_input_feedback(client, test_project):
143+
"""The production-shaped locator must not surface as an opaque server failure (#1879)."""
144+
locator = "workspace/project/https://example.invalid/notes"
145+
146+
async with Client(mcp) as mcp_client:
147+
with pytest.raises(ToolError) as exc_info:
148+
await mcp_client.call_tool(
149+
"bm_context",
150+
{"identifier": locator, "project": test_project.name},
151+
)
152+
153+
message = str(exc_info.value)
154+
assert "protocol scheme" in message
155+
assert "Internal server error" not in message

0 commit comments

Comments
 (0)