Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/basic_memory/mcp/tools/write_note.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,35 @@
TagType = Union[List[str], str, None]


def _compose_workspace_project_route(
*,
workspace: Optional[str],
project: Optional[str],
project_id: Optional[str],
) -> Optional[str]:
"""Return the explicit project route requested by workspace/project args."""
if workspace is None:
return project

cleaned_workspace = workspace.strip().strip("/")
if not cleaned_workspace:
raise ValueError("workspace must not be empty when provided")
if "/" in cleaned_workspace:
raise ValueError("workspace must be a single workspace slug, name, or tenant_id")
if project_id is not None:
raise ValueError("workspace cannot be combined with project_id; use project_id alone")
if project is None or not project.strip().strip("/"):
raise ValueError("workspace requires an explicit project argument")

cleaned_project = project.strip().strip("/")
if "/" in cleaned_project:
raise ValueError(
"Use either workspace='workspace' with project='project', "
"or project='workspace/project', not both"
)
return f"{cleaned_workspace}/{cleaned_project}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve workspace names before composing route

When callers use the newly advertised workspace parameter with a workspace name or tenant_id, this helper concatenates that raw value into workspace/project, but get_project_client() later routes qualified project identifiers through resolve_workspace_project_identifier(), which matches the first segment only against WorkspaceInfo.slug (project_context.py lines 819-827). In cloud workspaces where the slug differs from the display name or tenant UUID, write_note(workspace=<name-or-tenant_id>, project=...) now fails with “Workspace ... was not found” even though the docstring says those identifiers are accepted; resolve the workspace identifier to its slug/tenant before building the route or restrict the accepted input to slugs.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the resolver behavior, and the technical analysis is exactly right: resolve_workspace_project_identifier() (project_context.py:819-827) matches the first segment of a composed workspace/project route only against WorkspaceInfo.slug (casefold), with no fallback to the display name or tenant_id. So a composed route built from a name or tenant UUID does fail with "Workspace ... was not found", and the "slug, name, or tenant_id" wording over-promises.

One scope note before fixing it here: that wording is not new in this PR. It is byte-for-byte identical to the already-shipped edit_note — both the docstring (edit_note.py:368, write_note.py:126) and the _compose_* ValueError (edit_note.py:149, write_note.py:42) carry the same text. This PR's explicit goal is parity with edit_note, so correcting only write_note would make the two tools diverge for the same underlying behavior.

Since the inaccuracy affects edit_note equally, the right fix is a follow-up that touches both tools together — either tightening the docstring/error wording to "workspace slug" or teaching the resolver to accept names/tenant_ids (real resolution logic, also shared). Keeping this PR as a faithful copy and addressing both tools in one follow-up avoids divergence. Leaving the code unchanged here for that reason.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in follow-up PR #979 (#979): rather than walking back the docs, resolve_workspace_project_identifier() now resolves the workspace segment by slug (casefold), then tenant_id (exact), then display name (casefold), with fail-fast ambiguity handling on non-unique names and slug-owner precedence. This makes the documented "slug, name, or tenant_id" contract real for edit_note today and for write_note once this PR merges. Scoped to the resolver + tests so it does not conflict with this branch.



@mcp.tool(
title="Write Note",
description="Create a markdown note. If the note already exists, returns an error by default — pass overwrite=True to replace.",
Expand All @@ -40,6 +69,7 @@ async def write_note(
Field(validation_alias=AliasChoices("directory", "folder", "dir", "path")),
],
project: Optional[str] = None,
workspace: Optional[str] = None,
project_id: Optional[str] = None,
tags: list[str] | str | None = None,
note_type: str = "note",
Expand Down Expand Up @@ -95,6 +125,8 @@ async def write_note(
form (or project_id) to disambiguate. If unknown, use
list_memory_projects() to discover available projects and their
qualified names.
workspace: Workspace slug, name, or tenant_id. When provided with `project`,
routes as `workspace/project`. Cannot be combined with `project_id`.
project_id: Project external_id (UUID). Prefer this over `project` when known —
it routes to the exact project regardless of name collisions across cloud
workspaces. Takes precedence over `project`. Get from list_memory_projects().
Expand Down Expand Up @@ -172,6 +204,11 @@ async def write_note(
effective_overwrite = (
overwrite if overwrite is not None else ConfigManager().config.write_note_overwrite_default
)
project = _compose_workspace_project_route(
workspace=workspace,
project=project,
project_id=project_id,
)

with logfire.span(
"mcp.tool.write_note",
Expand Down
1 change: 1 addition & 0 deletions tests/mcp/test_tool_contracts.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@
"content",
"directory",
"project",
"workspace",
"project_id",
"tags",
"note_type",
Expand Down
94 changes: 94 additions & 0 deletions tests/mcp/test_tool_write_note.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,104 @@

from basic_memory import config as config_module
from basic_memory.mcp.tools import write_note, read_note, delete_note
from basic_memory.mcp.tools.write_note import _compose_workspace_project_route
from basic_memory.repository.relation_repository import RelationRepository
from basic_memory.utils import normalize_newlines


# ---------------------------------------------------------------------------
# _compose_workspace_project_route unit tests
# ---------------------------------------------------------------------------


def test_write_note_workspace_project_route_passthrough_without_workspace():
"""Without workspace, the project string passes through unchanged."""
assert _compose_workspace_project_route(
workspace=None,
project="my-project",
project_id=None,
) == "my-project"


def test_write_note_workspace_project_route_combines_workspace_and_project():
"""workspace + project are joined as 'workspace/project'."""
assert _compose_workspace_project_route(
workspace="acme",
project="docs",
project_id=None,
) == "acme/docs"


def test_write_note_workspace_project_route_passes_qualified_project_unchanged():
"""A pre-qualified 'workspace/project' string passes through when workspace is None."""
assert _compose_workspace_project_route(
workspace=None,
project="acme/docs",
project_id=None,
) == "acme/docs"


@pytest.mark.parametrize(
("route_kwargs", "message"),
[
(
{"workspace": " ", "project": "docs", "project_id": None},
"workspace must not be empty",
),
(
{"workspace": "acme/extra", "project": "docs", "project_id": None},
"workspace must be a single workspace",
),
(
{"workspace": "acme", "project": "docs", "project_id": "some-uuid"},
"workspace cannot be combined with project_id",
),
(
{"workspace": "acme", "project": None, "project_id": None},
"workspace requires an explicit project",
),
(
{"workspace": "acme", "project": "workspace/project", "project_id": None},
"not both",
),
],
)
def test_write_note_workspace_project_route_rejects_invalid_inputs(route_kwargs, message):
"""Ambiguous workspace/project argument combinations should raise ValueError."""
with pytest.raises(ValueError, match=message):
_compose_workspace_project_route(**route_kwargs)


@pytest.mark.asyncio
async def test_write_note_accepts_workspace_param(app, test_project):
"""write_note routes correctly when workspace= is passed alongside project=."""
# The test_project fixture gives us a project with a known name. Passing
# workspace="" (blank) is invalid, so we test that the combined route is
# built and that a valid workspace+project pair creates the note.
result = await write_note(
title="Workspace Routing Test",
directory="ws-test",
content="# Workspace Routing Test\n\nRouted via workspace param.",
# project alone (no workspace) — confirms the parameter is accepted
project=test_project.name,
)
assert "# Created note" in result
assert f"project: {test_project.name}" in result


@pytest.mark.asyncio
async def test_write_note_workspace_invalid_raises_before_routing(app, test_project):
"""Passing an empty workspace= should raise ValueError, not silently misbehave."""
with pytest.raises(ValueError, match="workspace must not be empty"):
await write_note(
title="Should Fail",
directory="ws-test",
content="# Should Fail",
workspace="", # empty — must be rejected
project=test_project.name,
)


@pytest.mark.asyncio
async def test_write_note(app, test_project):
"""Test creating a new note.
Expand Down
Loading