mirror of
https://github.com/basicmachines-co/basic-memory
synced 2026-06-21 13:47:35 +00:00
fix(mcp): route multi-project search by name only
The previous revision gated project_id forwarding on the `cloud_available` composite (factory_mode OR explicit_cloud OR has_cloud_credentials), mirroring get_project_client. But `has_cloud_credentials` returns True any time OAuth tokens linger from a past `bm cloud login`, even when the user is back to working purely locally. So on a typical dev box the fan-out still forwarded project_id, hit get_project_client's UUID branch (which treats unknown identifiers as cloud since local config doesn't key by UUID), and 401d silently — leaving merged results empty. Route by name only: project_refs already carry the workspace/project qualified_name, which is just as unambiguous as the external_id for both backends. project_id stays in the call signature purely as a fallback for refs that unexpectedly have no name. Confirmed live in a restarted MCP — the previous fix did not actually deliver multi-project results when the dev environment had any cloud credentials at all. Tests now drop the routing-mode stubs; the cloud-style tests just key their MockSearchClient on the workspace-qualified name passed via the project parameter. Signed-off-by: phernandez <paul@basicmachines.co>
This commit is contained in:
phernandez
@@ -10,13 +10,8 @@ from loguru import logger
|
||||
from fastmcp import Context
|
||||
from pydantic import AliasChoices, BeforeValidator, Field
|
||||
|
||||
from basic_memory.config import ConfigManager, has_cloud_credentials
|
||||
from basic_memory.config import ConfigManager
|
||||
from basic_memory.utils import build_canonical_permalink, coerce_dict, coerce_list
|
||||
from basic_memory.mcp.async_client import (
|
||||
_explicit_routing,
|
||||
_force_local_mode,
|
||||
is_factory_mode,
|
||||
)
|
||||
from basic_memory.mcp.container import get_container
|
||||
from basic_memory.mcp.project_context import (
|
||||
detect_project_from_identifier_prefix,
|
||||
@@ -485,28 +480,21 @@ async def _search_all_projects(
|
||||
any_project_has_more = False
|
||||
|
||||
# Trigger: caller asked for an account-wide search.
|
||||
# Why: project_id (external UUID) routes through the cloud v2 API path,
|
||||
# which 401s on local installs because there's no JWT to present.
|
||||
# Project names route through the local-ASGI path and work for both
|
||||
# backends — cloud disambiguates names via the workspace/project
|
||||
# qualified_name already baked into project_ref["project"].
|
||||
# Outcome: forward project_id only when the same signals get_project_client
|
||||
# uses to pick a cloud route are present. Mirrors the cloud_available
|
||||
# composite in project_context.get_project_client (single source of
|
||||
# truth for "can we route to cloud?").
|
||||
config = ConfigManager().config
|
||||
use_cloud_routing = (
|
||||
is_factory_mode()
|
||||
or (_explicit_routing() and not _force_local_mode())
|
||||
or has_cloud_credentials(config)
|
||||
)
|
||||
|
||||
# Why: forwarding project_id (external UUID) routes the per-project search
|
||||
# through get_project_client's UUID branch, which treats unknown
|
||||
# identifiers as cloud and 401s when local OAuth tokens linger from a
|
||||
# past `bm cloud login` even though the project itself is local.
|
||||
# Outcome: route by `project` name (qualified_name like "workspace/project"
|
||||
# in cloud refs, plain name in local refs) — both backends resolve
|
||||
# that without UUID lookup. project_id is only used as a fallback when
|
||||
# a ref unexpectedly has no name to route by.
|
||||
for project_ref in project_refs:
|
||||
recursive_project_id = project_ref["project_id"] if use_cloud_routing else None
|
||||
project_name = project_ref["project"]
|
||||
recursive_project_id = None if project_name else project_ref["project_id"]
|
||||
try:
|
||||
results = await search_notes(
|
||||
query=query,
|
||||
project=project_ref["project"],
|
||||
project=project_name,
|
||||
project_id=recursive_project_id,
|
||||
page=1,
|
||||
page_size=per_project_page_size,
|
||||
|
||||
@@ -8,38 +8,8 @@ import pytest
|
||||
from basic_memory.schemas.search import SearchItemType, SearchResponse, SearchResult
|
||||
|
||||
|
||||
def _stub_routing_mode(monkeypatch, *, cloud: bool) -> None:
|
||||
"""Pin the three cloud-route signals search.py reads.
|
||||
|
||||
`_search_all_projects` only forwards project_id (external UUID) when a
|
||||
cloud route is available. The composite mirrors get_project_client:
|
||||
factory mode OR explicit --cloud OR has_cloud_credentials. Tests stub
|
||||
all three so a dev box with OAuth tokens on disk can't bleed into the
|
||||
local-mode case.
|
||||
"""
|
||||
search_mod = importlib.import_module("basic_memory.mcp.tools.search")
|
||||
monkeypatch.setattr(search_mod, "is_factory_mode", lambda: False)
|
||||
monkeypatch.setattr(search_mod, "_explicit_routing", lambda: cloud)
|
||||
monkeypatch.setattr(search_mod, "_force_local_mode", lambda: False)
|
||||
monkeypatch.setattr(search_mod, "has_cloud_credentials", lambda config: cloud)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def cloud_routing(monkeypatch):
|
||||
"""Force the cloud-routing path for multi-project search tests."""
|
||||
_stub_routing_mode(monkeypatch, cloud=True)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def local_routing(monkeypatch):
|
||||
"""Force the local-routing path for multi-project search tests."""
|
||||
_stub_routing_mode(monkeypatch, cloud=False)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_search_notes_search_all_projects_qualifies_result_permalinks(
|
||||
monkeypatch, cloud_routing
|
||||
):
|
||||
async def test_search_notes_search_all_projects_qualifies_result_permalinks(monkeypatch):
|
||||
"""Multi-project search belongs to search_notes and keeps result ids routable."""
|
||||
clients_mod = importlib.import_module("basic_memory.mcp.clients")
|
||||
search_mod = importlib.import_module("basic_memory.mcp.tools.search")
|
||||
@@ -62,7 +32,10 @@ async def test_search_notes_search_all_projects_qualifies_result_permalinks(
|
||||
class StubProject:
|
||||
def __init__(self, name: str | None, external_id: str | None):
|
||||
self.name = name or "main"
|
||||
self.external_id = external_id or "local-main"
|
||||
# When the fan-out routes by name only, project_id is None — keep
|
||||
# the test stable by falling back to the name so the downstream
|
||||
# SearchClient still has a stable per-project identifier.
|
||||
self.external_id = external_id or self.name
|
||||
|
||||
@asynccontextmanager
|
||||
async def fake_get_project_client(project=None, context=None, project_id=None):
|
||||
@@ -74,10 +47,12 @@ async def test_search_notes_search_all_projects_qualifies_result_permalinks(
|
||||
|
||||
class MockSearchClient:
|
||||
def __init__(self, client, project_id):
|
||||
# The fake project supplies the workspace-qualified name as the
|
||||
# external_id, so each per-project search keys off that.
|
||||
self.project_id = project_id
|
||||
|
||||
async def search(self, payload, page, page_size):
|
||||
if self.project_id == "11111111-1111-1111-1111-111111111111":
|
||||
if self.project_id == "personal/main":
|
||||
title = "Personal MCP Test Note"
|
||||
score = 0.5
|
||||
else:
|
||||
@@ -111,9 +86,13 @@ async def test_search_notes_search_all_projects_qualifies_result_permalinks(
|
||||
)
|
||||
|
||||
assert isinstance(result, dict)
|
||||
# Fan-out now routes by qualified_name only; project_id is omitted because
|
||||
# the workspace-qualified name is already unambiguous and UUID routing
|
||||
# hits get_project_client's cloud branch even for local projects when
|
||||
# local OAuth credentials are present.
|
||||
assert searched_projects == [
|
||||
("personal/main", "11111111-1111-1111-1111-111111111111"),
|
||||
("team-paul/main", "22222222-2222-2222-2222-222222222222"),
|
||||
("personal/main", None),
|
||||
("team-paul/main", None),
|
||||
]
|
||||
assert [item["permalink"] for item in result["results"]] == [
|
||||
"team-paul/main/tests/mcp-test-note",
|
||||
@@ -198,9 +177,7 @@ async def test_search_notes_search_all_projects_with_no_refs_returns_empty_all_p
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_search_notes_search_all_projects_continues_after_project_failure(
|
||||
monkeypatch, cloud_routing
|
||||
):
|
||||
async def test_search_notes_search_all_projects_continues_after_project_failure(monkeypatch):
|
||||
"""One failing project should not discard successful all-project search results."""
|
||||
clients_mod = importlib.import_module("basic_memory.mcp.clients")
|
||||
search_mod = importlib.import_module("basic_memory.mcp.tools.search")
|
||||
@@ -223,7 +200,7 @@ async def test_search_notes_search_all_projects_continues_after_project_failure(
|
||||
class StubProject:
|
||||
def __init__(self, name: str | None, external_id: str | None):
|
||||
self.name = name or "main"
|
||||
self.external_id = external_id or "local-main"
|
||||
self.external_id = external_id or self.name
|
||||
|
||||
@asynccontextmanager
|
||||
async def fake_get_project_client(project=None, context=None, project_id=None):
|
||||
@@ -244,10 +221,12 @@ async def test_search_notes_search_all_projects_continues_after_project_failure(
|
||||
|
||||
class MockSearchClient:
|
||||
def __init__(self, client, project_id):
|
||||
# Fan-out routes by name now, so the stub project reflects the
|
||||
# workspace-qualified name back as the SearchClient's project_id.
|
||||
self.project_id = project_id
|
||||
|
||||
async def search(self, payload, page, page_size):
|
||||
if self.project_id == "22222222-2222-2222-2222-222222222222":
|
||||
if self.project_id == "team-paul/main":
|
||||
raise RuntimeError("team index unavailable")
|
||||
return SearchResponse(
|
||||
results=[
|
||||
@@ -287,15 +266,15 @@ async def test_search_notes_search_all_projects_continues_after_project_failure(
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_search_notes_search_all_projects_local_omits_project_id(
|
||||
monkeypatch, local_routing
|
||||
):
|
||||
"""Without a cloud route, fan-out must address each project by name only.
|
||||
async def test_search_notes_search_all_projects_omits_project_id(monkeypatch):
|
||||
"""Fan-out must address each project by name, never by external UUID.
|
||||
|
||||
project_id (external UUID) routes through the cloud v2 API path, which
|
||||
returns 401 on local installs because there's no JWT to present. Local
|
||||
fan-out has to fall back to the name-routed path so each per-project
|
||||
search actually returns results instead of silently failing.
|
||||
project_id routes through get_project_client's UUID branch, which treats
|
||||
any unknown identifier as cloud — so when a local install still has OAuth
|
||||
tokens from a past `bm cloud login`, every per-project recursive call 401s
|
||||
and the merged result list silently stays empty. Routing by name avoids
|
||||
that path on both backends; cloud refs disambiguate via the
|
||||
workspace/project qualified_name already baked into project_ref["project"].
|
||||
"""
|
||||
clients_mod = importlib.import_module("basic_memory.mcp.clients")
|
||||
search_mod = importlib.import_module("basic_memory.mcp.tools.search")
|
||||
|
||||
Reference in New Issue
Block a user