admin/basicmachines-co-basic-memory
1
0
Fork 0
mirror of https://github.com/basicmachines-co/basic-memory synced 2026-06-21 13:47:35 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(mcp): validate navigation pagination and fix recent_activity project display (#915)

Browse Source 
Signed-off-by: phernandez <paul@basicmachines.co>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Paul Hernandez
2026-06-07 23:03:00 -05:00
committed by GitHub
parent 8570d96bad
commit 7c0937f658
4 changed files with 277 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/basic_memory/mcp/tools/build_context.py
+12
View File
@@ -209,6 +209,18 @@ async def build_context(
Raises:
ToolError: If project doesn't exist or depth parameter is invalid
"""
# Validate pagination arguments before they reach the context service.
# Trigger: page < 1 or page_size < 1 (e.g. page_size=0 or negative).
# Why: a non-positive page_size flows into context_service as limit, where the
# primary slice does primary = primary[:limit] — so limit=0 truncates the
# requested entity to [] and the caller's valid memory:// lookup silently
# returns primary_count=0. Mirrors recent_activity's guard for consistency.
# Outcome: caller gets an explicit ValueError instead of a dropped primary result.
if page < 1:
raise ValueError(f"page must be >= 1, got {page}")
if page_size < 1:
raise ValueError(f"page_size must be >= 1, got {page_size}")
# Detect project from memory URL prefix before routing.
# project_id routes by external UUID, so it bypasses URL discovery entirely.
if project is None and project_id is None:
src/basic_memory/mcp/tools/recent_activity.py
+11 -5
View File
@@ -300,14 +300,20 @@ async def recent_activity(
else:
# Project-Specific Mode: Get activity for specific project
# Uses get_project_client() for per-project routing (local vs cloud)
logger.info(
f"Getting recent activity from project {resolved_project}: type={type}, depth={depth}, timeframe={timeframe}"
)
async with get_project_client(resolved_project, context=context, project_id=project_id) as (
client,
active_project,
):
# Trigger: caller routed by project_id (a UUID), so resolved_project holds the
# raw UUID rather than a human-readable name.
# Why: active_project.name is the canonical, display-safe project name regardless
# of whether routing was by name or by external_id.
# Outcome: logs and the formatted text header always show the project name.
logger.info(
f"Getting recent activity from project {active_project.name}: "
f"type={type}, depth={depth}, timeframe={timeframe}"
)
response = await call_get(
client,
f"/v2/projects/{active_project.external_id}/memory/recent",
@@ -319,7 +325,7 @@ async def recent_activity(
return _extract_recent_rows(activity_data)
# Format project-specific mode output
return _format_project_output(resolved_project, activity_data, timeframe, type, page)
return _format_project_output(active_project.name, activity_data, timeframe, type, page)
async def _get_project_activity(
src/basic_memory/mcp/tools/search.py
+13
View File
@@ -866,6 +866,19 @@ async def search_notes(
# Explicit project specification
results = await search_notes("project planning", project="my-project")
"""
# Validate pagination arguments before they reach the API/repository layer.
# Trigger: page < 1 or page_size < 1 (e.g. page_size=0 or a negative slice).
# Why: a non-positive page_size yields zero rows yet the router computes
# has_more = offset + len(results) < total, returning a misleading
# has_more=True with no reachable page; a negative page_size becomes an
# uncapped SQLite LIMIT. Mirrors recent_activity's guard so all navigation
# tools reject invalid pagination consistently.
# Outcome: caller gets an explicit ValueError instead of a silent bad payload.
if page < 1:
raise ValueError(f"page must be >= 1, got {page}")
if page_size < 1:
raise ValueError(f"page_size must be >= 1, got {page_size}")
# Avoid mutable-default-argument footguns. Treat None as "no filter".
# Lowercase note_types so "Chapter" matches the stored "chapter".
note_types = [t.lower() for t in note_types] if note_types else []
test-int/bughunt_fixes/test_navigation_pagination_integration.py
+241
View File
@@ -0,0 +1,241 @@
"""Bughunt regression tests: pagination + project-name display in navigation tools.
These integration tests come from the integration-test bug hunt. They exercise the
real MCP server (FastMCP Client), real DB, and real ASGI routing — no mocks.
Covered bugs:
- #9 search_notes accepted non-positive page_size and returned a misleading
has_more=true with zero rows (inconsistent with recent_activity validation).
- #10 build_context with page_size<=0 silently dropped the requested primary entity
(primary_count=0 for a valid memory:// URL).
- #13 recent_activity text output printed the raw project UUID instead of the
project name when routed via project_id.
"""
import json
import pytest
from fastmcp import Client
from fastmcp.exceptions import ToolError
from typer.testing import CliRunner
from basic_memory.cli.main import app as cli_app
runner = CliRunner()
def _parse(result):
return json.loads(result.content[0].text)
async def _seed_notes(client, project, n=15):
for i in range(n):
await client.call_tool(
"write_note",
{
"project": project,
"title": f"Pg Note {i + 1:02d}",
"directory": "pg",
"content": f"# Pg Note {i + 1:02d}\n\npagination probe content.",
"tags": "pg,probe",
},
)
# --- Bug #9: search_notes pagination validation ---
@pytest.mark.asyncio
async def test_search_notes_rejects_nonpositive_page_size(mcp_server, app, test_project):
"""search_notes must reject non-positive page_size like recent_activity does,
instead of returning a misleading has_more=true with zero rows."""
async with Client(mcp_server) as client:
await _seed_notes(client, test_project.name, 15)
# Baseline: recent_activity correctly rejects page_size < 1.
with pytest.raises(ToolError, match="page_size"):
await client.call_tool(
"recent_activity",
{"project": test_project.name, "page_size": -3, "output_format": "json"},
)
# search_notes must now reject page_size=0 with the same guard, rather than
# returning an empty payload that misleadingly claims more results exist.
with pytest.raises(ToolError, match="page_size"):
await client.call_tool(
"search_notes",
{
"project": test_project.name,
"query": "pagination",
"search_type": "text",
"page": 1,
"page_size": 0,
"output_format": "json",
},
)
# Negative page_size must not return an arbitrary uncapped slice — also rejected.
with pytest.raises(ToolError, match="page_size"):
await client.call_tool(
"search_notes",
{
"project": test_project.name,
"query": "pagination",
"search_type": "text",
"page": 1,
"page_size": -3,
"output_format": "json",
},
)
# page < 1 is rejected too.
with pytest.raises(ToolError, match="page"):
await client.call_tool(
"search_notes",
{
"project": test_project.name,
"query": "pagination",
"search_type": "text",
"page": 0,
"page_size": 5,
"output_format": "json",
},
)
# A valid page_size still works and reports honest pagination.
ok = await client.call_tool(
"search_notes",
{
"project": test_project.name,
"query": "pagination",
"search_type": "text",
"page": 1,
"page_size": 5,
"output_format": "json",
},
)
d_ok = _parse(ok)
assert len(d_ok["results"]) == 5
assert d_ok["has_more"] is True
# --- Bug #9 (CLI surface): search-notes --page-size 0 ---
def _write_cli(title, folder="pgcli"):
return runner.invoke(
cli_app,
[
"tool",
"write-note",
"--title",
title,
"--folder",
folder,
"--content",
f"# {title}\n\nclipagination probe body.",
],
)
def test_cli_search_notes_page_size_zero(app, app_config, test_project, config_manager):
"""`bm tool search-notes ... --page-size 0` must fail fast instead of returning a
misleading has_more=true with zero rows."""
for i in range(5):
w = _write_cli(f"CliPg Note {i:02d}")
assert w.exit_code == 0, w.output
res = runner.invoke(
cli_app,
["tool", "search-notes", "clipagination", "--local", "--page-size", "0"],
)
# The fix raises ValueError, which the CLI maps to a non-zero exit with a clear
# message — the faithful "no misleading pagination signal" outcome at the CLI.
assert res.exit_code != 0, res.output
assert "page_size" in res.output
# --- Bug #10: build_context must always return the requested primary entity ---
@pytest.mark.asyncio
async def test_build_context_nonpositive_page_size_drops_primary(mcp_server, app, test_project):
"""build_context with a valid URL must return its primary entity (or raise) —
a non-positive page_size must never silently drop it."""
async with Client(mcp_server) as client:
await client.call_tool(
"write_note",
{
"project": test_project.name,
"title": "Primary Note",
"directory": "ctx",
"content": "# Primary Note\n\n## Relations\n- relates_to [[Other Note]]\n",
"tags": "ctx",
},
)
await client.call_tool(
"write_note",
{
"project": test_project.name,
"title": "Other Note",
"directory": "ctx",
"content": "# Other Note\n\nrelated content.\n",
"tags": "ctx",
},
)
# Sanity: a normal page_size returns the primary note.
r_ok = await client.call_tool(
"build_context",
{"project": test_project.name, "url": "ctx/primary-note", "output_format": "json"},
)
d_ok = _parse(r_ok)
assert d_ok["metadata"]["primary_count"] == 1
# page_size=0 must NOT silently drop the requested entity. The fix rejects it
# with a clear error rather than returning primary_count=0.
with pytest.raises(ToolError, match="page_size"):
await client.call_tool(
"build_context",
{
"project": test_project.name,
"url": "ctx/primary-note",
"page_size": 0,
"output_format": "json",
},
)
# --- Bug #13: recent_activity header shows project name, not raw UUID ---
@pytest.mark.asyncio
async def test_recent_activity_project_id_header_shows_name_not_uuid(
mcp_server, app, test_project, config_manager
):
"""When routed via project_id, the text header must name the project, not the UUID."""
async with Client(mcp_server) as client:
await client.call_tool(
"write_note",
{
"project": test_project.name,
"title": "RA Header Note",
"directory": "ra",
"content": "# RA Header Note\n\nToken",
"output_format": "json",
},
)
result = await client.call_tool(
"recent_activity",
{"project_id": test_project.external_id, "output_format": "text"},
)
text = result.content[0].text
# The human-readable header should reference the project NAME, not the UUID.
assert test_project.external_id not in text, (
f"recent_activity header leaked the raw external_id UUID:\n{text[:300]}"
)
assert test_project.name in text, (
f"recent_activity header should contain the project name:\n{text[:300]}"
)