From 47736bae45edd22c72fed9cfded03564d71de6af Mon Sep 17 00:00:00 2001 From: phernandez Date: Mon, 31 Aug 2026 16:13:56 -0500 Subject: [PATCH 01/18] feat(mcp): route project-qualified paths in the POSIX tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Projects become mount points (#1415): every posix verb accepts /path — exactly the prefixed identifiers tool outputs and stored permalinks produce — resolved by one shared helper the CLI inherits. Explicit project params win only on agreement; disagreement refuses naming both. ls with no project lists active projects as the root directory. In multi-project configs an unqualified path that matches no project refuses with the copyable project list; single project configs keep resolving unqualified paths unchanged. Motivated by measured agent behavior in the #1398 A/B runs: agents faithfully quote prefixed output identifiers, omit the project arg (shell affordances prime cwd thinking), and the stateless default was silently wrong — both surfaces wrote a perfect relation into the wrong project. Known scope gap for cloud/factory-mode surfaces noted in review; follow-up tracked on #1415. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/cli/commands/posix.py | 64 +++-- src/basic_memory/mcp/project_context.py | 176 ++++++++++++ src/basic_memory/mcp/tools/posix_tools.py | 147 ++++++++-- tests/cli/test_cli_posix_verbs.py | 120 +++++++++ tests/mcp/conftest.py | 34 +++ tests/mcp/test_project_path_routing.py | 281 +++++++++++++++++++ tests/mcp/test_tool_posix.py | 314 ++++++++++++++++++++++ 7 files changed, 1094 insertions(+), 42 deletions(-) create mode 100644 tests/mcp/test_project_path_routing.py diff --git a/src/basic_memory/cli/commands/posix.py b/src/basic_memory/cli/commands/posix.py index 6586d3113..6cb2b6bc1 100644 --- a/src/basic_memory/cli/commands/posix.py +++ b/src/basic_memory/cli/commands/posix.py @@ -46,6 +46,10 @@ _validate_output_flags, console, ) + +# project_context is already loaded at CLI import time (command_utils imports +# it), so this costs nothing beyond the deferred-MCP budget (#886). +from basic_memory.mcp.project_context import resolve_project_path_route from basic_memory.schemas.directory import DEFAULT_DIRECTORY_PAGE_SIZE # MCP tool functions are imported inside each command: importing @@ -358,10 +362,16 @@ def _add_tree_branches(branch: Tree, entries: dict[str, _TreeEntry]) -> None: _add_tree_branches(child, entry.children) -def _display_tree(result: dict[str, Any], path: str) -> None: - """Render find results as a Rich tree rooted at the search path.""" - entries = _build_tree(list(result.get("nodes", [])), path) - tree = Tree(f"[bold cyan]{markup_escape(path)}[/bold cyan]") +def _display_tree(result: dict[str, Any], label: str, root: str) -> None: + """Render find results as a Rich tree rooted at the search path. + + ``label`` is the caller's spelling of the root; ``root`` is the routed + project-relative path the node paths actually start with — a qualified + '/dir' input strips its project prefix in the shared tool layer, + so the two differ exactly when the input carried a project prefix (#1415). + """ + entries = _build_tree(list(result.get("nodes", [])), root) + tree = Tree(f"[bold cyan]{markup_escape(label)}[/bold cyan]") if not entries: tree.add("[dim]empty[/dim]") _add_tree_branches(tree, entries) @@ -379,10 +389,14 @@ def _print_plain_tree_level(entries: dict[str, _TreeEntry], depth: int) -> None: _print_plain_tree_level(entry.children, depth + 1) -def _plain_tree(result: dict[str, Any], path: str) -> None: - """Render the tree as two-space-indented lines; pagination note on stderr.""" - print(path) - _print_plain_tree_level(_build_tree(list(result.get("nodes", [])), path), depth=1) +def _plain_tree(result: dict[str, Any], label: str, root: str) -> None: + """Render the tree as two-space-indented lines; pagination note on stderr. + + ``label``/``root`` split as in ``_display_tree``: print the caller's + spelling, strip the routed project-relative root from node paths. + """ + print(label) + _print_plain_tree_level(_build_tree(list(result.get("nodes", [])), root), depth=1) if result.get("has_more") is True: print(f"… more entries (page {result.get('page', 1)}; use --page)", file=sys.stderr) @@ -818,25 +832,33 @@ def tree( validate_routing_flags(local, cloud) _validate_output_flags(json_output, plain) - with force_routing(local=local, cloud=cloud): - result = run_with_cleanup( - mcp_find( - path, - name=name, - depth=depth, - page=page, - page_size=page_size, - project=project, - project_id=project_id, - ) + async def _routed_find() -> tuple[dict[str, Any], str]: + # find strips a recognized '/' prefix (#1415), so node + # paths come back project-relative; the same resolver derives the + # root the hierarchy rebuild must strip, or every node's first + # segment would duplicate under a '/dir' root. + route = await resolve_project_path_route(path, project=project, project_id=project_id) + root = f"/{route.path}" if route.stripped else path + listing = await mcp_find( + path, + name=name, + depth=depth, + page=page, + page_size=page_size, + project=project, + project_id=project_id, ) + return listing, root + + with force_routing(local=local, cloud=cloud): + result, root = run_with_cleanup(_routed_find()) mode = _resolve_output_mode(json_output, plain) if mode == "json": _print_json(result) elif mode == "plain": - _plain_tree(result, path) + _plain_tree(result, path, root) else: - _display_tree(result, path) + _display_tree(result, path, root) except (ValueError, ToolError) as e: typer.echo(f"Error: {e}", err=True) raise typer.Exit(1) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index 3aa6e137e..a8264b2ff 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -13,7 +13,9 @@ from __future__ import annotations import asyncio +import os from contextlib import asynccontextmanager, nullcontext +from dataclasses import dataclass from typing import ( TYPE_CHECKING, AsyncIterator, @@ -984,6 +986,180 @@ async def detect_project_from_identifier_prefix( return None +# --- Project-qualified path routing (POSIX tools, #1415) --- +# Projects are mount points: '/path' inputs route to that project so +# tool inputs accept exactly the prefixed identifiers tool outputs and stored +# permalinks produce. One resolver serves the MCP posix tools and, through +# them, the CLI verbs. + + +@dataclass(frozen=True) +class ProjectPathRoute: + """Effective routing for one posix call: project param + project-relative path. + + ``stripped=False`` means the input carried no recognized project prefix: + ``path`` is the caller's input byte-for-byte and ``project`` is the + explicit value that was passed (or None, meaning the existing default + resolution chain applies — only reachable in single-project or empty-config + setups; multi-project configs refuse instead). ``stripped=True`` means a + first-segment project was recognized: ``project`` is the canonical config + name (or workspace-qualified name) and ``path`` is the remainder with no + leading slash, "" meaning the project root. + """ + + project: Optional[str] + path: str + stripped: bool + + +class ProjectPrefixConflictError(ValueError): + """Explicit project param and the path's project prefix name different projects.""" + + +class UnqualifiedPathRefusedError(ValueError): + """Unqualified input in a multi-project config matched no active project.""" + + +def _active_project_prefixes(config: BasicMemoryConfig) -> str: + """Render the configured projects as copyable '/' prefixes.""" + permalinks = sorted(generate_permalink(name) for name in config.projects) + return ", ".join(f"{permalink}/" for permalink in permalinks) + + +def _detected_route_remainder(candidate: str, detected: str) -> str: + """Return the project-relative path left after the detected route prefix. + + A local project consumes one leading segment. A workspace-qualified route + ('/') consumes two only when the candidate spelled both + segments; a bare project prefix resolved into a workspace consumed one. + """ + segments = candidate.split("/") + route_permalinks = generate_permalink(detected).split("/") + consumed = 1 + if ( + len(route_permalinks) == 2 + and len(segments) >= 2 + and [generate_permalink(segment) for segment in segments[:2]] == route_permalinks + ): + consumed = 2 + return "/".join(segments[consumed:]) + + +def _project_routes_agree(detected: str, explicit: str) -> bool: + """True when a detected path prefix and an explicit project name the same project.""" + if generate_permalink(detected) == generate_permalink(explicit): + return True + detected_workspace, detected_project = _split_qualified_project_identifier_impl(detected) + explicit_workspace, explicit_project = _split_qualified_project_identifier_impl(explicit) + # A workspace-qualified spelling agrees with the unqualified spelling of the + # same project; two fully qualified spellings must match exactly (above). + if (detected_workspace is None) == (explicit_workspace is None): + return False + return generate_permalink(detected_project) == generate_permalink(explicit_project) + + +async def resolve_project_path_route( + path: str, + *, + project: Optional[str], + project_id: Optional[str], + context: Optional[Context] = None, +) -> ProjectPathRoute: + """Resolve a posix tool's path/identifier into an effective project route. + + First-segment project resolution (#1415), in order: + + 1. ``project_id`` (UUID) bypasses parsing entirely — comparing a path + prefix against a UUID would need an API round-trip. + 2. An explicit project (the ``BASIC_MEMORY_MCP_PROJECT`` constraint, else + the ``project`` param) wins: an agreeing path prefix is stripped, a + disagreeing one raises ProjectPrefixConflictError — never silently + preferring either. Agreement keeps the more-qualified spelling: an + explicit '/' outlives a bare local prefix match. + 3. Otherwise a first segment naming an active project routes there with + the remainder as the project-relative path. + 4. Otherwise, with more than one configured project, raise + UnqualifiedPathRefusedError instead of silently defaulting; with at + most one configured project, keep today's default resolution. + """ + if project_id is not None: + return ProjectPathRoute(project=project, path=path, stripped=False) + + # The env constraint is ProjectResolver's priority 1, so it participates in + # agree/strip and conflict exactly like the param it outranks. + explicit = os.environ.get("BASIC_MEMORY_MCP_PROJECT") or project + config = ConfigManager().config + candidate = normalize_project_reference(_identifier_path(path)).strip("/") + + detected: Optional[str] = None + remainder = "" + if "/" in candidate: + detected = await detect_project_from_identifier_prefix(candidate, config, context=context) + if detected is not None: + remainder = _detected_route_remainder(candidate, detected) + elif candidate: + # A single segment can name a project alone (ls "research" lists that + # project's root); split_project_prefix requires a slash, so match here. + candidate_permalink = generate_permalink(candidate) + detected = next( + ( + configured_name + for configured_name in config.projects + if generate_permalink(configured_name) == candidate_permalink + ), + None, + ) + + if explicit is not None: + if detected is None: + return ProjectPathRoute( + project=_canonicalize_project_name(explicit, config), path=path, stripped=False + ) + if _project_routes_agree(detected, explicit): + # Trigger: the explicit spelling is workspace-qualified while the + # path prefix matched an unqualified local config name. + # Why: a local project can shadow a same-named project in another + # workspace; dropping the explicitly named workspace would + # silently reroute the call to the local shadow. + # Outcome: the more-qualified explicit spelling carries the route; + # every other agreement keeps the detected (canonical) spelling. + detected_workspace, _ = _split_qualified_project_identifier_impl(detected) + explicit_workspace, _ = _split_qualified_project_identifier_impl(explicit) + routed = detected + if explicit_workspace is not None and detected_workspace is None: + routed = explicit + return ProjectPathRoute( + project=_canonicalize_project_name(routed, config), + path=remainder, + stripped=True, + ) + raise ProjectPrefixConflictError( + f"path names project '{detected}' but project '{explicit}' was passed — " + f"use '{detected}/' alone, or project='{explicit}' with a " + "project-relative path" + ) + + if detected is not None: + return ProjectPathRoute( + project=_canonicalize_project_name(detected, config), path=remainder, stripped=True + ) + + # Trigger: no explicit project, no recognized prefix, several projects configured. + # Why: the stateless server would otherwise fall back to the default project — + # the measured multi-project failure (#1415) this refusal removes. + # Outcome: a self-teaching error listing every project in copyable prefix form. + if len(config.projects) > 1: + first_segment = candidate.split("/", 1)[0] if candidate else "" + subject = f"no project '{first_segment}'" if first_segment else "no project specified" + raise UnqualifiedPathRefusedError( + f"{subject} — active projects: {_active_project_prefixes(config)}" + ) + + # Single-project ergonomics unchanged; an empty config (cloud-only client) + # keeps API-side default resolution. + return ProjectPathRoute(project=None, path=path, stripped=False) + + @asynccontextmanager async def get_project_client( project: Optional[str] = None, diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index b40b8157f..9bfe4ce0b 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -6,8 +6,22 @@ ``basic_memory.mcp.server`` flips their visibility from the ``enable_posix_tools`` config flag at lifespan startup, so no tool body ever checks config itself. + +Projects are mount points (#1415): when no ``project``/``project_id`` param is +given, a path or identifier whose first segment names an active project routes +there, with the remainder as the project-relative path — inputs accept exactly +the '/path' identifiers tool outputs produce. An explicit project +param plus an agreeing prefix strips the prefix; a disagreeing one refuses +naming both. In multi-project configs an unrecognized first segment refuses +with the active project list rather than silently defaulting. Collision rule: +the project always wins over a same-named top-level folder in the default +project, so that folder is only reachable unqualified in single-project +configs (where there is no ambiguity); the qualified '/folder/...' +form always reaches it. ``man`` is excluded — its ``project`` param names the +manual project, not a data project. """ +import os from typing import Any, Optional from fastmcp import Context @@ -17,11 +31,13 @@ from basic_memory.man import bundled_pages, find_page, parse_page_ref, render_index from basic_memory.mcp.container import get_container from basic_memory.mcp.note_reads import read_note_json_by_external_id -from basic_memory.mcp.project_context import get_project_client +from basic_memory.mcp.project_context import get_project_client, resolve_project_path_route from basic_memory.mcp.server import POSIX_TOOLS_TAG, mcp, set_posix_tools_visibility from basic_memory.schemas.directory import ( DEFAULT_DIRECTORY_PAGE_SIZE, MAX_DIRECTORY_PAGE_SIZE, + DirectoryListResponse, + DirectoryNode, ) from basic_memory.schemas.search import SearchItemType, SearchQuery, SearchRetrievalMode @@ -38,7 +54,7 @@ @mcp.tool( title="Cat", - description="Print a note's content.", + description="Print a note's content. Accepts '/path' identifiers.", tags={POSIX_TOOLS_TAG, "notes"}, annotations={ "title": "Cat", @@ -61,7 +77,8 @@ async def cat( """Print a note's content, optionally sliced by line range, section, or token budget. Args: - identifier: Note title, permalink, or memory:// URL (resolved exactly). + identifier: Note title, permalink, memory:// URL, or '/path' + identifier (resolved exactly). start_line: First line to include (1-indexed, inclusive). end_line: Last line to include (inclusive). Defaults to the last line. section: Heading to slice to: "Decisions", path form "Auth/Decisions" @@ -124,7 +141,15 @@ async def cat( if server_side_slice and (start_line is not None or end_line is not None): lines_param = f"{start_line or 1}-{'' if end_line is None else end_line}" - async with get_project_client(project, context=context, project_id=project_id) as ( + # '/path' identifiers route to their project; route.path is the + # identifier unchanged when no prefix was recognized. + route = await resolve_project_path_route( + identifier, project=project, project_id=project_id, context=context + ) + if route.stripped and not route.path: + raise ValueError(f"cat: '{identifier}' names a project, not a note") + + async with get_project_client(route.project, context=context, project_id=project_id) as ( client, active_project, ): @@ -132,7 +157,7 @@ async def cat( from basic_memory.mcp.clients import KnowledgeClient, ResourceClient knowledge_client = KnowledgeClient(client, active_project.external_id) - entity_id = await knowledge_client.resolve_entity(identifier, strict=True) + entity_id = await knowledge_client.resolve_entity(route.path, strict=True) payload: dict[str, Any] = dict( await read_note_json_by_external_id( knowledge_client=knowledge_client, @@ -173,7 +198,7 @@ def _grep_retrieval_mode(literal: bool) -> SearchRetrievalMode: @mcp.tool( title="Grep", - description="Search note content for a pattern.", + description="Search note content for a pattern. Multi-project configs require 'project'.", tags={POSIX_TOOLS_TAG, "search"}, annotations={ "title": "Grep", @@ -198,7 +223,7 @@ async def grep( literal: Force literal full-text matching instead of semantic search. page: Page number (1-indexed). page_size: Results per page. - project: Project name. Optional - the server resolves the default. + project: Project name. Required when more than one project is configured. project_id: Project external_id (UUID); takes precedence over `project`. context: Optional FastMCP context. @@ -212,12 +237,19 @@ async def grep( if page_size < 1: raise ValueError(f"page_size must be >= 1, got {page_size}") + # grep's pattern is never parsed as a path — search text like "error/timeout" + # must not be mistaken for a mount. Routing participates for the refusal rule + # only: unqualified multi-project calls fail loudly instead of defaulting. + route = await resolve_project_path_route( + "", project=project, project_id=project_id, context=context + ) + query = SearchQuery( text=pattern, retrieval_mode=_grep_retrieval_mode(literal), entity_types=[SearchItemType.ENTITY], ) - async with get_project_client(project, context=context, project_id=project_id) as ( + async with get_project_client(route.project, context=context, project_id=project_id) as ( client, active_project, ): @@ -229,9 +261,46 @@ async def grep( return response.model_dump(mode="json", exclude_none=True) +async def _project_mount_listing(*, page: int, page_size: int) -> dict[str, Any]: + """Render the active projects as directory entries (the mount-point view). + + Reuses list_memory_projects' stdio enumeration path: in-process ASGI + locally, the same call over HTTP in global cloud mode. Each row's + ``directory_path`` is the copyable '/' prefix form. + """ + # Import here to avoid circular import + from basic_memory.mcp.async_client import get_client + from basic_memory.mcp.clients import ProjectClient + + async with get_client() as client: + project_list = await ProjectClient(client).list_projects() + + rows = sorted( + ( + DirectoryNode( + name=item.name, + directory_path=f"/{item.permalink}", + permalink=item.permalink, + type="directory", + ) + for item in project_list.projects + ), + key=lambda node: node.name, + ) + start = (page - 1) * page_size + listing = DirectoryListResponse( + nodes=rows[start : start + page_size], + page=page, + page_size=page_size, + total=len(rows), + has_more=start + page_size < len(rows), + ) + return listing.model_dump(mode="json") + + @mcp.tool( title="Ls", - description="List one directory level.", + description="List one directory level. '/' lists projects; paths accept '/path'.", tags={POSIX_TOOLS_TAG, "navigation"}, annotations={ "title": "Ls", @@ -251,10 +320,13 @@ async def ls( """List the immediate contents of one directory. Args: - path: Directory path to list (default: project root). + path: Directory path to list. '/' (the default) with no project param + lists the active projects as mount points; '/path' routes + into that project. page: Page number (1-indexed). page_size: Nodes per page. - project: Project name. Optional - the server resolves the default. + project: Project name. Optional - '/' lists projects; qualified paths + route themselves; multi-project configs refuse other unqualified paths. project_id: Project external_id (UUID); takes precedence over `project`. context: Optional FastMCP context. @@ -268,7 +340,24 @@ async def ls( if page_size > MAX_DIRECTORY_PAGE_SIZE: raise ValueError(f"page_size must be <= {MAX_DIRECTORY_PAGE_SIZE}, got {page_size}") - async with get_project_client(project, context=context, project_id=project_id) as ( + # Trigger: bare root with no project addressed (param, UUID, or env constraint). + # Why: the mount-point view puts project discovery in-band — ls "/" shows the + # mount table, ls "" shows that project's root (#1415). + # Outcome: list the active projects as directory entries; no project client. + if ( + project is None + and project_id is None + and not os.environ.get("BASIC_MEMORY_MCP_PROJECT") + and not path.strip().strip("/") + ): + return await _project_mount_listing(page=page, page_size=page_size) + + route = await resolve_project_path_route( + path, project=project, project_id=project_id, context=context + ) + list_path = f"/{route.path}" if route.stripped else path + + async with get_project_client(route.project, context=context, project_id=project_id) as ( client, active_project, ): @@ -276,13 +365,13 @@ async def ls( from basic_memory.mcp.clients import DirectoryClient directory_client = DirectoryClient(client, active_project.external_id) - listing = await directory_client.list(path, depth=1, page=page, page_size=page_size) + listing = await directory_client.list(list_path, depth=1, page=page, page_size=page_size) return listing.model_dump(mode="json") @mcp.tool( title="Find", - description="Recursively list files matching a name glob.", + description="Recursively list files matching a name glob. Paths accept '/path'.", tags={POSIX_TOOLS_TAG, "navigation"}, annotations={ "title": "Find", @@ -304,12 +393,14 @@ async def find( """Recursively list files under a directory, optionally filtered by name glob. Args: - path: Directory to start from (default: project root). + path: Directory to start from (default: project root). '/path' + routes into that project. name: File-name glob to match, e.g. "*.md". None matches everything. depth: How many levels to recurse (1-10, default: 10). page: Page number (1-indexed). page_size: Nodes per page. - project: Project name. Optional - the server resolves the default. + project: Project name. Optional - qualified paths route themselves; + multi-project configs refuse unqualified paths. project_id: Project external_id (UUID); takes precedence over `project`. context: Optional FastMCP context. @@ -325,7 +416,15 @@ async def find( if page_size > MAX_DIRECTORY_PAGE_SIZE: raise ValueError(f"page_size must be <= {MAX_DIRECTORY_PAGE_SIZE}, got {page_size}") - async with get_project_client(project, context=context, project_id=project_id) as ( + # The directory API is project-scoped, so cross-project find does not exist: + # find "/" with no project in a multi-project config refuses, teaching the + # per-project '/path' form instead. + route = await resolve_project_path_route( + path, project=project, project_id=project_id, context=context + ) + list_path = f"/{route.path}" if route.stripped else path + + async with get_project_client(route.project, context=context, project_id=project_id) as ( client, active_project, ): @@ -334,7 +433,7 @@ async def find( directory_client = DirectoryClient(client, active_project.external_id) listing = await directory_client.list( - path, + list_path, depth=depth, file_name_glob=name, page=page, @@ -345,7 +444,7 @@ async def find( @mcp.tool( title="Tail", - description="Show recently changed notes.", + description="Show recently changed notes. Multi-project configs require 'project'.", tags={POSIX_TOOLS_TAG, "navigation", "notes"}, annotations={ "title": "Tail", @@ -366,7 +465,7 @@ async def tail( Args: timeframe: Time window, e.g. "7d", "yesterday", "2 days ago". lines: Maximum number of rows to return (1-100). - project: Project name. Optional - the server resolves the default. + project: Project name. Required when more than one project is configured. project_id: Project external_id (UUID); takes precedence over `project`. context: Optional FastMCP context. @@ -378,7 +477,13 @@ async def tail( if lines > _MAX_TAIL_LINES: raise ValueError(f"lines must be <= {_MAX_TAIL_LINES}, got {lines}") - async with get_project_client(project, context=context, project_id=project_id) as ( + # tail has no path to carry a project prefix, so routing participates for + # the refusal rule only: unqualified multi-project calls fail loudly. + route = await resolve_project_path_route( + "", project=project, project_id=project_id, context=context + ) + + async with get_project_client(route.project, context=context, project_id=project_id) as ( client, active_project, ): diff --git a/tests/cli/test_cli_posix_verbs.py b/tests/cli/test_cli_posix_verbs.py index 03dda4601..175d61aab 100644 --- a/tests/cli/test_cli_posix_verbs.py +++ b/tests/cli/test_cli_posix_verbs.py @@ -19,6 +19,11 @@ from typer.testing import CliRunner from basic_memory.cli.main import app as cli_app +from basic_memory.config_models import ProjectEntry +from basic_memory.mcp.project_context import ( + ProjectPrefixConflictError, + UnqualifiedPathRefusedError, +) runner = CliRunner() @@ -139,6 +144,23 @@ def _dir_node(**overrides): LS_RESULT_MORE = {**LS_RESULT, "has_more": True} +# The mount-point view (#1415): ls "/" with no project addressed returns the +# active projects as bare directory nodes whose directory_path is the copyable +# '/' prefix form. Same DirectoryListResponse contract as any listing, +# so the existing renderers must handle it untouched. +LS_MOUNT_RESULT = { + "nodes": [ + _dir_node(name="main", directory_path="/main", permalink="main", type="directory"), + _dir_node( + name="research", directory_path="/research", permalink="research", type="directory" + ), + ], + "page": 1, + "page_size": 10, + "total": 2, + "has_more": False, +} + FIND_RESULT = { "nodes": [ _dir_node(name="specs", directory_path="/specs", type="directory"), @@ -598,6 +620,29 @@ def test_ls_defaults_and_paging_passthrough(mock_ls): assert mock_ls.call_args.kwargs["page_size"] == 50 +@patch("basic_memory.mcp.tools.ls", new_callable=AsyncMock, return_value=LS_MOUNT_RESULT) +def test_ls_plain_mount_view_prints_copyable_prefixes(mock_ls): + """bm ls with no project emits one '//' line per mount, so the + prefix form the routing rules teach is copyable straight from the output.""" + result = _tty_invoke(["ls", "--plain"]) + + assert result.exit_code == 0, result.output + assert result.stdout == "/main/\n/research/\n" + + +@patch("basic_memory.mcp.tools.ls", new_callable=AsyncMock, return_value=LS_MOUNT_RESULT) +def test_ls_rich_mount_view_renders_bare_directory_nodes(mock_ls): + """Mount rows carry no title/updated_at; the rich renderer shows them fine.""" + result = _tty_invoke(["ls"]) + + flat = _flattened(result.output) + assert result.exit_code == 0, result.output + _assert_not_json(result.output) + assert "main/" in result.output + assert "research/" in result.output + assert "total 2" in flat + + # --------------------------------------------------------------------------- # find # --------------------------------------------------------------------------- @@ -771,6 +816,46 @@ def test_tree_passes_find_arguments_through(mock_find): assert mock_find.call_args.kwargs["depth"] == 2 +# The tool layer strips a recognized '/' prefix (#1415), so a +# qualified tree root gets back PROJECT-RELATIVE node paths. +TREE_QUALIFIED_RESULT = { + "nodes": [ + _dir_node(name="notes", directory_path="/notes", type="directory"), + _dir_node(name="foo.md", file_path="notes/foo.md", directory_path="/notes/foo.md"), + ], + "page": 1, + "page_size": 10, + "total": 2, + "has_more": False, +} + + +@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=TREE_QUALIFIED_RESULT) +def test_tree_qualified_path_matches_project_flag_hierarchy( + mock_find, config_manager, tmp_path_factory +): + """'bm tree /dir' and 'bm tree /dir --project ' are the + same call by rule 2, so they must render the same hierarchy: the rebuild + strips the routed project-relative root, not the caller's qualified + spelling, or the first path segment duplicates under the root (#1415).""" + config = config_manager.load_config() + config.projects["second-project"] = ProjectEntry( + path=str(tmp_path_factory.mktemp("second-project-cli")) + ) + config_manager.save_config(config) + + qualified = _tty_invoke(["tree", "second-project/notes", "--plain"]) + flagged = _tty_invoke(["tree", "/notes", "--project", "second-project", "--plain"]) + + assert qualified.exit_code == 0, qualified.output + assert flagged.exit_code == 0, flagged.output + assert qualified.stdout == "second-project/notes\n foo.md\n" + # Only the printed root label may differ between the equivalent spellings. + assert qualified.stdout.splitlines()[1:] == flagged.stdout.splitlines()[1:] + # The tool still receives the caller's spelling; routing stays in the tool layer. + assert mock_find.call_args_list[0].args == ("second-project/notes",) + + # --------------------------------------------------------------------------- # Errors and routing (shared command scaffold, exercised per verb) # --------------------------------------------------------------------------- @@ -799,6 +884,41 @@ def test_verb_value_error_exits_nonzero(mock_cat): assert "Error: start_line must be >= 1" in result.output +@patch( + "basic_memory.mcp.tools.ls", + new_callable=AsyncMock, + side_effect=UnqualifiedPathRefusedError( + "no project 'notes' — active projects: main/, research/" + ), +) +def test_verb_refusal_reaches_stderr_with_exit_1(mock_ls): + """The multi-project refusal (#1415) is a ValueError subclass, so the + existing verb error mapping delivers its self-teaching message unchanged.""" + result = _invoke(["ls", "/notes"]) + + assert result.exit_code == 1 + assert "Error: no project 'notes' — active projects: main/, research/" in result.stderr + + +@patch( + "basic_memory.mcp.tools.cat", + new_callable=AsyncMock, + side_effect=ProjectPrefixConflictError( + "path names project 'research' but project 'main' was passed — use " + "'research/' alone, or project='main' with a project-relative path" + ), +) +def test_project_flag_with_conflicting_prefix_exits_nonzero(mock_cat): + """--project plus a qualified path passes both through verbatim; the tool + layer owns the conflict decision and the CLI just reports it.""" + result = _invoke(["cat", "research/notes/foo", "--project", "main"]) + + assert result.exit_code == 1 + assert mock_cat.call_args.args == ("research/notes/foo",) + assert mock_cat.call_args.kwargs["project"] == "main" + assert "Error: path names project 'research' but project 'main' was passed" in result.stderr + + @patch("basic_memory.mcp.tools.cat", new_callable=AsyncMock, return_value=CAT_RESULT) def test_local_and_cloud_together_errors(mock_cat): result = _invoke(["cat", "specs/search", "--local", "--cloud"]) diff --git a/tests/mcp/conftest.py b/tests/mcp/conftest.py index 7d53b946c..40577b239 100644 --- a/tests/mcp/conftest.py +++ b/tests/mcp/conftest.py @@ -9,8 +9,12 @@ from fastmcp import FastMCP from httpx import AsyncClient, ASGITransport +from basic_memory import db from basic_memory.api.app import app as fastapi_app +from basic_memory.config_models import ProjectEntry from basic_memory.deps import get_engine_factory, get_app_config +from basic_memory.models.project import Project +from basic_memory.repository.project_repository import ProjectRepository from basic_memory.services.search_service import SearchService from basic_memory.mcp.server import mcp as mcp_server @@ -90,3 +94,33 @@ def test_entity_data(): async def init_search_index(search_service: SearchService): """Initialize search index. Request this fixture explicitly in tests that need it.""" await search_service.init_search_index() + + +@pytest_asyncio.fixture +async def second_project(config_manager, engine_factory, tmp_path_factory) -> Project: + """A second active project (DB row + config entry) for multi-project routing tests. + + Both halves matter: the API resolves projects from the database, while the + MCP client layer routes by the local config — a project absent from config + routes CLOUD by default and dies without credentials. The path lives outside + config_home because test-project's path IS config_home; nesting one project + inside another would make its files ambiguous between the two. + """ + project_path = tmp_path_factory.mktemp("second-project-home") + _, session_maker = engine_factory + async with db.scoped_session(session_maker) as session: + project = await ProjectRepository().create( + session, + { + "name": "second-project", + "description": "Second project for multi-project routing tests", + "path": str(project_path), + "is_active": True, + "is_default": False, + }, + ) + + config = config_manager.load_config() + config.projects["second-project"] = ProjectEntry(path=str(project_path)) + config_manager.save_config(config) + return project diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py new file mode 100644 index 000000000..42150e8a4 --- /dev/null +++ b/tests/mcp/test_project_path_routing.py @@ -0,0 +1,281 @@ +"""Direct unit tests for resolve_project_path_route (#1415). + +The resolver is the single routing seam for the posix tools: '/path' +inputs route by first segment, an explicit project must agree with a path +prefix, and multi-project configs refuse unqualified input instead of +defaulting. These tests drive the function branch-by-branch against configs +written through the test ConfigManager; no API client is involved because +local-config matching never leaves the process. +""" + +import re + +import pytest + +from basic_memory.config_models import ProjectEntry +from basic_memory.mcp.project_context import ( + ProjectPathRoute, + ProjectPrefixConflictError, + UnqualifiedPathRefusedError, + _detected_route_remainder, + _project_routes_agree, + resolve_project_path_route, +) + + +@pytest.fixture(autouse=True) +def clean_project_env(monkeypatch): + """The env constraint acts as an explicit project; clear it by default.""" + monkeypatch.delenv("BASIC_MEMORY_MCP_PROJECT", raising=False) + + +@pytest.fixture +def multi_project_config(config_manager, tmp_path_factory): + """Three local projects, one with a spaced display name for permalink tests.""" + config = config_manager.load_config() + config.projects["second-project"] = ProjectEntry( + path=str(tmp_path_factory.mktemp("second-project-config")) + ) + config.projects["My Research"] = ProjectEntry( + path=str(tmp_path_factory.mktemp("my-research-config")) + ) + config_manager.save_config(config) + return config_manager + + +@pytest.fixture +def empty_project_config(config_manager): + """A config with no projects at all (cloud-only local client).""" + config = config_manager.load_config() + config.projects = {} + config_manager.save_config(config) + return config_manager + + +# --- project_id passthrough --- + + +@pytest.mark.asyncio +async def test_project_id_bypasses_prefix_parsing(multi_project_config): + """project_id routes by external UUID, so even a conflicting-looking prefix + is never examined — documented limitation, mirroring read_note.""" + route = await resolve_project_path_route( + "second-project/notes/foo", + project="test-project", + project_id="11111111-1111-1111-1111-111111111111", + ) + + assert route == ProjectPathRoute( + project="test-project", path="second-project/notes/foo", stripped=False + ) + + +# --- rule 1: first-segment routing --- + + +@pytest.mark.asyncio +async def test_first_segment_routes_with_remainder(multi_project_config): + route = await resolve_project_path_route( + "second-project/notes/foo", project=None, project_id=None + ) + + assert route == ProjectPathRoute(project="second-project", path="notes/foo", stripped=True) + + +@pytest.mark.asyncio +async def test_single_segment_project_names_project_root(multi_project_config): + """A bare project name routes with an empty remainder — ls 'second-project' + lists that project's root.""" + route = await resolve_project_path_route("second-project", project=None, project_id=None) + + assert route == ProjectPathRoute(project="second-project", path="", stripped=True) + + +@pytest.mark.asyncio +async def test_memory_url_prefix_routes(multi_project_config): + route = await resolve_project_path_route( + "memory://second-project/notes/foo", project=None, project_id=None + ) + + assert route == ProjectPathRoute(project="second-project", path="notes/foo", stripped=True) + + +@pytest.mark.asyncio +async def test_namespace_syntax_routes(multi_project_config): + """'project::note' normalizes to path syntax before prefix detection.""" + route = await resolve_project_path_route( + "second-project::notes/foo", project=None, project_id=None + ) + + assert route == ProjectPathRoute(project="second-project", path="notes/foo", stripped=True) + + +@pytest.mark.asyncio +async def test_display_name_matches_by_permalink(multi_project_config): + """The permalink form routes to the canonical config name, spaces and all.""" + route = await resolve_project_path_route("my-research/notes/foo", project=None, project_id=None) + + assert route == ProjectPathRoute(project="My Research", path="notes/foo", stripped=True) + + +@pytest.mark.asyncio +async def test_glob_first_segment_never_routes(multi_project_config): + """split_project_prefix's '*' guard: a glob first segment is search input, + not a mount — it falls through to the multi-project refusal.""" + with pytest.raises(UnqualifiedPathRefusedError, match=re.escape("no project 'second-*'")): + await resolve_project_path_route("second-*/notes/foo", project=None, project_id=None) + + +# --- rule 2: explicit project agree/strip/conflict --- + + +@pytest.mark.asyncio +async def test_explicit_project_canonicalized_when_no_prefix(multi_project_config): + """No recognized prefix: the path passes through untouched and the explicit + project is canonicalized to its config spelling.""" + route = await resolve_project_path_route("notes/foo", project="my-research", project_id=None) + + assert route == ProjectPathRoute(project="My Research", path="notes/foo", stripped=False) + + +@pytest.mark.asyncio +async def test_agreeing_prefix_strips_with_explicit_project(multi_project_config): + route = await resolve_project_path_route( + "my-research/notes/foo", project="My Research", project_id=None + ) + + assert route == ProjectPathRoute(project="My Research", path="notes/foo", stripped=True) + + +@pytest.mark.asyncio +async def test_conflicting_prefix_raises_naming_both(multi_project_config): + """The conflict message names both projects and offers both spellings.""" + with pytest.raises(ProjectPrefixConflictError) as excinfo: + await resolve_project_path_route( + "second-project/notes/foo", project="My Research", project_id=None + ) + + assert str(excinfo.value) == ( + "path names project 'second-project' but project 'My Research' was passed — " + "use 'second-project/' alone, or project='My Research' with a " + "project-relative path" + ) + + +# --- workspace-qualified spellings --- +# The remainder/agreement helpers are pure functions; driving the qualified +# spellings through them directly avoids standing up cloud workspace discovery. + + +def test_detected_route_remainder_spelled_workspace_route_consumes_two_segments(): + """A workspace-qualified route spelled as both path segments consumes both.""" + assert _detected_route_remainder("other/research/notes/foo", "other/research") == "notes/foo" + + +def test_detected_route_remainder_bare_prefix_resolved_into_workspace_consumes_one(): + """A bare project prefix that resolved into a workspace consumed one segment.""" + assert _detected_route_remainder("research/notes/foo", "other/research") == "notes/foo" + + +def test_project_routes_agree_across_mixed_qualification(): + """A workspace-qualified spelling agrees with the unqualified spelling of + the same project, in either direction; different projects never agree.""" + assert _project_routes_agree("research", "other/research") + assert _project_routes_agree("other/research", "research") + assert not _project_routes_agree("second-project", "other/research") + + +@pytest.mark.asyncio +async def test_explicit_workspace_qualified_project_survives_local_prefix_agreement( + multi_project_config, +): + """An explicit '/' param carries the route even when the + path prefix matches a same-named local project — the explicitly named + workspace is never silently swapped for the local shadow.""" + route = await resolve_project_path_route( + "second-project/notes/foo", project="other/second-project", project_id=None + ) + + assert route == ProjectPathRoute( + project="other/second-project", path="notes/foo", stripped=True + ) + + +# --- rule 4: multi-project refusal messages --- + + +@pytest.mark.asyncio +async def test_unqualified_path_refusal_lists_projects_sorted(multi_project_config): + with pytest.raises(UnqualifiedPathRefusedError) as excinfo: + await resolve_project_path_route("notes/foo", project=None, project_id=None) + + assert str(excinfo.value) == ( + "no project 'notes' — active projects: my-research/, second-project/, test-project/" + ) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("path", ["", "/"]) +async def test_empty_input_refuses_with_no_project_specified(multi_project_config, path): + """grep/tail (no path) and a bare root land here in multi-project configs.""" + with pytest.raises(UnqualifiedPathRefusedError) as excinfo: + await resolve_project_path_route(path, project=None, project_id=None) + + assert str(excinfo.value) == ( + "no project specified — active projects: my-research/, second-project/, test-project/" + ) + + +# --- single-project and empty-config passthrough --- + + +@pytest.mark.asyncio +@pytest.mark.parametrize("path", ["notes/foo", "test/root", "", "some-title"]) +async def test_single_project_input_passes_through_unchanged(config_manager, path): + """One configured project keeps today's default resolution; 'test' is not + falsely stripped as a prefix of 'test-project' (permalink comparison).""" + route = await resolve_project_path_route(path, project=None, project_id=None) + + assert route == ProjectPathRoute(project=None, path=path, stripped=False) + + +@pytest.mark.asyncio +async def test_empty_config_passes_through(empty_project_config): + """A cloud-only local client (no local mount table) keeps API-side default + resolution — refusal needs a config that can enumerate projects.""" + route = await resolve_project_path_route("anything/x", project=None, project_id=None) + + assert route == ProjectPathRoute(project=None, path="anything/x", stripped=False) + + +# --- env constraint as effective explicit project --- + + +@pytest.mark.asyncio +async def test_env_constraint_agreeing_prefix_strips(multi_project_config, monkeypatch): + monkeypatch.setenv("BASIC_MEMORY_MCP_PROJECT", "second-project") + + route = await resolve_project_path_route( + "second-project/notes/foo", project=None, project_id=None + ) + + assert route == ProjectPathRoute(project="second-project", path="notes/foo", stripped=True) + + +@pytest.mark.asyncio +async def test_env_constraint_conflicting_prefix_refuses(multi_project_config, monkeypatch): + monkeypatch.setenv("BASIC_MEMORY_MCP_PROJECT", "test-project") + + with pytest.raises(ProjectPrefixConflictError, match="but project 'test-project' was passed"): + await resolve_project_path_route("second-project/notes/foo", project=None, project_id=None) + + +@pytest.mark.asyncio +async def test_env_constraint_prevents_refusal_for_empty_input(multi_project_config, monkeypatch): + """The constraint is an explicit project: grep/tail-style empty input routes + to it instead of refusing.""" + monkeypatch.setenv("BASIC_MEMORY_MCP_PROJECT", "second-project") + + route = await resolve_project_path_route("", project=None, project_id=None) + + assert route == ProjectPathRoute(project="second-project", path="", stripped=False) diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index f63f0724a..5400558ae 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -11,9 +11,20 @@ from fastmcp.exceptions import ToolError import basic_memory.mcp.tools.posix_tools as posix_tools +from basic_memory.mcp.project_context import ( + ProjectPrefixConflictError, + UnqualifiedPathRefusedError, +) from basic_memory.mcp.tools import cat, find, grep, ls, man, tail, write_note from basic_memory.schemas.search import SearchRetrievalMode + +@pytest.fixture +def no_project_constraint(monkeypatch): + """Clear the env project constraint so unqualified routing paths are reachable.""" + monkeypatch.delenv("BASIC_MEMORY_MCP_PROJECT", raising=False) + + # --- cat --- @@ -392,6 +403,8 @@ async def test_grep_rejects_bad_arguments(kwargs, message): @pytest.mark.asyncio async def test_ls_root_listing(client, test_graph, test_project): + # New contract (#1415): with no project addressed, ls "/" lists projects as + # mount points — this test pins the project-scoped case via project=. result = await ls(project=test_project.name) assert result["total"] == 1 @@ -417,6 +430,7 @@ async def test_ls_directory_contents(client, test_graph, test_project): @pytest.mark.asyncio async def test_ls_empty_project(client, test_project): + # Project-scoped case (#1415): project= bypasses the mount-point view. result = await ls(project=test_project.name) assert result["total"] == 0 @@ -614,3 +628,303 @@ async def test_man_query_missing_manual_project_raises(client, test_project): # silently searching the wrong project. with pytest.raises(RuntimeError, match="no credentials found"): await man(query="anything") + + +# --- project-qualified routing (#1415) --- +# Projects are mount points: '/path' inputs route to that project, an +# explicit project must agree with a path prefix, and multi-project configs +# refuse unqualified input with the active project list instead of defaulting. + + +# -- ls "/" mount-point view -- + + +@pytest.mark.asyncio +async def test_ls_root_lists_projects_as_mount_points( + client, test_project, second_project, no_project_constraint +): + """ls "/" with no project addressed is the mount table, sorted by name.""" + result = await ls() + + assert result["total"] == 2 + assert result["has_more"] is False + assert [node["name"] for node in result["nodes"]] == ["second-project", "test-project"] + for node in result["nodes"]: + assert node["type"] == "directory" + # directory_path is the copyable '/' prefix form. + assert node["directory_path"] == f"/{node['permalink']}" + + +@pytest.mark.asyncio +async def test_ls_root_mount_view_in_single_project_config( + client, test_graph, test_project, no_project_constraint +): + """The mount view is unconditional (#1415): even a single-project config + lists the mount table at "/" so in-band discovery is uniform.""" + result = await ls() + + assert result["total"] == 1 + assert result["nodes"][0]["name"] == "test-project" + assert result["nodes"][0]["directory_path"] == "/test-project" + assert result["nodes"][0]["type"] == "directory" + + +@pytest.mark.asyncio +async def test_ls_mount_view_paginates_over_project_rows( + client, test_project, second_project, no_project_constraint +): + first = await ls(page=1, page_size=1) + last = await ls(page=2, page_size=1) + + assert first["total"] == 2 + assert first["has_more"] is True + assert [node["name"] for node in first["nodes"]] == ["second-project"] + assert [node["name"] for node in last["nodes"]] == ["test-project"] + assert last["has_more"] is False + + +# -- rule 1: first-segment routing per verb -- + + +@pytest.mark.asyncio +async def test_ls_single_segment_project_name_lists_its_root( + client, test_project, second_project, no_project_constraint +): + await write_note( + title="Second Root Note", + directory="notes", + content="# Second Root Note\n\nsecond project content", + project="second-project", + ) + + result = await ls("second-project") + + names = {node["name"] for node in result["nodes"]} + assert names == {"notes"} + + +@pytest.mark.asyncio +async def test_ls_qualified_path_routes_into_project_directory( + client, test_project, second_project, no_project_constraint +): + await write_note( + title="Second Dir Note", + directory="notes", + content="# Second Dir Note", + project="second-project", + ) + + result = await ls("/second-project/notes") + + names = {node["name"] for node in result["nodes"]} + assert names == {"Second Dir Note.md"} + + +@pytest.mark.asyncio +async def test_find_qualified_path_routes_to_project( + client, test_project, second_project, no_project_constraint +): + await write_note( + title="Second Find Note", + directory="notes", + content="# Second Find Note", + project="second-project", + ) + + result = await find("second-project", name="*.md") + + names = {node["name"] for node in result["nodes"]} + assert names == {"Second Find Note.md"} + + +@pytest.mark.asyncio +async def test_cat_qualified_identifier_equals_explicit_project_read( + client, test_graph, test_project, second_project, no_project_constraint +): + """'test-project/test/root' with no project param reads the same note as + project='test-project' + 'test/root' — inputs accept what outputs produce.""" + qualified = await cat("test-project/test/root") + explicit = await cat("test/root", project=test_project.name) + + assert qualified == explicit + assert qualified["title"] == "Root" + + +@pytest.mark.asyncio +async def test_tool_output_permalink_round_trips_into_cat( + client, test_project, second_project, no_project_constraint +): + """Round trip: stored permalinks are already project-qualified, so tail's + output is a valid cat identifier with no project param anywhere — and the + mount view's prefix is that permalink's first segment.""" + await write_note( + title="Round Trip", + directory="notes", + content="# Round Trip\n\nround trip body", + project="second-project", + ) + + rows = await tail(project="second-project") + permalink = next(row["permalink"] for row in rows if row["title"] == "Round Trip") + assert permalink == "second-project/notes/round-trip" + + mounts = await ls() + mount_prefixes = {node["directory_path"] for node in mounts["nodes"]} + assert f"/{permalink.split('/', 1)[0]}" in mount_prefixes + + result = await cat(permalink) + + assert result["title"] == "Round Trip" + assert "round trip body" in result["content"] + + +# -- rule 2: explicit project + prefix agree/conflict -- + + +@pytest.mark.asyncio +async def test_explicit_project_with_agreeing_prefix_strips( + client, test_graph, test_project, second_project, no_project_constraint +): + qualified = await ls("/test-project/test", project=test_project.name) + relative = await ls("/test", project=test_project.name) + + assert qualified == relative + assert qualified["total"] == 5 + + +@pytest.mark.asyncio +async def test_explicit_project_with_conflicting_prefix_refuses( + client, test_project, second_project, no_project_constraint +): + """A disagreeing prefix is never silently resolved either way.""" + with pytest.raises( + ProjectPrefixConflictError, + match="path names project 'second-project' but project 'test-project' was passed", + ): + await cat("second-project/notes/foo", project=test_project.name) + + +# -- rule 4: multi-project unqualified refusal, self-teaching message -- + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("verb", "kwargs", "subject"), + [ + ("cat", {"identifier": "notes/foo"}, "no project 'notes'"), + ("ls", {"path": "/notes"}, "no project 'notes'"), + ("find", {"path": "/x"}, "no project 'x'"), + ("grep", {"pattern": "needle"}, "no project specified"), + ("tail", {}, "no project specified"), + ], +) +async def test_unqualified_input_refuses_in_multi_project_config( + client, test_project, second_project, no_project_constraint, verb, kwargs, subject +): + """Each verb refuses rather than silently defaulting, listing every active + project in copyable prefix form.""" + tool = {"cat": cat, "ls": ls, "find": find, "grep": grep, "tail": tail}[verb] + + with pytest.raises(UnqualifiedPathRefusedError) as excinfo: + await tool(**kwargs) + + assert str(excinfo.value) == (f"{subject} — active projects: second-project/, test-project/") + + +@pytest.mark.asyncio +async def test_grep_argument_validation_precedes_refusal( + client, test_project, second_project, no_project_constraint +): + """Bad-argument errors keep firing before any routing decision.""" + with pytest.raises(ValueError, match="pattern must not be empty"): + await grep("") + + +# -- rule 5: single-project passthrough and near-collisions -- + + +@pytest.mark.asyncio +async def test_single_project_unqualified_paths_pass_through( + client, test_graph, test_project, no_project_constraint +): + """One configured project keeps today's ergonomics: unqualified paths route + to the default project, and 'test' is not falsely stripped as a prefix of + 'test-project' (permalink comparison, not startswith).""" + listing = await ls("/test") + assert listing["total"] == 5 + + note = await cat("test/root") + assert note["title"] == "Root" + + rows = await tail() + assert {row["title"] for row in rows} >= {"Root"} + + found = await grep("Root", literal=True) + assert found["results"] + + +@pytest.mark.asyncio +async def test_project_named_folder_is_reachable_double_qualified( + client, test_project, second_project, no_project_constraint +): + """Collision rule: the project wins the first segment, so a top-level folder + named like its own project is addressed by double-qualifying.""" + await write_note( + title="Shadowed", + directory="second-project", + content="# Shadowed\n\nshadowed body", + project="second-project", + ) + + listing = await ls("second-project/second-project") + assert {node["name"] for node in listing["nodes"]} == {"Shadowed.md"} + + result = await cat("second-project/second-project/shadowed") + assert result["title"] == "Shadowed" + + +@pytest.mark.asyncio +async def test_cat_bare_project_name_is_an_error( + client, test_project, second_project, no_project_constraint +): + with pytest.raises(ValueError, match="names a project, not a note"): + await cat("second-project") + + +# -- env constraint (BASIC_MEMORY_MCP_PROJECT) -- + + +@pytest.mark.asyncio +async def test_env_constraint_counts_as_explicit_project( + client, test_graph, test_project, second_project, monkeypatch +): + """The env constraint participates exactly like the project param: no + refusal, agreeing prefixes strip, disagreeing prefixes conflict, and + ls "/" lists the constrained project's root, not the mount table.""" + monkeypatch.setenv("BASIC_MEMORY_MCP_PROJECT", test_project.name) + + rows = await tail() + assert {row["title"] for row in rows} >= {"Root"} + + stripped = await ls("/test-project/test") + assert stripped["total"] == 5 + + constrained_root = await ls() + assert {node["name"] for node in constrained_root["nodes"]} == {"test"} + + with pytest.raises(ProjectPrefixConflictError): + await cat("second-project/notes/foo") + + +# -- project_id passthrough -- + + +@pytest.mark.asyncio +async def test_project_id_routes_without_prefix_parsing( + client, test_graph, test_project, second_project, no_project_constraint +): + """project_id routes by external UUID and bypasses prefix parsing entirely, + so a multi-project config needs no qualification.""" + result = await cat("test/root", project_id=test_project.external_id) + + assert result["title"] == "Root" From 72caf4023dd0bcbb49259cf7a3fc90f025a0fbc7 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 09:57:32 -0500 Subject: [PATCH 02/18] fix(mcp): route and refuse from one addressable-project set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mount view and the routing resolver read different sources, so a cloud tenant could advertise /research in ls '/' while routing asked the local config, found nothing, refused nothing, and fell through to the default project. In a team workspace that default is one shared mutable is_default flag, so an unqualified call could silently read or write another member's project. Root cause of why the existing multi-project refusal never fired in the hosted server: BasicMemoryConfig always materializes a placeholder 'main' project, so config.projects is never empty and len(config.projects) > 1 was never a usable signal for a cloud session. Both surfaces now read one addressable_projects() set — config locally, the session's project listing in factory/cloud mode, memoized per MCP request. The refusal counts that same set, so anything ls '/' advertises is addressable, asserted as a property over the advertised list rather than a hand-picked example. Nothing consults is_default. Single-project workspaces still resolve unqualified references, and the local path adds no new call. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 173 ++++++++++++--- src/basic_memory/mcp/tools/posix_tools.py | 88 ++++---- tests/mcp/test_project_path_routing.py | 256 +++++++++++++++++++++- 3 files changed, 435 insertions(+), 82 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index a8264b2ff..fe5abcc1c 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -991,6 +991,11 @@ async def detect_project_from_identifier_prefix( # tool inputs accept exactly the prefixed identifiers tool outputs and stored # permalinks produce. One resolver serves the MCP posix tools and, through # them, the CLI verbs. +# +# The mount table and the routing table are one set (addressable_projects), so +# every mount `ls /` advertises is reachable by name and an unqualified +# reference in a many-project workspace refuses instead of picking a default +# (#1421). @dataclass(frozen=True) @@ -1000,11 +1005,11 @@ class ProjectPathRoute: ``stripped=False`` means the input carried no recognized project prefix: ``path`` is the caller's input byte-for-byte and ``project`` is the explicit value that was passed (or None, meaning the existing default - resolution chain applies — only reachable in single-project or empty-config - setups; multi-project configs refuse instead). ``stripped=True`` means a - first-segment project was recognized: ``project`` is the canonical config - name (or workspace-qualified name) and ``path`` is the remainder with no - leading slash, "" meaning the project root. + resolution chain applies — only reachable when the session addresses at + most one project; several addressable projects refuse instead). + ``stripped=True`` means a first-segment project was recognized: ``project`` + is the canonical config name (or workspace-qualified name) and ``path`` is + the remainder with no leading slash, "" meaning the project root. """ project: Optional[str] @@ -1017,13 +1022,98 @@ class ProjectPrefixConflictError(ValueError): class UnqualifiedPathRefusedError(ValueError): - """Unqualified input in a multi-project config matched no active project.""" + """Unqualified input matched no project in a workspace that addresses several.""" -def _active_project_prefixes(config: BasicMemoryConfig) -> str: - """Render the configured projects as copyable '/' prefixes.""" - permalinks = sorted(generate_permalink(name) for name in config.projects) - return ", ".join(f"{permalink}/" for permalink in permalinks) +@dataclass(frozen=True) +class AddressableProject: + """One project this session can both advertise and route to. + + ``name`` is the routing identifier handed to ``get_project_client``; + ``permalink`` is the first path segment agents copy out of tool output. + """ + + name: str + permalink: str + + +# The session's own project listing, memoized for one MCP request: routing and +# the mount view ask the same question, so a request pays for it at most once. +_SESSION_PROJECT_LIST_STATE_KEY = "session_project_list" + + +def _session_routes_to_cloud() -> bool: + """Return True when this session's project-less client is a tenant route. + + Mirrors ``get_client()``'s own decision for a call that names no project: + factory injection first (the hosted MCP server), then an explicit --cloud + flag. Everything else is the local ASGI app, whose mount table is the local + config. The distinction matters because BasicMemoryConfig always + materializes a placeholder 'main' project, so a non-empty ``config.projects`` + proves nothing about a cloud session's real projects. + """ + from basic_memory.mcp.async_client import ( + _explicit_routing, + _force_local_mode, + is_factory_mode, + ) + + return is_factory_mode() or (_explicit_routing() and not _force_local_mode()) + + +async def _session_project_list(context: Optional[Context] = None) -> ProjectList: + """List the projects reachable through this session's own route.""" + if context: + cached_raw = await context.get_state(_SESSION_PROJECT_LIST_STATE_KEY) + if isinstance(cached_raw, dict): + return ProjectList.model_validate(cached_raw) + + # Deferred imports to avoid circular dependency with the client modules. + from basic_memory.mcp.async_client import get_client + from basic_memory.mcp.clients import ProjectClient + + async with get_client() as client: + project_list = await ProjectClient(client).list_projects() + + if context: + await context.set_state(_SESSION_PROJECT_LIST_STATE_KEY, project_list.model_dump()) + return project_list + + +async def addressable_projects( + context: Optional[Context] = None, +) -> tuple[AddressableProject, ...]: + """Return every project this session can address, sorted by name. + + One source answers two questions that must never disagree: which mounts + ``ls /`` advertises, and which first path segment names a project. When + they came from different sources, a cloud session could advertise + ``/research`` at the root and then fail to recognize ``research/notes/x``, + silently routing it to a default project instead (#1421). + + A locally routed session's config IS its mount table, so it answers with no + network call. A cloud session's projects live in the tenant database and + its local config holds only the placeholder 'main' entry, so the session's + own project listing answers instead — the same call the mount view has + always made to render the root. + """ + if _session_routes_to_cloud(): + project_list = await _session_project_list(context=context) + projects = ( + AddressableProject(name=item.name, permalink=item.permalink) + for item in project_list.projects + ) + else: + projects = ( + AddressableProject(name=name, permalink=generate_permalink(name)) + for name in ConfigManager().config.projects + ) + return tuple(sorted(projects, key=lambda project: project.name)) + + +def _addressable_project_prefixes(projects: tuple[AddressableProject, ...]) -> str: + """Render addressable projects as copyable '/' prefixes.""" + return ", ".join(f"{permalink}/" for permalink in sorted(item.permalink for item in projects)) def _detected_route_remainder(candidate: str, detected: str) -> str: @@ -1076,11 +1166,15 @@ async def resolve_project_path_route( disagreeing one raises ProjectPrefixConflictError — never silently preferring either. Agreement keeps the more-qualified spelling: an explicit '/' outlives a bare local prefix match. - 3. Otherwise a first segment naming an active project routes there with - the remainder as the project-relative path. - 4. Otherwise, with more than one configured project, raise - UnqualifiedPathRefusedError instead of silently defaulting; with at - most one configured project, keep today's default resolution. + 3. Otherwise a first segment naming an addressable project routes there + with the remainder as the project-relative path. + 4. Otherwise, when the session addresses more than one project, raise + UnqualifiedPathRefusedError instead of silently defaulting; a session + that addresses at most one project keeps today's default resolution. + + Rules 3 and 4 read the set from ``addressable_projects`` — the same set + ``ls /`` advertises — so a project can never be listed at the root and then + go unrecognized as a path prefix (#1421). """ if project_id is not None: return ProjectPathRoute(project=project, path=path, stripped=False) @@ -1093,22 +1187,32 @@ async def resolve_project_path_route( detected: Optional[str] = None remainder = "" + addressable: tuple[AddressableProject, ...] | None = None if "/" in candidate: detected = await detect_project_from_identifier_prefix(candidate, config, context=context) if detected is not None: remainder = _detected_route_remainder(candidate, detected) - elif candidate: - # A single segment can name a project alone (ls "research" lists that - # project's root); split_project_prefix requires a slash, so match here. - candidate_permalink = generate_permalink(candidate) - detected = next( - ( - configured_name - for configured_name in config.projects - if generate_permalink(configured_name) == candidate_permalink - ), + + # Trigger: no workspace-qualified route resolved and the input still has a + # leading segment — a bare mount name ('ls research'), or a cloud session + # whose local config holds only the placeholder 'main' entry. + # Why: split_project_prefix needs a slash, and the detection above reads the + # local config and the workspace index; neither can name a project that + # only this session's own listing knows about (#1421). Advertising a mount + # at '/' that a path prefix cannot name is the bug being closed. + # Outcome: a first segment matching an addressable project routes there, + # with the remainder as the project-relative path. + if detected is None and candidate: + addressable = await addressable_projects(context=context) + first_segment, _, remaining_segments = candidate.partition("/") + first_permalink = generate_permalink(first_segment) + mount = next( + (item for item in addressable if item.permalink == first_permalink), None, ) + if mount is not None: + detected = mount.name + remainder = remaining_segments if explicit is not None: if detected is None: @@ -1144,19 +1248,24 @@ async def resolve_project_path_route( project=_canonicalize_project_name(detected, config), path=remainder, stripped=True ) - # Trigger: no explicit project, no recognized prefix, several projects configured. - # Why: the stateless server would otherwise fall back to the default project — - # the measured multi-project failure (#1415) this refusal removes. + # Trigger: no explicit project, no recognized prefix, several addressable projects. + # Why: the stateless server would otherwise fall back to a default project — + # the measured multi-project failure (#1415) this refusal removes. In a + # team workspace that default is one shared mutable is_default flag, so a + # teammate flipping it silently redirects this call, writes included; the + # refusal is what keeps unqualified references from depending on it (#1421). # Outcome: a self-teaching error listing every project in copyable prefix form. - if len(config.projects) > 1: + if addressable is None: + addressable = await addressable_projects(context=context) + if len(addressable) > 1: first_segment = candidate.split("/", 1)[0] if candidate else "" subject = f"no project '{first_segment}'" if first_segment else "no project specified" raise UnqualifiedPathRefusedError( - f"{subject} — active projects: {_active_project_prefixes(config)}" + f"{subject} — active projects: {_addressable_project_prefixes(addressable)}" ) - # Single-project ergonomics unchanged; an empty config (cloud-only client) - # keeps API-side default resolution. + # A session that addresses at most one project has no ambiguity to protect + # against, so unqualified input keeps today's default resolution. return ProjectPathRoute(project=None, path=path, stripped=False) diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index 9bfe4ce0b..5ce192403 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -8,17 +8,21 @@ checks config itself. Projects are mount points (#1415): when no ``project``/``project_id`` param is -given, a path or identifier whose first segment names an active project routes -there, with the remainder as the project-relative path — inputs accept exactly -the '/path' identifiers tool outputs produce. An explicit project -param plus an agreeing prefix strips the prefix; a disagreeing one refuses -naming both. In multi-project configs an unrecognized first segment refuses -with the active project list rather than silently defaulting. Collision rule: -the project always wins over a same-named top-level folder in the default -project, so that folder is only reachable unqualified in single-project -configs (where there is no ambiguity); the qualified '/folder/...' -form always reaches it. ``man`` is excluded — its ``project`` param names the -manual project, not a data project. +given, a path or identifier whose first segment names an addressable project +routes there, with the remainder as the project-relative path — inputs accept +exactly the '/path' identifiers tool outputs produce. An explicit +project param plus an agreeing prefix strips the prefix; a disagreeing one +refuses naming both. Where more than one project is addressable — several local +projects, or a cloud workspace holding several — an unrecognized first segment +refuses with the project list rather than silently defaulting (#1421); the +mount view and the resolver read one list, so anything ``ls "/"`` advertises is +addressable by name. + +Collision rule: the project always wins over a same-named top-level folder in +the default project, so that folder is only reachable unqualified when a single +project is addressable (where there is no ambiguity); the qualified +'/folder/...' form always reaches it. ``man`` is excluded — its +``project`` param names the manual project, not a data project. """ import os @@ -31,7 +35,11 @@ from basic_memory.man import bundled_pages, find_page, parse_page_ref, render_index from basic_memory.mcp.container import get_container from basic_memory.mcp.note_reads import read_note_json_by_external_id -from basic_memory.mcp.project_context import get_project_client, resolve_project_path_route +from basic_memory.mcp.project_context import ( + addressable_projects, + get_project_client, + resolve_project_path_route, +) from basic_memory.mcp.server import POSIX_TOOLS_TAG, mcp, set_posix_tools_visibility from basic_memory.schemas.directory import ( DEFAULT_DIRECTORY_PAGE_SIZE, @@ -198,7 +206,7 @@ def _grep_retrieval_mode(literal: bool) -> SearchRetrievalMode: @mcp.tool( title="Grep", - description="Search note content for a pattern. Multi-project configs require 'project'.", + description="Search note content for a pattern. Requires 'project' when several are addressable.", tags={POSIX_TOOLS_TAG, "search"}, annotations={ "title": "Grep", @@ -223,7 +231,7 @@ async def grep( literal: Force literal full-text matching instead of semantic search. page: Page number (1-indexed). page_size: Results per page. - project: Project name. Required when more than one project is configured. + project: Project name. Required when more than one project is addressable. project_id: Project external_id (UUID); takes precedence over `project`. context: Optional FastMCP context. @@ -261,32 +269,25 @@ async def grep( return response.model_dump(mode="json", exclude_none=True) -async def _project_mount_listing(*, page: int, page_size: int) -> dict[str, Any]: - """Render the active projects as directory entries (the mount-point view). +async def _project_mount_listing( + *, page: int, page_size: int, context: Context | None +) -> dict[str, Any]: + """Render the addressable projects as directory entries (the mount-point view). - Reuses list_memory_projects' stdio enumeration path: in-process ASGI - locally, the same call over HTTP in global cloud mode. Each row's - ``directory_path`` is the copyable '/' prefix form. + Sources ``addressable_projects`` — the same set the path resolver routes by + — so every mount advertised here is reachable as '/path' (#1421). + Each row's ``directory_path`` is the copyable '/' prefix form, and + the set already arrives sorted by project name. """ - # Import here to avoid circular import - from basic_memory.mcp.async_client import get_client - from basic_memory.mcp.clients import ProjectClient - - async with get_client() as client: - project_list = await ProjectClient(client).list_projects() - - rows = sorted( - ( - DirectoryNode( - name=item.name, - directory_path=f"/{item.permalink}", - permalink=item.permalink, - type="directory", - ) - for item in project_list.projects - ), - key=lambda node: node.name, - ) + rows = [ + DirectoryNode( + name=item.name, + directory_path=f"/{item.permalink}", + permalink=item.permalink, + type="directory", + ) + for item in await addressable_projects(context=context) + ] start = (page - 1) * page_size listing = DirectoryListResponse( nodes=rows[start : start + page_size], @@ -326,7 +327,8 @@ async def ls( page: Page number (1-indexed). page_size: Nodes per page. project: Project name. Optional - '/' lists projects; qualified paths - route themselves; multi-project configs refuse other unqualified paths. + route themselves; other unqualified paths refuse when several + projects are addressable. project_id: Project external_id (UUID); takes precedence over `project`. context: Optional FastMCP context. @@ -350,7 +352,7 @@ async def ls( and not os.environ.get("BASIC_MEMORY_MCP_PROJECT") and not path.strip().strip("/") ): - return await _project_mount_listing(page=page, page_size=page_size) + return await _project_mount_listing(page=page, page_size=page_size, context=context) route = await resolve_project_path_route( path, project=project, project_id=project_id, context=context @@ -400,7 +402,7 @@ async def find( page: Page number (1-indexed). page_size: Nodes per page. project: Project name. Optional - qualified paths route themselves; - multi-project configs refuse unqualified paths. + unqualified paths refuse when several projects are addressable. project_id: Project external_id (UUID); takes precedence over `project`. context: Optional FastMCP context. @@ -444,7 +446,7 @@ async def find( @mcp.tool( title="Tail", - description="Show recently changed notes. Multi-project configs require 'project'.", + description="Show recently changed notes. Requires 'project' when several are addressable.", tags={POSIX_TOOLS_TAG, "navigation", "notes"}, annotations={ "title": "Tail", @@ -465,7 +467,7 @@ async def tail( Args: timeframe: Time window, e.g. "7d", "yesterday", "2 days ago". lines: Maximum number of rows to return (1-100). - project: Project name. Required when more than one project is configured. + project: Project name. Required when more than one project is addressable. project_id: Project external_id (UUID); takes precedence over `project`. context: Optional FastMCP context. diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index 42150e8a4..a4e884ef3 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -1,17 +1,24 @@ -"""Direct unit tests for resolve_project_path_route (#1415). +"""Direct unit tests for resolve_project_path_route (#1415, #1421). The resolver is the single routing seam for the posix tools: '/path' inputs route by first segment, an explicit project must agree with a path -prefix, and multi-project configs refuse unqualified input instead of -defaulting. These tests drive the function branch-by-branch against configs -written through the test ConfigManager; no API client is involved because -local-config matching never leaves the process. +prefix, and unqualified input refuses instead of defaulting whenever more than +one project is addressable. The local-config tests drive the function +branch-by-branch against configs written through the test ConfigManager; no API +client is involved there because local-config matching never leaves the +process. The cloud section at the bottom stands up a factory-mode session, +where the project list comes from the tenant instead. """ import re +from contextlib import asynccontextmanager +from dataclasses import dataclass +from typing import AsyncIterator, Optional import pytest +import basic_memory.mcp.async_client as async_client +import basic_memory.mcp.project_context as project_context from basic_memory.config_models import ProjectEntry from basic_memory.mcp.project_context import ( ProjectPathRoute, @@ -21,6 +28,12 @@ _project_routes_agree, resolve_project_path_route, ) +from basic_memory.mcp.project_context_identifiers import unqualified_project_identifier +from basic_memory.mcp.tools import grep, ls +from basic_memory.schemas.cloud import WorkspaceInfo +from basic_memory.schemas.project_info import ProjectItem, ProjectList +from basic_memory.utils import generate_permalink +from tests.mcp.conftest import ContextState, ctx @pytest.fixture(autouse=True) @@ -241,8 +254,11 @@ async def test_single_project_input_passes_through_unchanged(config_manager, pat @pytest.mark.asyncio async def test_empty_config_passes_through(empty_project_config): - """A cloud-only local client (no local mount table) keeps API-side default - resolution — refusal needs a config that can enumerate projects.""" + """An emptied config still materializes the placeholder 'main' project, so a + locally routed session addresses exactly one project and keeps today's + default resolution — there is nothing to disambiguate.""" + assert list(empty_project_config.config.projects) == ["main"] + route = await resolve_project_path_route("anything/x", project=None, project_id=None) assert route == ProjectPathRoute(project=None, path="anything/x", stripped=False) @@ -279,3 +295,229 @@ async def test_env_constraint_prevents_refusal_for_empty_input(multi_project_con route = await resolve_project_path_route("", project=None, project_id=None) assert route == ProjectPathRoute(project="second-project", path="", stripped=False) + + +# --- cloud/factory sessions (#1421) --- +# The hosted MCP server installs a client factory and keeps project state in the +# tenant database, not in config: BasicMemoryConfig always materializes a +# placeholder 'main' entry, so local config proves nothing about a cloud +# session's projects. These tests stand up that shape and pin the invariant +# that ties the mount view to the resolver — everything `ls "/"` advertises must +# be addressable — plus the refusal that keeps an unqualified reference from +# landing on whichever project a teammate last flagged as default. + + +@dataclass +class _CloudSession: + """What a stood-up cloud session exposes to a test.""" + + projects: tuple[ProjectItem, ...] + listings: list[ProjectList] + + +def _routed_project_permalink(route: ProjectPathRoute) -> str: + """The project a route landed on, as its bare permalink. + + Cloud routes come back workspace-qualified ('team/research'), so compare on + the project segment the mount view advertises. + """ + assert route.project is not None + return generate_permalink(unqualified_project_identifier(route.project)) + + +@pytest.fixture +def cloud_session(monkeypatch, config_manager): + """Build a factory-mode session whose workspace holds the named projects. + + Both the mount view and workspace discovery read the same project listing + the tenant serves, exactly as they do in production: the injected factory + client answers `get_client()` for the mount view and `get_client(workspace=)` + for the workspace index. Every listing call is recorded so tests can pin how + often routing pays for it. + """ + + def build(*names: str, default: Optional[str] = None) -> _CloudSession: + config = config_manager.load_config() + # The cloud nulls its config cache so default_project reads as None; + # projects={} still round-trips back as the placeholder 'main' entry. + config.projects = {} + config.default_project = None + config_manager.save_config(config) + + projects = tuple( + ProjectItem( + id=index + 1, + external_id=f"{generate_permalink(name)}-external-id", + name=name, + path=f"/app/data/{generate_permalink(name)}", + is_default=name == default, + ) + for index, name in enumerate(names) + ) + project_list = ProjectList(projects=list(projects), default_project=default) + workspace = WorkspaceInfo( + tenant_id="team-tenant", + workspace_type="organization", + slug="team", + name="Team", + role="editor", + is_default=True, + ) + + listings: list[ProjectList] = [] + + @asynccontextmanager + async def fake_get_client(*args, **kwargs) -> AsyncIterator[object]: + yield object() + + async def fake_list_projects(self) -> ProjectList: + listings.append(project_list) + return project_list + + async def fake_get_available_workspaces(context=None) -> list[WorkspaceInfo]: + return [workspace] + + monkeypatch.setattr(async_client, "is_factory_mode", lambda: True) + monkeypatch.setattr(async_client, "get_client", fake_get_client) + monkeypatch.setattr( + "basic_memory.mcp.clients.project.ProjectClient.list_projects", + fake_list_projects, + ) + monkeypatch.setattr( + project_context, + "get_available_workspaces", + fake_get_available_workspaces, + ) + return _CloudSession(projects=projects, listings=listings) + + return build + + +@pytest.mark.asyncio +async def test_cloud_workspace_refuses_unqualified_path(cloud_session): + """The reported failure (#1421): with only a placeholder config entry to + enumerate, an unqualified path used to route to a default project. It now + refuses, naming every addressable project in copyable prefix form.""" + cloud_session("research", "engineering", "personal-notes") + + with pytest.raises(UnqualifiedPathRefusedError) as excinfo: + await resolve_project_path_route("notes/foo", project=None, project_id=None) + + assert str(excinfo.value) == ( + "no project 'notes' — active projects: engineering/, personal-notes/, research/" + ) + + +@pytest.mark.asyncio +async def test_cloud_workspace_refuses_pathless_call_through_the_tool(cloud_session): + """grep/tail carry no path to qualify, so the refusal is the tool's answer: + a silent search of one project out of several is the bug being removed.""" + cloud_session("research", "engineering") + + with pytest.raises(UnqualifiedPathRefusedError) as excinfo: + await grep("needle") + + assert str(excinfo.value) == ("no project specified — active projects: engineering/, research/") + + +@pytest.mark.asyncio +async def test_cloud_qualified_path_routes_to_that_project(cloud_session): + """A qualified '/notes/x' routes to that exact project — the + workspace-qualified spelling keeps the tenant explicit — and the remainder + becomes the project-relative path.""" + cloud_session("research", "engineering", "personal-notes") + + route = await resolve_project_path_route("research/notes/x", project=None, project_id=None) + + assert route.stripped is True + assert route.path == "notes/x" + assert route.project == "team/research" + assert unqualified_project_identifier(route.project) == "research" + + +@pytest.mark.asyncio +async def test_cloud_every_advertised_mount_is_addressable(cloud_session): + """Round trip over the advertised list: `ls "/"` and the resolver read one + source, so every mount the root advertises routes — as a bare mount name + and as a path under it. A mount that listed but did not route is exactly + the mismatch #1421 reported.""" + cloud_session("research", "engineering", "personal-notes") + + mounts = await ls() + + assert [node["name"] for node in mounts["nodes"]] == [ + "engineering", + "personal-notes", + "research", + ] + for node in mounts["nodes"]: + permalink = node["permalink"] + assert node["directory_path"] == f"/{permalink}" + + root_route = await resolve_project_path_route(permalink, project=None, project_id=None) + assert root_route.stripped is True + assert root_route.path == "" + assert _routed_project_permalink(root_route) == permalink + + path_route = await resolve_project_path_route( + f"{permalink}/notes/x", project=None, project_id=None + ) + assert path_route.stripped is True + assert path_route.path == "notes/x" + assert _routed_project_permalink(path_route) == permalink + + +@pytest.mark.asyncio +async def test_cloud_single_project_workspace_resolves_unqualified(cloud_session): + """One project in the workspace means no ambiguity to protect against, so + unqualified references keep resolving unchanged.""" + cloud_session("research") + + route = await resolve_project_path_route("notes/foo", project=None, project_id=None) + + assert route == ProjectPathRoute(project=None, path="notes/foo", stripped=False) + + +@pytest.mark.asyncio +async def test_cloud_refusal_does_not_consult_the_default_flag(cloud_session): + """is_default is one shared mutable flag in a team workspace, so it must not + decide where an unqualified call lands: a workspace whose default points at + 'alpha' still refuses while 'beta' and 'gamma' exist.""" + session = cloud_session("alpha", "beta", "gamma", default="alpha") + + assert [project.name for project in session.projects if project.is_default] == ["alpha"] + + with pytest.raises(UnqualifiedPathRefusedError) as excinfo: + await resolve_project_path_route("", project=None, project_id=None) + + # The flagged project is listed as one choice among equals, never selected. + assert str(excinfo.value) == "no project specified — active projects: alpha/, beta/, gamma/" + + # And it hijacks nothing that named a project: qualified input still routes + # where the caller said, not to whatever carries the flag. + route = await resolve_project_path_route("beta/notes/x", project=None, project_id=None) + + assert _routed_project_permalink(route) == "beta" + + +@pytest.mark.asyncio +async def test_cloud_project_listing_is_fetched_once_per_request(cloud_session): + """The cost of knowing the workspace's projects: one listing per MCP + request, memoized in context state, no matter how many resolutions the + request makes. Without a context (CLI calls) each resolution pays.""" + session = cloud_session("research", "engineering") + context = ContextState() + + first = await resolve_project_path_route( + "research", project=None, project_id=None, context=ctx(context) + ) + assert first.stripped is True + assert len(session.listings) == 1 + + session.listings.clear() + second = await resolve_project_path_route( + "engineering", project=None, project_id=None, context=ctx(context) + ) + + assert second.stripped is True + assert session.listings == [] From 30ac59b14eae6fcca4df59d396763c9f722fdd2b Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 12:16:47 -0500 Subject: [PATCH 03/18] fix(mcp): let an advertised mount claim its first path segment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Workspace-qualified parsing ran before the advertised-project lookup, so when a project's permalink was also an accessible workspace slug, the next segment naming a project in that workspace won. With /team advertised and workspace team holding project docs, cat('team/docs/x') read workspace team's docs project instead of the docs directory in the advertised team project — another project's data, from a name ls / promised. The advertised list is a promise, so it now claims the first segment first; workspace-qualified spellings resolve only when no mount matches. The collision this creates is pinned rather than swallowed: while a project's permalink equals a workspace slug, that workspace's other projects lose their qualified path spelling. They stay addressable through the project parameter, and pairing the two now raises a prefix conflict instead of quietly agreeing, which is what teaches the escape. Two side effects worth having: mount-prefixed paths skip workspace discovery entirely, and ls 'research' and ls 'research/notes' now agree on the project spelling. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 73 ++++++++++++++++----- tests/mcp/test_project_path_routing.py | 85 +++++++++++++++++++++++-- 2 files changed, 134 insertions(+), 24 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index fe5abcc1c..2e5a1cc7a 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -996,6 +996,22 @@ async def detect_project_from_identifier_prefix( # every mount `ls /` advertises is reachable by name and an unqualified # reference in a many-project workspace refuses instead of picking a default # (#1421). +# +# Precedence: the mount table wins the first path segment, ahead of +# workspace-qualified '//' parsing. A project permalink +# can also be an accessible workspace's slug, and only one reading of '/...' +# can win. Mount-wins is what the advertised list promises — a name `ls /` shows +# must address that mount, or we advertise a name that resolves somewhere else, +# which is worse than not advertising it at all. +# +# The cost of that choice, stated plainly: when a project's permalink equals an +# accessible workspace's slug, that workspace's OTHER projects lose their +# qualified path spelling. With '/team' advertised as a mount, 'team/docs/x' is +# project 'team', path 'docs/x' — never workspace 'team', project 'docs'. Those +# projects stay addressable through the project param (project='team/docs' with +# the project-relative path 'x'), which is the escape hatch the prefix-conflict +# message already teaches, so nothing becomes unreachable — only differently +# spelled. @dataclass(frozen=True) @@ -1168,13 +1184,19 @@ async def resolve_project_path_route( explicit '/' outlives a bare local prefix match. 3. Otherwise a first segment naming an addressable project routes there with the remainder as the project-relative path. - 4. Otherwise, when the session addresses more than one project, raise + 4. Otherwise a workspace-qualified '//' spelling + routes to that project in that workspace — those projects belong to + workspaces this session's own route does not list, so they never appear + in the mount table rule 3 reads. + 5. Otherwise, when the session addresses more than one project, raise UnqualifiedPathRefusedError instead of silently defaulting; a session that addresses at most one project keeps today's default resolution. - Rules 3 and 4 read the set from ``addressable_projects`` — the same set + Rules 3 and 5 read the set from ``addressable_projects`` — the same set ``ls /`` advertises — so a project can never be listed at the root and then - go unrecognized as a path prefix (#1421). + go unrecognized as a path prefix (#1421). Rule 3 deliberately precedes rule + 4; see the mount-precedence note above this section for the collision that + ordering resolves and the spelling it costs. """ if project_id is not None: return ProjectPathRoute(project=project, path=path, stripped=False) @@ -1188,21 +1210,21 @@ async def resolve_project_path_route( detected: Optional[str] = None remainder = "" addressable: tuple[AddressableProject, ...] | None = None - if "/" in candidate: - detected = await detect_project_from_identifier_prefix(candidate, config, context=context) - if detected is not None: - remainder = _detected_route_remainder(candidate, detected) - # Trigger: no workspace-qualified route resolved and the input still has a - # leading segment — a bare mount name ('ls research'), or a cloud session - # whose local config holds only the placeholder 'main' entry. - # Why: split_project_prefix needs a slash, and the detection above reads the - # local config and the workspace index; neither can name a project that - # only this session's own listing knows about (#1421). Advertising a mount - # at '/' that a path prefix cannot name is the bug being closed. - # Outcome: a first segment matching an addressable project routes there, - # with the remainder as the project-relative path. - if detected is None and candidate: + # --- Rule 3: the advertised mount table claims the first segment --- + # Trigger: the input carries a leading segment at all — a bare mount name + # ('ls research') or a path under one. + # Why: this set is what `ls /` advertises, and an advertised name that + # resolves somewhere else is worse than one never advertised. The workspace + # parse below would take '//...' first, so a project whose + # permalink is also an accessible workspace slug would hand 'team/docs/x' + # to project 'docs' in workspace 'team' instead of the mount named 'team' + # — reading another project's data under an advertised name. + # Outcome: a first segment matching an addressable project routes there with + # the remainder as the project-relative path, and workspace discovery is + # never consulted for it. A local session pays nothing (its config is its + # mount table); a cloud session pays one per-request memoized listing. + if candidate: addressable = await addressable_projects(context=context) first_segment, _, remaining_segments = candidate.partition("/") first_permalink = generate_permalink(first_segment) @@ -1214,6 +1236,23 @@ async def resolve_project_path_route( detected = mount.name remainder = remaining_segments + # --- Rule 4: workspace-qualified spellings for everything else --- + # Trigger: no advertised mount claimed the first segment and the input still + # has more than one segment to parse. + # Why: '//' addresses projects in workspaces this + # session's own route does not list, so they are absent from the mount + # table above and would otherwise be unreachable. Local-config prefixes + # also resolve here — a locally routed session's config IS its mount + # table, so rule 3 already claimed those; what reaches this line is a + # cloud-routed session whose local config names a project its own tenant + # listing does not. + # Outcome: those spellings keep resolving exactly as before; the mount + # collision described above is the only address this ordering takes away. + if detected is None and "/" in candidate: + detected = await detect_project_from_identifier_prefix(candidate, config, context=context) + if detected is not None: + remainder = _detected_route_remainder(candidate, detected) + if explicit is not None: if detected is None: return ProjectPathRoute( diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index a4e884ef3..96efda2b8 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -422,17 +422,88 @@ async def test_cloud_workspace_refuses_pathless_call_through_the_tool(cloud_sess @pytest.mark.asyncio async def test_cloud_qualified_path_routes_to_that_project(cloud_session): - """A qualified '/notes/x' routes to that exact project — the - workspace-qualified spelling keeps the tenant explicit — and the remainder - becomes the project-relative path.""" + """A qualified '/notes/x' routes to that exact project by the mount + name the root advertises, with the remainder as the project-relative path. + + The mount table is already scoped to this session's own route, so it is the + authority on that first segment; `ls "research"` has always routed by the + bare name, and the path form now agrees with it instead of qualifying the + tenant only when a slash happened to be present. + """ cloud_session("research", "engineering", "personal-notes") route = await resolve_project_path_route("research/notes/x", project=None, project_id=None) - assert route.stripped is True - assert route.path == "notes/x" - assert route.project == "team/research" - assert unqualified_project_identifier(route.project) == "research" + assert route == ProjectPathRoute(project="research", path="notes/x", stripped=True) + assert unqualified_project_identifier(route.project or "") == "research" + + +@pytest.mark.asyncio +async def test_cloud_workspace_qualified_path_without_mount_collision_still_routes(cloud_session): + """No project is named 'team', so the workspace slug is free to claim the + first segment: '//' resolves through workspace + discovery, qualified name and all. Mount precedence takes nothing away from + the spellings that address other workspaces.""" + cloud_session("research", "engineering") + + route = await resolve_project_path_route("team/research/notes/x", project=None, project_id=None) + + assert route == ProjectPathRoute(project="team/research", path="notes/x", stripped=True) + + +@pytest.mark.asyncio +async def test_cloud_mount_wins_over_colliding_workspace_slug(cloud_session): + """The collision: 'team' is both an advertised mount and this workspace's + slug, and that workspace also holds a project 'docs'. The mount wins the + first segment, so 'team/docs/x' is project 'team', path 'docs/x' — never + project 'docs' in workspace 'team'. Attempting workspace-qualified parsing + first served another project's data under an advertised name.""" + cloud_session("team", "docs") + + route = await resolve_project_path_route("team/docs/x", project=None, project_id=None) + + assert route == ProjectPathRoute(project="team", path="docs/x", stripped=True) + + +@pytest.mark.asyncio +async def test_cloud_slug_collision_shadowed_project_stays_addressable(cloud_session): + """The documented cost of mount-wins, pinned: while a mount named 'team' + exists, project 'docs' in workspace 'team' loses its qualified *path* + spelling. It stays addressable through the project param — project-relative, + or with an agreeing prefix — and the shadowed spelling conflicts loudly + rather than resolving either way.""" + cloud_session("team", "docs") + + relative = await resolve_project_path_route("x", project="team/docs", project_id=None) + assert relative == ProjectPathRoute(project="team/docs", path="x", stripped=False) + + agreeing = await resolve_project_path_route("docs/x", project="team/docs", project_id=None) + assert agreeing == ProjectPathRoute(project="team/docs", path="x", stripped=True) + + with pytest.raises(ProjectPrefixConflictError, match="path names project 'team'"): + await resolve_project_path_route("team/docs/x", project="team/docs", project_id=None) + + +@pytest.mark.asyncio +async def test_cloud_every_advertised_mount_is_addressable_under_slug_collision(cloud_session): + """The round-trip invariant survives the collision, which is the point of + mount precedence: a mount named after the workspace slug still routes to + itself, and so does every other mount beside it.""" + cloud_session("team", "docs") + + mounts = await ls() + + assert [node["name"] for node in mounts["nodes"]] == ["docs", "team"] + for node in mounts["nodes"]: + permalink = node["permalink"] + + root_route = await resolve_project_path_route(permalink, project=None, project_id=None) + assert root_route == ProjectPathRoute(project=node["name"], path="", stripped=True) + + path_route = await resolve_project_path_route( + f"{permalink}/notes/x", project=None, project_id=None + ) + assert path_route == ProjectPathRoute(project=node["name"], path="notes/x", stripped=True) @pytest.mark.asyncio From 99d6d8c66ba7040b5beb0326f3edaa0969ef04d5 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 16:00:22 -0500 Subject: [PATCH 04/18] fix(mcp): bind posix mounts to their workspace and project permalink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings on the #1415 path resolver, all in the mount table that `ls "/"` advertises and `resolve_project_path_route` routes by. Keep advertised mounts bound to their workspace. A hosted session's own route is one tenant, but `get_project_client` re-resolves a bare project name against every accessible workspace, and project names are unique only inside one of them. With the session on a non-default workspace and the same project permalink in the default one, `resolve_workspace_project_ from_index` picked the default workspace's copy by its is_default flag on any call before a workspace was cached — so `cat("docs/x")` could read a different tenant's project under a name the session's own root had just advertised. `AddressableProject` now carries the project's external_id and `ProjectPathRoute` hands it on as `project_id`, which the index resolves exactly. An explicit workspace-qualified project still wins and drops the mount id with it, so that escape hatch is unchanged. Recognize workspace-qualified project roots. Workspace-qualified memory URLs require three segments, because `memory://main/notes` has to stay readable as project `main`. A posix path only reaches that parse after the mount table declined its leading segment, so nothing addressable can be meant by it and the two-segment form is unambiguous — but it was rejected anyway, leaving `ls "acme/docs/notes"` resolving while `ls "acme/docs"`, that same project's root, had no spelling at all and fell through to the multi-project refusal. The looser `split_workspace_route_segments` parse now serves the posix resolver only; memory URLs keep the strict form. Handle multi-segment project permalinks in mounts. Project names may contain '/', and generate_permalink keeps it, so a project 'Research/2026' advertises the two-segment mount '/research/2026'. Matching only the first segment listed that mount at the root and then could not enter it; the whole permalink now has to match, longest first. Regression tests cover each: the cross-workspace binding (id lands in the session's workspace where the bare name still falls to the default flag), the workspace-qualified root, the multi-segment mount and its shorter sibling, and the fail-soft/fail-loud split on a workspace that cannot be discovered. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 169 +++++++++--- .../mcp/project_context_identifiers.py | 20 ++ src/basic_memory/mcp/tools/posix_tools.py | 15 +- tests/mcp/test_project_path_routing.py | 260 +++++++++++++++++- 4 files changed, 412 insertions(+), 52 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index 2e5a1cc7a..a71c2aed9 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -66,6 +66,7 @@ split_qualified_project_identifier as _split_qualified_project_identifier_impl, split_workspace_identifier_segments as _split_workspace_identifier_segments, split_workspace_memory_url_segments as _split_workspace_memory_url_segments, + split_workspace_route_segments as _split_workspace_route_segments, unqualified_project_identifier as _unqualified_project_identifier, ) from basic_memory.mcp.workspace_project_index import ( @@ -928,6 +929,17 @@ async def detect_project_from_memory_url_prefix( return await detect_project_from_identifier_prefix(identifier, config, context=context) +# Workspace discovery is best-effort for prefix detection: an identifier that +# names no reachable workspace/project simply stays unrouted, because it may not +# have meant a workspace at all. Anything outside this set is a real failure and +# propagates. +_WORKSPACE_DISCOVERY_FALLBACK_ERRORS = ( + "not found", + "no accessible workspaces", + "unable to discover", +) + + async def detect_project_from_identifier_prefix( identifier: str, config: BasicMemoryConfig, @@ -947,11 +959,6 @@ async def detect_project_from_identifier_prefix( return None if _workspace_identifier_discovery_available(identifier, config): - workspace_discovery_fallback_errors = ( - "not found", - "no accessible workspaces", - "unable to discover", - ) try: workspace_resolution = await resolve_workspace_qualified_identifier( identifier, @@ -959,7 +966,7 @@ async def detect_project_from_identifier_prefix( ) except ValueError as exc: message = str(exc).lower() - if any(error in message for error in workspace_discovery_fallback_errors): + if any(error in message for error in _WORKSPACE_DISCOVERY_FALLBACK_ERRORS): return None raise @@ -977,7 +984,7 @@ async def detect_project_from_identifier_prefix( ) except ValueError as exc: message = str(exc).lower() - if any(error in message for error in workspace_discovery_fallback_errors): + if any(error in message for error in _WORKSPACE_DISCOVERY_FALLBACK_ERRORS): return None raise @@ -997,12 +1004,13 @@ async def detect_project_from_identifier_prefix( # reference in a many-project workspace refuses instead of picking a default # (#1421). # -# Precedence: the mount table wins the first path segment, ahead of +# Precedence: the mount table wins the leading path segments, ahead of # workspace-qualified '//' parsing. A project permalink # can also be an accessible workspace's slug, and only one reading of '/...' # can win. Mount-wins is what the advertised list promises — a name `ls /` shows # must address that mount, or we advertise a name that resolves somewhere else, -# which is worse than not advertising it at all. +# which is worse than not advertising it at all. "Segments", plural: a project +# name may contain '/', so its advertised permalink can span more than one. # # The cost of that choice, stated plainly: when a project's permalink equals an # accessible workspace's slug, that workspace's OTHER projects lose their @@ -1016,21 +1024,29 @@ async def detect_project_from_identifier_prefix( @dataclass(frozen=True) class ProjectPathRoute: - """Effective routing for one posix call: project param + project-relative path. + """Effective routing for one posix call: project params + project-relative path. ``stripped=False`` means the input carried no recognized project prefix: ``path`` is the caller's input byte-for-byte and ``project`` is the explicit value that was passed (or None, meaning the existing default resolution chain applies — only reachable when the session addresses at most one project; several addressable projects refuse instead). - ``stripped=True`` means a first-segment project was recognized: ``project`` + ``stripped=True`` means a project prefix was recognized: ``project`` is the canonical config name (or workspace-qualified name) and ``path`` is the remainder with no leading slash, "" meaning the project root. + + ``project_id`` is the effective external_id for the call — the caller's own + when they passed one, otherwise the id of the advertised mount that claimed + the prefix. Callers pass both fields to ``get_project_client`` verbatim; the + id is what keeps a mount bound to the workspace that advertised it, since a + bare project name can name a different project in another accessible + workspace (#1421). """ project: Optional[str] path: str stripped: bool + project_id: Optional[str] = None class ProjectPrefixConflictError(ValueError): @@ -1045,12 +1061,18 @@ class UnqualifiedPathRefusedError(ValueError): class AddressableProject: """One project this session can both advertise and route to. - ``name`` is the routing identifier handed to ``get_project_client``; - ``permalink`` is the first path segment agents copy out of tool output. + ``name`` is the routing identifier handed to ``get_project_client`` and + ``permalink`` is the path prefix agents copy out of tool output. A cloud + session also carries ``external_id``: project names are unique only inside + one workspace, so the UUID is the only identifier that pins a mount to the + workspace whose listing advertised it. A locally routed session reads its + mounts from config, which holds no UUIDs, and has no second workspace to be + confused with, so ``external_id`` is None there. """ name: str permalink: str + external_id: Optional[str] = None # The session's own project listing, memoized for one MCP request: routing and @@ -1116,7 +1138,11 @@ async def addressable_projects( if _session_routes_to_cloud(): project_list = await _session_project_list(context=context) projects = ( - AddressableProject(name=item.name, permalink=item.permalink) + AddressableProject( + name=item.name, + permalink=item.permalink, + external_id=item.external_id, + ) for item in project_list.projects ) else: @@ -1132,6 +1158,63 @@ def _addressable_project_prefixes(projects: tuple[AddressableProject, ...]) -> s return ", ".join(f"{permalink}/" for permalink in sorted(item.permalink for item in projects)) +def _claim_mount_prefix( + candidate: str, + projects: tuple[AddressableProject, ...], +) -> tuple[AddressableProject, str] | None: + """Return the mount whose permalink claims the candidate's leading segments. + + A project name may itself contain '/', and generate_permalink keeps that + separator, so a project named 'Research/2026' advertises the two-segment + mount '/research/2026'. Comparing only the first segment would leave that + mount listed at the root and impossible to enter, so the whole permalink has + to match; the longest match wins, which is also the only reading that can be + right when one mount's permalink prefixes another's. + """ + segments = candidate.split("/") + claimed: tuple[AddressableProject, str] | None = None + claimed_depth = 0 + for project in projects: + depth = project.permalink.count("/") + 1 + if depth > len(segments) or depth <= claimed_depth: + continue + if generate_permalink("/".join(segments[:depth])) != project.permalink: + continue + claimed = (project, "/".join(segments[depth:])) + claimed_depth = depth + return claimed + + +async def _detect_workspace_project_root( + candidate: str, + config: BasicMemoryConfig, + context: Optional[Context] = None, +) -> Optional[str]: + """Resolve a bare '/' candidate to that project's root. + + Workspace-qualified memory URLs require three segments so that + 'memory://main/notes' stays readable as project 'main'. A posix path only + reaches here after the mount table declined its first segment, so nothing + addressable can be meant by it and the two-segment form is unambiguous — + without this, 'ls acme/docs/notes' resolved while 'ls acme/docs' (that same + project's root) had no spelling at all. + """ + segments = _split_workspace_route_segments(candidate) + if segments is None or segments[2]: + return None + if not _cloud_workspace_discovery_available(config): + return None + + try: + resolution = await _resolve_workspace_segments(candidate, segments, context=context) + except ValueError as exc: + if any(error in str(exc).lower() for error in _WORKSPACE_DISCOVERY_FALLBACK_ERRORS): + return None + raise + + return resolution.project_identifier if resolution is not None else None + + def _detected_route_remainder(candidate: str, detected: str) -> str: """Return the project-relative path left after the detected route prefix. @@ -1182,12 +1265,13 @@ async def resolve_project_path_route( disagreeing one raises ProjectPrefixConflictError — never silently preferring either. Agreement keeps the more-qualified spelling: an explicit '/' outlives a bare local prefix match. - 3. Otherwise a first segment naming an addressable project routes there + 3. Otherwise leading segments naming an addressable project route there with the remainder as the project-relative path. - 4. Otherwise a workspace-qualified '//' spelling + 4. Otherwise a workspace-qualified '/[/]' spelling routes to that project in that workspace — those projects belong to workspaces this session's own route does not list, so they never appear - in the mount table rule 3 reads. + in the mount table rule 3 reads. With no path it names that project's + root, the same way a bare mount name does. 5. Otherwise, when the session addresses more than one project, raise UnqualifiedPathRefusedError instead of silently defaulting; a session that addresses at most one project keeps today's default resolution. @@ -1199,7 +1283,7 @@ async def resolve_project_path_route( ordering resolves and the spelling it costs. """ if project_id is not None: - return ProjectPathRoute(project=project, path=path, stripped=False) + return ProjectPathRoute(project=project, path=path, stripped=False, project_id=project_id) # The env constraint is ProjectResolver's priority 1, so it participates in # agree/strip and conflict exactly like the param it outranks. @@ -1209,9 +1293,10 @@ async def resolve_project_path_route( detected: Optional[str] = None remainder = "" + mount_project_id: Optional[str] = None addressable: tuple[AddressableProject, ...] | None = None - # --- Rule 3: the advertised mount table claims the first segment --- + # --- Rule 3: the advertised mount table claims the leading segments --- # Trigger: the input carries a leading segment at all — a bare mount name # ('ls research') or a path under one. # Why: this set is what `ls /` advertises, and an advertised name that @@ -1220,24 +1305,20 @@ async def resolve_project_path_route( # permalink is also an accessible workspace slug would hand 'team/docs/x' # to project 'docs' in workspace 'team' instead of the mount named 'team' # — reading another project's data under an advertised name. - # Outcome: a first segment matching an addressable project routes there with - # the remainder as the project-relative path, and workspace discovery is - # never consulted for it. A local session pays nothing (its config is its - # mount table); a cloud session pays one per-request memoized listing. + # Outcome: a leading segment matching an addressable project routes there + # with the remainder as the project-relative path, and workspace discovery + # is never consulted for it. A local session pays nothing (its config is + # its mount table); a cloud session pays one per-request memoized listing. if candidate: addressable = await addressable_projects(context=context) - first_segment, _, remaining_segments = candidate.partition("/") - first_permalink = generate_permalink(first_segment) - mount = next( - (item for item in addressable if item.permalink == first_permalink), - None, - ) - if mount is not None: + claimed = _claim_mount_prefix(candidate, addressable) + if claimed is not None: + mount, remainder = claimed detected = mount.name - remainder = remaining_segments + mount_project_id = mount.external_id # --- Rule 4: workspace-qualified spellings for everything else --- - # Trigger: no advertised mount claimed the first segment and the input still + # Trigger: no advertised mount claimed the leading segments and the input still # has more than one segment to parse. # Why: '//' addresses projects in workspaces this # session's own route does not list, so they are absent from the mount @@ -1252,6 +1333,10 @@ async def resolve_project_path_route( detected = await detect_project_from_identifier_prefix(candidate, config, context=context) if detected is not None: remainder = _detected_route_remainder(candidate, detected) + else: + # A bare '/' names that project's root, so the + # remainder stays empty — see _detect_workspace_project_root. + detected = await _detect_workspace_project_root(candidate, config, context=context) if explicit is not None: if detected is None: @@ -1264,17 +1349,20 @@ async def resolve_project_path_route( # Why: a local project can shadow a same-named project in another # workspace; dropping the explicitly named workspace would # silently reroute the call to the local shadow. - # Outcome: the more-qualified explicit spelling carries the route; - # every other agreement keeps the detected (canonical) spelling. + # Outcome: the more-qualified explicit spelling carries the route, + # and drops the mount id that names this session's workspace with + # it; every other agreement keeps the detected (canonical) + # spelling and stays bound to the mount that matched. detected_workspace, _ = _split_qualified_project_identifier_impl(detected) explicit_workspace, _ = _split_qualified_project_identifier_impl(explicit) - routed = detected - if explicit_workspace is not None and detected_workspace is None: - routed = explicit + prefer_explicit = explicit_workspace is not None and detected_workspace is None return ProjectPathRoute( - project=_canonicalize_project_name(routed, config), + project=_canonicalize_project_name( + explicit if prefer_explicit else detected, config + ), path=remainder, stripped=True, + project_id=None if prefer_explicit else mount_project_id, ) raise ProjectPrefixConflictError( f"path names project '{detected}' but project '{explicit}' was passed — " @@ -1284,7 +1372,10 @@ async def resolve_project_path_route( if detected is not None: return ProjectPathRoute( - project=_canonicalize_project_name(detected, config), path=remainder, stripped=True + project=_canonicalize_project_name(detected, config), + path=remainder, + stripped=True, + project_id=mount_project_id, ) # Trigger: no explicit project, no recognized prefix, several addressable projects. diff --git a/src/basic_memory/mcp/project_context_identifiers.py b/src/basic_memory/mcp/project_context_identifiers.py index af1e33154..4e866dfe0 100644 --- a/src/basic_memory/mcp/project_context_identifiers.py +++ b/src/basic_memory/mcp/project_context_identifiers.py @@ -103,6 +103,26 @@ def split_workspace_identifier_segments(identifier: str) -> tuple[str, str, str] return workspace_slug, project_identifier, remainder +def split_workspace_route_segments(identifier: str) -> tuple[str, str, str] | None: + """Split ``/[/]`` where the trailing path may be empty. + + The strict three-segment parse above is what memory URLs need: there, + ``memory://main/notes`` has to stay readable as project ``main``. A posix + path only reaches this looser parse after the advertised mount table has + declined its first segment, so no addressable project can be meant by it and + the bare ``/`` form unambiguously names that project's + root. + """ + normalized = normalize_project_reference(identifier_path(identifier)).strip("/") + parts = normalized.split("/", 2) + if len(parts) < 2: + return None + workspace_slug, project_identifier = parts[0], parts[1] + if not workspace_slug or not project_identifier: + return None + return workspace_slug, project_identifier, parts[2] if len(parts) == 3 else "" + + def split_workspace_memory_url_segments(identifier: str) -> tuple[str, str, str] | None: """Split ``memory:////`` into route segments.""" if not identifier.strip().startswith("memory://"): diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index 5ce192403..233efdf68 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -16,7 +16,10 @@ projects, or a cloud workspace holding several — an unrecognized first segment refuses with the project list rather than silently defaulting (#1421); the mount view and the resolver read one list, so anything ``ls "/"`` advertises is -addressable by name. +addressable by name. The resolver answers with both routing fields — the project +and its external_id — and every tool hands that pair to ``get_project_client`` +verbatim, which is what keeps a cloud mount bound to the workspace whose listing +advertised it. Collision rule: the project always wins over a same-named top-level folder in the default project, so that folder is only reachable unqualified when a single @@ -157,7 +160,7 @@ async def cat( if route.stripped and not route.path: raise ValueError(f"cat: '{identifier}' names a project, not a note") - async with get_project_client(route.project, context=context, project_id=project_id) as ( + async with get_project_client(route.project, context=context, project_id=route.project_id) as ( client, active_project, ): @@ -257,7 +260,7 @@ async def grep( retrieval_mode=_grep_retrieval_mode(literal), entity_types=[SearchItemType.ENTITY], ) - async with get_project_client(route.project, context=context, project_id=project_id) as ( + async with get_project_client(route.project, context=context, project_id=route.project_id) as ( client, active_project, ): @@ -359,7 +362,7 @@ async def ls( ) list_path = f"/{route.path}" if route.stripped else path - async with get_project_client(route.project, context=context, project_id=project_id) as ( + async with get_project_client(route.project, context=context, project_id=route.project_id) as ( client, active_project, ): @@ -426,7 +429,7 @@ async def find( ) list_path = f"/{route.path}" if route.stripped else path - async with get_project_client(route.project, context=context, project_id=project_id) as ( + async with get_project_client(route.project, context=context, project_id=route.project_id) as ( client, active_project, ): @@ -485,7 +488,7 @@ async def tail( "", project=project, project_id=project_id, context=context ) - async with get_project_client(route.project, context=context, project_id=project_id) as ( + async with get_project_client(route.project, context=context, project_id=route.project_id) as ( client, active_project, ): diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index 96efda2b8..6e03c6c23 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -27,8 +27,12 @@ _detected_route_remainder, _project_routes_agree, resolve_project_path_route, + resolve_workspace_project_identifier, +) +from basic_memory.mcp.project_context_identifiers import ( + split_workspace_route_segments, + unqualified_project_identifier, ) -from basic_memory.mcp.project_context_identifiers import unqualified_project_identifier from basic_memory.mcp.tools import grep, ls from basic_memory.schemas.cloud import WorkspaceInfo from basic_memory.schemas.project_info import ProjectItem, ProjectList @@ -71,7 +75,8 @@ def empty_project_config(config_manager): @pytest.mark.asyncio async def test_project_id_bypasses_prefix_parsing(multi_project_config): """project_id routes by external UUID, so even a conflicting-looking prefix - is never examined — documented limitation, mirroring read_note.""" + is never examined — documented limitation, mirroring read_note. The route + echoes the caller's id so tool call sites can pass the route alone.""" route = await resolve_project_path_route( "second-project/notes/foo", project="test-project", @@ -79,7 +84,10 @@ async def test_project_id_bypasses_prefix_parsing(multi_project_config): ) assert route == ProjectPathRoute( - project="test-project", path="second-project/notes/foo", stripped=False + project="test-project", + path="second-project/notes/foo", + stripped=False, + project_id="11111111-1111-1111-1111-111111111111", ) @@ -131,6 +139,33 @@ async def test_display_name_matches_by_permalink(multi_project_config): assert route == ProjectPathRoute(project="My Research", path="notes/foo", stripped=True) +@pytest.mark.asyncio +async def test_multi_segment_project_permalink_routes(config_manager, tmp_path_factory): + """Project names may contain '/', and generate_permalink keeps it, so a + mount's permalink can span several segments. Comparing only the first + segment advertised '/research/2026' at the root and then could not enter it; + the longest matching permalink wins, so the nested name beats its own + prefix.""" + config = config_manager.load_config() + config.projects["Research"] = ProjectEntry(path=str(tmp_path_factory.mktemp("research"))) + config.projects["Research/2026"] = ProjectEntry( + path=str(tmp_path_factory.mktemp("research-2026")) + ) + config_manager.save_config(config) + + root = await resolve_project_path_route("research/2026", project=None, project_id=None) + assert root == ProjectPathRoute(project="Research/2026", path="", stripped=True) + + nested = await resolve_project_path_route( + "research/2026/notes/x", project=None, project_id=None + ) + assert nested == ProjectPathRoute(project="Research/2026", path="notes/x", stripped=True) + + # The one-segment mount still claims paths its longer sibling does not. + shallow = await resolve_project_path_route("research/notes/x", project=None, project_id=None) + assert shallow == ProjectPathRoute(project="Research", path="notes/x", stripped=True) + + @pytest.mark.asyncio async def test_glob_first_segment_never_routes(multi_project_config): """split_project_prefix's '*' guard: a glob first segment is search input, @@ -190,6 +225,16 @@ def test_detected_route_remainder_bare_prefix_resolved_into_workspace_consumes_o assert _detected_route_remainder("research/notes/foo", "other/research") == "notes/foo" +def test_split_workspace_route_segments_needs_two_named_segments(): + """The path form makes the trailing path optional, but both route segments + still have to be there: one segment names no project, and an empty one (a + '//' in the input) names nothing at all.""" + assert split_workspace_route_segments("acme") is None + assert split_workspace_route_segments("acme//docs") is None + assert split_workspace_route_segments("acme/docs") == ("acme", "docs", "") + assert split_workspace_route_segments("acme/docs/notes/x") == ("acme", "docs", "notes/x") + + def test_project_routes_agree_across_mixed_qualification(): """A workspace-qualified spelling agrees with the unqualified spelling of the same project, in either direction; different projects never agree.""" @@ -434,7 +479,12 @@ async def test_cloud_qualified_path_routes_to_that_project(cloud_session): route = await resolve_project_path_route("research/notes/x", project=None, project_id=None) - assert route == ProjectPathRoute(project="research", path="notes/x", stripped=True) + assert route == ProjectPathRoute( + project="research", + path="notes/x", + stripped=True, + project_id="research-external-id", + ) assert unqualified_project_identifier(route.project or "") == "research" @@ -451,6 +501,41 @@ async def test_cloud_workspace_qualified_path_without_mount_collision_still_rout assert route == ProjectPathRoute(project="team/research", path="notes/x", stripped=True) +@pytest.mark.asyncio +async def test_cloud_workspace_qualified_project_root_routes(cloud_session): + """'/' with no path names that project's root, exactly as + a bare mount name does. Only the three-segment form used to resolve, so a + cross-workspace project could be listed into ('acme/docs/notes') but its own + root ('acme/docs') had no spelling at all — it fell through to the + multi-project refusal.""" + cloud_session("research", "engineering") + + route = await resolve_project_path_route("team/research", project=None, project_id=None) + + assert route == ProjectPathRoute(project="team/research", path="", stripped=True) + + +@pytest.mark.asyncio +async def test_cloud_workspace_project_root_falls_through_without_workspaces( + cloud_session, monkeypatch +): + """Workspace discovery stays best-effort for prefix detection: with no + reachable workspace, '/' is simply not a route and lands + on the ordinary refusal. Input that shaped like a workspace route may never + have meant one, so a discovery failure must not become the caller's error.""" + cloud_session("research", "engineering") + + async def no_workspaces(context=None) -> list[WorkspaceInfo]: + return [] + + monkeypatch.setattr(project_context, "get_available_workspaces", no_workspaces) + + with pytest.raises(UnqualifiedPathRefusedError) as excinfo: + await resolve_project_path_route("acme/docs", project=None, project_id=None) + + assert str(excinfo.value) == "no project 'acme' — active projects: engineering/, research/" + + @pytest.mark.asyncio async def test_cloud_mount_wins_over_colliding_workspace_slug(cloud_session): """The collision: 'team' is both an advertised mount and this workspace's @@ -462,7 +547,9 @@ async def test_cloud_mount_wins_over_colliding_workspace_slug(cloud_session): route = await resolve_project_path_route("team/docs/x", project=None, project_id=None) - assert route == ProjectPathRoute(project="team", path="docs/x", stripped=True) + assert route == ProjectPathRoute( + project="team", path="docs/x", stripped=True, project_id="team-external-id" + ) @pytest.mark.asyncio @@ -497,13 +584,19 @@ async def test_cloud_every_advertised_mount_is_addressable_under_slug_collision( for node in mounts["nodes"]: permalink = node["permalink"] + external_id = f"{permalink}-external-id" + root_route = await resolve_project_path_route(permalink, project=None, project_id=None) - assert root_route == ProjectPathRoute(project=node["name"], path="", stripped=True) + assert root_route == ProjectPathRoute( + project=node["name"], path="", stripped=True, project_id=external_id + ) path_route = await resolve_project_path_route( f"{permalink}/notes/x", project=None, project_id=None ) - assert path_route == ProjectPathRoute(project=node["name"], path="notes/x", stripped=True) + assert path_route == ProjectPathRoute( + project=node["name"], path="notes/x", stripped=True, project_id=external_id + ) @pytest.mark.asyncio @@ -592,3 +685,156 @@ async def test_cloud_project_listing_is_fetched_once_per_request(cloud_session): assert second.stripped is True assert session.listings == [] + + +# --- mounts stay bound to the workspace that advertised them (#1421) --- +# A hosted session's own route is one tenant, but workspace discovery reaches +# every workspace the account can see, and project names are unique only inside +# one of them. These tests stand up that shape: two accessible workspaces, the +# session bound to the non-default one, and the same project permalink in both. + +_SESSION_DOCS_ID = "11111111-1111-1111-1111-111111111111" +_DEFAULT_DOCS_ID = "22222222-2222-2222-2222-222222222222" + + +@dataclass +class _FakeHttpClient: + """Stands in for the routed client, carrying only the workspace selector.""" + + workspace: Optional[str] + + +@pytest.fixture +def cross_workspace_session(monkeypatch, config_manager): + """Build a factory session on a non-default workspace beside the default one. + + ``get_client()`` with no selector is the session's own tenant — what the + hosted server hands a call that names no workspace, and therefore what the + mount view lists. ``get_client(workspace=...)`` is how the workspace index + reaches each accessible tenant, so the two answer with different projects. + """ + + def build(*, failed_tenant: Optional[str] = None) -> tuple[WorkspaceInfo, WorkspaceInfo]: + config = config_manager.load_config() + config.projects = {} + config.default_project = None + config_manager.save_config(config) + + session_workspace = WorkspaceInfo( + tenant_id="session-tenant", + workspace_type="organization", + slug="beta", + name="Beta", + role="editor", + is_default=False, + ) + default_workspace = WorkspaceInfo( + tenant_id="default-tenant", + workspace_type="personal", + slug="acme", + name="Acme", + role="owner", + is_default=True, + ) + listings = { + "session-tenant": ProjectList( + projects=[ + ProjectItem( + id=1, + external_id=_SESSION_DOCS_ID, + name="docs", + path="/app/data/docs", + is_default=True, + ) + ], + default_project="docs", + ), + "default-tenant": ProjectList( + projects=[ + ProjectItem( + id=1, + external_id=_DEFAULT_DOCS_ID, + name="docs", + path="/app/data/docs", + is_default=True, + ) + ], + default_project="docs", + ), + } + + @asynccontextmanager + async def fake_get_client(*args, **kwargs) -> AsyncIterator[object]: + yield _FakeHttpClient(workspace=kwargs.get("workspace")) + + async def fake_list_projects(self) -> ProjectList: + tenant = self.http_client.workspace or session_workspace.tenant_id + if tenant == failed_tenant: + raise RuntimeError(f"tenant {tenant} is unavailable") + return listings[tenant] + + async def fake_get_available_workspaces(context=None) -> list[WorkspaceInfo]: + return [session_workspace, default_workspace] + + monkeypatch.setattr(async_client, "is_factory_mode", lambda: True) + monkeypatch.setattr(async_client, "get_client", fake_get_client) + monkeypatch.setattr( + "basic_memory.mcp.clients.project.ProjectClient.list_projects", + fake_list_projects, + ) + monkeypatch.setattr( + project_context, + "get_available_workspaces", + fake_get_available_workspaces, + ) + return session_workspace, default_workspace + + return build + + +@pytest.mark.asyncio +async def test_cloud_mount_routes_by_the_id_that_names_its_workspace(cross_workspace_session): + """The mount view lists this session's own tenant, so a mount it advertises + has to route there. Carrying only the bare name let the cross-workspace + index re-resolve 'docs' by the is_default flag: on a first call — before any + workspace is cached — `cat("docs/x")` read the default workspace's 'docs' + under a name the session workspace advertised.""" + session_workspace, default_workspace = cross_workspace_session() + + route = await resolve_project_path_route("docs/x", project=None, project_id=None) + + assert route == ProjectPathRoute( + project="docs", path="x", stripped=True, project_id=_SESSION_DOCS_ID + ) + + # The id is what the shared index consumes, and it is the half that decides: + # by id the route lands in the session's workspace, while the bare name + # still falls through to whichever workspace carries the default flag. + by_id = await resolve_workspace_project_identifier(route.project_id or "") + by_name = await resolve_workspace_project_identifier("docs") + + assert by_id.workspace.tenant_id == session_workspace.tenant_id + assert by_name.workspace.tenant_id == default_workspace.tenant_id + + +@pytest.mark.asyncio +async def test_cloud_explicit_qualified_project_drops_the_mount_id(cross_workspace_session): + """The escape hatch keeps working: an explicit '/' names + a workspace of its own, so it must not inherit the mount's id and be routed + back to this session's tenant.""" + cross_workspace_session() + + route = await resolve_project_path_route("docs/x", project="acme/docs", project_id=None) + + assert route == ProjectPathRoute(project="acme/docs", path="x", stripped=True, project_id=None) + + +@pytest.mark.asyncio +async def test_cloud_workspace_project_root_surfaces_a_failed_workspace(cross_workspace_session): + """A '/' root whose workspace could not be listed is a + real failure, not an unrecognized path: it must say so rather than fall + through to a refusal that claims the project does not exist.""" + cross_workspace_session(failed_tenant="default-tenant") + + with pytest.raises(ValueError, match="could not be loaded"): + await resolve_project_path_route("acme/docs", project=None, project_id=None) From cf8eea29ef7e31d30bcf1efde957edefabf15c5c Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 16:47:41 -0500 Subject: [PATCH 05/18] fix(mcp): route only explicitly workspace-qualified posix paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule 4 of resolve_project_path_route called the generic identifier detector, which — when no advertised mount claimed the leading segments and the input was not '//' — fell back to resolving the bare first segment against every accessible workspace. In a hosted session that is a cross-tenant read. With the current workspace holding only 'research' and another accessible workspace holding 'notes', the ordinary project-relative path `cat("notes/foo")` resolved to ProjectPathRoute(project='acme/notes', path='foo') and served the other workspace's project. This is the same class of bug as the preceding commit, reached by a different path: there a bare project name was re-resolved across workspaces and fell to the is_default flag; here an unqualified first path segment does. Both violate the rule this PR establishes — an identifier that names no addressable mount is refused, not resolved to a plausible candidate somewhere else. Rule 4 now parses only fully qualified routes: both segments must match, the first an accessible workspace slug and the second a project inside that workspace. _detect_workspace_project_root and _detected_route_remainder collapse into one _detect_workspace_qualified_- route that handles the pathless root and the path form together and returns the remainder from the same parse that matched the route, so the two can no longer disagree about how many segments were consumed. That remainder helper existed only to special-case the bare-prefix fallback, which is the leak. Unchanged: advertised mounts still win the leading segments, explicit project='acme/docs' still routes, 'acme/docs/notes' and the 'acme/docs' root still resolve. An unqualified name that is not addressable here now raises UnqualifiedPathRefusedError naming this session's own mounts. detect_project_from_identifier_prefix keeps the bare-prefix fallback for read_note and search, which have no mount table and no refusal rule; it gains a direct test, since the posix resolver no longer exercises it. Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 98 +++++++------- tests/mcp/test_project_context.py | 64 +++++++++ tests/mcp/test_project_path_routing.py | 164 ++++++++++++++++++------ 3 files changed, 238 insertions(+), 88 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index a71c2aed9..5f6acc0b5 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -1185,24 +1185,41 @@ def _claim_mount_prefix( return claimed -async def _detect_workspace_project_root( +async def _detect_workspace_qualified_route( candidate: str, config: BasicMemoryConfig, context: Optional[Context] = None, -) -> Optional[str]: - """Resolve a bare '/' candidate to that project's root. +) -> tuple[str, str] | None: + """Resolve an explicitly qualified '/[/]' candidate. + + Returns the qualified project identifier and the project-relative remainder, + or None when the candidate does not spell a reachable workspace route. + + Both segments must match — the first an accessible workspace slug, the + second a project inside *that* workspace — so this never reaches a project + the caller did not name. Resolving only the first segment and searching + every accessible workspace for it is what let an ordinary project-relative + path ('notes/foo', where this session's workspace has no 'notes') route + into another tenant's same-named project (#1421). An unqualified first + segment now falls through to the refusal below, which names the mounts this + session can actually address. Workspace-qualified memory URLs require three segments so that 'memory://main/notes' stays readable as project 'main'. A posix path only - reaches here after the mount table declined its first segment, so nothing - addressable can be meant by it and the two-segment form is unambiguous — - without this, 'ls acme/docs/notes' resolved while 'ls acme/docs' (that same - project's root) had no spelling at all. + reaches here after the mount table declined its leading segments, so nothing + addressable can be meant by it and the two-segment form unambiguously names + that project's root — without which 'ls acme/docs/notes' resolved while + 'ls acme/docs' (that same project's root) had no spelling at all. """ segments = _split_workspace_route_segments(candidate) - if segments is None or segments[2]: + if segments is None: return None - if not _cloud_workspace_discovery_available(config): + # One guard covers both shapes. For the three-segment form it matches the + # identifier detector this replaced: a local session holding cloud + # credentials may consult discovery for an unmistakable workspace route. A + # two-segment identifier never splits into three, so for the pathless root + # form the same call narrows to cloud-routed sessions, as it did before. + if not _workspace_identifier_discovery_available(candidate, config): return None try: @@ -1212,26 +1229,11 @@ async def _detect_workspace_project_root( return None raise - return resolution.project_identifier if resolution is not None else None - - -def _detected_route_remainder(candidate: str, detected: str) -> str: - """Return the project-relative path left after the detected route prefix. - - A local project consumes one leading segment. A workspace-qualified route - ('/') consumes two only when the candidate spelled both - segments; a bare project prefix resolved into a workspace consumed one. - """ - segments = candidate.split("/") - route_permalinks = generate_permalink(detected).split("/") - consumed = 1 - if ( - len(route_permalinks) == 2 - and len(segments) >= 2 - and [generate_permalink(segment) for segment in segments[:2]] == route_permalinks - ): - consumed = 2 - return "/".join(segments[consumed:]) + if resolution is None: + return None + # The remainder comes straight from the parse, so the route and the path it + # leaves behind can never disagree about how many segments were consumed. + return resolution.project_identifier, segments[2] def _project_routes_agree(detected: str, explicit: str) -> bool: @@ -1267,11 +1269,12 @@ async def resolve_project_path_route( explicit '/' outlives a bare local prefix match. 3. Otherwise leading segments naming an addressable project route there with the remainder as the project-relative path. - 4. Otherwise a workspace-qualified '/[/]' spelling - routes to that project in that workspace — those projects belong to - workspaces this session's own route does not list, so they never appear - in the mount table rule 3 reads. With no path it names that project's - root, the same way a bare mount name does. + 4. Otherwise an explicitly workspace-qualified '/[/]' + spelling routes to that project in that workspace — those projects belong + to workspaces this session's own route does not list, so they never appear + in the mount table rule 3 reads. Both segments must match, so an + unqualified first segment never reaches another workspace. With no path it + names that project's root, the same way a bare mount name does. 5. Otherwise, when the session addresses more than one project, raise UnqualifiedPathRefusedError instead of silently defaulting; a session that addresses at most one project keeps today's default resolution. @@ -1317,26 +1320,21 @@ async def resolve_project_path_route( detected = mount.name mount_project_id = mount.external_id - # --- Rule 4: workspace-qualified spellings for everything else --- + # --- Rule 4: explicitly workspace-qualified spellings for everything else --- # Trigger: no advertised mount claimed the leading segments and the input still # has more than one segment to parse. - # Why: '//' addresses projects in workspaces this + # Why: '/[/]' addresses projects in workspaces this # session's own route does not list, so they are absent from the mount - # table above and would otherwise be unreachable. Local-config prefixes - # also resolve here — a locally routed session's config IS its mount - # table, so rule 3 already claimed those; what reaches this line is a - # cloud-routed session whose local config names a project its own tenant - # listing does not. - # Outcome: those spellings keep resolving exactly as before; the mount - # collision described above is the only address this ordering takes away. + # table above and would otherwise be unreachable. + # Outcome: only a route naming BOTH an accessible workspace and a project + # inside it resolves here. An unqualified first segment falls through to + # the refusal below instead of being searched for across every accessible + # workspace — that search read another tenant's same-named project under + # an ordinary project-relative path (#1421). if detected is None and "/" in candidate: - detected = await detect_project_from_identifier_prefix(candidate, config, context=context) - if detected is not None: - remainder = _detected_route_remainder(candidate, detected) - else: - # A bare '/' names that project's root, so the - # remainder stays empty — see _detect_workspace_project_root. - detected = await _detect_workspace_project_root(candidate, config, context=context) + qualified = await _detect_workspace_qualified_route(candidate, config, context=context) + if qualified is not None: + detected, remainder = qualified if explicit is not None: if detected is None: diff --git a/tests/mcp/test_project_context.py b/tests/mcp/test_project_context.py index 431b4ffa3..4a98fb6ff 100644 --- a/tests/mcp/test_project_context.py +++ b/tests/mcp/test_project_context.py @@ -1055,6 +1055,70 @@ async def fake_index(context=None, force_refresh=False): ) +@pytest.mark.asyncio +async def test_detect_project_from_identifier_prefix_falls_back_to_bare_project_name( + monkeypatch, +): + """An identifier that is not workspace-qualified still resolves its first + segment by searching every accessible workspace for a project of that name. + + This is the legacy detector read_note and search share. The posix path + resolver deliberately does NOT use it: a bare first segment that names no + addressable mount must refuse rather than reach another workspace's + same-named project (#1421), so its rule 4 parses only fully qualified + '/[/]' routes. Pinned here so the behavior these + two tools still depend on is exercised on its own terms. + """ + import basic_memory.mcp.project_context as project_context + from basic_memory.config import BasicMemoryConfig + from basic_memory.mcp.project_context import ( + WorkspaceProjectEntry, + _build_workspace_project_index, + detect_project_from_identifier_prefix, + ) + + personal = _workspace( + tenant_id="personal-tenant", + workspace_type="personal", + slug="personal", + name="Personal", + role="owner", + is_default=True, + ) + index = _build_workspace_project_index( + (personal,), + ( + WorkspaceProjectEntry( + workspace=personal, + project=_project("notes", id=1, external_id="personal-notes-id"), + ), + ), + ) + + async def fake_index(context=None, force_refresh=False): + return index + + monkeypatch.setattr(project_context, "_ensure_workspace_project_index", fake_index) + # A hosted (factory) session is what makes workspace discovery available to + # an identifier that is not itself workspace-qualified. + monkeypatch.setattr("basic_memory.mcp.async_client.is_factory_mode", lambda: True) + + # BasicMemoryConfig always materializes a placeholder 'main' entry, so the + # local-prefix check ahead of the fallback sees a config that names no 'notes'. + config = BasicMemoryConfig(projects={}, cloud_api_key="bmc_test123") + + # 'notes/...' names no workspace, so the qualified parse declines and the + # bare first segment is resolved across workspaces instead. + assert ( + await detect_project_from_identifier_prefix("notes/to-do-list", config) == "personal/notes" + ) + + # A first segment that names no project anywhere is an ordinary path, not a + # route: the lookup miss stays best-effort and leaves the identifier unrouted + # rather than becoming the caller's error. + assert await detect_project_from_identifier_prefix("absent/to-do-list", config) is None + + @pytest.mark.asyncio async def test_resolve_workspace_qualified_memory_url_ignores_workspace_project_miss( monkeypatch, diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index 6e03c6c23..a4c897be8 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -24,7 +24,6 @@ ProjectPathRoute, ProjectPrefixConflictError, UnqualifiedPathRefusedError, - _detected_route_remainder, _project_routes_agree, resolve_project_path_route, resolve_workspace_project_identifier, @@ -211,18 +210,10 @@ async def test_conflicting_prefix_raises_naming_both(multi_project_config): # --- workspace-qualified spellings --- -# The remainder/agreement helpers are pure functions; driving the qualified -# spellings through them directly avoids standing up cloud workspace discovery. - - -def test_detected_route_remainder_spelled_workspace_route_consumes_two_segments(): - """A workspace-qualified route spelled as both path segments consumes both.""" - assert _detected_route_remainder("other/research/notes/foo", "other/research") == "notes/foo" - - -def test_detected_route_remainder_bare_prefix_resolved_into_workspace_consumes_one(): - """A bare project prefix that resolved into a workspace consumed one segment.""" - assert _detected_route_remainder("research/notes/foo", "other/research") == "notes/foo" +# The agreement helper is a pure function; driving the qualified spellings +# through it directly avoids standing up cloud workspace discovery. The +# remainder is no longer derived separately — it comes from the same parse that +# matched the route, so the qualified-path tests below pin it end to end. def test_split_workspace_route_segments_needs_two_named_segments(): @@ -695,6 +686,21 @@ async def test_cloud_project_listing_is_fetched_once_per_request(cloud_session): _SESSION_DOCS_ID = "11111111-1111-1111-1111-111111111111" _DEFAULT_DOCS_ID = "22222222-2222-2222-2222-222222222222" +_SESSION_RESEARCH_ID = "33333333-3333-3333-3333-333333333333" +_SESSION_ENGINEERING_ID = "44444444-4444-4444-4444-444444444444" +_DEFAULT_NOTES_ID = "55555555-5555-5555-5555-555555555555" + +# Every (workspace, project) pair gets its own id: the same project name living +# in two workspaces is the whole point of this section, and the id is what pins +# a mount to the workspace that advertised it. UUID-shaped because the index +# looks an external_id up as a UUID before falling back to a name search. +_WORKSPACE_PROJECT_IDS = { + ("session-tenant", "docs"): _SESSION_DOCS_ID, + ("session-tenant", "research"): _SESSION_RESEARCH_ID, + ("session-tenant", "engineering"): _SESSION_ENGINEERING_ID, + ("default-tenant", "docs"): _DEFAULT_DOCS_ID, + ("default-tenant", "notes"): _DEFAULT_NOTES_ID, +} @dataclass @@ -704,6 +710,23 @@ class _FakeHttpClient: workspace: Optional[str] +def _tenant_listing(tenant_id: str, names: tuple[str, ...]) -> ProjectList: + """Build one tenant's project listing, the first name carrying is_default.""" + return ProjectList( + projects=[ + ProjectItem( + id=index + 1, + external_id=_WORKSPACE_PROJECT_IDS[(tenant_id, name)], + name=name, + path=f"/app/data/{generate_permalink(name)}", + is_default=index == 0, + ) + for index, name in enumerate(names) + ], + default_project=names[0], + ) + + @pytest.fixture def cross_workspace_session(monkeypatch, config_manager): """Build a factory session on a non-default workspace beside the default one. @@ -714,7 +737,12 @@ def cross_workspace_session(monkeypatch, config_manager): reaches each accessible tenant, so the two answer with different projects. """ - def build(*, failed_tenant: Optional[str] = None) -> tuple[WorkspaceInfo, WorkspaceInfo]: + def build( + *, + failed_tenant: Optional[str] = None, + session_projects: tuple[str, ...] = ("docs",), + default_projects: tuple[str, ...] = ("docs",), + ) -> tuple[WorkspaceInfo, WorkspaceInfo]: config = config_manager.load_config() config.projects = {} config.default_project = None @@ -737,30 +765,8 @@ def build(*, failed_tenant: Optional[str] = None) -> tuple[WorkspaceInfo, Worksp is_default=True, ) listings = { - "session-tenant": ProjectList( - projects=[ - ProjectItem( - id=1, - external_id=_SESSION_DOCS_ID, - name="docs", - path="/app/data/docs", - is_default=True, - ) - ], - default_project="docs", - ), - "default-tenant": ProjectList( - projects=[ - ProjectItem( - id=1, - external_id=_DEFAULT_DOCS_ID, - name="docs", - path="/app/data/docs", - is_default=True, - ) - ], - default_project="docs", - ), + "session-tenant": _tenant_listing("session-tenant", session_projects), + "default-tenant": _tenant_listing("default-tenant", default_projects), } @asynccontextmanager @@ -838,3 +844,85 @@ async def test_cloud_workspace_project_root_surfaces_a_failed_workspace(cross_wo with pytest.raises(ValueError, match="could not be loaded"): await resolve_project_path_route("acme/docs", project=None, project_id=None) + + +# --- an unqualified first segment never leaves this session's workspace (#1421) --- +# The companion to mount-id binding above. That one covers a name present in +# both workspaces; these cover a name present only in the *other* one, where +# there is no mount to claim the segment and the workspace fallback used to +# resolve the bare name across every accessible workspace. + + +@pytest.mark.asyncio +async def test_cloud_unqualified_first_segment_never_reaches_another_workspace( + cross_workspace_session, +): + """The leak: this session's workspace has no 'notes', another accessible + workspace does, and `cat("notes/foo")` is an ordinary project-relative path. + Resolving the bare first segment against every accessible workspace found + the other tenant's 'notes' and read it. It must refuse instead, naming only + the mounts this session can actually address.""" + cross_workspace_session( + session_projects=("research", "engineering"), + default_projects=("notes",), + ) + + with pytest.raises(UnqualifiedPathRefusedError) as excinfo: + await resolve_project_path_route("notes/foo", project=None, project_id=None) + + # The other workspace's project is named nowhere in the refusal — it was + # never addressable from here, so advertising it would teach a wrong route. + assert str(excinfo.value) == ("no project 'notes' — active projects: engineering/, research/") + + +@pytest.mark.asyncio +async def test_cloud_unqualified_first_segment_stays_local_in_a_single_project_workspace( + cross_workspace_session, +): + """Same shape, one project in this workspace: rule 5 has no ambiguity to + refuse, so the path stays unstripped for the ordinary default resolution. + The point is where it does *not* go — a lone mount must not make the + cross-workspace name lookup the tiebreaker.""" + cross_workspace_session(session_projects=("research",), default_projects=("notes",)) + + route = await resolve_project_path_route("notes/foo", project=None, project_id=None) + + assert route == ProjectPathRoute(project=None, path="notes/foo", stripped=False) + + +@pytest.mark.asyncio +async def test_cloud_empty_route_segment_is_not_a_workspace_route(cross_workspace_session): + """An empty segment names nothing, so 'acme//notes' is not the qualified + spelling of anything even though 'acme' is a real workspace slug. It refuses + rather than being repaired into a route the caller did not write.""" + cross_workspace_session( + session_projects=("research", "engineering"), + default_projects=("notes",), + ) + + with pytest.raises(UnqualifiedPathRefusedError) as excinfo: + await resolve_project_path_route("acme//notes", project=None, project_id=None) + + assert str(excinfo.value) == ("no project 'acme' — active projects: engineering/, research/") + + +@pytest.mark.asyncio +async def test_cloud_other_workspace_stays_reachable_when_qualified(cross_workspace_session): + """Refuse-don't-default takes no address away: naming the workspace is how a + caller reaches it deliberately, as a path and through the project param, and + both spellings still land on the other tenant's project.""" + cross_workspace_session( + session_projects=("research", "engineering"), + default_projects=("notes",), + ) + + qualified_path = await resolve_project_path_route( + "acme/notes/foo", project=None, project_id=None + ) + assert qualified_path == ProjectPathRoute(project="acme/notes", path="foo", stripped=True) + + root = await resolve_project_path_route("acme/notes", project=None, project_id=None) + assert root == ProjectPathRoute(project="acme/notes", path="", stripped=True) + + explicit = await resolve_project_path_route("foo", project="acme/notes", project_id=None) + assert explicit == ProjectPathRoute(project="acme/notes", path="foo", stripped=False) From d86200075700cc2e5f824868d3eee8ea6bac7c41 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 17:25:52 -0500 Subject: [PATCH 06/18] fix(mcp): make project identity and returned paths one rule each MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two invariants the posix routing layer kept restating per call site, and one cost that fell out of fixing the second. Project identity is a whole permalink, not a segment. Mount matching learned that last commit; the workspace parser had rediscovered the same wrong assumption, so a workspace holding 'Research/2026' parsed 'acme/research/2026/notes' as project 'research' plus path '2026/notes' and either missed or, beside a real 'research', served the wrong project. The fix is not a third careful parser: split_project_permalink_prefix is now the only function that turns a path into a project plus a project-relative path, and it takes the candidate set as an argument, because how many segments a project consumes is a fact about the known projects and never about the string. Mount routing, workspace routes, and local-config prefix detection all go through it. What closes the class is the deletion, not the helper: split_workspace_identifier_segments, split_workspace_route_segments, and split_workspace_memory_url_segments are gone. They were the shape-only parses that could express "the project is segment one", and nothing is left to copy from. What remains is split_workspace_slug_prefix, which splits only the workspace slug — genuinely one segment — and hands the rest to the matcher. Reverting the longest-match loop alone now fails both the mount test and the workspace test, which is the evidence they are one rule. A path a routed verb returns must be a path the resolver accepts. A qualified 'ls research' strips the mount before the project-scoped API sees it, so the API answers '/notes', and feeding that back refused as unqualified — or opened a different project mounted as 'notes'. Verbs that can strip a prefix now re-attach it through one function. Only addressing fields move: directory_path and file_path are what callers feed back. permalink is deliberately left alone, being an identity with its own canonical form. Two tests that asserted the qualified and --project spellings return identical payloads now assert the round trip instead: the payloads legitimately differ by addressing frame, and frame-independence was what broke the loop. bm tree resolved twice. Once find returns qualified paths, the root tree strips has to carry the prefix too, so find_listing returns the listing and that root from one resolution. A CLI invocation carries no FastMCP context, so the second resolution was a second project-list round trip on every cloud call, and a second workspace index build for a qualified path. Memory-URL strictness is unchanged and now stated where it is enforced: resolve_workspace_qualified_identifier requires a path after the project, which is what keeps 'memory://main/notes' readable as project 'main'. The posix resolver keeps the pathless form and says why at its own call site. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/cli/commands/posix.py | 39 ++-- src/basic_memory/mcp/project_context.py | 146 +++++++-------- .../mcp/project_context_identifiers.py | 103 +++++++---- src/basic_memory/mcp/tools/posix_tools.py | 166 ++++++++++++++---- tests/cli/test_cli_posix_verbs.py | 134 ++++++++++---- tests/mcp/test_project_path_routing.py | 61 ++++++- tests/mcp/test_tool_posix.py | 116 +++++++++++- 7 files changed, 554 insertions(+), 211 deletions(-) diff --git a/src/basic_memory/cli/commands/posix.py b/src/basic_memory/cli/commands/posix.py index 6cb2b6bc1..8f38634d4 100644 --- a/src/basic_memory/cli/commands/posix.py +++ b/src/basic_memory/cli/commands/posix.py @@ -47,9 +47,6 @@ console, ) -# project_context is already loaded at CLI import time (command_utils imports -# it), so this costs nothing beyond the deferred-MCP budget (#886). -from basic_memory.mcp.project_context import resolve_project_path_route from basic_memory.schemas.directory import DEFAULT_DIRECTORY_PAGE_SIZE # MCP tool functions are imported inside each command: importing @@ -825,33 +822,29 @@ def tree( # tree is a client-side recombination of find: the same flat listing, with # the hierarchy rebuilt from directory paths for display, so its JSON # contract is exactly find's payload. - from basic_memory.mcp.tools import find as mcp_find + from basic_memory.mcp.tools.posix_tools import find_listing from fastmcp.exceptions import ToolError try: validate_routing_flags(local, cloud) _validate_output_flags(json_output, plain) - async def _routed_find() -> tuple[dict[str, Any], str]: - # find strips a recognized '/' prefix (#1415), so node - # paths come back project-relative; the same resolver derives the - # root the hierarchy rebuild must strip, or every node's first - # segment would duplicate under a '/dir' root. - route = await resolve_project_path_route(path, project=project, project_id=project_id) - root = f"/{route.path}" if route.stripped else path - listing = await mcp_find( - path, - name=name, - depth=depth, - page=page, - page_size=page_size, - project=project, - project_id=project_id, - ) - return listing, root - + # find returns the listing and the root its node paths are relative to + # from one resolution. Resolving here as well cost a second project-list + # round trip on every cloud call, because a CLI invocation has no + # FastMCP context for the per-request cache to live in (#1421). with force_routing(local=local, cloud=cloud): - result, root = run_with_cleanup(_routed_find()) + result, root = run_with_cleanup( + find_listing( + path, + name=name, + depth=depth, + page=page, + page_size=page_size, + project=project, + project_id=project_id, + ) + ) mode = _resolve_output_mode(json_output, plain) if mode == "json": _print_json(result) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index 5f6acc0b5..abbbbf23b 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -63,10 +63,10 @@ identifier_path as _identifier_path, project_matches_identifier as _project_matches_identifier, split_project_prefix as _split_project_prefix, + is_workspace_route_shaped as _is_workspace_route_shaped, + split_project_permalink_prefix as _split_project_permalink_prefix, split_qualified_project_identifier as _split_qualified_project_identifier_impl, - split_workspace_identifier_segments as _split_workspace_identifier_segments, - split_workspace_memory_url_segments as _split_workspace_memory_url_segments, - split_workspace_route_segments as _split_workspace_route_segments, + split_workspace_slug_prefix as _split_workspace_slug_prefix, unqualified_project_identifier as _unqualified_project_identifier, ) from basic_memory.mcp.workspace_project_index import ( @@ -250,10 +250,7 @@ def _workspace_identifier_discovery_available( if _explicit_routing() and _force_local_mode(): return False - return ( - has_cloud_credentials(config) - and _split_workspace_identifier_segments(identifier) is not None - ) + return has_cloud_credentials(config) and _is_workspace_route_shaped(identifier) async def resolve_workspace_qualified_memory_url( @@ -261,32 +258,48 @@ async def resolve_workspace_qualified_memory_url( context: Optional[Context] = None, ) -> WorkspaceMemoryUrlResolution | None: """Resolve a workspace-qualified memory URL against accessible workspaces.""" - segments = _split_workspace_memory_url_segments(identifier) - if segments is None: + if not identifier.strip().startswith("memory://"): return None - - return await _resolve_workspace_segments(identifier, segments, context=context) + return await resolve_workspace_qualified_identifier(identifier, context=context) async def resolve_workspace_qualified_identifier( identifier: str, context: Optional[Context] = None, ) -> WorkspaceMemoryUrlResolution | None: - """Resolve a workspace-qualified permalink or memory URL against accessible workspaces.""" - segments = _split_workspace_identifier_segments(identifier) - if segments is None: + """Resolve a workspace-qualified permalink or memory URL against accessible workspaces. + + A path is required after the project. That is what keeps + 'memory://main/notes' readable as project 'main' with note 'notes': a + workspace route has to name something *inside* the project, or the same + string would have two readings and the project-prefix resolver would lose + the ones it has always owned. The posix resolver takes the pathless form + (a project root is a legitimate thing to list) and says so at its own call + site. + """ + resolved = await _resolve_workspace_route(identifier, context=context) + if resolved is None or not resolved[1]: return None - - return await _resolve_workspace_segments(identifier, segments, context=context) + return resolved[0] -async def _resolve_workspace_segments( +async def _resolve_workspace_route( identifier: str, - segments: tuple[str, str, str], context: Optional[Context] = None, -) -> WorkspaceMemoryUrlResolution | None: - """Resolve parsed workspace/project/path segments against accessible workspaces.""" - workspace_slug, project_identifier, remainder = segments +) -> tuple[WorkspaceMemoryUrlResolution, str] | None: + """Resolve '/[/]' to its project and remaining path. + + The project half is matched by ``split_project_permalink_prefix`` against + the projects of *that* workspace, so a project whose permalink spans + several segments ('Research/2026') is reachable and the remainder always + comes from the same match that chose the project — the two can never + disagree about how many segments were consumed. + """ + slug_split = _split_workspace_slug_prefix(identifier) + if slug_split is None: + return None + workspace_slug, rest = slug_split + index = await _ensure_workspace_project_index(context=context) workspace = next( (item for item in index.workspaces if item.slug.casefold() == workspace_slug.casefold()), @@ -295,13 +308,23 @@ async def _resolve_workspace_segments( if workspace is None: return None - project_permalink = generate_permalink(project_identifier) - matches = [ - entry - for entry in index.entries_by_permalink.get(project_permalink, ()) - if entry.workspace.tenant_id == workspace.tenant_id - ] - if not matches: + entries_by_permalink: dict[str, WorkspaceProjectEntry] = {} + for entry in index.entries: + if entry.workspace.tenant_id != workspace.tenant_id: + continue + collision = entries_by_permalink.setdefault(entry.project.permalink, entry) + if collision is not entry: + details = ", ".join( + f"{item.qualified_name} ({item.project.external_id})" for item in (collision, entry) + ) + raise ValueError( + f"Project '{entry.project.permalink}' matched multiple projects in workspace " + f"'{workspace.name}' ({workspace.slug}). Project permalinks must be unique. " + f"Matches: {details}" + ) + + claimed = _split_project_permalink_prefix(rest, entries_by_permalink) + if claimed is None: if any( failed_workspace.tenant_id == workspace.tenant_id for failed_workspace in index.failed_workspaces @@ -311,32 +334,24 @@ async def _resolve_workspace_segments( "could not be loaded. Retry after workspace discovery recovers." ) - # Trigger: first segment matches a workspace slug but the second does not - # match a project in that workspace. - # Why: workspace-qualified URLs require both route segments to match; otherwise + # Trigger: first segment matches a workspace slug but nothing after it + # matches a project in that workspace. + # Why: workspace-qualified routes require both halves to match; otherwise # existing project-prefixed URLs like `memory://main/notes/foo` can collide # with a workspace slug named `main`. # Outcome: treat this as not workspace-qualified and let the caller use # the existing project-prefix/default-project resolver. return None - if len(matches) > 1: - details = ", ".join( - f"{entry.qualified_name} ({entry.project.external_id})" for entry in matches - ) - raise ValueError( - f"Project '{project_identifier}' matched multiple projects in workspace " - f"'{workspace.name}' ({workspace.slug}). Project permalinks must be unique. " - f"Matches: {details}" - ) - entry = matches[0] + project_permalink, remainder = claimed + entry = entries_by_permalink[project_permalink] canonical_path = _canonical_memory_path_for_workspace( workspace_slug=entry.workspace.slug, workspace_type=entry.workspace.workspace_type, project_permalink=entry.project.permalink, remainder=remainder, ) - return WorkspaceMemoryUrlResolution(entry=entry, canonical_path=canonical_path) + return WorkspaceMemoryUrlResolution(entry=entry, canonical_path=canonical_path), remainder async def get_available_workspaces(context: Optional[Context] = None) -> list[WorkspaceInfo]: @@ -1164,25 +1179,16 @@ def _claim_mount_prefix( ) -> tuple[AddressableProject, str] | None: """Return the mount whose permalink claims the candidate's leading segments. - A project name may itself contain '/', and generate_permalink keeps that - separator, so a project named 'Research/2026' advertises the two-segment - mount '/research/2026'. Comparing only the first segment would leave that - mount listed at the root and impossible to enter, so the whole permalink has - to match; the longest match wins, which is also the only reading that can be - right when one mount's permalink prefixes another's. + The mount table is one candidate set among several, so the matching itself + lives in ``split_project_permalink_prefix``; this only maps the winning + permalink back to the project that owns it. """ - segments = candidate.split("/") - claimed: tuple[AddressableProject, str] | None = None - claimed_depth = 0 - for project in projects: - depth = project.permalink.count("/") + 1 - if depth > len(segments) or depth <= claimed_depth: - continue - if generate_permalink("/".join(segments[:depth])) != project.permalink: - continue - claimed = (project, "/".join(segments[depth:])) - claimed_depth = depth - return claimed + by_permalink = {project.permalink: project for project in projects} + claimed = _split_project_permalink_prefix(candidate, by_permalink) + if claimed is None: + return None + permalink, remainder = claimed + return by_permalink[permalink], remainder async def _detect_workspace_qualified_route( @@ -1211,29 +1217,31 @@ async def _detect_workspace_qualified_route( that project's root — without which 'ls acme/docs/notes' resolved while 'ls acme/docs' (that same project's root) had no spelling at all. """ - segments = _split_workspace_route_segments(candidate) - if segments is None: + if _split_workspace_slug_prefix(candidate) is None: return None # One guard covers both shapes. For the three-segment form it matches the # identifier detector this replaced: a local session holding cloud # credentials may consult discovery for an unmistakable workspace route. A - # two-segment identifier never splits into three, so for the pathless root - # form the same call narrows to cloud-routed sessions, as it did before. + # two-segment identifier is not route-shaped, so for the pathless root form + # the same call narrows to cloud-routed sessions, as it did before. if not _workspace_identifier_discovery_available(candidate, config): return None try: - resolution = await _resolve_workspace_segments(candidate, segments, context=context) + resolved = await _resolve_workspace_route(candidate, context=context) except ValueError as exc: if any(error in str(exc).lower() for error in _WORKSPACE_DISCOVERY_FALLBACK_ERRORS): return None raise - if resolution is None: + if resolved is None: return None - # The remainder comes straight from the parse, so the route and the path it - # leaves behind can never disagree about how many segments were consumed. - return resolution.project_identifier, segments[2] + # Unlike the memory-URL caller, the pathless form is kept: a project root is + # a legitimate thing to list. The remainder comes from the same match that + # chose the project, so the route and the path it leaves behind can never + # disagree about how many segments were consumed. + resolution, remainder = resolved + return resolution.project_identifier, remainder def _project_routes_agree(detected: str, explicit: str) -> bool: diff --git a/src/basic_memory/mcp/project_context_identifiers.py b/src/basic_memory/mcp/project_context_identifiers.py index 4e866dfe0..67464f262 100644 --- a/src/basic_memory/mcp/project_context_identifiers.py +++ b/src/basic_memory/mcp/project_context_identifiers.py @@ -3,7 +3,7 @@ from __future__ import annotations from dataclasses import dataclass -from typing import Optional +from typing import Iterable, Optional from basic_memory.config import BasicMemoryConfig from basic_memory.mcp.workspace_project_index import WorkspaceProjectEntry @@ -91,43 +91,71 @@ def identifier_path(identifier: str) -> str: return memory_url_path(stripped) if stripped.startswith("memory://") else stripped -def split_workspace_identifier_segments(identifier: str) -> tuple[str, str, str] | None: - """Split ``//`` identifiers into route segments.""" - normalized = normalize_project_reference(identifier_path(identifier)).strip("/") - parts = normalized.split("/", 2) - if len(parts) != 3: +def split_project_permalink_prefix( + path: str, + permalinks: Iterable[str], +) -> tuple[str, str] | None: + """Split a path into the project permalink it names and the remainder. + + This is the *only* way to turn a path-shaped input into a project plus a + project-relative path, and it deliberately takes the candidate set as an + argument: a project name may contain '/', and ``generate_permalink`` + preserves it, so how many leading segments a project consumes is a fact + about the known projects, never about the string. Callers that split on + the first '/' instead kept rediscovering that — for mounts (#1421), then + again for workspace routes. + + The longest permalink wins, which is also the only reading that can be + right when one project's permalink prefixes another's ('research' beside + 'research/2026'). Returns None when no permalink claims the leading + segments; the remainder has no leading slash and is "" at the root. + """ + segments = normalize_project_reference(identifier_path(path)).strip("/").split("/") + # An empty interior segment ('a//b') names nothing. Matching around it would + # repair a malformed path into a route the caller did not write. + if not all(segments): return None - workspace_slug, project_identifier, remainder = parts - if not workspace_slug or not project_identifier or not remainder: + + claimed: tuple[str, str] | None = None + claimed_depth = 0 + for permalink in permalinks: + depth = permalink.count("/") + 1 + if depth > len(segments) or depth <= claimed_depth: + continue + if generate_permalink("/".join(segments[:depth])) != permalink: + continue + claimed = (permalink, "/".join(segments[depth:])) + claimed_depth = depth + return claimed + + +def split_workspace_slug_prefix(identifier: str) -> tuple[str, str] | None: + """Split ``/`` — the workspace half of a qualified route. + + A workspace slug is always one segment, so this parse is pure shape. What + follows it is a project permalink plus a path, which only + ``split_project_permalink_prefix`` can separate. + """ + normalized = normalize_project_reference(identifier_path(identifier)).strip("/") + workspace_slug, _, rest = normalized.partition("/") + # A rest starting with '/' means the project segment is empty ('acme//notes'). + # The caller wrote nothing there, so this is not a workspace route. + if not workspace_slug or not rest or rest.startswith("/"): return None - return workspace_slug, project_identifier, remainder + return workspace_slug, rest -def split_workspace_route_segments(identifier: str) -> tuple[str, str, str] | None: - """Split ``/[/]`` where the trailing path may be empty. +def is_workspace_route_shaped(identifier: str) -> bool: + """True when an identifier has enough segments to spell workspace/project/path. - The strict three-segment parse above is what memory URLs need: there, - ``memory://main/notes`` has to stay readable as project ``main``. A posix - path only reaches this looser parse after the advertised mount table has - declined its first segment, so no addressable project can be meant by it and - the bare ``/`` form unambiguously names that project's - root. + A shape question only — it gates whether workspace discovery may be + consulted at all, and deliberately says nothing about where the project + ends. Three segments is the unmistakable form: fewer could equally be a + project-relative path in the session's own project. """ normalized = normalize_project_reference(identifier_path(identifier)).strip("/") parts = normalized.split("/", 2) - if len(parts) < 2: - return None - workspace_slug, project_identifier = parts[0], parts[1] - if not workspace_slug or not project_identifier: - return None - return workspace_slug, project_identifier, parts[2] if len(parts) == 3 else "" - - -def split_workspace_memory_url_segments(identifier: str) -> tuple[str, str, str] | None: - """Split ``memory:////`` into route segments.""" - if not identifier.strip().startswith("memory://"): - return None - return split_workspace_identifier_segments(identifier) + return len(parts) == 3 and all(parts) def canonical_memory_path_for_workspace( @@ -223,11 +251,16 @@ def detect_project_from_url_prefix( """Return the local config project matching a memory URL path prefix.""" path = memory_url_path(identifier) if identifier.strip().startswith("memory://") else identifier normalized = normalize_project_reference(path) + # The '*' guard belongs to the glob case: a globbed first segment is search + # input, not a project prefix, and must not be matched against any project. prefix, _ = split_project_prefix(normalized) if prefix is None: return None - prefix_permalink = generate_permalink(prefix) - for project_name in config.projects: - if generate_permalink(project_name) == prefix_permalink: - return project_name - return None + + by_permalink = {generate_permalink(name): name for name in config.projects} + claimed = split_project_permalink_prefix(normalized, by_permalink) + # A prefix with no remainder names the project itself, not a path in it; + # split_project_prefix already rejected that shape above. + if claimed is None or not claimed[1]: + return None + return by_permalink[claimed[0]] diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index 233efdf68..e8a439573 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -39,6 +39,7 @@ from basic_memory.mcp.container import get_container from basic_memory.mcp.note_reads import read_note_json_by_external_id from basic_memory.mcp.project_context import ( + ProjectPathRoute, addressable_projects, get_project_client, resolve_project_path_route, @@ -51,6 +52,60 @@ DirectoryNode, ) from basic_memory.schemas.search import SearchItemType, SearchQuery, SearchRetrievalMode +from basic_memory.utils import generate_permalink + +# --- Round-trip coherence --- +# A path a routed verb returns must be a path the resolver accepts. When a call +# addresses its project in the path ('ls research/notes'), the project prefix is +# stripped before the project-scoped API sees it, so that API answers in +# project-relative paths — '/notes'. Handing those back unchanged breaks the +# navigation loop the mount model promises: feeding '/notes' into `ls` refuses as +# unqualified, or worse, opens a *different* project that happens to be mounted +# as 'notes'. +# +# So every verb that can strip a prefix re-attaches it to the paths it returns, +# through this one function. Only addressing fields are re-qualified: +# `directory_path` and `file_path` are what a caller feeds back to `ls`, `find`, +# and `cat`. `permalink` is deliberately left alone — it is an identity with its +# own canonical form (permalinks_include_project, workspace qualification), and +# re-prefixing it here would mint a second, competing spelling of it. +_ROUTED_PATH_FIELDS = frozenset({"directory_path", "file_path"}) + + +def _requalified_path(value: str, prefix: str) -> str: + """Re-attach a stripped project prefix, preserving the field's slash shape.""" + if value.startswith("/"): + return f"/{prefix}" if value == "/" else f"/{prefix}{value}" + return f"{prefix}/{value}" if value else prefix + + +def _requalify(payload: Any, prefix: str) -> Any: + """Rewrite addressing fields anywhere in a response payload.""" + if isinstance(payload, dict): + return { + key: _requalified_path(value, prefix) + if key in _ROUTED_PATH_FIELDS and isinstance(value, str) + else _requalify(value, prefix) + for key, value in payload.items() + } + if isinstance(payload, list): + return [_requalify(item, prefix) for item in payload] + return payload + + +def qualify_routed_paths(payload: Any, route: ProjectPathRoute) -> Any: + """Return ``payload`` with the project prefix ``route`` stripped put back. + + A no-op when the route stripped nothing — then the caller addressed the + project some other way (an explicit param, or a single-project session) and + the project-relative paths it gets back are the ones it can feed back. + """ + if not route.stripped or route.project is None: + return payload + # The permalink form is the spelling `ls "/"` advertises and the resolver + # normalizes to, so it round-trips for display names too ('My Research'). + return _requalify(payload, generate_permalink(route.project)) + # The manual project holds the non-bundled manual pages as ordinary notes; # `man` falls back to it for page reads and searches it in query mode. @@ -182,7 +237,7 @@ async def cat( ) if server_side_slice or (start_line is None and end_line is None): - return payload + return qualify_routed_paths(payload, route) lines = str(payload["content"]).splitlines() total_lines = len(lines) @@ -192,7 +247,7 @@ async def cat( payload["start_line"] = first payload["end_line"] = last payload["total_lines"] = total_lines - return payload + return qualify_routed_paths(payload, route) def _grep_retrieval_mode(literal: bool) -> SearchRetrievalMode: @@ -371,22 +426,26 @@ async def ls( directory_client = DirectoryClient(client, active_project.external_id) listing = await directory_client.list(list_path, depth=1, page=page, page_size=page_size) - return listing.model_dump(mode="json") + return qualify_routed_paths(listing.model_dump(mode="json"), route) -@mcp.tool( - title="Find", - description="Recursively list files matching a name glob. Paths accept '/path'.", - tags={POSIX_TOOLS_TAG, "navigation"}, - annotations={ - "title": "Find", - "readOnlyHint": True, - "destructiveHint": False, - "openWorldHint": False, - }, -) -async def find( +def routed_listing_root(path: str, route: ProjectPathRoute) -> str: + """The directory the returned paths are relative to, in *their* address space. + + Returned paths carry the project prefix whenever the call put it in the + path, so the root a caller strips to rebuild a hierarchy must carry it too — + and in the permalink spelling the payload uses, not the caller's ('My + Research' vs 'my-research'). + """ + if not route.stripped or route.project is None: + return path + prefix = generate_permalink(route.project) + return f"{prefix}/{route.path}" if route.path else prefix + + +async def find_listing( path: str = "/", + *, name: Optional[str] = None, depth: int = _MAX_FIND_DEPTH, page: int = 1, @@ -394,23 +453,13 @@ async def find( project: Optional[str] = None, project_id: Optional[str] = None, context: Context | None = None, -) -> dict[str, Any]: - """Recursively list files under a directory, optionally filtered by name glob. +) -> tuple[dict[str, Any], str]: + """find's body: the listing, plus the root its paths are relative to. - Args: - path: Directory to start from (default: project root). '/path' - routes into that project. - name: File-name glob to match, e.g. "*.md". None matches everything. - depth: How many levels to recurse (1-10, default: 10). - page: Page number (1-indexed). - page_size: Nodes per page. - project: Project name. Optional - qualified paths route themselves; - unqualified paths refuse when several projects are addressable. - project_id: Project external_id (UUID); takes precedence over `project`. - context: Optional FastMCP context. - - Returns: - The directory listing as JSON: nodes, pagination, and totals. + `bm tree` needs both halves — the listing to render and the root to strip — + and resolving the path twice to get them cost a second project-list round + trip on every cloud CLI call, since CLI calls carry no FastMCP context for + the per-request cache to live in. One resolve now answers both. """ if depth < 1 or depth > _MAX_FIND_DEPTH: raise ValueError(f"depth must be between 1 and {_MAX_FIND_DEPTH}, got {depth}") @@ -444,7 +493,60 @@ async def find( page=page, page_size=page_size, ) - return listing.model_dump(mode="json") + payload = qualify_routed_paths(listing.model_dump(mode="json"), route) + + return payload, routed_listing_root(path, route) + + +@mcp.tool( + title="Find", + description="Recursively list files matching a name glob. Paths accept '/path'.", + tags={POSIX_TOOLS_TAG, "navigation"}, + annotations={ + "title": "Find", + "readOnlyHint": True, + "destructiveHint": False, + "openWorldHint": False, + }, +) +async def find( + path: str = "/", + name: Optional[str] = None, + depth: int = _MAX_FIND_DEPTH, + page: int = 1, + page_size: int = DEFAULT_DIRECTORY_PAGE_SIZE, + project: Optional[str] = None, + project_id: Optional[str] = None, + context: Context | None = None, +) -> dict[str, Any]: + """Recursively list files under a directory, optionally filtered by name glob. + + Args: + path: Directory to start from (default: project root). '/path' + routes into that project. + name: File-name glob to match, e.g. "*.md". None matches everything. + depth: How many levels to recurse (1-10, default: 10). + page: Page number (1-indexed). + page_size: Nodes per page. + project: Project name. Optional - qualified paths route themselves; + unqualified paths refuse when several projects are addressable. + project_id: Project external_id (UUID); takes precedence over `project`. + context: Optional FastMCP context. + + Returns: + The directory listing as JSON: nodes, pagination, and totals. + """ + listing, _ = await find_listing( + path, + name=name, + depth=depth, + page=page, + page_size=page_size, + project=project, + project_id=project_id, + context=context, + ) + return listing @mcp.tool( diff --git a/tests/cli/test_cli_posix_verbs.py b/tests/cli/test_cli_posix_verbs.py index 175d61aab..e209ce55b 100644 --- a/tests/cli/test_cli_posix_verbs.py +++ b/tests/cli/test_cli_posix_verbs.py @@ -12,7 +12,7 @@ import json import os -from unittest.mock import AsyncMock, patch +from unittest.mock import AsyncMock, Mock, patch import pytest from fastmcp.exceptions import ToolError @@ -278,16 +278,33 @@ def _assert_not_json(output: str) -> None: json.loads(output) -# Every verb with its patch target and a realistic payload. head shares cat's -# tool and payload; tree shares find's — their JSON contracts are identical. +# Every verb with its patch target, the payload it must print, and what the +# patched callable returns. head shares cat's tool and payload; tree's JSON +# contract is still find's, but it calls find_listing, which hands back the +# listing and the root its paths are relative to from one resolution (#1421). +FIND_LISTING_TARGET = "basic_memory.mcp.tools.posix_tools.find_listing" VERB_CASES = [ - pytest.param(["cat", "specs/search"], "basic_memory.mcp.tools.cat", CAT_RESULT, id="cat"), - pytest.param(["head", "specs/search"], "basic_memory.mcp.tools.cat", CAT_RESULT, id="head"), - pytest.param(["grep", "ranking"], "basic_memory.mcp.tools.grep", GREP_RESULT, id="grep"), - pytest.param(["ls", "/specs"], "basic_memory.mcp.tools.ls", LS_RESULT, id="ls"), - pytest.param(["find", "/specs"], "basic_memory.mcp.tools.find", FIND_RESULT, id="find"), - pytest.param(["tail"], "basic_memory.mcp.tools.tail", TAIL_RESULT, id="tail"), - pytest.param(["tree", "/specs"], "basic_memory.mcp.tools.find", TREE_RESULT, id="tree"), + pytest.param( + ["cat", "specs/search"], "basic_memory.mcp.tools.cat", CAT_RESULT, CAT_RESULT, id="cat" + ), + pytest.param( + ["head", "specs/search"], "basic_memory.mcp.tools.cat", CAT_RESULT, CAT_RESULT, id="head" + ), + pytest.param( + ["grep", "ranking"], "basic_memory.mcp.tools.grep", GREP_RESULT, GREP_RESULT, id="grep" + ), + pytest.param(["ls", "/specs"], "basic_memory.mcp.tools.ls", LS_RESULT, LS_RESULT, id="ls"), + pytest.param( + ["find", "/specs"], "basic_memory.mcp.tools.find", FIND_RESULT, FIND_RESULT, id="find" + ), + pytest.param(["tail"], "basic_memory.mcp.tools.tail", TAIL_RESULT, TAIL_RESULT, id="tail"), + pytest.param( + ["tree", "/specs"], + FIND_LISTING_TARGET, + TREE_RESULT, + (TREE_RESULT, "/specs"), + id="tree", + ), ] @@ -296,10 +313,10 @@ def _assert_not_json(output: str) -> None: # --------------------------------------------------------------------------- -@pytest.mark.parametrize(("args", "target", "payload"), VERB_CASES) -def test_verb_non_tty_outputs_tool_payload_verbatim(args, target, payload): +@pytest.mark.parametrize(("args", "target", "payload", "tool_return"), VERB_CASES) +def test_verb_non_tty_outputs_tool_payload_verbatim(args, target, payload, tool_return): """Piped output is the MCP tool's return value, unchanged (auto-JSON).""" - with patch(target, new_callable=AsyncMock, return_value=payload) as mock_tool: + with patch(target, new_callable=AsyncMock, return_value=tool_return) as mock_tool: result = _invoke(args) assert result.exit_code == 0, result.output @@ -307,10 +324,10 @@ def test_verb_non_tty_outputs_tool_payload_verbatim(args, target, payload): mock_tool.assert_called_once() -@pytest.mark.parametrize(("args", "target", "payload"), VERB_CASES) -def test_verb_json_flag_overrides_tty(args, target, payload): +@pytest.mark.parametrize(("args", "target", "payload", "tool_return"), VERB_CASES) +def test_verb_json_flag_overrides_tty(args, target, payload, tool_return): """--json wins over the interactive renderer on a TTY.""" - with patch(target, new_callable=AsyncMock, return_value=payload): + with patch(target, new_callable=AsyncMock, return_value=tool_return): result = _tty_invoke([*args, "--json"]) assert result.exit_code == 0, result.output @@ -737,7 +754,7 @@ def test_tail_lines_and_timeframe_passthrough(mock_tail): # --------------------------------------------------------------------------- -@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=TREE_RESULT) +@patch(FIND_LISTING_TARGET, new_callable=AsyncMock, return_value=(TREE_RESULT, "/")) def test_tree_plain_rebuilds_nesting_from_flat_nodes(mock_find): """The API page is flat; nesting is rebuilt from directory_path segments, including a synthesized parent for files whose directory node was filtered @@ -748,7 +765,7 @@ def test_tree_plain_rebuilds_nesting_from_flat_nodes(mock_find): assert result.stdout == "/\n specs/\n search.md\n auth/\n deep.md\n" -@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=TREE_RESULT) +@patch(FIND_LISTING_TARGET, new_callable=AsyncMock, return_value=(TREE_RESULT, "/")) def test_tree_rich_output(mock_find): result = _tty_invoke(["tree"]) @@ -759,7 +776,7 @@ def test_tree_rich_output(mock_find): assert "deep.md" in result.output -@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=TREE_ROOTED_RESULT) +@patch(FIND_LISTING_TARGET, new_callable=AsyncMock, return_value=(TREE_ROOTED_RESULT, "/specs")) def test_tree_rooted_path_skips_the_root_node(mock_find): """The search root's own node must not render as its own child.""" result = _tty_invoke(["tree", "/specs", "--plain"]) @@ -768,11 +785,7 @@ def test_tree_rooted_path_skips_the_root_node(mock_find): assert result.stdout == "/specs\n intro.md\n" -@patch( - "basic_memory.mcp.tools.find", - new_callable=AsyncMock, - return_value=TREE_DIR_AFTER_FILE_RESULT, -) +@patch(FIND_LISTING_TARGET, new_callable=AsyncMock, return_value=(TREE_DIR_AFTER_FILE_RESULT, "/")) def test_tree_directory_node_after_synthesized_intermediate(mock_find): """A directory listed after a file already implied it stays one directory.""" result = _tty_invoke(["tree", "--plain"]) @@ -781,7 +794,7 @@ def test_tree_directory_node_after_synthesized_intermediate(mock_find): assert result.stdout == "/\n specs/\n search.md\n" -@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=TREE_RESULT_MORE) +@patch(FIND_LISTING_TARGET, new_callable=AsyncMock, return_value=(TREE_RESULT_MORE, "/")) def test_tree_rich_reports_more_entries(mock_find): result = _tty_invoke(["tree"]) @@ -789,7 +802,7 @@ def test_tree_rich_reports_more_entries(mock_find): assert "more entries" in _flattened(result.output) -@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=TREE_RESULT_MORE) +@patch(FIND_LISTING_TARGET, new_callable=AsyncMock, return_value=(TREE_RESULT_MORE, "/")) def test_tree_plain_more_entries_note_goes_to_stderr(mock_find): result = _tty_invoke(["tree", "--plain"]) @@ -798,7 +811,7 @@ def test_tree_plain_more_entries_note_goes_to_stderr(mock_find): assert "more entries" in result.stderr -@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=TREE_RESULT_EMPTY) +@patch(FIND_LISTING_TARGET, new_callable=AsyncMock, return_value=(TREE_RESULT_EMPTY, "/")) def test_tree_rich_empty(mock_find): result = _tty_invoke(["tree"]) @@ -806,7 +819,7 @@ def test_tree_rich_empty(mock_find): assert "empty" in result.output -@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=TREE_RESULT) +@patch(FIND_LISTING_TARGET, new_callable=AsyncMock, return_value=(TREE_RESULT, "/")) def test_tree_passes_find_arguments_through(mock_find): result = _invoke(["tree", "/specs", "--name", "*.md", "--depth", "2"]) @@ -816,28 +829,47 @@ def test_tree_passes_find_arguments_through(mock_find): assert mock_find.call_args.kwargs["depth"] == 2 -# The tool layer strips a recognized '/' prefix (#1415), so a -# qualified tree root gets back PROJECT-RELATIVE node paths. +# A qualified tree root gets back node paths that carry the project prefix, and +# the root to strip carries it too — both from the one resolution (#1421). The +# --project spelling is the other addressing frame: project-relative throughout. TREE_QUALIFIED_RESULT = { "nodes": [ - _dir_node(name="notes", directory_path="/notes", type="directory"), - _dir_node(name="foo.md", file_path="notes/foo.md", directory_path="/notes/foo.md"), + _dir_node(name="notes", directory_path="/second-project/notes", type="directory"), + _dir_node( + name="foo.md", + file_path="second-project/notes/foo.md", + directory_path="/second-project/notes/foo.md", + ), ], "page": 1, "page_size": 10, "total": 2, "has_more": False, } +TREE_RELATIVE_RESULT = { + **TREE_QUALIFIED_RESULT, + "nodes": [ + _dir_node(name="notes", directory_path="/notes", type="directory"), + _dir_node(name="foo.md", file_path="notes/foo.md", directory_path="/notes/foo.md"), + ], +} -@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=TREE_QUALIFIED_RESULT) +@patch( + FIND_LISTING_TARGET, + new_callable=AsyncMock, + side_effect=[ + (TREE_QUALIFIED_RESULT, "second-project/notes"), + (TREE_RELATIVE_RESULT, "/notes"), + ], +) def test_tree_qualified_path_matches_project_flag_hierarchy( mock_find, config_manager, tmp_path_factory ): """'bm tree /dir' and 'bm tree /dir --project ' are the - same call by rule 2, so they must render the same hierarchy: the rebuild - strips the routed project-relative root, not the caller's qualified - spelling, or the first path segment duplicates under the root (#1415).""" + same call by rule 2, so they must render the same hierarchy — each stripping + the root in its own addressing frame, or the project segment duplicates + under the root (#1415).""" config = config_manager.load_config() config.projects["second-project"] = ProjectEntry( path=str(tmp_path_factory.mktemp("second-project-cli")) @@ -854,6 +886,7 @@ def test_tree_qualified_path_matches_project_flag_hierarchy( assert qualified.stdout.splitlines()[1:] == flagged.stdout.splitlines()[1:] # The tool still receives the caller's spelling; routing stays in the tool layer. assert mock_find.call_args_list[0].args == ("second-project/notes",) + assert mock_find.call_count == 2 # --------------------------------------------------------------------------- @@ -861,8 +894,8 @@ def test_tree_qualified_path_matches_project_flag_hierarchy( # --------------------------------------------------------------------------- -@pytest.mark.parametrize(("args", "target", "payload"), VERB_CASES) -def test_verb_tool_error_exits_nonzero(args, target, payload): +@pytest.mark.parametrize(("args", "target", "payload", "tool_return"), VERB_CASES) +def test_verb_tool_error_exits_nonzero(args, target, payload, tool_return): """A strict-resolve miss (ToolError) becomes stderr + exit 1, per verb.""" with patch(target, new_callable=AsyncMock, side_effect=ToolError("Entity not found: nope")): result = _invoke(args) @@ -942,3 +975,28 @@ async def capture(*args, **kwargs): assert result.exit_code == 0, result.output assert seen["force_local"] == "true" + + +@patch( + FIND_LISTING_TARGET, + new_callable=AsyncMock, + return_value=(TREE_QUALIFIED_RESULT, "second-project/notes"), +) +def test_tree_routes_the_path_exactly_once(mock_find): + """tree gets the listing and the root from one resolution, so it must not + resolve the path itself. + + A CLI invocation carries no FastMCP context, so the per-request project-list + cache has nowhere to live: a second resolution is a second project-list + round trip on every cloud call, and for a workspace-qualified path a second + workspace/project index build (#1421). Patching the resolver to explode is + the check — if tree still reaches for it, this fails loudly. + """ + boom = Mock(side_effect=AssertionError("tree resolved the path a second time")) + with patch("basic_memory.mcp.project_context.resolve_project_path_route", boom): + result = _tty_invoke(["tree", "second-project/notes", "--plain"]) + + assert result.exit_code == 0, result.output + assert result.stdout == "second-project/notes\n foo.md\n" + mock_find.assert_called_once() + boom.assert_not_called() diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index a4c897be8..f8ec36ac5 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -27,9 +27,11 @@ _project_routes_agree, resolve_project_path_route, resolve_workspace_project_identifier, + resolve_workspace_qualified_identifier, + resolve_workspace_qualified_memory_url, ) from basic_memory.mcp.project_context_identifiers import ( - split_workspace_route_segments, + split_project_permalink_prefix, unqualified_project_identifier, ) from basic_memory.mcp.tools import grep, ls @@ -216,14 +218,34 @@ async def test_conflicting_prefix_raises_naming_both(multi_project_config): # matched the route, so the qualified-path tests below pin it end to end. -def test_split_workspace_route_segments_needs_two_named_segments(): - """The path form makes the trailing path optional, but both route segments - still have to be there: one segment names no project, and an empty one (a - '//' in the input) names nothing at all.""" - assert split_workspace_route_segments("acme") is None - assert split_workspace_route_segments("acme//docs") is None - assert split_workspace_route_segments("acme/docs") == ("acme", "docs", "") - assert split_workspace_route_segments("acme/docs/notes/x") == ("acme", "docs", "notes/x") +def test_split_project_permalink_prefix_matches_whole_permalinks_longest_first(): + """The one place a path becomes (project, project-relative path). It takes + the candidate set precisely because how many segments a project consumes is + a fact about the known projects, not about the string.""" + permalinks = ["research", "research/2026"] + + assert split_project_permalink_prefix("research/2026/notes", permalinks) == ( + "research/2026", + "notes", + ) + assert split_project_permalink_prefix("research/notes", permalinks) == ("research", "notes") + assert split_project_permalink_prefix("research/2026", permalinks) == ("research/2026", "") + assert split_project_permalink_prefix("engineering/notes", permalinks) is None + # Spelling is normalized per segment, so display names match their permalink. + assert split_project_permalink_prefix("My Research/notes", ["my-research"]) == ( + "my-research", + "notes", + ) + + +@pytest.mark.asyncio +async def test_workspace_resolvers_reject_non_routes_without_discovery(): + """Both entry points refuse shapes that cannot be a workspace route before + touching discovery — no workspace index is built for a plain memory URL or a + single-segment identifier, which is what keeps read_note and search off the + cloud round trip.""" + assert await resolve_workspace_qualified_memory_url("second-project/notes/x") is None + assert await resolve_workspace_qualified_identifier("single-segment") is None def test_project_routes_agree_across_mixed_qualification(): @@ -506,6 +528,27 @@ async def test_cloud_workspace_qualified_project_root_routes(cloud_session): assert route == ProjectPathRoute(project="team/research", path="", stripped=True) +@pytest.mark.asyncio +async def test_cloud_workspace_route_matches_multi_segment_project_permalink(cloud_session): + """A workspace route splits project from path by matching that workspace's + project permalinks, not by taking one segment. The mount table learned this + first; the workspace parser rediscovered the same wrong assumption, so both + now go through split_project_permalink_prefix and neither can drift.""" + cloud_session("Research/2026", "research") + + nested = await resolve_project_path_route( + "team/research/2026/notes", project=None, project_id=None + ) + assert nested == ProjectPathRoute(project="team/research/2026", path="notes", stripped=True) + + root = await resolve_project_path_route("team/research/2026", project=None, project_id=None) + assert root == ProjectPathRoute(project="team/research/2026", path="", stripped=True) + + # The shorter sibling still claims its own paths — longest match, not first. + sibling = await resolve_project_path_route("team/research/notes", project=None, project_id=None) + assert sibling == ProjectPathRoute(project="team/research", path="notes", stripped=True) + + @pytest.mark.asyncio async def test_cloud_workspace_project_root_falls_through_without_workspaces( cloud_session, monkeypatch diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index 5400558ae..e953fa0e7 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -742,12 +742,22 @@ async def test_cat_qualified_identifier_equals_explicit_project_read( client, test_graph, test_project, second_project, no_project_constraint ): """'test-project/test/root' with no project param reads the same note as - project='test-project' + 'test/root' — inputs accept what outputs produce.""" + project='test-project' + 'test/root'. + + The payloads are not identical, and deliberately so: each answers in the + addressing frame its caller used, so the file_path it hands back is one the + same call shape accepts again. Content is what must match. + """ qualified = await cat("test-project/test/root") explicit = await cat("test/root", project=test_project.name) - assert qualified == explicit - assert qualified["title"] == "Root" + assert qualified["title"] == explicit["title"] == "Root" + assert qualified["content"] == explicit["content"] + + # Each frame's file_path round-trips in that same frame. + assert qualified["file_path"] == f"test-project/{explicit['file_path']}" + assert (await cat(qualified["file_path"]))["title"] == "Root" + assert (await cat(explicit["file_path"], project=test_project.name))["title"] == "Root" @pytest.mark.asyncio @@ -788,8 +798,14 @@ async def test_explicit_project_with_agreeing_prefix_strips( qualified = await ls("/test-project/test", project=test_project.name) relative = await ls("/test", project=test_project.name) - assert qualified == relative - assert qualified["total"] == 5 + # Same listing, each in its caller's addressing frame (see cat's twin above). + assert qualified["total"] == relative["total"] == 5 + assert [node["name"] for node in qualified["nodes"]] == [ + node["name"] for node in relative["nodes"] + ] + assert [node["file_path"] for node in qualified["nodes"]] == [ + f"test-project/{node['file_path']}" for node in relative["nodes"] + ] @pytest.mark.asyncio @@ -928,3 +944,93 @@ async def test_project_id_routes_without_prefix_parsing( result = await cat("test/root", project_id=test_project.external_id) assert result["title"] == "Root" + + +# -- round-trip coherence: a returned path is an accepted path (#1421) -- +# The property belongs to the routing layer, not to any one verb, so these tests +# enumerate the verbs rather than naming them. A seventh posix verb that accepts +# a path has to answer for the property here before it can ship. + + +def _path_accepting_posix_verbs() -> set[str]: + """Posix verbs whose first parameter is a routable path or identifier. + + Derived from the tools themselves so a new one cannot slip past the round-trip + tests below by simply not being listed. + """ + import inspect + + verbs = {} + for verb in (cat, grep, ls, find, tail, man): + fn = getattr(verb, "fn", verb) + first = next(iter(inspect.signature(fn).parameters), None) + if first in {"path", "identifier"}: + verbs[fn.__name__] = verb + return set(verbs) + + +def test_path_accepting_verbs_are_the_ones_covered_below(): + """Pins the set the round-trip tests cover. Adding a path-accepting verb + fails here, which is the prompt to give it the same guarantee — the class is + closed by this enumeration, not by every author remembering.""" + assert _path_accepting_posix_verbs() == {"cat", "ls", "find"} + + +@pytest.mark.asyncio +async def test_ls_returned_paths_route_back_to_the_same_project( + client, test_project, second_project, no_project_constraint +): + """A qualified `ls` advertises child paths; feeding one back must reach the + same project. Returning the API's project-relative '/notes' refused as + unqualified — or, with a project mounted as 'notes', opened that one.""" + await write_note( + title="Second Root Note", + directory="notes", + content="# Second Root Note", + project="second-project", + ) + + listing = await ls("second-project") + child = next(node for node in listing["nodes"] if node["name"] == "notes") + assert child["directory_path"] == "/second-project/notes" + + # The advertised path is accepted verbatim, with no project param. + nested = await ls(child["directory_path"]) + assert {node["name"] for node in nested["nodes"]} == {"Second Root Note.md"} + + +@pytest.mark.asyncio +async def test_find_returned_paths_route_back_to_the_same_project( + client, test_project, second_project, no_project_constraint +): + """find advertises both directory_path and file_path; each must address the + project the call routed to, so `cat` and `ls` accept them unchanged.""" + await write_note( + title="Second Root Note", + directory="notes", + content="# Second Root Note", + project="second-project", + ) + + listing = await find("second-project") + file_node = next(node for node in listing["nodes"] if node["type"] == "file") + dir_node = next(node for node in listing["nodes"] if node["type"] == "directory") + + assert file_node["file_path"].startswith("second-project/") + assert dir_node["directory_path"].startswith("/second-project") + + assert (await cat(file_node["file_path"]))["title"] == "Second Root Note" + assert (await ls(dir_node["directory_path"]))["total"] >= 1 + + +@pytest.mark.asyncio +async def test_unrouted_listings_keep_project_relative_paths( + client, test_graph, test_project, second_project, no_project_constraint +): + """The prefix goes back only when the call put it in the path. An explicit + project param is a different addressing frame: those paths are fed back with + the same param, so re-prefixing them would break that round trip instead.""" + listing = await ls("/test", project=test_project.name) + + assert all(not node["file_path"].startswith("test-project/") for node in listing["nodes"]) + assert (await ls("/test", project=test_project.name))["total"] == listing["total"] From b61e391fca3af06fc92cab2d4f29b73c1252dd30 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 18:01:34 -0500 Subject: [PATCH 07/18] fix(mcp): requalify by position, and stop inferring workspaces from slashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings on a004ac3d, both mine, and the second narrows a claim I made. Re-qualification corrupted note content. qualify_routed_paths walked the payload rewriting any key spelled file_path or directory_path, and a note's frontmatter is free-form user YAML in that same payload. A note whose author wrote `file_path: imports/source.md` came back as `second-project/imports/source.md`, disagreeing with both its own content and the file on disk — canonical content silently altered in what we hand back. Transport metadata and note content can spell a key the same way and only position tells them apart, so one rule still decides whether to requalify and with what (_route_prefix), and each response schema now says where: qualify_note_paths touches the top-level file_path only, qualify_listing_paths walks nodes and their children. frontmatter and permalink are never touched. The escape hatch failed for slash-bearing project names. With mount 'Research/2026' detected and project='acme/Research/2026' passed, _project_routes_agree split both identifiers on the first slash, read the detected mount as workspace 'Research' plus project '2026', and rejected two agreeing spellings as a conflict. That is worth stating plainly rather than patching quietly: my "the class is closed" claim was too broad. What I closed was one manifestation — turning a path into (project, project-relative path), which now has one implementation that cannot be called without the candidate set. The same root fact, that a project name may contain '/', has a second manifestation I left open: splitting an identifier into (workspace, project). split_qualified_project_identifier still guessed, and six call sites depended on the guess. Converted in this pass rather than one per report: - _project_routes_agree and its prefer_explicit twin are replaced by _agreed_route_project, which returns the project both spellings name and the spelling that wins from one comparison. It needs no candidate set because a workspace slug is exactly one segment, so the qualified spelling is the bare one plus exactly one leading segment. That accepts 'acme/Research/2026' for 'Research/2026' while still rejecting it for a project named '2026', which a plain suffix test would have got wrong. - resolve_workspace_project_from_index now tries the whole identifier as a project permalink before reading its first segment as a workspace, so a cloud project named 'Research/2026' is routable at all. The v2 project router already resolved exact-first; this matches it. - get_project_client's per-project-config branch uses the config key verbatim, since it came from config.projects and is the project name by construction. - resolve_project_and_path strips only the workspace slug it actually knows from context instead of guessing one. - unqualified_project_identifier is deleted; it had no remaining caller. split_qualified_project_identifier survives with a docstring saying what it cannot answer and that it is a fallback after an exact lookup misses. Both its remaining callers now order it that way. The honest statement is therefore: path-splitting is closed by construction; identifier-splitting is correct by ordering at both call sites, not by construction. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 81 +++++++++----- .../mcp/project_context_identifiers.py | 17 +-- src/basic_memory/mcp/tools/posix_tools.py | 95 +++++++++++------ .../mcp/workspace_project_index.py | 15 ++- tests/mcp/test_project_path_routing.py | 100 +++++++++++++++--- tests/mcp/test_tool_posix.py | 38 +++++++ 6 files changed, 259 insertions(+), 87 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index abbbbf23b..278a7f795 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -67,7 +67,6 @@ split_project_permalink_prefix as _split_project_permalink_prefix, split_qualified_project_identifier as _split_qualified_project_identifier_impl, split_workspace_slug_prefix as _split_workspace_slug_prefix, - unqualified_project_identifier as _unqualified_project_identifier, ) from basic_memory.mcp.workspace_project_index import ( WORKSPACE_PROJECT_INDEX_STATE_KEY as _WORKSPACE_PROJECT_INDEX_STATE_KEY, @@ -818,7 +817,11 @@ async def resolve_project_and_path( workspace_context = current_workspace_permalink_context() if workspace_context and project: workspace_prefix = generate_permalink(workspace_context.workspace_slug) - project_permalink = generate_permalink(_unqualified_project_identifier(project)) + # Strip only this workspace's own slug. Guessing that the first + # segment of `project` is a workspace mangles a project whose name + # contains '/' ('Research/2026' -> '2026'), and the workspace here + # is known, so nothing has to be inferred. + project_permalink = generate_permalink(project).removeprefix(f"{workspace_prefix}/") qualified_prefix = f"{workspace_prefix}/{project_permalink}" if normalized_path == qualified_prefix or normalized_path.startswith( f"{qualified_prefix}/" @@ -1244,17 +1247,40 @@ async def _detect_workspace_qualified_route( return resolution.project_identifier, remainder -def _project_routes_agree(detected: str, explicit: str) -> bool: - """True when a detected path prefix and an explicit project name the same project.""" +def _workspace_qualifies(qualified: str, bare: str) -> bool: + """True when ``qualified`` is ``bare`` with exactly one workspace slug in front. + + Asking "is this identifier workspace-qualified?" of a single string is not + answerable — a project name may contain '/', so 'Research/2026' and + 'acme/docs' have the same shape. Comparing two spellings of the *same* + project is answerable without any candidate set, because a workspace slug is + exactly one segment: the qualified spelling is the bare one plus exactly one + leading segment. That is the whole rule, and it is why 'acme/Research/2026' + qualifies 'Research/2026' while it does not qualify a project named '2026'. + """ + qualified_permalink = generate_permalink(qualified) + bare_permalink = generate_permalink(bare) + return qualified_permalink.endswith(f"/{bare_permalink}") and ( + qualified_permalink.count("/") - bare_permalink.count("/") == 1 + ) + + +def _agreed_route_project(detected: str, explicit: str) -> str | None: + """The project both spellings name, or None when they name different projects. + + Returns the more-qualified spelling, so an explicit '/' + outlives a bare prefix match: a local project can shadow a same-named + project in another workspace, and dropping the explicitly named workspace + would silently reroute the call to the local shadow. Agreement and which + spelling wins come from one comparison, so they cannot disagree. + """ if generate_permalink(detected) == generate_permalink(explicit): - return True - detected_workspace, detected_project = _split_qualified_project_identifier_impl(detected) - explicit_workspace, explicit_project = _split_qualified_project_identifier_impl(explicit) - # A workspace-qualified spelling agrees with the unqualified spelling of the - # same project; two fully qualified spellings must match exactly (above). - if (detected_workspace is None) == (explicit_workspace is None): - return False - return generate_permalink(detected_project) == generate_permalink(explicit_project) + return detected + if _workspace_qualifies(explicit, detected): + return explicit + if _workspace_qualifies(detected, explicit): + return detected + return None async def resolve_project_path_route( @@ -1349,23 +1375,19 @@ async def resolve_project_path_route( return ProjectPathRoute( project=_canonicalize_project_name(explicit, config), path=path, stripped=False ) - if _project_routes_agree(detected, explicit): + routed = _agreed_route_project(detected, explicit) + if routed is not None: # Trigger: the explicit spelling is workspace-qualified while the # path prefix matched an unqualified local config name. - # Why: a local project can shadow a same-named project in another - # workspace; dropping the explicitly named workspace would - # silently reroute the call to the local shadow. - # Outcome: the more-qualified explicit spelling carries the route, - # and drops the mount id that names this session's workspace with - # it; every other agreement keeps the detected (canonical) - # spelling and stays bound to the mount that matched. - detected_workspace, _ = _split_qualified_project_identifier_impl(detected) - explicit_workspace, _ = _split_qualified_project_identifier_impl(explicit) - prefer_explicit = explicit_workspace is not None and detected_workspace is None + # Why: the explicitly named workspace must survive, or the call + # silently reroutes to a same-named local shadow. + # Outcome: when the explicit spelling wins it also drops the mount + # id that names this session's own workspace; every other + # agreement keeps the detected (canonical) spelling and stays + # bound to the mount that matched. + prefer_explicit = routed is explicit return ProjectPathRoute( - project=_canonicalize_project_name( - explicit if prefer_explicit else detected, config - ), + project=_canonicalize_project_name(routed, config), path=remainder, stripped=True, project_id=None if prefer_explicit else mount_project_id, @@ -1565,10 +1587,13 @@ async def get_project_client( active_ws: WorkspaceInfo | None = None resolved_entry: WorkspaceProjectEntry | None = None workspace_id: str - project_for_api = _unqualified_project_identifier(resolved_project) if project_entry and project_entry.workspace_id: - # Per-project config stores the cloud tenant id directly + # Per-project config stores the cloud tenant id directly. The + # identifier came out of config.projects, so it is the project name + # verbatim — splitting a workspace off it would mangle a name that + # legitimately contains '/'. + project_for_api = resolved_project workspace_id = project_entry.workspace_id active_ws = await _workspace_metadata_by_tenant_id(workspace_id, context=context) else: diff --git a/src/basic_memory/mcp/project_context_identifiers.py b/src/basic_memory/mcp/project_context_identifiers.py index 67464f262..2c581e23c 100644 --- a/src/basic_memory/mcp/project_context_identifiers.py +++ b/src/basic_memory/mcp/project_context_identifiers.py @@ -69,7 +69,16 @@ def project_matches_identifier(project_item: ProjectItem, identifier: Optional[s def split_qualified_project_identifier(identifier: str) -> tuple[str | None, str]: - """Split ``/`` identifiers for cloud routing.""" + """Split ``/`` identifiers for cloud routing. + + This guesses: a project name may contain '/', so 'acme/docs' and + 'Research/2026' are the same shape and no string can say which is which. + Call it only after an exact whole-identifier lookup has already missed, so + the guess is a fallback rather than the first reading — see + ``resolve_workspace_project_from_index`` and the v2 project router, which + both resolve exact-first for that reason. To compare two spellings of the + same project, use ``_workspace_qualifies`` instead, which needs no guess. + """ cleaned = identifier.strip() if "/" not in cleaned: return None, cleaned @@ -79,12 +88,6 @@ def split_qualified_project_identifier(identifier: str) -> tuple[str | None, str return workspace_slug, project_identifier -def unqualified_project_identifier(identifier: str) -> str: - """Return the project segment from an optional qualified identifier.""" - _, project_identifier = split_qualified_project_identifier(identifier) - return project_identifier - - def identifier_path(identifier: str) -> str: """Return the routable path portion of a raw identifier or memory URL.""" stripped = identifier.strip() diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index e8a439573..d905a1361 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -63,13 +63,27 @@ # unqualified, or worse, opens a *different* project that happens to be mounted # as 'notes'. # -# So every verb that can strip a prefix re-attaches it to the paths it returns, -# through this one function. Only addressing fields are re-qualified: -# `directory_path` and `file_path` are what a caller feeds back to `ls`, `find`, -# and `cat`. `permalink` is deliberately left alone — it is an identity with its -# own canonical form (permalinks_include_project, workspace qualification), and -# re-prefixing it here would mint a second, competing spelling of it. -_ROUTED_PATH_FIELDS = frozenset({"directory_path", "file_path"}) +# One rule decides *whether* and *with what* (_route_prefix); each response +# schema says *where*. That split is deliberate. Rewriting by key name anywhere +# in the payload also rewrote a note's own frontmatter when the author happened +# to use a `file_path:` key, so a routed `cat` returned frontmatter that +# disagreed with both its own `content` and the stored file. Transport metadata +# and note content are different things that can spell a key the same way, and +# only position tells them apart. + + +def _route_prefix(route: ProjectPathRoute) -> str | None: + """The prefix a routed response must re-attach, or None if nothing was stripped. + + None means the caller addressed the project some other way (an explicit + param, or a single-project session), so the project-relative paths it gets + back are already the ones it can feed back. + """ + if not route.stripped or route.project is None: + return None + # The permalink form is the spelling `ls "/"` advertises and the resolver + # normalizes to, so it round-trips for display names too ('My Research'). + return generate_permalink(route.project) def _requalified_path(value: str, prefix: str) -> str: @@ -79,32 +93,45 @@ def _requalified_path(value: str, prefix: str) -> str: return f"{prefix}/{value}" if value else prefix -def _requalify(payload: Any, prefix: str) -> Any: - """Rewrite addressing fields anywhere in a response payload.""" - if isinstance(payload, dict): - return { - key: _requalified_path(value, prefix) - if key in _ROUTED_PATH_FIELDS and isinstance(value, str) - else _requalify(value, prefix) - for key, value in payload.items() - } - if isinstance(payload, list): - return [_requalify(item, prefix) for item in payload] - return payload +def qualify_note_paths(payload: dict[str, Any], route: ProjectPathRoute) -> dict[str, Any]: + """Re-qualify a note payload's transport path. - -def qualify_routed_paths(payload: Any, route: ProjectPathRoute) -> Any: - """Return ``payload`` with the project prefix ``route`` stripped put back. - - A no-op when the route stripped nothing — then the caller addressed the - project some other way (an explicit param, or a single-project session) and - the project-relative paths it gets back are the ones it can feed back. + ``file_path`` is the note's address and is re-qualified. ``frontmatter`` is + the note's own YAML — canonical content that must come back byte for byte, + even when it carries keys named like transport fields — and ``permalink`` is + an identity with its own canonical form; neither is touched. """ - if not route.stripped or route.project is None: + prefix = _route_prefix(route) + if prefix is None: return payload - # The permalink form is the spelling `ls "/"` advertises and the resolver - # normalizes to, so it round-trips for display names too ('My Research'). - return _requalify(payload, generate_permalink(route.project)) + return {**payload, "file_path": _requalified_path(payload["file_path"], prefix)} + + +def _requalified_directory_node(node: dict[str, Any], prefix: str) -> dict[str, Any]: + """Re-qualify one DirectoryNode's addressing fields, and its children.""" + requalified = dict(node) + directory_path = node.get("directory_path") + if isinstance(directory_path, str): + requalified["directory_path"] = _requalified_path(directory_path, prefix) + # file_path is Optional on DirectoryNode: directory rows carry no file. + file_path = node.get("file_path") + if isinstance(file_path, str): + requalified["file_path"] = _requalified_path(file_path, prefix) + children = node.get("children") + if children: + requalified["children"] = [_requalified_directory_node(child, prefix) for child in children] + return requalified + + +def qualify_listing_paths(payload: dict[str, Any], route: ProjectPathRoute) -> dict[str, Any]: + """Re-qualify a directory listing's node addressing fields.""" + prefix = _route_prefix(route) + if prefix is None: + return payload + return { + **payload, + "nodes": [_requalified_directory_node(node, prefix) for node in payload["nodes"]], + } # The manual project holds the non-bundled manual pages as ordinary notes; @@ -237,7 +264,7 @@ async def cat( ) if server_side_slice or (start_line is None and end_line is None): - return qualify_routed_paths(payload, route) + return qualify_note_paths(payload, route) lines = str(payload["content"]).splitlines() total_lines = len(lines) @@ -247,7 +274,7 @@ async def cat( payload["start_line"] = first payload["end_line"] = last payload["total_lines"] = total_lines - return qualify_routed_paths(payload, route) + return qualify_note_paths(payload, route) def _grep_retrieval_mode(literal: bool) -> SearchRetrievalMode: @@ -426,7 +453,7 @@ async def ls( directory_client = DirectoryClient(client, active_project.external_id) listing = await directory_client.list(list_path, depth=1, page=page, page_size=page_size) - return qualify_routed_paths(listing.model_dump(mode="json"), route) + return qualify_listing_paths(listing.model_dump(mode="json"), route) def routed_listing_root(path: str, route: ProjectPathRoute) -> str: @@ -493,7 +520,7 @@ async def find_listing( page=page, page_size=page_size, ) - payload = qualify_routed_paths(listing.model_dump(mode="json"), route) + payload = qualify_listing_paths(listing.model_dump(mode="json"), route) return payload, routed_listing_root(path, route) diff --git a/src/basic_memory/mcp/workspace_project_index.py b/src/basic_memory/mcp/workspace_project_index.py index f40c780f9..cd8a2c478 100644 --- a/src/basic_memory/mcp/workspace_project_index.py +++ b/src/basic_memory/mcp/workspace_project_index.py @@ -270,8 +270,19 @@ async def resolve_workspace_project_from_index( from basic_memory.mcp.project_context_identifiers import split_qualified_project_identifier - workspace_identifier, project_identifier = split_qualified_project_identifier(project) - project_permalink = generate_permalink(project_identifier) + # Try the whole identifier as a project permalink before reading its first + # segment as a workspace. A project name may contain '/', so 'Research/2026' + # and 'acme/docs' are the same shape and only the index can tell them apart; + # without this, a slash-bearing project name was unroutable, failing with + # "Workspace 'Research' was not found". The v2 project router resolves + # exact-first for the same reason. + whole_permalink = generate_permalink(project) + if whole_permalink in index.entries_by_permalink: + workspace_identifier, project_identifier = None, project + project_permalink = whole_permalink + else: + workspace_identifier, project_identifier = split_qualified_project_identifier(project) + project_permalink = generate_permalink(project_identifier) if workspace_identifier: workspace = match_workspace_identifier(index.workspaces, workspace_identifier) diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index f8ec36ac5..2a29b9e46 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -24,7 +24,7 @@ ProjectPathRoute, ProjectPrefixConflictError, UnqualifiedPathRefusedError, - _project_routes_agree, + _agreed_route_project, resolve_project_path_route, resolve_workspace_project_identifier, resolve_workspace_qualified_identifier, @@ -32,7 +32,6 @@ ) from basic_memory.mcp.project_context_identifiers import ( split_project_permalink_prefix, - unqualified_project_identifier, ) from basic_memory.mcp.tools import grep, ls from basic_memory.schemas.cloud import WorkspaceInfo @@ -167,6 +166,33 @@ async def test_multi_segment_project_permalink_routes(config_manager, tmp_path_f assert shallow == ProjectPathRoute(project="Research", path="notes/x", stripped=True) +@pytest.mark.asyncio +async def test_multi_segment_mount_agrees_with_explicit_workspace_spelling( + config_manager, tmp_path_factory +): + """The explicit-workspace escape hatch has to survive slash-bearing names. + + With mount 'Research/2026' detected and project='acme/Research/2026' passed, + inferring qualification from the first slash read the detected mount as + workspace 'Research' plus project '2026', so two agreeing spellings were + rejected as a conflict. Segment count settles it without guessing. + """ + config = config_manager.load_config() + config.projects["Research/2026"] = ProjectEntry( + path=str(tmp_path_factory.mktemp("research-2026-agree")) + ) + config_manager.save_config(config) + + route = await resolve_project_path_route( + "research/2026/notes/x", project="acme/Research/2026", project_id=None + ) + + # The explicitly named workspace survives, and the mount id goes with it. + assert route == ProjectPathRoute( + project="acme/Research/2026", path="notes/x", stripped=True, project_id=None + ) + + @pytest.mark.asyncio async def test_glob_first_segment_never_routes(multi_project_config): """split_project_prefix's '*' guard: a glob first segment is search input, @@ -248,12 +274,27 @@ async def test_workspace_resolvers_reject_non_routes_without_discovery(): assert await resolve_workspace_qualified_identifier("single-segment") is None -def test_project_routes_agree_across_mixed_qualification(): +def test_agreed_route_project_across_mixed_qualification(): """A workspace-qualified spelling agrees with the unqualified spelling of - the same project, in either direction; different projects never agree.""" - assert _project_routes_agree("research", "other/research") - assert _project_routes_agree("other/research", "research") - assert not _project_routes_agree("second-project", "other/research") + the same project, in either direction, and the more-qualified one carries + the route; different projects never agree. + + Agreement is decided by segment count, not by looking for a slash: a + workspace slug is exactly one segment, so 'acme/Research/2026' qualifies the + project 'Research/2026' while it does not qualify a project named '2026'. + Asking "is this identifier workspace-qualified?" of one string is not + answerable at all once project names may contain '/'. + """ + assert _agreed_route_project("research", "other/research") == "other/research" + assert _agreed_route_project("other/research", "research") == "other/research" + assert _agreed_route_project("second-project", "other/research") is None + + # Slash-bearing project names: the escape hatch has to keep working. + assert _agreed_route_project("Research/2026", "acme/Research/2026") == "acme/Research/2026" + # ...without agreeing with a different project that merely shares a tail. + assert _agreed_route_project("2026", "acme/Research/2026") is None + # A mount named after a workspace still conflicts with that workspace route. + assert _agreed_route_project("team", "team/docs") is None @pytest.mark.asyncio @@ -373,14 +414,18 @@ class _CloudSession: listings: list[ProjectList] -def _routed_project_permalink(route: ProjectPathRoute) -> str: - """The project a route landed on, as its bare permalink. +def _routes_to_project(route: ProjectPathRoute, permalink: str) -> bool: + """True when a route landed on the project the mount view advertises. - Cloud routes come back workspace-qualified ('team/research'), so compare on - the project segment the mount view advertises. + Cloud routes may come back workspace-qualified ('team/research'), so accept + either the bare permalink or that permalink behind exactly one workspace + segment — the same rule the resolver uses to compare two spellings. """ assert route.project is not None - return generate_permalink(unqualified_project_identifier(route.project)) + routed = generate_permalink(route.project) + return routed == permalink or ( + routed.endswith(f"/{permalink}") and routed.count("/") - permalink.count("/") == 1 + ) @pytest.fixture @@ -498,7 +543,7 @@ async def test_cloud_qualified_path_routes_to_that_project(cloud_session): stripped=True, project_id="research-external-id", ) - assert unqualified_project_identifier(route.project or "") == "research" + assert _routes_to_project(route, "research") @pytest.mark.asyncio @@ -570,6 +615,29 @@ async def no_workspaces(context=None) -> list[WorkspaceInfo]: assert str(excinfo.value) == "no project 'acme' — active projects: engineering/, research/" +@pytest.mark.asyncio +async def test_cloud_slash_bearing_project_resolves_by_its_own_name(cloud_session): + """Resolving a project identifier tries the whole name before reading its + first segment as a workspace. + + 'Research/2026' and 'acme/docs' are the same shape, so a slash-bearing + project name used to be unroutable — the lookup took 'Research' for a + workspace and failed with "Workspace 'Research' was not found". The v2 + project router already resolved exact-first for this reason; the index now + matches it. + """ + cloud_session("Research/2026", "engineering") + + entry = await resolve_workspace_project_identifier("Research/2026") + + assert entry.project.name == "Research/2026" + assert entry.qualified_name == "team/research/2026" + + # The workspace-qualified spelling still resolves through the split. + qualified = await resolve_workspace_project_identifier("team/Research/2026") + assert qualified.project.name == "Research/2026" + + @pytest.mark.asyncio async def test_cloud_mount_wins_over_colliding_workspace_slug(cloud_session): """The collision: 'team' is both an advertised mount and this workspace's @@ -655,14 +723,14 @@ async def test_cloud_every_advertised_mount_is_addressable(cloud_session): root_route = await resolve_project_path_route(permalink, project=None, project_id=None) assert root_route.stripped is True assert root_route.path == "" - assert _routed_project_permalink(root_route) == permalink + assert _routes_to_project(root_route, permalink) path_route = await resolve_project_path_route( f"{permalink}/notes/x", project=None, project_id=None ) assert path_route.stripped is True assert path_route.path == "notes/x" - assert _routed_project_permalink(path_route) == permalink + assert _routes_to_project(path_route, permalink) @pytest.mark.asyncio @@ -695,7 +763,7 @@ async def test_cloud_refusal_does_not_consult_the_default_flag(cloud_session): # where the caller said, not to whatever carries the flag. route = await resolve_project_path_route("beta/notes/x", project=None, project_id=None) - assert _routed_project_permalink(route) == "beta" + assert _routes_to_project(route, "beta") @pytest.mark.asyncio diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index e953fa0e7..f6684b54d 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -5,9 +5,11 @@ assert on the JSON shapes the canonical `output_format="json"` paths produce. """ +from pathlib import Path from types import SimpleNamespace import pytest +import yaml from fastmcp.exceptions import ToolError import basic_memory.mcp.tools.posix_tools as posix_tools @@ -1034,3 +1036,39 @@ async def test_unrouted_listings_keep_project_relative_paths( assert all(not node["file_path"].startswith("test-project/") for node in listing["nodes"]) assert (await ls("/test", project=test_project.name))["total"] == listing["total"] + + +@pytest.mark.asyncio +async def test_routed_cat_never_rewrites_note_frontmatter( + client, test_project, second_project, no_project_constraint +): + """Re-qualification touches transport metadata, never note content. + + Frontmatter is the author's own YAML and may legitimately carry keys spelled + like transport fields. Rewriting by key name anywhere in the payload turned + `file_path: imports/source.md` into `second-project/imports/source.md`, so a + routed read returned frontmatter that disagreed with both its own `content` + and the file on disk. Position, not spelling, separates the two. + """ + await write_note( + title="Imported Note", + directory="notes", + content="# Imported Note\n\nbody", + project="second-project", + metadata={"file_path": "imports/source.md", "directory_path": "/imports"}, + ) + + payload = await cat("second-project/notes/imported-note") + + # The stored file is the authority: parse its frontmatter block and require + # the response to reproduce it exactly. + stored = (Path(second_project.path) / "notes" / "Imported Note.md").read_text() + _, _, rest = stored.partition("---\n") + block, _, _ = rest.partition("\n---") + assert yaml.safe_load(block) == payload["frontmatter"] + assert payload["frontmatter"]["file_path"] == "imports/source.md" + assert payload["frontmatter"]["directory_path"] == "/imports" + + # The transport path is still qualified, so the round trip holds. + assert payload["file_path"] == "second-project/notes/Imported Note.md" + assert (await cat(payload["file_path"]))["title"] == "Imported Note" From 8214e043ce623af648382686bf244e0bbeb95be4 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 18:34:41 -0500 Subject: [PATCH 08/18] fix(mcp): decide route versus path by precedence, not by parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In a cloud session mounting one project, an ordinary relative path left that project whenever its leading segments happened to name an accessible workspace and a project inside it. With 'research' the sole mount and workspace 'acme' holding 'docs', cat("acme/docs/foo") served acme's docs project instead of reading acme/docs/foo inside research. Passing project="research" did not rescue it: the detected workspace route conflicted with the named project and raised instead, so the path could neither stay home on its own nor be pinned there. Route versus path is not decidable by parsing. 'acme/docs/foo' is a well-formed workspace route and a well-formed folder path, and neither the string nor the set of existing projects recovers which the caller meant. So this is a precedence order, deliberately, and it is written down above the section. One question settles it — does the call already say which project it means? 1. An explicit project (param or env constraint) says so. The remaining path is inside it and nothing reroutes. A prefix naming a different addressable mount still conflicts: that is a contradiction in one call, not ambiguity. 2. Otherwise, with several projects addressable, an unqualified path cannot resolve at all — it refuses. Reading the leading segments as a route is then the only way the input can mean anything, so route wins. 3. Otherwise one project is addressable, the path already resolves inside it, and route parsing would take a working read and send it to another tenant. Path wins. Mounts keep sitting above all three: a name `ls /` advertises always addresses that mount. In code this is two conditions on rule 4 rather than a special case, and they are the whole rule. Unchanged: unaddressable unqualified names still refuse, mount precedence and the prefix conflict still hold, 'acme/docs/notes' and the 'acme/docs' root still resolve wherever an unqualified path would have refused anyway, and memory-URL semantics are untouched. One existing test moved from one mounted project to two: test_cloud_workspace_project_root_surfaces_a_failed_workspace is about the failed-workspace error, and workspace routes are now only parsed where an unqualified path could not resolve. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 41 ++++++++++++++-- tests/mcp/test_project_path_routing.py | 64 ++++++++++++++++++++++++- 2 files changed, 99 insertions(+), 6 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index 278a7f795..abc79c5fb 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -1030,6 +1030,28 @@ async def detect_project_from_identifier_prefix( # which is worse than not advertising it at all. "Segments", plural: a project # name may contain '/', so its advertised permalink can span more than one. # +# Route versus path is inherently ambiguous, so the answer is a precedence +# order, not a parser. 'acme/docs/foo' is a well-formed workspace route AND a +# well-formed folder path inside the caller's own project; no analysis of the +# string, and no amount of knowing which projects exist, recovers which the +# caller meant. One question settles it: does the call already say which project +# it means? +# +# 1. An explicit project (param or env constraint) says so. The remaining path +# is inside that project and nothing reroutes it. A prefix naming a +# *different* addressable mount still conflicts rather than being silently +# preferred — that is a contradiction in one call, not an ambiguity. +# 2. Otherwise, if several projects are addressable, an unqualified path +# cannot resolve at all (it refuses, below). Reading the leading segments +# as a route is then the only way the input can mean anything, so route +# wins. +# 3. Otherwise the session addresses one project, the path already resolves +# inside it, and route parsing would take a working input and send it to +# another tenant. Path wins. +# +# Rule 3 (mounts) sits above all of this: a name `ls /` advertises always +# addresses that mount. +# # The cost of that choice, stated plainly: when a project's permalink equals an # accessible workspace's slug, that workspace's OTHER projects lose their # qualified path spelling. With '/team' advertised as a mount, 'team/docs/x' is @@ -1355,17 +1377,28 @@ async def resolve_project_path_route( mount_project_id = mount.external_id # --- Rule 4: explicitly workspace-qualified spellings for everything else --- - # Trigger: no advertised mount claimed the leading segments and the input still - # has more than one segment to parse. + # Trigger: no advertised mount claimed the leading segments, the input has + # more than one segment, no project was named, and this session addresses + # more than one project. # Why: '/[/]' addresses projects in workspaces this # session's own route does not list, so they are absent from the mount - # table above and would otherwise be unreachable. + # table above and would otherwise be unreachable. But 'acme/docs/foo' is + # equally a well-formed folder path inside the caller's own project, and + # nothing in the string or the project set recovers which was meant — see + # the route-versus-path precedence note above this section for why the + # two conditions on this line are the whole answer. # Outcome: only a route naming BOTH an accessible workspace and a project # inside it resolves here. An unqualified first segment falls through to # the refusal below instead of being searched for across every accessible # workspace — that search read another tenant's same-named project under # an ordinary project-relative path (#1421). - if detected is None and "/" in candidate: + if ( + detected is None + and "/" in candidate + and explicit is None + and addressable is not None + and len(addressable) > 1 + ): qualified = await _detect_workspace_qualified_route(candidate, config, context=context) if qualified is not None: detected, remainder = qualified diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index 2a29b9e46..ecd9d14c6 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -934,6 +934,59 @@ async def test_cloud_mount_routes_by_the_id_that_names_its_workspace(cross_works assert by_name.workspace.tenant_id == default_workspace.tenant_id +@pytest.mark.asyncio +async def test_sole_project_keeps_workspace_shaped_paths_at_home(cross_workspace_session): + """Route versus path, decided by the precedence order rather than a parse. + + With one mounted project, 'acme/docs/foo' resolves perfectly well as a + folder inside it, so it stays there — even though 'acme' is a real + accessible workspace holding a real project 'docs'. Parsing it as a route + took a working in-project read and served another tenant's project instead. + """ + cross_workspace_session(session_projects=("research",), default_projects=("docs",)) + + route = await resolve_project_path_route("acme/docs/foo", project=None, project_id=None) + + assert route == ProjectPathRoute(project=None, path="acme/docs/foo", stripped=False) + + +@pytest.mark.asyncio +async def test_named_project_keeps_the_rest_of_the_path_inside_it(cross_workspace_session): + """An explicit project settles route-versus-path: the caller said which + project they mean, so the remaining path is inside it and nothing reroutes. + + This spelling previously raised a prefix conflict, which left the reported + bug with no workaround at all — the path could neither stay home on its own + nor be pinned there. + """ + cross_workspace_session(session_projects=("research",), default_projects=("docs",)) + + route = await resolve_project_path_route("acme/docs/foo", project="research", project_id=None) + + assert route == ProjectPathRoute(project="research", path="acme/docs/foo", stripped=False) + + +@pytest.mark.asyncio +async def test_workspace_route_still_wins_when_the_path_could_not_resolve( + cross_workspace_session, +): + """The other half of the order: with several projects mounted, an + unqualified path refuses anyway, so reading the leading segments as a route + is the only way the input can mean anything. Cross-workspace addressing must + not be lost to the fix above.""" + cross_workspace_session( + session_projects=("research", "engineering"), default_projects=("docs",) + ) + + route = await resolve_project_path_route("acme/docs/foo", project=None, project_id=None) + + assert route == ProjectPathRoute(project="acme/docs", path="foo", stripped=True) + + # And an unaddressable name still refuses rather than defaulting. + with pytest.raises(UnqualifiedPathRefusedError): + await resolve_project_path_route("nope/nothing/x", project=None, project_id=None) + + @pytest.mark.asyncio async def test_cloud_explicit_qualified_project_drops_the_mount_id(cross_workspace_session): """The escape hatch keeps working: an explicit '/' names @@ -950,8 +1003,15 @@ async def test_cloud_explicit_qualified_project_drops_the_mount_id(cross_workspa async def test_cloud_workspace_project_root_surfaces_a_failed_workspace(cross_workspace_session): """A '/' root whose workspace could not be listed is a real failure, not an unrecognized path: it must say so rather than fall - through to a refusal that claims the project does not exist.""" - cross_workspace_session(failed_tenant="default-tenant") + through to a refusal that claims the project does not exist. + + Two mounted projects, because workspace routes are only parsed when an + unqualified path could not resolve anyway — see the precedence order. + """ + cross_workspace_session( + failed_tenant="default-tenant", + session_projects=("research", "engineering"), + ) with pytest.raises(ValueError, match="could not be loaded"): await resolve_project_path_route("acme/docs", project=None, project_id=None) From 1f6194f534f8ea979c0c098c64b2254d1855729c Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 18:52:04 -0500 Subject: [PATCH 09/18] fix(mcp): let a qualified name reach the workspace it names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression from the exact-first ordering in the previous commit, reported on that change. With workspace 'acme' holding 'docs' and another accessible workspace holding a project literally named 'acme/docs', the posix route 'acme/docs/x' resolved its workspace correctly, then handed on only the qualified *name* — and the index, preferring a whole-permalink match, read it as the other tenant's project and ran the call there. Fixed at both levels, because they fail independently. The index now tries both readings, qualified first. Qualified wins because it names a workspace explicitly: taking it for some other workspace's whole permalink runs the call against a tenant the caller did not name. The whole-permalink reading stays as the fallback, which is what makes a slash-bearing project name routable at all — 'Research/2026' still resolves where no workspace is called 'Research'. This is the same shape of answer as the route-versus-path order: two legitimate readings, one written-down precedence, not a parser that pretends to know. And the route no longer throws the answer away. _detect_workspace_qualified_route already holds the resolved entry, so it hands on that entry's external_id the way the mount table has since mounts were bound to their workspace. A route that resolved an entry now always carries its id, so nothing downstream re-resolves it by a name that can collide. With the id present the index's ordering is not even consulted for this path — the ordering fix is what covers the explicit project= spelling, which has no id to carry. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 17 ++- .../mcp/workspace_project_index.py | 41 ++++-- tests/mcp/test_project_path_routing.py | 126 ++++++++++++++++-- 3 files changed, 159 insertions(+), 25 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index abc79c5fb..5e6cfdb2f 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -1220,11 +1220,12 @@ async def _detect_workspace_qualified_route( candidate: str, config: BasicMemoryConfig, context: Optional[Context] = None, -) -> tuple[str, str] | None: +) -> tuple[str, str, str] | None: """Resolve an explicitly qualified '/[/]' candidate. - Returns the qualified project identifier and the project-relative remainder, - or None when the candidate does not spell a reachable workspace route. + Returns the qualified project identifier, the project-relative remainder, + and the resolved project's external_id, or None when the candidate does not + spell a reachable workspace route. Both segments must match — the first an accessible workspace slug, the second a project inside *that* workspace — so this never reaches a project @@ -1264,9 +1265,13 @@ async def _detect_workspace_qualified_route( # Unlike the memory-URL caller, the pathless form is kept: a project root is # a legitimate thing to list. The remainder comes from the same match that # chose the project, so the route and the path it leaves behind can never - # disagree about how many segments were consumed. + # disagree about how many segments were consumed. The external_id rides + # along for the same reason the mount table's does: this parse already knows + # the exact entry, and handing on only the qualified *name* would let it be + # re-resolved against a different workspace that happens to hold a project + # literally named '/'. resolution, remainder = resolved - return resolution.project_identifier, remainder + return resolution.project_identifier, remainder, resolution.entry.project.external_id def _workspace_qualifies(qualified: str, bare: str) -> bool: @@ -1401,7 +1406,7 @@ async def resolve_project_path_route( ): qualified = await _detect_workspace_qualified_route(candidate, config, context=context) if qualified is not None: - detected, remainder = qualified + detected, remainder, mount_project_id = qualified if explicit is not None: if detected is None: diff --git a/src/basic_memory/mcp/workspace_project_index.py b/src/basic_memory/mcp/workspace_project_index.py index cd8a2c478..26a8a0ad6 100644 --- a/src/basic_memory/mcp/workspace_project_index.py +++ b/src/basic_memory/mcp/workspace_project_index.py @@ -270,19 +270,38 @@ async def resolve_workspace_project_from_index( from basic_memory.mcp.project_context_identifiers import split_qualified_project_identifier - # Try the whole identifier as a project permalink before reading its first - # segment as a workspace. A project name may contain '/', so 'Research/2026' - # and 'acme/docs' are the same shape and only the index can tell them apart; - # without this, a slash-bearing project name was unroutable, failing with - # "Workspace 'Research' was not found". The v2 project router resolves - # exact-first for the same reason. + # '/' and a project literally named 'acme/docs' are the + # same shape, and only the index can tell them apart — so try both readings, + # qualified first. Qualified wins because it is the spelling this module's + # own disambiguation errors teach, and because it names a workspace + # explicitly: reading it as some other workspace's whole permalink runs the + # call against a tenant the caller did not name. The whole-permalink reading + # is the fallback, which is what makes a slash-bearing project name routable + # at all rather than failing with "Workspace 'Research' was not found". + workspace_identifier, project_identifier = split_qualified_project_identifier(project) whole_permalink = generate_permalink(project) - if whole_permalink in index.entries_by_permalink: + qualified_workspace = ( + next( + ( + workspace + for workspace in index.workspaces + if workspace.slug.casefold() == workspace_identifier.casefold() + or workspace.tenant_id == workspace_identifier + or workspace.name.casefold() == workspace_identifier.casefold() + ), + None, + ) + if workspace_identifier + else None + ) + qualified_hit = qualified_workspace is not None and any( + entry.workspace.tenant_id == qualified_workspace.tenant_id + for entry in index.entries_by_permalink.get(generate_permalink(project_identifier), ()) + ) + if not qualified_hit and whole_permalink in index.entries_by_permalink: workspace_identifier, project_identifier = None, project - project_permalink = whole_permalink - else: - workspace_identifier, project_identifier = split_qualified_project_identifier(project) - project_permalink = generate_permalink(project_identifier) + + project_permalink = generate_permalink(project_identifier) if workspace_identifier: workspace = match_workspace_identifier(index.workspaces, workspace_identifier) diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index ecd9d14c6..5fcd8c796 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -34,6 +34,12 @@ split_project_permalink_prefix, ) from basic_memory.mcp.tools import grep, ls +from basic_memory.mcp.workspace_project_index import ( + WorkspaceProjectEntry, + WorkspaceProjectIndex, + build_workspace_project_index, + resolve_workspace_project_from_index, +) from basic_memory.schemas.cloud import WorkspaceInfo from basic_memory.schemas.project_info import ProjectItem, ProjectList from basic_memory.utils import generate_permalink @@ -556,7 +562,14 @@ async def test_cloud_workspace_qualified_path_without_mount_collision_still_rout route = await resolve_project_path_route("team/research/notes/x", project=None, project_id=None) - assert route == ProjectPathRoute(project="team/research", path="notes/x", stripped=True) + # The route carries the resolved entry's id, so nothing re-resolves the + # qualified name against a workspace that did not answer it. + assert route == ProjectPathRoute( + project="team/research", + path="notes/x", + stripped=True, + project_id="research-external-id", + ) @pytest.mark.asyncio @@ -570,7 +583,9 @@ async def test_cloud_workspace_qualified_project_root_routes(cloud_session): route = await resolve_project_path_route("team/research", project=None, project_id=None) - assert route == ProjectPathRoute(project="team/research", path="", stripped=True) + assert route == ProjectPathRoute( + project="team/research", path="", stripped=True, project_id="research-external-id" + ) @pytest.mark.asyncio @@ -584,14 +599,29 @@ async def test_cloud_workspace_route_matches_multi_segment_project_permalink(clo nested = await resolve_project_path_route( "team/research/2026/notes", project=None, project_id=None ) - assert nested == ProjectPathRoute(project="team/research/2026", path="notes", stripped=True) + assert nested == ProjectPathRoute( + project="team/research/2026", + path="notes", + stripped=True, + project_id="research/2026-external-id", + ) root = await resolve_project_path_route("team/research/2026", project=None, project_id=None) - assert root == ProjectPathRoute(project="team/research/2026", path="", stripped=True) + assert root == ProjectPathRoute( + project="team/research/2026", + path="", + stripped=True, + project_id="research/2026-external-id", + ) # The shorter sibling still claims its own paths — longest match, not first. sibling = await resolve_project_path_route("team/research/notes", project=None, project_id=None) - assert sibling == ProjectPathRoute(project="team/research", path="notes", stripped=True) + assert sibling == ProjectPathRoute( + project="team/research", + path="notes", + stripped=True, + project_id="research-external-id", + ) @pytest.mark.asyncio @@ -980,7 +1010,9 @@ async def test_workspace_route_still_wins_when_the_path_could_not_resolve( route = await resolve_project_path_route("acme/docs/foo", project=None, project_id=None) - assert route == ProjectPathRoute(project="acme/docs", path="foo", stripped=True) + assert route == ProjectPathRoute( + project="acme/docs", path="foo", stripped=True, project_id=_DEFAULT_DOCS_ID + ) # And an unaddressable name still refuses rather than defaulting. with pytest.raises(UnqualifiedPathRefusedError): @@ -1090,10 +1122,88 @@ async def test_cloud_other_workspace_stays_reachable_when_qualified(cross_worksp qualified_path = await resolve_project_path_route( "acme/notes/foo", project=None, project_id=None ) - assert qualified_path == ProjectPathRoute(project="acme/notes", path="foo", stripped=True) + assert qualified_path == ProjectPathRoute( + project="acme/notes", path="foo", stripped=True, project_id=_DEFAULT_NOTES_ID + ) root = await resolve_project_path_route("acme/notes", project=None, project_id=None) - assert root == ProjectPathRoute(project="acme/notes", path="", stripped=True) + assert root == ProjectPathRoute( + project="acme/notes", path="", stripped=True, project_id=_DEFAULT_NOTES_ID + ) explicit = await resolve_project_path_route("foo", project="acme/notes", project_id=None) assert explicit == ProjectPathRoute(project="acme/notes", path="foo", stripped=False) + + +# --- a qualified name must reach the workspace it names (#1421) --- + + +def _index_with(*entries: tuple[WorkspaceInfo, str, str]) -> WorkspaceProjectIndex: + """Build a workspace index from (workspace, project name, external_id) rows.""" + by_tenant: dict[str, WorkspaceInfo] = {} + for workspace, _, _ in entries: + by_tenant.setdefault(workspace.tenant_id, workspace) + workspaces = tuple(by_tenant.values()) + return build_workspace_project_index( + workspaces, + tuple( + WorkspaceProjectEntry( + workspace=workspace, + project=ProjectItem( + id=index + 1, + external_id=external_id, + name=name, + path=f"/app/data/{generate_permalink(name)}", + ), + ) + for index, (workspace, name, external_id) in enumerate(entries) + ), + ) + + +@pytest.mark.asyncio +async def test_qualified_name_beats_a_colliding_whole_permalink(): + """'/' and a project literally named 'acme/docs' are the + same shape, so the index tries both readings — qualified first. + + Preferring the whole permalink sent a route that explicitly named workspace + 'acme' to a *different* tenant that happened to hold a project called + 'acme/docs'. The fallback still exists: it is what makes a slash-bearing + name routable when its first segment names no workspace at all. + """ + acme = WorkspaceInfo( + tenant_id="acme-tenant", + workspace_type="organization", + slug="acme", + name="Acme", + role="editor", + is_default=True, + ) + beta = WorkspaceInfo( + tenant_id="beta-tenant", + workspace_type="organization", + slug="beta", + name="Beta", + role="editor", + is_default=False, + ) + index = _index_with( + (acme, "docs", "11111111-1111-1111-1111-111111111111"), + (beta, "acme/docs", "22222222-2222-2222-2222-222222222222"), + (beta, "Research/2026", "33333333-3333-3333-3333-333333333333"), + ) + + qualified = await resolve_workspace_project_from_index(index, "acme/docs") + assert qualified.workspace.slug == "acme" + assert qualified.project.name == "docs" + + # The fallback: no workspace is named 'Research', so the whole permalink wins. + slash_bearing = await resolve_workspace_project_from_index(index, "Research/2026") + assert slash_bearing.workspace.slug == "beta" + assert slash_bearing.project.name == "Research/2026" + + # And the collided project is still reachable by the id that names it exactly. + by_id = await resolve_workspace_project_from_index( + index, "22222222-2222-2222-2222-222222222222" + ) + assert by_id.project.name == "acme/docs" From 8fcac4204ec30395d7aa48ce882b92bdac151a8d Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 19:08:49 -0500 Subject: [PATCH 10/18] fix(core): make every advertised mount addressable, root included MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two P2s, both about a mount that the routing rules cannot enter. A project name whose permalink is empty is unaddressable. generate_permalink reduces pure punctuation or emoji to "", so a project named '!!!' or a fire emoji advertised itself at the root as directory_path '/', was indistinguishable from every other such project in the mount list, and could not be entered by the path it advertised — feeding '/' back re-invoked the mount listing. Refused at add_project, the one boundary that creates projects, so an unaddressable mount cannot come into existence rather than being coped with downstream. Names still only need one letter, digit, or CJK character. The pathless workspace root now gets the same permission as the path form. In a locally routed session holding cloud credentials, 'acme/docs/note' resolved through workspace discovery while 'acme/docs' — that same project's root, the thing ls needs to enter it — refused, because the shared gate demands three segments outside factory or explicit-cloud sessions. That three-segment requirement belongs to *identifier* detection, which serves read_note and search: they have no mount table to decline the segments first and no refusal rule behind them, so only the unmistakable form may reach for the network. The posix resolver has both, and by the time it asks, the mount table has declined, no project was named, and several are addressable — so an unqualified path refuses anyway and route parsing takes nothing away. The gate is split in two named pieces that say which question each answers. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 28 +++++++--- src/basic_memory/services/project_service.py | 17 +++++- tests/mcp/test_project_path_routing.py | 54 ++++++++++++++++++++ tests/services/test_project_service.py | 18 +++++++ 4 files changed, 109 insertions(+), 8 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index 5e6cfdb2f..866493119 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -241,6 +241,14 @@ def _workspace_identifier_discovery_available( if _cloud_workspace_discovery_available(config): return True + # Identifier detection serves read_note and search, which have no mount + # table and no refusal rule, so only the unmistakable three-segment form may + # reach for the network here. + return _local_cloud_discovery_allowed(config) and _is_workspace_route_shaped(identifier) + + +def _local_cloud_discovery_allowed(config: BasicMemoryConfig) -> bool: + """Return True when a locally routed session may still consult cloud discovery.""" from basic_memory.mcp.async_client import ( _explicit_routing, _force_local_mode, @@ -249,7 +257,7 @@ def _workspace_identifier_discovery_available( if _explicit_routing() and _force_local_mode(): return False - return has_cloud_credentials(config) and _is_workspace_route_shaped(identifier) + return has_cloud_credentials(config) async def resolve_workspace_qualified_memory_url( @@ -1245,12 +1253,18 @@ async def _detect_workspace_qualified_route( """ if _split_workspace_slug_prefix(candidate) is None: return None - # One guard covers both shapes. For the three-segment form it matches the - # identifier detector this replaced: a local session holding cloud - # credentials may consult discovery for an unmistakable workspace route. A - # two-segment identifier is not route-shaped, so for the pathless root form - # the same call narrows to cloud-routed sessions, as it did before. - if not _workspace_identifier_discovery_available(candidate, config): + # The pathless root gets the same permission as the path form. The + # three-segment requirement in _workspace_identifier_discovery_available + # guards *identifier* detection, where a two-segment string is more likely + # an ordinary relative path than a workspace route. Here that judgement has + # already been made: the mount table declined these segments, no project was + # named, and several are addressable, so an unqualified path refuses anyway + # (see the precedence order). Keeping the stricter gate only made + # 'acme/docs/note' resolve while 'acme/docs' — that project's own root — + # refused, in a locally routed session holding cloud credentials. + if not _cloud_workspace_discovery_available(config) and not _local_cloud_discovery_allowed( + config + ): return None try: diff --git a/src/basic_memory/services/project_service.py b/src/basic_memory/services/project_service.py index 336744238..0c44f4202 100644 --- a/src/basic_memory/services/project_service.py +++ b/src/basic_memory/services/project_service.py @@ -223,8 +223,23 @@ async def add_project( set_default: Whether to set this project as the default Raises: - ValueError: If the project already exists or path collides with existing project + ValueError: If the project already exists, the name has no permalink, + or the path collides with an existing project """ + # Trigger: a name whose permalink is empty — pure punctuation or emoji + # ('!!!', '💥'), which generate_permalink reduces to "". + # Why: the permalink is the project's address. An empty one advertises + # itself at the root as '/', is indistinguishable from every other such + # project, and cannot be entered by the path it advertises — it breaks + # the rule that anything the mount view lists is addressable (#1421). + # Outcome: refused at the one boundary that creates projects, so an + # unaddressable mount cannot exist rather than being handled downstream. + if not generate_permalink(name).strip("/"): + raise ValueError( + f"Project name '{name}' has no usable permalink. Names need at least one " + "letter, digit, or CJK character so the project has an address." + ) + # If project_root is set, constrain all projects to that directory project_root = self.config_manager.config.project_root sanitized_name = None diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index 5fcd8c796..72e104fef 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -572,6 +572,60 @@ async def test_cloud_workspace_qualified_path_without_mount_collision_still_rout ) +@pytest.mark.asyncio +async def test_local_cloud_session_lists_a_workspace_project_root( + multi_project_config, monkeypatch +): + """The pathless root gets the same discovery permission as the path form. + + In a locally routed session holding cloud credentials, 'acme/docs/note' + resolved through workspace discovery while 'acme/docs' — that same project's + root, and the thing `ls` needs to enter it — refused. The stricter + three-segment gate belongs to identifier detection (read_note, search), + which has no mount table to decline first and no refusal rule behind it. + """ + workspace = WorkspaceInfo( + tenant_id="acme-tenant", + workspace_type="organization", + slug="acme", + name="Acme", + role="editor", + is_default=True, + ) + + async def fake_workspaces(context=None) -> list[WorkspaceInfo]: + return [workspace] + + async def fake_entries(ws, context=None): + return ( + WorkspaceProjectEntry( + workspace=ws, + project=ProjectItem( + id=1, + external_id="99999999-9999-9999-9999-999999999999", + name="docs", + path="/app/data/docs", + ), + ), + ) + + monkeypatch.setattr(project_context, "get_available_workspaces", fake_workspaces) + monkeypatch.setattr(project_context, "_fetch_workspace_project_entries", fake_entries) + monkeypatch.setattr(project_context, "has_cloud_credentials", lambda config: True) + + root = await resolve_project_path_route("acme/docs", project=None, project_id=None) + nested = await resolve_project_path_route("acme/docs/note", project=None, project_id=None) + + assert root == ProjectPathRoute( + project="acme/docs", + path="", + stripped=True, + project_id="99999999-9999-9999-9999-999999999999", + ) + assert nested.project == "acme/docs" + assert nested.path == "note" + + @pytest.mark.asyncio async def test_cloud_workspace_qualified_project_root_routes(cloud_session): """'/' with no path names that project's root, exactly as diff --git a/tests/services/test_project_service.py b/tests/services/test_project_service.py index b22d6d21b..ee59e15fe 100644 --- a/tests/services/test_project_service.py +++ b/tests/services/test_project_service.py @@ -221,6 +221,24 @@ async def test_get_project_info(project_service: ProjectService, test_graph, tes assert isinstance(info.system, SystemStatus) +@pytest.mark.asyncio +@pytest.mark.parametrize("name", ["💥", "!!!", "---", "/"]) +async def test_add_project_rejects_names_without_a_permalink( + project_service: ProjectService, name: str +): + """A project's permalink is its address, and generate_permalink reduces pure + punctuation or emoji to "". + + Such a project advertises itself at the root as '/', is indistinguishable + from every other one like it, and cannot be entered by the path it + advertises — breaking the rule that anything the mount view lists is + addressable. Refused where projects are created, so it cannot exist. + """ + with tempfile.TemporaryDirectory() as temp_dir: + with pytest.raises(ValueError, match="no usable permalink"): + await project_service.add_project(name, temp_dir) + + @pytest.mark.asyncio async def test_add_project_async(project_service: ProjectService): """Test adding a project with the updated async method.""" From 19662cb7e7fa00f39dc0870f8b8e250da0822140 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 19:21:57 -0500 Subject: [PATCH 11/18] fix(mcp): route-versus-path protects exactly one project, not none MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on the precedence gate from 8214e043, reported against it. A factory session whose connection-time workspace holds no projects fell through both halves of the order: 'acme/docs/note' skipped workspace parsing because the session did not address more than one project, and then skipped the refusal for the same reason, so it routed with project=None into the empty workspace instead of the accessible project the caller had named. The order already said why this is wrong, and the code said "more than one" where the reasoning said "exactly one". Path wins over route because the path already resolves inside the session's sole project; with no projects there is nothing for it to resolve inside, so route parsing takes nothing away — and an empty mount table cannot refuse on the caller's behalf either. The gate now reads != 1, and the note above the section says exactly one and why. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 12 ++-- tests/mcp/test_project_path_routing.py | 75 +++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 5 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index 866493119..a43f0972c 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -1053,9 +1053,11 @@ async def detect_project_from_identifier_prefix( # cannot resolve at all (it refuses, below). Reading the leading segments # as a route is then the only way the input can mean anything, so route # wins. -# 3. Otherwise the session addresses one project, the path already resolves -# inside it, and route parsing would take a working input and send it to -# another tenant. Path wins. +# 3. Otherwise the session addresses *exactly* one project, the path already +# resolves inside it, and route parsing would take a working input and send +# it to another tenant. Path wins. Exactly one, because with none there is +# no project for the path to resolve inside — nothing is taken away, and +# the empty mount table cannot refuse on the caller's behalf either. # # Rule 3 (mounts) sits above all of this: a name `ls /` advertises always # addresses that mount. @@ -1398,7 +1400,7 @@ async def resolve_project_path_route( # --- Rule 4: explicitly workspace-qualified spellings for everything else --- # Trigger: no advertised mount claimed the leading segments, the input has # more than one segment, no project was named, and this session addresses - # more than one project. + # any number of projects other than exactly one. # Why: '/[/]' addresses projects in workspaces this # session's own route does not list, so they are absent from the mount # table above and would otherwise be unreachable. But 'acme/docs/foo' is @@ -1416,7 +1418,7 @@ async def resolve_project_path_route( and "/" in candidate and explicit is None and addressable is not None - and len(addressable) > 1 + and len(addressable) != 1 ): qualified = await _detect_workspace_qualified_route(candidate, config, context=context) if qualified is not None: diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index 72e104fef..e615d4d0e 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -1261,3 +1261,78 @@ async def test_qualified_name_beats_a_colliding_whole_permalink(): index, "22222222-2222-2222-2222-222222222222" ) assert by_id.project.name == "acme/docs" + + +@pytest.mark.asyncio +async def test_empty_session_workspace_still_reaches_a_named_workspace(monkeypatch, config_manager): + """Route-versus-path protects *exactly* one addressable project, not none. + + A factory session whose connection-time workspace holds no projects has no + project for a relative path to resolve inside, and an empty mount table + cannot refuse on the caller's behalf either — so 'acme/docs/note' fell + through to project=None and tried the empty workspace. With nothing to take + away, the explicitly named workspace wins. + """ + config = config_manager.load_config() + config.projects = {} + config.default_project = None + config_manager.save_config(config) + + empty_ws = WorkspaceInfo( + tenant_id="empty-tenant", + workspace_type="organization", + slug="beta", + name="Beta", + role="editor", + is_default=False, + ) + acme = WorkspaceInfo( + tenant_id="acme-tenant", + workspace_type="organization", + slug="acme", + name="Acme", + role="editor", + is_default=True, + ) + + @asynccontextmanager + async def fake_get_client(*a, **k): + yield object() + + async def fake_list_projects(self): + return ProjectList(projects=[], default_project=None) # session workspace is EMPTY + + async def fake_workspaces(context=None): + return [empty_ws, acme] + + async def fake_entries(ws, context=None): + if ws.tenant_id != "acme-tenant": + return () + return ( + WorkspaceProjectEntry( + workspace=ws, + project=ProjectItem( + id=1, + external_id="77777777-7777-7777-7777-777777777777", + name="docs", + path="/app/docs", + ), + ), + ) + + monkeypatch.setattr(async_client, "is_factory_mode", lambda: True) + monkeypatch.setattr(async_client, "get_client", fake_get_client) + monkeypatch.setattr( + "basic_memory.mcp.clients.project.ProjectClient.list_projects", fake_list_projects + ) + monkeypatch.setattr(project_context, "get_available_workspaces", fake_workspaces) + monkeypatch.setattr(project_context, "_fetch_workspace_project_entries", fake_entries) + + route = await resolve_project_path_route("acme/docs/note", project=None, project_id=None) + + assert route == ProjectPathRoute( + project="acme/docs", + path="note", + stripped=True, + project_id="77777777-7777-7777-7777-777777777777", + ) From 6f1bd64b55e69ea4cf4e31adb193dcfcc6802d94 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 19:34:00 -0500 Subject: [PATCH 12/18] fix(core): reject project permalinks with any empty segment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on the empty-permalink validation in 8fcac420, reported against it. That check stripped slashes before testing, so a project named '/foo' passed: its permalink is '/foo', stripping leaves 'foo', and the name looked fine. The mount view then advertised '//foo' while the resolver, which strips leading slashes off the candidate before matching, could never match the stored '/foo' — the advertised mount could not be entered, and in a single-project setup the path fell through to the default instead. The right test is the one the resolver actually performs: it matches a permalink segment by segment, so every segment must be non-empty. That covers both shapes at once — '' (pure punctuation or emoji) and '/foo' (empty leading segment) — and it is the same rule split_project_permalink_prefix applies to candidate paths, where an empty interior segment already refuses rather than being repaired. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/services/project_service.py | 20 ++++++++++++-------- tests/services/test_project_service.py | 12 +++++++----- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/basic_memory/services/project_service.py b/src/basic_memory/services/project_service.py index 0c44f4202..93dfb3659 100644 --- a/src/basic_memory/services/project_service.py +++ b/src/basic_memory/services/project_service.py @@ -226,18 +226,22 @@ async def add_project( ValueError: If the project already exists, the name has no permalink, or the path collides with an existing project """ - # Trigger: a name whose permalink is empty — pure punctuation or emoji - # ('!!!', '💥'), which generate_permalink reduces to "". - # Why: the permalink is the project's address. An empty one advertises - # itself at the root as '/', is indistinguishable from every other such - # project, and cannot be entered by the path it advertises — it breaks - # the rule that anything the mount view lists is addressable (#1421). + # Trigger: a name whose permalink has an empty segment — pure punctuation + # or emoji ('!!!', '💥') reduce to "", and a leading slash ('/foo') + # leaves an empty first segment. + # Why: the permalink is the project's address, and the resolver matches + # it segment by segment against a path whose leading slashes are + # already stripped. An empty segment means no path can ever match it: + # '' advertises at the root as '/', indistinguishable from every other + # such project, and '/foo' advertises '//foo' and cannot be entered. + # Either way the mount view lists something unaddressable (#1421). # Outcome: refused at the one boundary that creates projects, so an # unaddressable mount cannot exist rather than being handled downstream. - if not generate_permalink(name).strip("/"): + if not all(generate_permalink(name).split("/")): raise ValueError( f"Project name '{name}' has no usable permalink. Names need at least one " - "letter, digit, or CJK character so the project has an address." + "letter, digit, or CJK character in every path segment, and may not start " + "with '/', so the project has an address." ) # If project_root is set, constrain all projects to that directory diff --git a/tests/services/test_project_service.py b/tests/services/test_project_service.py index ee59e15fe..eb5391c4b 100644 --- a/tests/services/test_project_service.py +++ b/tests/services/test_project_service.py @@ -222,17 +222,19 @@ async def test_get_project_info(project_service: ProjectService, test_graph, tes @pytest.mark.asyncio -@pytest.mark.parametrize("name", ["💥", "!!!", "---", "/"]) +@pytest.mark.parametrize("name", ["💥", "!!!", "---", "/", "/foo", "/a/b"]) async def test_add_project_rejects_names_without_a_permalink( project_service: ProjectService, name: str ): """A project's permalink is its address, and generate_permalink reduces pure punctuation or emoji to "". - Such a project advertises itself at the root as '/', is indistinguishable - from every other one like it, and cannot be entered by the path it - advertises — breaking the rule that anything the mount view lists is - addressable. Refused where projects are created, so it cannot exist. + The resolver matches a permalink segment by segment against a path whose + leading slashes are already stripped, so any empty segment is unmatchable: + "" advertises at the root as '/', indistinguishable from every other such + project, and '/foo' advertises '//foo' and cannot be entered. Either breaks + the rule that anything the mount view lists is addressable, so both are + refused where projects are created. """ with tempfile.TemporaryDirectory() as temp_dir: with pytest.raises(ValueError, match="no usable permalink"): From f66b3b7dd356e933446ee1a24702288936fd341b Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 19:47:56 -0500 Subject: [PATCH 13/18] fix(mcp): strip an explicit project's own prefix without discovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on the precedence gate, reported against it, and a cost I saw when writing that gate and should have reported rather than accepted quietly. Rule 2 has always stripped a path prefix that agrees with the explicit project. That only worked for spellings the mount table recognizes; a workspace-qualified project reached agreement solely through rule 4's discovery, which the precedence order now declines to run when a project was named. So the caller's own two spellings of one project stopped agreeing: cat("acme/docs/foo", project="acme/docs") -> path 'acme/docs/foo' which asks the already-selected docs project for a nested path that is not there. It also broke the round trip the requalification fix established, since a routed ls("acme/docs") returns exactly that prefixed form. The agreement never needed the network. It is the caller's own two spellings of one project, so it is decidable locally: match the explicit project's permalink against the candidate's leading segments with the same primitive everything else uses, and strip on a hit. That runs before the rule 4 gate and leaves the gate alone — a prefix naming something *other* than the named project still stays part of the path, which is the behaviour the previous commit established. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 16 ++++++++++++++++ tests/mcp/test_project_path_routing.py | 24 ++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index a43f0972c..259539b47 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -1397,6 +1397,22 @@ async def resolve_project_path_route( detected = mount.name mount_project_id = mount.external_id + # --- Rule 3b: the explicit project's own permalink, spelled as the prefix --- + # Trigger: a project was named, no mount claimed the segments, and the path + # begins with that same project's permalink. + # Why: rule 2 already strips a prefix that agrees with the explicit project; + # that only worked for spellings the mount table recognizes, so a + # workspace-qualified project reached it solely through rule 4's discovery + # — which the precedence order now (correctly) declines to run when a + # project was named. The agreement is decidable right here without any + # network: it is the caller's own two spellings of one project. + # Outcome: cat("acme/docs/foo", project="acme/docs") reads 'foo', and a path + # from a routed ls("acme/docs") round-trips with the project param set. + if detected is None and explicit is not None: + explicit_claim = _split_project_permalink_prefix(candidate, (generate_permalink(explicit),)) + if explicit_claim is not None: + detected, remainder = explicit, explicit_claim[1] + # --- Rule 4: explicitly workspace-qualified spellings for everything else --- # Trigger: no advertised mount claimed the leading segments, the input has # more than one segment, no project was named, and this session addresses diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index e615d4d0e..b48abf554 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -1034,6 +1034,30 @@ async def test_sole_project_keeps_workspace_shaped_paths_at_home(cross_workspace assert route == ProjectPathRoute(project=None, path="acme/docs/foo", stripped=False) +@pytest.mark.asyncio +async def test_named_project_strips_its_own_qualified_prefix(cross_workspace_session): + """Rule 2 — an agreeing prefix strips — has to hold for the qualified + spelling too, and it is decidable without any network. + + Only rule 4's discovery used to recognize a workspace-qualified project in + the path, and the precedence order now declines to run that when a project + was named. So the caller's own two spellings of one project stopped + agreeing: 'foo' was read as 'acme/docs/foo'. It also broke the round trip, + since a routed ls("acme/docs") returns exactly this prefixed form. + """ + cross_workspace_session(session_projects=("research",), default_projects=("docs",)) + + route = await resolve_project_path_route("acme/docs/foo", project="acme/docs", project_id=None) + + assert route == ProjectPathRoute(project="acme/docs", path="foo", stripped=True) + + # A prefix that is NOT the named project still stays part of the path. + unrelated = await resolve_project_path_route( + "acme/docs/foo", project="research", project_id=None + ) + assert unrelated.path == "acme/docs/foo" + + @pytest.mark.asyncio async def test_named_project_keeps_the_rest_of_the_path_inside_it(cross_workspace_session): """An explicit project settles route-versus-path: the caller said which From 195c48b4818a89ba33a31125ad52d226f96fd123 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 20:03:51 -0500 Subject: [PATCH 14/18] fix(mcp): keep emitted workspace routes replayable with one mount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverses the mount-count half of the precedence gate from 8214e043, because it made this layer's own emitted addresses unroutable. In a factory session whose sole mount is 'docs', ls("docs", project="acme/docs") routes to acme/docs, strips the agreeing prefix, and requalifies its children as 'acme/docs/...'. Replaying one without the project argument hit the len(addressable) != 1 gate, skipped workspace parsing, and read the sole mount's same-named path in the *other* tenant. A navigation path returned for one workspace silently read another. The two findings are in direct tension and both are silent wrong-project reads, so the tie cannot break on harm. It breaks on whose string it is. The canonical qualified form is one we emit and publish as an address, so it has to route back. A user-typed relative path that coincidentally spells a real accessible workspace *and* a real project inside it is a collision, and it has an explicit, documented fix: name the project, and rule 1 keeps the whole path inside it — which is exactly what the earlier finding asked for and what rule 3b now makes work for the qualified spelling too. Coincidence loses to the published address. Rule 2 therefore no longer consults the mount count. It still requires both halves to match a real workspace and a real project in it, so a merely workspace-shaped path like 'notes/2026/foo' matches nothing and stays relative on its own — that case is pinned alongside the replay case. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 34 +++++++++---------- tests/mcp/test_project_path_routing.py | 44 +++++++++++++++++++++---- 2 files changed, 54 insertions(+), 24 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index 259539b47..f18f06af3 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -1049,15 +1049,20 @@ async def detect_project_from_identifier_prefix( # is inside that project and nothing reroutes it. A prefix naming a # *different* addressable mount still conflicts rather than being silently # preferred — that is a contradiction in one call, not an ambiguity. -# 2. Otherwise, if several projects are addressable, an unqualified path -# cannot resolve at all (it refuses, below). Reading the leading segments -# as a route is then the only way the input can mean anything, so route -# wins. -# 3. Otherwise the session addresses *exactly* one project, the path already -# resolves inside it, and route parsing would take a working input and send -# it to another tenant. Path wins. Exactly one, because with none there is -# no project for the path to resolve inside — nothing is taken away, and -# the empty mount table cannot refuse on the caller's behalf either. +# 2. Otherwise the leading segments are read as a route, but only when they +# name an accessible workspace AND a project inside it. A path that merely +# looks workspace-shaped ('notes/2026/foo') matches nothing and stays +# relative on its own. +# +# Rule 2 deliberately does not depend on how many projects the session mounts. +# It did briefly, to keep a coincidentally workspace-shaped relative path at +# home in a one-mount session; that made the qualified paths this layer *itself* +# emits unroutable there, since replaying one without the project argument read +# the sole mount's same-named path instead. Both readings are a silent +# wrong-project read, so the tie breaks on whose string it is: the emitted +# canonical form is ours and must route back, while a user-typed path that +# collides with a real workspace and project has an explicit, documented fix in +# rule 1. Coincidence loses to the address we published. # # Rule 3 (mounts) sits above all of this: a name `ls /` advertises always # addresses that mount. @@ -1415,8 +1420,7 @@ async def resolve_project_path_route( # --- Rule 4: explicitly workspace-qualified spellings for everything else --- # Trigger: no advertised mount claimed the leading segments, the input has - # more than one segment, no project was named, and this session addresses - # any number of projects other than exactly one. + # more than one segment, and no project was named. # Why: '/[/]' addresses projects in workspaces this # session's own route does not list, so they are absent from the mount # table above and would otherwise be unreachable. But 'acme/docs/foo' is @@ -1429,13 +1433,7 @@ async def resolve_project_path_route( # the refusal below instead of being searched for across every accessible # workspace — that search read another tenant's same-named project under # an ordinary project-relative path (#1421). - if ( - detected is None - and "/" in candidate - and explicit is None - and addressable is not None - and len(addressable) != 1 - ): + if detected is None and "/" in candidate and explicit is None: qualified = await _detect_workspace_qualified_route(candidate, config, context=context) if qualified is not None: detected, remainder, mount_project_id = qualified diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index b48abf554..d7683c4ca 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -1019,19 +1019,51 @@ async def test_cloud_mount_routes_by_the_id_that_names_its_workspace(cross_works @pytest.mark.asyncio -async def test_sole_project_keeps_workspace_shaped_paths_at_home(cross_workspace_session): +async def test_emitted_cross_workspace_path_replays_with_one_mount(cross_workspace_session): + """A path this layer emits must route back through the same session. + + `ls("docs", project="acme/docs")` strips the agreeing prefix and requalifies + its children as 'acme/docs/...'. Replaying one without the project argument + used to hit the mount-count gate, skip workspace parsing, and read the sole + mount's same-named path in the *other* tenant — a navigation path returned + for one workspace silently reading another. + """ + cross_workspace_session(session_projects=("docs",), default_projects=("docs",)) + + original = await resolve_project_path_route("docs", project="acme/docs", project_id=None) + assert original.project == "acme/docs" + + replay = await resolve_project_path_route("acme/docs/notes", project=None, project_id=None) + + assert replay == ProjectPathRoute( + project="acme/docs", path="notes", stripped=True, project_id=_DEFAULT_DOCS_ID + ) + + +@pytest.mark.asyncio +async def test_workspace_shaped_path_routes_even_with_one_mount(cross_workspace_session): """Route versus path, decided by the precedence order rather than a parse. - With one mounted project, 'acme/docs/foo' resolves perfectly well as a - folder inside it, so it stays there — even though 'acme' is a real - accessible workspace holding a real project 'docs'. Parsing it as a route - took a working in-project read and served another tenant's project instead. + An unqualified path whose leading segments name a real accessible workspace + and a real project inside it is read as a route, whatever the session + mounts. This briefly depended on the mount count, to keep a coincidentally + workspace-shaped relative path at home in a one-mount session — but that + made the qualified paths this layer itself emits unroutable there, and both + readings are a silent wrong-project read. The tie breaks on whose string it + is: the emitted canonical form is ours and must route back; a user-typed + collision has the explicit fix asserted in the test below. """ cross_workspace_session(session_projects=("research",), default_projects=("docs",)) route = await resolve_project_path_route("acme/docs/foo", project=None, project_id=None) - assert route == ProjectPathRoute(project=None, path="acme/docs/foo", stripped=False) + assert route == ProjectPathRoute( + project="acme/docs", path="foo", stripped=True, project_id=_DEFAULT_DOCS_ID + ) + + # A path that only looks workspace-shaped matches no workspace and stays put. + coincidence = await resolve_project_path_route("notes/2026/foo", project=None, project_id=None) + assert coincidence == ProjectPathRoute(project=None, path="notes/2026/foo", stripped=False) @pytest.mark.asyncio From 9ba36c51a8ff682c2dbd261808f2e0098e383f57 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 20:18:05 -0500 Subject: [PATCH 15/18] fix(core): refuse two projects sharing one mount permalink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Distinct names can normalize to one permalink — 'My Docs' beside 'my-docs' — and the permalink is the address, so that is one address for two projects. ls("/") advertised both at /my-docs while the mount lookup kept whichever sorted last, so every qualified read under that prefix went to one project and the other was unreachable. Worse, the mount lookup and _canonicalize_project_name disagreed about which one: the route reported 'My Docs' while the mount entry was 'my-docs', so a path could read one project's content under the other's name. Refused in two places, because they cover different populations. add_project rejects a name whose permalink matches an existing project's, which is where the second of the pair would be created. Only a config written before this check, or edited by hand, can still hold a collision. For those, the resolver refuses the route instead of picking. A silent wrong-project read is worse than a loud failure, so AmbiguousMountError names both projects and points at the two ways out — rename one, or pass project= with the exact name. ls("/") still lists both, which is where the collision is visible. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 22 ++++++++++++++++++- src/basic_memory/services/project_service.py | 18 +++++++++++++++ tests/mcp/test_project_path_routing.py | 23 ++++++++++++++++++++ tests/services/test_project_service.py | 16 ++++++++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index f18f06af3..9df947553 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -1112,6 +1112,10 @@ class UnqualifiedPathRefusedError(ValueError): """Unqualified input matched no project in a workspace that addresses several.""" +class AmbiguousMountError(ValueError): + """Two addressable projects share one permalink, so a path names both.""" + + @dataclass(frozen=True) class AddressableProject: """One project this session can both advertise and route to. @@ -1223,7 +1227,23 @@ def _claim_mount_prefix( lives in ``split_project_permalink_prefix``; this only maps the winning permalink back to the project that owns it. """ - by_permalink = {project.permalink: project for project in projects} + by_permalink: dict[str, AddressableProject] = {} + for project in projects: + collision = by_permalink.setdefault(project.permalink, project) + # Trigger: two addressable projects share a permalink ('My Docs' beside + # 'my-docs'). add_project refuses this now, so only a config written + # before that check, or edited by hand, can reach here. + # Why: the permalink is the address. Silently keeping one would route + # every path under it to whichever project happened to sort last and + # read that project's content under the other's name. + # Outcome: refuse the whole route and name both, rather than pick. + if collision is not project: + raise AmbiguousMountError( + f"projects '{collision.name}' and '{project.name}' share the permalink " + f"'{project.permalink}', so that path names both. Rename one, or pass " + "project= with the exact name." + ) + claimed = _split_project_permalink_prefix(candidate, by_permalink) if claimed is None: return None diff --git a/src/basic_memory/services/project_service.py b/src/basic_memory/services/project_service.py index 93dfb3659..f8bff9b9c 100644 --- a/src/basic_memory/services/project_service.py +++ b/src/basic_memory/services/project_service.py @@ -270,6 +270,24 @@ async def add_project( async with db.scoped_session(self.session_maker) as session: existing_projects = await self.repository.find_all(session, use_load_options=False) + + # Trigger: a different name that normalizes to an existing project's + # permalink ('My Docs' beside 'my-docs'). + # Why: the permalink is the address, so two of them are one address + # for two projects. The mount view advertises both at the same + # path and the resolver can only pick one, leaving the other + # unreachable and its paths reading the wrong project's content. + # Outcome: refused here, where the second one would be created. + name_permalink = generate_permalink(name) + for existing in existing_projects: + if existing.name != name and generate_permalink(existing.name) == name_permalink: + raise ValueError( + f"Project name '{name}' has the same permalink as existing project " + f"'{existing.name}' ('{name_permalink}'). Project permalinks are " + "addresses and must be unique; choose a name that differs by more " + "than case, spacing, or punctuation." + ) + if project_root: # Check for case-insensitive path collisions with existing projects for existing in existing_projects: diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index d7683c4ca..86355e7e4 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -23,6 +23,7 @@ from basic_memory.mcp.project_context import ( ProjectPathRoute, ProjectPrefixConflictError, + AmbiguousMountError, UnqualifiedPathRefusedError, _agreed_route_project, resolve_project_path_route, @@ -199,6 +200,28 @@ async def test_multi_segment_mount_agrees_with_explicit_workspace_spelling( ) +@pytest.mark.asyncio +async def test_colliding_mount_permalinks_refuse_rather_than_pick(config_manager, tmp_path_factory): + """Two names that normalize to one permalink are one address for two + projects, and the resolver must not choose. + + add_project refuses this now, so only a config written before that check or + edited by hand can reach it — but silently keeping whichever sorted last + read that project's content under the other's name, which is worse than a + loud failure naming both. + """ + config = config_manager.load_config() + config.projects["My Docs"] = ProjectEntry(path=str(tmp_path_factory.mktemp("my-docs-a"))) + config.projects["my-docs"] = ProjectEntry(path=str(tmp_path_factory.mktemp("my-docs-b"))) + config_manager.save_config(config) + + with pytest.raises(AmbiguousMountError) as excinfo: + await resolve_project_path_route("my-docs/notes", project=None, project_id=None) + + assert "My Docs" in str(excinfo.value) + assert "my-docs" in str(excinfo.value) + + @pytest.mark.asyncio async def test_glob_first_segment_never_routes(multi_project_config): """split_project_prefix's '*' guard: a glob first segment is search input, diff --git a/tests/services/test_project_service.py b/tests/services/test_project_service.py index eb5391c4b..a273b3317 100644 --- a/tests/services/test_project_service.py +++ b/tests/services/test_project_service.py @@ -241,6 +241,22 @@ async def test_add_project_rejects_names_without_a_permalink( await project_service.add_project(name, temp_dir) +@pytest.mark.asyncio +async def test_add_project_rejects_a_colliding_permalink( + project_service: ProjectService, test_project +): + """Permalinks are addresses, so two projects cannot share one. + + 'My Docs' beside 'my-docs' advertised both mounts at the same path and the + resolver could only pick one, leaving the other unreachable and its paths + reading the wrong project's content. + """ + with tempfile.TemporaryDirectory() as temp_dir: + colliding = test_project.name.upper().replace("-", " ") + with pytest.raises(ValueError, match="same permalink as existing project"): + await project_service.add_project(colliding, temp_dir) + + @pytest.mark.asyncio async def test_add_project_async(project_service: ProjectService): """Test adding a project with the updated async method.""" From 52754f96a7efc08c23654fa0b77403e46a62259d Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 20:31:15 -0500 Subject: [PATCH 16/18] fix(mcp): give the workspace probe the authoritative slug precedence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression from the qualified-first ordering in 1f6194f5, reported against it. The probe that decides whether the qualified reading resolves carried its own inline matcher — one pass over the workspaces taking the first of slug, tenant_id, or display name to hit. match_workspace_identifier gives slugs *global* precedence over tenant ids over display names, so the two disagreed whenever one workspace's display name equalled another's slug. With 'foo' as one workspace's display name and another's slug, resolving 'foo/docs' probed the display-name workspace, found no 'docs' in it, called the qualified reading a miss, and fell back to a whole-permalink project literally named 'foo/docs' in a third workspace. A route naming a real slug read across two workspace boundaries. The precedence now has one definition. find_workspace_identifier holds it and returns None when nothing matches, so a caller probing whether a segment *is* a workspace can ask without catching; match_workspace_identifier is that plus the not-found error. Same shape as the other fixes in this series: the bug was a second implementation of a rule that already existed, so the fix is to delete the second one rather than teach it the same lesson. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- .../mcp/workspace_project_index.py | 40 ++++++++---- tests/mcp/test_project_path_routing.py | 65 +++++++++++++++++++ 2 files changed, 91 insertions(+), 14 deletions(-) diff --git a/src/basic_memory/mcp/workspace_project_index.py b/src/basic_memory/mcp/workspace_project_index.py index 26a8a0ad6..fddfffe13 100644 --- a/src/basic_memory/mcp/workspace_project_index.py +++ b/src/basic_memory/mcp/workspace_project_index.py @@ -214,11 +214,20 @@ def format_qualified_choices(entries: Sequence[WorkspaceProjectEntry]) -> str: return " or ".join(entry.qualified_name for entry in entries) -def match_workspace_identifier( +def find_workspace_identifier( workspaces: tuple[WorkspaceInfo, ...], workspace_identifier: str, -) -> WorkspaceInfo: - """Resolve a qualified route segment by slug, tenant_id, then display name.""" +) -> WorkspaceInfo | None: + """Resolve a qualified route segment by slug, tenant_id, then display name. + + Returns None when nothing matches, so a caller probing whether a segment + *is* a workspace can ask without catching. The precedence lives here only: + slugs win globally over tenant ids, which win over display names, so one + workspace's display name never shadows another's slug. A second matcher + that walked the list once and took the first field to hit would decide by + iteration order instead, and did — it sent a route naming a slug-owned + workspace to a whole-permalink project in a third workspace entirely. + """ slug_matches = [ workspace for workspace in workspaces @@ -247,7 +256,19 @@ def match_workspace_identifier( if name_matches: return name_matches[0] - available = ", ".join(workspace.slug for workspace in workspaces) + return None + + +def match_workspace_identifier( + workspaces: tuple[WorkspaceInfo, ...], + workspace_identifier: str, +) -> WorkspaceInfo: + """Resolve a qualified route segment, failing when nothing matches.""" + workspace = find_workspace_identifier(workspaces, workspace_identifier) + if workspace is not None: + return workspace + + available = ", ".join(item.slug for item in workspaces) raise ValueError( f"Workspace '{workspace_identifier}' was not found by slug, tenant_id, or name. " f"Available workspace slugs: {available}" @@ -281,16 +302,7 @@ async def resolve_workspace_project_from_index( workspace_identifier, project_identifier = split_qualified_project_identifier(project) whole_permalink = generate_permalink(project) qualified_workspace = ( - next( - ( - workspace - for workspace in index.workspaces - if workspace.slug.casefold() == workspace_identifier.casefold() - or workspace.tenant_id == workspace_identifier - or workspace.name.casefold() == workspace_identifier.casefold() - ), - None, - ) + find_workspace_identifier(index.workspaces, workspace_identifier) if workspace_identifier else None ) diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index 86355e7e4..7042c0415 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -1294,6 +1294,71 @@ def _index_with(*entries: tuple[WorkspaceInfo, str, str]) -> WorkspaceProjectInd ) +@pytest.mark.asyncio +async def test_slug_precedence_survives_the_whole_permalink_probe(): + """One workspace's display name must not shadow another's slug. + + match_workspace_identifier gives slugs global precedence. The probe that + decides whether the qualified reading resolves had its own inline matcher + that took the first field to hit in iteration order, so a display-name + workspace won, the qualified reading looked like a miss, and the request + fell through to an unrelated whole-permalink project in a third workspace — + a cross-workspace read from a route that named a real slug. + """ + named_foo = WorkspaceInfo( + tenant_id="t1", + workspace_type="organization", + slug="wsone", + name="foo", + role="editor", + is_default=False, + ) + slug_foo = WorkspaceInfo( + tenant_id="t2", + workspace_type="organization", + slug="foo", + name="Second", + role="editor", + is_default=False, + ) + third = WorkspaceInfo( + tenant_id="t3", + workspace_type="organization", + slug="third", + name="Third", + role="editor", + is_default=False, + ) + index = build_workspace_project_index( + (named_foo, slug_foo, third), + ( + WorkspaceProjectEntry( + workspace=slug_foo, + project=ProjectItem( + id=1, + external_id="11111111-1111-1111-1111-111111111111", + name="docs", + path="/app/docs", + ), + ), + WorkspaceProjectEntry( + workspace=third, + project=ProjectItem( + id=2, + external_id="22222222-2222-2222-2222-222222222222", + name="foo/docs", + path="/app/foo-docs", + ), + ), + ), + ) + + entry = await resolve_workspace_project_from_index(index, "foo/docs") + + assert entry.workspace.slug == "foo" + assert entry.project.name == "docs" + + @pytest.mark.asyncio async def test_qualified_name_beats_a_colliding_whole_permalink(): """'/' and a project literally named 'acme/docs' are the From 1303e8a79b1f499d117a4ac924ff44a662b97db9 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 21:19:00 -0500 Subject: [PATCH 17/18] fix(mcp): decide route agreement by identity, not by counting slashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With local projects 'docs' and 'team/docs' both present, cat("team/docs/note", project="docs") detected the 'team/docs' mount and then the shape heuristic read its extra leading segment as a workspace qualifier for 'docs'. The two spellings "agreed", the explicit selection was discarded, and the call read note from team/docs instead of raising ProjectPrefixConflictError. Two real projects were treated as one project spelled two ways. Identity now settles it, and the strings are never consulted when it can. In the explicit branch `detected` is always a mount — rule 4 does not run when a project was named, and rule 3b sets detected to the explicit value itself — so when `explicit` also names an addressable project, both sides have a resolved local identity. Same project, they agree; different projects, that is a contradiction in one call and it raises, whatever their shapes suggest. The shape comparison is not removed, and is worth being explicit about. It now runs in exactly one case: `explicit` names no addressable project, so it addresses another workspace. There is no local identity to compare against, and resolving one would need the workspace discovery that rule 1 deliberately skips when a project was named — so the segment-count rule stays as the fallback for identities this session cannot resolve, rather than as a shortcut around ones it can. That keeps project='acme/docs' and project='other/second-project' working as the documented cross-workspace escape hatch. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 34 ++++++++-- tests/mcp/test_project_path_routing.py | 83 +++++++++++++++++++++---- 2 files changed, 102 insertions(+), 15 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index 9df947553..73d3d1192 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -1333,17 +1333,43 @@ def _workspace_qualifies(qualified: str, bare: str) -> bool: ) -def _agreed_route_project(detected: str, explicit: str) -> str | None: +def _agreed_route_project( + detected: str, + explicit: str, + addressable: tuple[AddressableProject, ...], +) -> str | None: """The project both spellings name, or None when they name different projects. + Identity first, spelling only as a last resort. When ``explicit`` names a + project this session addresses, the question is settled without looking at + the strings at all: the two either name the same addressable project or two + different ones, and two different ones are a conflict. Deciding that by + shape instead read 'team/docs' as a workspace-qualified spelling of 'docs' + and served the wrong project, silently, in a config where both exist. + + The shape comparison survives for exactly one case: ``explicit`` names no + addressable project, so it addresses another workspace. There is no local + identity to compare against — resolving it would take the workspace + discovery that rule 1 deliberately does not run when a project was named — + and a workspace slug is exactly one segment, so the qualified spelling is + the bare one plus exactly one leading segment. That is a genuine fallback + for identities this session cannot resolve, not a shortcut around ones it + can. + Returns the more-qualified spelling, so an explicit '/' outlives a bare prefix match: a local project can shadow a same-named project in another workspace, and dropping the explicitly named workspace - would silently reroute the call to the local shadow. Agreement and which - spelling wins come from one comparison, so they cannot disagree. + would silently reroute the call to the local shadow. """ if generate_permalink(detected) == generate_permalink(explicit): return detected + + # Both name projects this session can address, and they are not the same + # one — a contradiction in a single call, whatever their spellings suggest. + explicit_permalink = generate_permalink(explicit) + if any(project.permalink == explicit_permalink for project in addressable): + return None + if _workspace_qualifies(explicit, detected): return explicit if _workspace_qualifies(detected, explicit): @@ -1463,7 +1489,7 @@ async def resolve_project_path_route( return ProjectPathRoute( project=_canonicalize_project_name(explicit, config), path=path, stripped=False ) - routed = _agreed_route_project(detected, explicit) + routed = _agreed_route_project(detected, explicit, addressable or ()) if routed is not None: # Trigger: the explicit spelling is workspace-qualified while the # path prefix matched an unqualified local config name. diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index 7042c0415..351db8912 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -21,6 +21,7 @@ import basic_memory.mcp.project_context as project_context from basic_memory.config_models import ProjectEntry from basic_memory.mcp.project_context import ( + AddressableProject, ProjectPathRoute, ProjectPrefixConflictError, AmbiguousMountError, @@ -222,6 +223,36 @@ async def test_colliding_mount_permalinks_refuse_rather_than_pick(config_manager assert "my-docs" in str(excinfo.value) +@pytest.mark.asyncio +async def test_sibling_slash_bearing_project_conflicts_with_its_prefix( + config_manager, tmp_path_factory +): + """'docs' and 'team/docs' are two projects, not two spellings of one. + + The path names the 'team/docs' mount while the caller named 'docs', so this + is the ordinary prefix conflict — but the shape heuristic read the extra + leading segment as a workspace qualifier, made them agree, and discarded the + explicit selection to read the other project instead. + """ + config = config_manager.load_config() + config.projects["docs"] = ProjectEntry(path=str(tmp_path_factory.mktemp("docs-sibling"))) + config.projects["team/docs"] = ProjectEntry( + path=str(tmp_path_factory.mktemp("team-docs-sibling")) + ) + config_manager.save_config(config) + + with pytest.raises(ProjectPrefixConflictError) as excinfo: + await resolve_project_path_route("team/docs/note", project="docs", project_id=None) + + assert "team/docs" in str(excinfo.value) + assert "'docs' was passed" in str(excinfo.value) + + # Each project is still reachable by naming it and giving a relative path. + for name in ("docs", "team/docs"): + route = await resolve_project_path_route(f"{name}/note", project=name, project_id=None) + assert route == ProjectPathRoute(project=name, path="note", stripped=True) + + @pytest.mark.asyncio async def test_glob_first_segment_never_routes(multi_project_config): """split_project_prefix's '*' guard: a glob first segment is search input, @@ -308,22 +339,52 @@ def test_agreed_route_project_across_mixed_qualification(): the same project, in either direction, and the more-qualified one carries the route; different projects never agree. - Agreement is decided by segment count, not by looking for a slash: a - workspace slug is exactly one segment, so 'acme/Research/2026' qualifies the - project 'Research/2026' while it does not qualify a project named '2026'. - Asking "is this identifier workspace-qualified?" of one string is not - answerable at all once project names may contain '/'. + This is the fallback path, reached only when the explicit spelling names no + project this session addresses — so there is no identity to compare and the + shapes are all there is. Then segment count decides, not the mere presence + of a slash: a workspace slug is exactly one segment, so 'acme/Research/2026' + qualifies the project 'Research/2026' while it does not qualify a project + named '2026'. """ - assert _agreed_route_project("research", "other/research") == "other/research" - assert _agreed_route_project("other/research", "research") == "other/research" - assert _agreed_route_project("second-project", "other/research") is None + outside = () + + assert _agreed_route_project("research", "other/research", outside) == "other/research" + assert _agreed_route_project("other/research", "research", outside) == "other/research" + assert _agreed_route_project("second-project", "other/research", outside) is None # Slash-bearing project names: the escape hatch has to keep working. - assert _agreed_route_project("Research/2026", "acme/Research/2026") == "acme/Research/2026" + assert ( + _agreed_route_project("Research/2026", "acme/Research/2026", outside) + == "acme/Research/2026" + ) # ...without agreeing with a different project that merely shares a tail. - assert _agreed_route_project("2026", "acme/Research/2026") is None + assert _agreed_route_project("2026", "acme/Research/2026", outside) is None # A mount named after a workspace still conflicts with that workspace route. - assert _agreed_route_project("team", "team/docs") is None + assert _agreed_route_project("team", "team/docs", outside) is None + + +def test_agreed_route_project_prefers_identity_over_shape(): + """When the explicit spelling names a project this session addresses, the + answer comes from identity and the shapes are never consulted. + + 'team/docs' beside 'docs' are two real projects, not one project spelled two + ways. Reading the extra leading segment as a workspace qualifier made them + agree, so an explicit project='docs' was silently discarded and the call read + the other project instead of conflicting. + """ + addressable = ( + AddressableProject(name="docs", permalink="docs"), + AddressableProject(name="team/docs", permalink="team/docs"), + ) + + assert _agreed_route_project("team/docs", "docs", addressable) is None + assert _agreed_route_project("docs", "team/docs", addressable) is None + + # The same project under two spellings still agrees, by permalink equality. + assert _agreed_route_project("Team/Docs", "team/docs", addressable) == "Team/Docs" + + # With no such project addressable, the shape fallback still applies. + assert _agreed_route_project("team/docs", "docs", ()) == "team/docs" @pytest.mark.asyncio From 24024d9dfd70d0277ece19bf919f4598d05eed9b Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 22:08:31 -0500 Subject: [PATCH 18/18] fix(mcp): scope the duplicate-mount error to the paths that name it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ambiguity check from 9ba36c51 raised while building the lookup table, before knowing whether the requested path used the duplicated permalink. With 'My Docs' and 'my-docs' in a config, every non-empty path in the session failed — an unrelated 'other/note' among them — and so did cat("note", project="Other"), which is the exact-name escape hatch the error message itself recommends and which never goes through that lookup at all. A stale pair of entries somewhere in the config broke the whole session. An ambiguity should fail the calls that depend on it and no others, so the loop now records the colliding pair instead of rejecting the table, and the error fires only when the permalink a path actually claimed is one of them. It is not weakened: a path naming both projects still refuses rather than picking, and still names both so the caller can act. That makes the suggested escape hatch real, and the second half makes it correct. canonicalize_project_name matched by permalink alone, so a caller who named one colliding project exactly got whichever the config listed first — possibly the other project. It now takes an exact configured name before falling back to permalink matching, the same exact-before-fuzzy ordering the v2 project router and the workspace index already use. Naming either side exactly now reaches that side. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/mcp/project_context.py | 37 ++++++++++++------- .../mcp/project_context_identifiers.py | 9 +++++ tests/mcp/test_project_path_routing.py | 37 +++++++++++++++++++ 3 files changed, 69 insertions(+), 14 deletions(-) diff --git a/src/basic_memory/mcp/project_context.py b/src/basic_memory/mcp/project_context.py index 73d3d1192..fbad0a89e 100644 --- a/src/basic_memory/mcp/project_context.py +++ b/src/basic_memory/mcp/project_context.py @@ -1227,27 +1227,36 @@ def _claim_mount_prefix( lives in ``split_project_permalink_prefix``; this only maps the winning permalink back to the project that owns it. """ + # Two addressable projects can share a permalink ('My Docs' beside + # 'my-docs'). add_project refuses that now, so only a config written before + # the check, or edited by hand, still holds one. Record the pair rather than + # rejecting the table: an ambiguity is only a problem for the paths that + # actually resolve to it, and failing at build time failed every route in + # the session — including the exact-name escape hatch the error recommends, + # which does not go through this lookup at all. by_permalink: dict[str, AddressableProject] = {} + collisions: dict[str, AddressableProject] = {} for project in projects: - collision = by_permalink.setdefault(project.permalink, project) - # Trigger: two addressable projects share a permalink ('My Docs' beside - # 'my-docs'). add_project refuses this now, so only a config written - # before that check, or edited by hand, can reach here. - # Why: the permalink is the address. Silently keeping one would route - # every path under it to whichever project happened to sort last and - # read that project's content under the other's name. - # Outcome: refuse the whole route and name both, rather than pick. - if collision is not project: - raise AmbiguousMountError( - f"projects '{collision.name}' and '{project.name}' share the permalink " - f"'{project.permalink}', so that path names both. Rename one, or pass " - "project= with the exact name." - ) + first = by_permalink.setdefault(project.permalink, project) + if first is not project: + collisions.setdefault(project.permalink, project) claimed = _split_project_permalink_prefix(candidate, by_permalink) if claimed is None: return None permalink, remainder = claimed + + # This path names the duplicated permalink, so it genuinely names two + # projects. Refuse it — silently picking one would read that project's + # content under the other's name — and name both so the caller can act. + duplicate = collisions.get(permalink) + if duplicate is not None: + raise AmbiguousMountError( + f"projects '{by_permalink[permalink].name}' and '{duplicate.name}' share the " + f"permalink '{permalink}', so that path names both. Rename one, or pass " + "project= with the exact name." + ) + return by_permalink[permalink], remainder diff --git a/src/basic_memory/mcp/project_context_identifiers.py b/src/basic_memory/mcp/project_context_identifiers.py index 2c581e23c..24ed01bdc 100644 --- a/src/basic_memory/mcp/project_context_identifiers.py +++ b/src/basic_memory/mcp/project_context_identifiers.py @@ -50,6 +50,15 @@ def canonicalize_project_name( if project_name is None: return None + # Exact before fuzzy, the same ordering the v2 project router and the + # workspace index use. It matters when two configured names share a + # permalink: matching by permalink alone would answer a caller who named one + # of them exactly with whichever the config happened to list first — the + # other project — which is precisely the escape hatch the ambiguity error + # tells them to use. + if project_name in config.projects: + return project_name + requested_permalink = generate_permalink(project_name) for configured_name in config.projects: if generate_permalink(configured_name) == requested_permalink: diff --git a/tests/mcp/test_project_path_routing.py b/tests/mcp/test_project_path_routing.py index 351db8912..1f3d0411b 100644 --- a/tests/mcp/test_project_path_routing.py +++ b/tests/mcp/test_project_path_routing.py @@ -223,6 +223,43 @@ async def test_colliding_mount_permalinks_refuse_rather_than_pick(config_manager assert "my-docs" in str(excinfo.value) +@pytest.mark.asyncio +async def test_colliding_mounts_only_fail_the_paths_that_name_them( + config_manager, tmp_path_factory +): + """An ambiguity fails the calls that depend on it, and no others. + + Raising while building the lookup table rejected every non-empty path in the + session, so an unrelated project became unreachable and the exact-name + escape hatch the error itself recommends did not work — that call never goes + through the ambiguous lookup at all. + """ + config = config_manager.load_config() + config.projects["My Docs"] = ProjectEntry(path=str(tmp_path_factory.mktemp("dup-a"))) + config.projects["my-docs"] = ProjectEntry(path=str(tmp_path_factory.mktemp("dup-b"))) + config.projects["Other"] = ProjectEntry(path=str(tmp_path_factory.mktemp("dup-other"))) + config_manager.save_config(config) + + # A path claimed by an unrelated mount is unaffected. + unrelated = await resolve_project_path_route("other/note", project=None, project_id=None) + assert unrelated == ProjectPathRoute(project="Other", path="note", stripped=True) + + # So is the escape hatch, for the unrelated project... + hatch = await resolve_project_path_route("note", project="Other", project_id=None) + assert hatch == ProjectPathRoute(project="Other", path="note", stripped=False) + + # ...and for either side of the collision, which naming exactly must reach. + for name in ("My Docs", "my-docs"): + route = await resolve_project_path_route("note", project=name, project_id=None) + assert route == ProjectPathRoute(project=name, path="note", stripped=False) + + # The path that genuinely names both still refuses, naming both. + with pytest.raises(AmbiguousMountError) as excinfo: + await resolve_project_path_route("my-docs/note", project=None, project_id=None) + assert "My Docs" in str(excinfo.value) + assert "my-docs" in str(excinfo.value) + + @pytest.mark.asyncio async def test_sibling_slash_bearing_project_conflicts_with_its_prefix( config_manager, tmp_path_factory