fix(mcp): resolve projects by external_id, remove workspace from MCP tools#777
fix(mcp): resolve projects by external_id, remove workspace from MCP tools#777phernandez merged 5 commits intomainfrom
Conversation
|
Claude finished @phernandez's task in 3m 11s —— View job PR Review: Resolve projects by external_id, remove workspace from MCP toolsTasks:
SummaryThis is a well-scoped cleanup PR that removes Issues🔴 Missing test for the UUID fast-path — coverage gapThe most significant new feature — resolving projects by The test fixtures at lines 591/595 use non-UUID strings ( Given the 100% coverage requirement, this needs a test like: @pytest.mark.asyncio
async def test_resolve_workspace_project_identifier_by_external_id(monkeypatch):
"""Projects can be resolved unambiguously by external_id UUID."""
...
entries = (
WorkspaceProjectEntry(
workspace=personal,
project=_project("My Notes", id=1, external_id="11111111-1111-1111-1111-111111111111"),
),
)
...
resolved = await resolve_workspace_project_identifier("11111111-1111-1111-1111-111111111111")
assert resolved.project.external_id == "11111111-1111-1111-1111-111111111111"🟡 Silent collision in
|
| Item | Status |
|---|---|
| Follows coding conventions (CLAUDE.md) | ✅ Mostly — see comment-style note above |
| Python 3.12+ type annotations and async patterns | ✅ |
| SQLAlchemy 2.0 best practices | ✅ (no DB layer changes) |
| FastAPI and Typer conventions followed | ✅ |
| 100-character line length | ✅ |
| No commented-out code blocks | ✅ |
| Unit tests for new functions/methods | ❌ UUID fast-path is untested |
| Integration tests for new MCP tools | ✅ Contract tests updated correctly |
| Test coverage for edge cases | ✅ Default-workspace fallback is tested |
| 100% test coverage maintained | ❌ UUID fast-path not covered |
| Documentation/docstrings updated | ✅ get_project_client and resolve_workspace_project_identifier docstrings updated |
| CLAUDE.md updated if conventions change | N/A |
| MCP tools follow atomic, composable design | ✅ |
| Database changes include Alembic migrations | N/A |
| Preserves local-first architecture principles | ✅ |
| Knowledge graph operations maintain consistency | ✅ |
| No hardcoded secrets or credentials | ✅ |
| Input validation for MCP tools | ✅ UUID parsing via uuid.UUID() is clean |
| Proper error handling and logging | ✅ |
| No sensitive data in logs | ✅ |
| Windows-compatible file path comparisons | N/A |
| No emojis/unicode in console/log output | ✅ |
Positive Notes
- Excellent surface-area reduction: Removing
workspacefrom 15 tool signatures is a clean, breaking-free simplification — LLMs no longer need to know about the workspace concept. - UUID fast-path approach: Using
try: UUID(project)is clean and doesn't require format-prefix heuristics. - Default-workspace fallback: Preferring the default workspace over raising
ValueErroron ambiguous names is a better LLM UX. external_idinlist_memory_projectsoutput: Smart — LLMs can now discover and use stable project identifiers in subsequent calls._fetch_cloud_projectsremoval: Consolidating to the workspace-index approach removes a separate code path, reducing complexity.
The main ask before merging is a test covering the UUID resolution fast-path.
Workspaces are an implementation detail that should not be exposed to MCP tool callers. This change: - 🔑 Adds external_id (UUID) lookup to project resolution so callers can reference projects unambiguously without knowing which workspace owns them - 🗑️ Removes the `workspace` parameter from all 15 MCP tool signatures - 🔄 Falls back to the default workspace when a project name exists in multiple workspaces (instead of erroring) - 📋 Includes external_id in list_memory_projects output so LLMs can discover and use project UUIDs Closes basicmachines-co/basic-memory-cloud#673 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
phernandez
force-pushed
the
project-id-resolution
branch
from
17b4142 to
4915e13
Compare
|
Claude finished @phernandez's task in 2m 21s —— View job PR Review: Resolve projects by external_id, remove workspace from MCP toolsTasks:
SummaryThis is a well-scoped PR with a clear goal: let LLMs reference projects by UUID ( Issues🔴 Missing test: UUID fast path is never exercised
# project_context.py:561-567
try:
UUID(project)
entry = index.entries_by_external_id.get(project)
if entry:
return entry
except ValueError:
passHowever, no test passes a real UUID string to this function. Every test fixture uses non-UUID To hit 100% coverage (as required by CLAUDE.md) and validate the feature works, a test like this is needed: @pytest.mark.asyncio
async def test_resolve_workspace_project_identifier_by_external_id(monkeypatch):
import basic_memory.mcp.project_context as project_context
from basic_memory.mcp.project_context import (
WorkspaceProjectEntry,
_build_workspace_project_index,
resolve_workspace_project_identifier,
)
VALID_UUID = "11111111-1111-1111-1111-111111111111"
personal = _workspace(tenant_id="personal-tenant", ..., is_default=True)
entries = (
WorkspaceProjectEntry(
workspace=personal,
project=_project("Notes", id=1, external_id=VALID_UUID),
),
)
index = _build_workspace_project_index((personal,), entries)
async def fake_index(context=None): return index
monkeypatch.setattr(project_context, "_ensure_workspace_project_index", fake_index)
resolved = await resolve_workspace_project_identifier(VALID_UUID)
assert resolved.project.external_id == VALID_UUID🟡 Mutable
|
phernandez
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4915e1373e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| UUID(project) | ||
| entry = index.entries_by_external_id.get(project) | ||
| if entry: |
There was a problem hiding this comment.
The new external-id fast path validates project with UUID(project) but then looks up entries_by_external_id using the original string, so valid UUID spellings like uppercase hex, brace-wrapped values, or urn:uuid: forms are accepted as UUIDs but fail lookup and fall through to name/permalink resolution. In those cases a real project external ID can incorrectly return “not found” (or resolve ambiguously by name) even though the UUID is valid. Canonicalizing once (e.g., str(UUID(project))) before dictionary lookup would make UUID handling robust.
Useful? React with 👍 / 👎.
The CLI tool commands still passed workspace= to MCP tool functions after the workspace parameter was removed from all MCP tool signatures. Also removes the workspace passthrough test and applies ruff formatting. Signed-off-by: phernandez <paul@basicmemory.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 2m 8s —— View job PR Review: Resolve projects by external_id, remove workspace from MCP toolsTasks:
SummaryThis PR removes The workspace removal is clean and correct. Two issues remain unaddressed from the prior reviews. Issues🔴 UUID fast-path still has no test — 100% coverage requirement unmet
try:
UUID(project)
entry = index.entries_by_external_id.get(project)
if entry:
return entry
except ValueError:
passEvery test that exercises The A test like this is needed: @pytest.mark.asyncio
async def test_resolve_workspace_project_identifier_by_external_id(monkeypatch):
import basic_memory.mcp.project_context as project_context
from basic_memory.mcp.project_context import (
WorkspaceProjectEntry,
_build_workspace_project_index,
resolve_workspace_project_identifier,
)
VALID_UUID = "11111111-1111-1111-1111-111111111111"
personal = _workspace(tenant_id="personal-tenant", workspace_type="personal",
name="Personal", role="owner", is_default=True)
entries = (
WorkspaceProjectEntry(
workspace=personal,
project=_project("My Notes", id=1, external_id=VALID_UUID),
),
)
index = _build_workspace_project_index((personal,), entries)
async def fake_index(context=None):
return index
monkeypatch.setattr(project_context, "_ensure_workspace_project_index", fake_index)
resolved = await resolve_workspace_project_identifier(VALID_UUID)
assert resolved.project.external_id == VALID_UUID🟡 UUID not canonicalized before dict lookup — Codex P2 not addressed
Fix is one line: # before (line 563)
entry = index.entries_by_external_id.get(project)
# after — normalize to canonical lowercase-hyphenated form
entry = index.entries_by_external_id.get(str(UUID(project)))Note: this requires moving try:
canonical = str(UUID(project))
entry = index.entries_by_external_id.get(canonical)
if entry:
return entry
except ValueError:
pass🟡
|
| Item | Status |
|---|---|
| Follows coding conventions (CLAUDE.md) | ✅ |
| Python 3.12+ type annotations and async patterns | ✅ |
| SQLAlchemy 2.0 best practices | ✅ (no DB changes) |
| FastAPI and Typer conventions followed | ✅ |
| 100-character line length | ✅ |
| No commented-out code blocks | ✅ |
| Unit tests for new functions/methods | ❌ UUID fast-path branch is untested |
| Integration tests for new MCP tools | ✅ Contract tests updated |
| Test coverage for edge cases | ✅ Default-workspace fallback tested |
| 100% test coverage maintained | ❌ UUID fast-path if entry: return entry not hit by any test |
| Documentation/docstrings updated | ✅ |
| CLAUDE.md updated if conventions change | N/A |
| MCP tools follow atomic, composable design | ✅ |
| Database changes include Alembic migrations | N/A |
| Preserves local-first architecture principles | ✅ |
| Knowledge graph operations maintain consistency | ✅ |
| No hardcoded secrets or credentials | ✅ |
| Input validation for MCP tools | ✅ UUID parsing in try/except is clean |
| Proper error handling and logging | ✅ |
| No sensitive data in logs | ✅ |
| Windows-compatible file path comparisons | N/A |
| No emojis/unicode in console/log output | ✅ (unicode in MCP result strings was pre-existing) |
What the second commit fixes
4e8a62b correctly removes the remaining workspace= argument passthrough in CLI tool.py commands that survived the first commit — those would have caused TypeError at runtime since the MCP tool signatures no longer accept workspace. Also applies ruff formatting and removes the now-invalid test_get_project_client_rejects_workspace_for_local_project test. This is a necessary cleanup.
Positive Notes
- Removing
workspacefrom all 15 MCP tool signatures is the right long-term direction — workspaces are an internal routing detail. - The
try: UUID(project) / except ValueError: passpattern is idiomatic for UUID detection. - Default-workspace fallback on ambiguous names is a significant UX improvement over the previous error.
external_idinlist_memory_projectsoutput makes UUID routing discoverable in one call._fetch_cloud_projectsremoval consolidates the code path correctly.
Main ask before merging: add the UUID fast-path test and canonicalize the UUID before the dict lookup (one-liner fix).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e8a62be5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| response = await search_notes( | ||
| project=active_project.name, | ||
| workspace=workspace, | ||
| query=identifier_text, |
There was a problem hiding this comment.
When read_note falls back to _search_candidates, it now calls search_notes with only project=active_project.name. In CLI/direct invocations (context=None), this loses the tenant selection from the original get_project_client resolution; if the same project name exists in multiple workspaces, search_notes re-resolves the name and can select the default workspace (see resolve_workspace_project_identifier ambiguity handling), returning content from the wrong workspace. This is especially reproducible for title-based reads that hit the fallback path after direct entity resolution misses.
Useful? React with 👍 / 👎.
Adds optional `project_id: Optional[str] = None` (UUID) parameter to all 14
project-scoped MCP tools so callers can address projects unambiguously across
cloud workspaces without depending on name resolution.
What changed:
- Canonicalize UUID input via str(UUID(...)) in resolve_workspace_project_identifier
so uppercase, brace-wrapped, urn:uuid, and unhyphenated forms all match the
stored external_id (Codex P2)
- Add project_id parameter to: read_note, write_note, search_notes, edit_note,
delete_note, view_note, read_content, move_note, list_directory, canvas,
recent_activity, build_context, schema_validate/infer/diff, search_notes_ui,
read_note_ui. project_id takes precedence over project when both are passed.
- Update get_project_client to route UUID identifiers correctly in pure local
mode. Without this, get_project_mode(uuid) defaults to CLOUD and fails for
users without cloud credentials. Fix gates the cloud-by-default branch on
cloud actually being available, and skips per-project routing for UUIDs in
Step 4 (local mode routes everything via the same ASGI client; the API
resolves the UUID via /v2/projects/resolve)
- Wire read_note._search_candidates to pass project_id=active_project.external_id
to the inner search_notes call so workspace selection from the outer
get_project_client is preserved across re-resolution. Without this, names
that collide across workspaces could re-resolve to a different tenant via
the default-workspace fallback in CLI/context=None paths (Codex P1)
- Switch resolve_project_and_path callers (read_note, read_content, search)
to pass active_project.name instead of the raw project arg so cache hits
even when project_id was used or project name was wrong/ambiguous
Documentation:
- AI assistant guide (live MCP resource) explains project vs project_id and
when to prefer the UUID
- Extended guide adds a dedicated `project` vs `project_id` section
- list_memory_projects docstring points LLMs at external_id for unambiguous
addressing
- Per-tool project_id docstring is directive ("Prefer this over `project`
when known") rather than conditional ("Use this when...")
Tests:
- 6 new unit tests in test_project_context.py covering UUID lookup, four
normalization variants, project_id precedence, and pure-local UUID routing
- 2 new integration tests verify project_id end-to-end through MCP transport
- Tool signature contract test and telemetry tests updated for new parameter
Closes basicmachines-co/basic-memory-cloud#673
Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 2m 12s —— View job PR Review: fix(mcp): resolve projects by external_id, remove workspace from MCP toolsTasks:
SummaryThe latest commit ( All prior 🔴 issues resolved ✅UUID canonicalization — canonical_external_id = str(UUID(project))
entry = index.entries_by_external_id.get(canonical_external_id)Uppercase, brace-wrapped ( UUID fast-path tests — all four added to
response = await search_notes(
project=active_project.name,
project_id=active_project.external_id, # preserves outer workspace selection
...
)The inner
Remaining issue🟡
|
| Item | Status |
|---|---|
| Follows Basic Memory's coding conventions (CLAUDE.md) | ✅ Literate style, decision-point comments present |
| Python 3.12+ type annotations and async patterns | ✅ |
| SQLAlchemy 2.0 best practices | ✅ (no DB changes) |
| FastAPI and Typer conventions followed | ✅ |
| 100-character line length | ✅ |
| No commented-out code blocks | ✅ |
| Unit tests for new functions/methods | ✅ UUID fast-path fully covered |
| Integration tests for new MCP tools | ✅ test_read_note_by_project_id + test_read_note_project_id_takes_precedence_over_name in test-int/ |
| Test coverage for edge cases | ✅ UUID normalization variants, local-mode regression, precedence |
| 100% test coverage maintained | ✅ UUID fast-path if entry: return entry branch is now exercised |
| Documentation/docstrings updated | ✅ get_project_client, resolve_workspace_project_identifier, all 14 tool docstrings updated |
| CLAUDE.md updated if conventions change | N/A |
| MCP tools follow atomic, composable design | ✅ |
| Database changes include Alembic migrations | N/A |
| Preserves local-first architecture principles | ✅ |
| Knowledge graph operations maintain consistency | ✅ |
| No hardcoded secrets or credentials | ✅ |
| Input validation for MCP tools | ✅ UUID() in try/except |
| Proper error handling and logging | ✅ |
| No sensitive data in logs | ✅ |
| Windows-compatible file path comparisons | N/A |
| No emojis/unicode in console/log output | ✅ |
Positive highlights
- All prior 🔴 blockers addressed in this commit — the PR is now correct end-to-end.
- The
str(UUID(project))canonicalization one-liner is the minimal correct fix for the case-variant lookup gap. test_resolve_workspace_project_identifier_normalizes_uuid_formsiterates over all four alternative UUID representations in a single parametrized loop — clean test design.- The
project_id=active_project.external_idfix in_search_candidatescorrectly solves the workspace-pinning problem without requiring a separateproject_idfield propagation chain through every fallback call. view_note → read_note → search_noteschain now propagatesproject_idall the way through.
The create_memory_project external_id gap is the only remaining item and it's cosmetic — happy to merge as-is or include it in a follow-up.
bm-clawd
left a comment
bm-clawd
left a comment
There was a problem hiding this comment.
Reviewed against the goals stated in the PR description. LGTM, ship once integration CI is green. This is the right shape for the workspace-ambiguity problem.
What I checked
1. resolve_workspace_project_identifier UUID fast-path — str(UUID(project)) canonicalizes uppercase, brace-wrapped, urn:uuid:, and unhyphenated forms to the same key as the stored external_id. The entries_by_external_id dict is built once when the index is constructed, so UUID lookups are O(1) and bypass name/permalink resolution entirely. ✅
2. project_id parameter on all 14 project-scoped tools — verified every tool listed in the PR description actually has project_id: Optional[str] = None and threads it into get_project_client(...). Delegating tools (view_note → read_note, ui_sdk → search_notes/read_note) propagate it through. ✅
3. The get_project_client UUID-in-local-mode fix is the load-bearing change. config.get_project_mode(unknown_identifier) defaults to CLOUD because the local config keys by project name. A UUID will always be unknown locally, so without this PR's guard a project_id argument would have falsely routed every local user through the cloud client and errored without credentials. The fix is in two places:
- Step 3 only enters the cloud routing path when cloud is actually available (
factory_mode or explicit_cloud_routing or has_cloud_credentials(config)). - Step 4 builds
client_kwargs = {} if project_id else {"project_name": resolved_project}so the local ASGI client doesn't try to consult per-project mode for a UUID it can't recognize.
The regression testtest_get_project_client_with_project_id_routes_locally_without_cloudpins this exact failure mode. ✅
4. Internal tool→tool routing preservation — read_note._search_candidates passes project_id=active_project.external_id into search_notes. Without that, the inner get_project_client(...) call would re-resolve from a bare name and could land in a different tenant via the default-workspace fallback when context is unavailable (CLI, context=None). This was Codex P1 and the resolution is more thorough than the original suggestion — name-based routing still works for local single-workspace users because the inner call also receives project=active_project.name. ✅
5. Removal of the workspace MCP parameter — list_memory_projects no longer takes workspace, but the merged output now includes external_id for every project. The docstring tells the LLM where to find UUIDs and how to use them. The flow is closed end-to-end: discover via list_memory_projects() → pick UUID → pass as project_id → routed to the exact project regardless of name collisions. ✅
6. Backwards compat — pure local single-workspace users keep working with no changes (still pass project="name", no UUID needed, name-keyed local config still resolves). ✅
Minor things I'd flag (non-blocking)
-
_search_candidatespasses bothproject=active_project.nameandproject_id=active_project.external_id. Sinceproject_idtakes precedence per the contract, the name is technically redundant. The comment in the code explains that this preserves workspace selection across re-resolution, but it would be slightly cleaner to either drop the name (let UUID alone disambiguate) or add a one-line comment saying "name is defensive — kept for log clarity and any code paths that readprojectdirectly." Cosmetic. -
get_project_clientStep 1 callsresolve_project_parameter(project_identifier=<uuid>, context=context). That passes the UUID throughProjectResolverand_canonicalize_project_name, which loops overconfig.projectsdoing permalink matches that will never hit. Cheap, but on the order of tens of microseconds × 14 tools × hot paths. Consider an earlytry: UUID(project_identifier)branch that skips the resolver entirely when the input is already a canonical external_id — same shape as the fast-path you added inresolve_workspace_project_identifier. Not blocking. -
detect_project_from_url_prefixis unaware ofproject_id. If a caller passesproject=None, project_id=<uuid>, identifier="memory://specs/foo", the URL-prefix detector still runs and may setproject="specs"from the prefix.project_idthen takes precedence inget_project_clientso routing is correct, but the resolved active project might end up disagreeing with what the user expected the URL-prefix to mean. Worth a comment inread_notenoting thatproject_idoverrides URL prefix detection by design, or short-circuit the prefix detection whenproject_idis set. Edge case but worth a follow-up. -
Validation symmetry:
resolve_workspace_project_identifierraises a clear "Project '...' was not found in indexed cloud workspaces" error with the available list. But if a UUID is passed and doesn't match anyexternal_id, the fast-path falls through to_split_qualified_project_identifierand then a permalink lookup that will also fail. The eventual error is "Project '' was not found in indexed cloud workspaces" with a list of names — not great UX for a UUID typo. A targeted "no project with that external_id" error would help, but it's an edge case. -
Telemetry:
requested_project_idis added to spans. Worth confirming this is accepted by the existing telemetry filters / not flagged as PII (it's not —external_idis an opaque UUID, not the cloud user's tenant_id). Sanity check, not a change request.
Risk assessment
- Local single-workspace users: zero behavior change. Name-based contract preserved.
- Cloud single-workspace users: zero behavior change unless they opt into UUIDs. The default-workspace fallback for ambiguous names replaces the old "exists in multiple workspaces" error, which is less surprising for the common case.
- Cloud multi-workspace users with name collisions: previously errored, now resolvable via
project_id. Strictly better. - The internal CLI/context=None re-resolution path: previously could pick the wrong tenant on default-workspace fallback. Fixed.
- Failure mode: passing an unknown UUID errors clearly. Passing a local-mode UUID without cloud routes locally and the API resolves it via
/v2/projects/resolve. No silent wrong-tenant routes that I can construct.
Tying back to PR #666
Worth noting: PR #666 fixed the cloud-side data loss when move_note raced its own webhooks. PR #777 closes the upstream ambiguity that causes MCP callers to land in the wrong tenant in the first place. Together they make multi-workspace MCP routing correct end-to-end — caller picks an unambiguous identifier (#777), tenant performs the move without losing data on stale webhooks (#666). Good shipping order.
Approving once the test matrix lands green. Nice work on isolating the UUID-in-local-mode trap before it shipped — that one would have been a confusing customer report.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25be29eef0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cloud_available = factory_mode or explicit_cloud_routing or has_cloud_credentials(config) | ||
| if project_id and not cloud_available: | ||
| project_mode = ProjectMode.LOCAL |
There was a problem hiding this comment.
When project_id is provided and cloud credentials exist, this branch leaves project_mode as CLOUD for unknown identifiers (get_project_mode(uuid) defaults to cloud), so local projects addressed by external UUID are misrouted through cloud resolution. That breaks the newly added project_id path for local projects and can also break read_note fallbacks now that _search_candidates forwards active_project.external_id; with a logged-in user, title/text fallback can hit cloud instead of the already-resolved local project and fail or return wrong-tenant data.
Useful? React with 👍 / 👎.
Previously the JSON and text responses from create_memory_project omitted external_id (UUID), forcing callers that just created a project to make a second list_memory_projects() round-trip before they could use UUID routing via the new project_id parameter. Both code paths now include it: - already-exists branch: returns existing_match.external_id - new-project branch: returns status_response.new_project.external_id - text output: adds "External ID:" line to Project Details Tests cover: - already-exists JSON path returns valid external_id string - create-new JSON path returns valid external_id string - text output includes "External ID:" line Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 1m 36s —— View job PR Review: fix(mcp): resolve projects by external_id, remove workspace from MCP toolsTasks:
SummaryThe latest commit ( Latest commit:
|
| Issue | Status |
|---|---|
🔴 UUID fast-path (try: UUID(project)) has no test |
✅ Resolved in commit 25be29e — 4 dedicated tests added |
| 🔴 UUID not canonicalized before dict lookup | ✅ Resolved — canonical_external_id = str(UUID(project)) normalizes uppercase/brace/urn forms |
🔴 _search_candidates loses workspace selection on re-resolution |
✅ Resolved — project_id=active_project.external_id passed to inner search_notes |
🟡 create_memory_project JSON omits external_id |
✅ Resolved in commit 6c7d7d5 — both code paths now include it |
Checklist
| Item | Status |
|---|---|
| Follows Basic Memory's coding conventions (CLAUDE.md) | ✅ Decision-point comments, section headers, literate style |
| Python 3.12+ type annotations and async patterns | ✅ |
| SQLAlchemy 2.0 best practices | ✅ (no DB layer changes) |
| FastAPI and Typer conventions followed | ✅ |
| 100-character line length | ✅ |
| No commented-out code blocks | ✅ |
| Unit tests for new functions/methods | ✅ UUID fast-path fully covered (4 tests) |
| Integration tests for new MCP tools | ✅ test_read_note_by_project_id + test_read_note_project_id_takes_precedence_over_name in test-int/ |
| Test coverage for edge cases | ✅ UUID normalization variants, local-mode regression, precedence, already-exists + new-project create paths |
| 100% test coverage maintained | ✅ |
| Documentation/docstrings updated | ✅ All 14 tool docstrings, get_project_client, resolve_workspace_project_identifier, assistant guide |
| CLAUDE.md updated if conventions change | N/A |
| MCP tools follow atomic, composable design | ✅ |
| Database changes include Alembic migrations | N/A |
| Preserves local-first architecture principles | ✅ Pure-local users: zero behavior change |
| Knowledge graph operations maintain consistency | ✅ |
| No hardcoded secrets or credentials | ✅ |
| Input validation for MCP tools | ✅ UUID() in try/except |
| Proper error handling and logging | ✅ |
| No sensitive data in logs | ✅ |
| Windows-compatible file path comparisons | N/A |
| No emojis/unicode in console/log output | ✅ |
Positive highlights
- Surface-area reduction is clean: removing
workspacefrom all 15 MCP tool signatures is the right long-term direction. - UUID fast-path is minimal and idiomatic:
try: str(UUID(project)) / except ValueError: passavoids regex and handles all variant forms. create_memory_projectnow gives LLMs a complete picture: UUID is immediately available after create — no second round-trip needed.- Backward compatible: callers still passing only
project(name string) keep working unchanged. - Local-mode regression is pinned:
test_get_project_client_with_project_id_routes_locally_without_cloudprevents the false-cloud-routing bug from regressing silently.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c7d7d5e2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| content=content, | ||
| directory=folder, | ||
| project=project, |
There was a problem hiding this comment.
Removing --workspace from bm tool without adding a project_id alternative leaves these commands unable to disambiguate same-named projects across cloud workspaces. In multi-workspace accounts, project name resolution now falls back to the default workspace, so write-note (and the other wrappers following this pattern) can read/write the wrong tenant with no way for the CLI caller to force the intended project. Expose --project-id and forward it to the MCP tool call.
Useful? React with 👍 / 👎.
Removing --workspace without exposing a --project-id alternative left `bm tool` commands unable to disambiguate same-named projects across cloud workspaces. In multi-workspace accounts, name resolution falls back to the default workspace, so write-note (and the other wrappers) could read/write the wrong tenant with no override available to the CLI caller. Adds --project-id (UUID) to all 9 project-scoped CLI tool commands and forwards it to the underlying MCP tool's project_id parameter: - write-note, read-note, edit-note - build-context, recent-activity, search-notes - schema-validate, schema-infer, schema-diff Test coverage: - New test_write_note_project_id_passthrough verifies --project-id reaches the MCP tool's project_id kwarg Closes the P1 finding from Codex review. Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 3m 18s —— View job PR Review: fix(mcp): resolve projects by external_id, remove workspace from MCP toolsTasks:
SummaryAll 🔴 blockers from prior review rounds are resolved. The latest commit ( Latest commit (
|
| Issue | Status |
|---|---|
| UUID fast-path has no test | ✅ Four dedicated tests added (test_resolve_workspace_project_identifier_resolves_by_external_id, test_resolve_workspace_project_identifier_normalizes_uuid_forms, test_get_project_client_with_project_id_routes_locally_without_cloud, test_get_project_client_prefers_project_id_over_project_name) |
| UUID not canonicalized before dict lookup | ✅ canonical_external_id = str(UUID(project)) normalizes uppercase/brace/urn/unhyphenated forms |
_search_candidates loses workspace selection on re-resolution |
✅ project_id=active_project.external_id passed through to inner search_notes |
create_memory_project JSON omits external_id |
✅ Both already-exists and new-project paths now return external_id; text output also shows • External ID: |
CLI commands had no UUID disambiguator after --workspace was removed |
✅ --project-id added to all 8 bm tool subcommands in this commit |
Remaining minor observations (non-blocking)
🟡 Edge case: local project + cloud credentials + project_id UUID
When a user has cloud credentials AND a local-only project, passing project_id=<local_uuid> routes through the cloud branch (because cloud_available = True and the UUID is unknown to the local name-keyed config). The cloud workspace index won't contain a local-only project's UUID, so the call fails. The error message ("Project not found in indexed cloud workspaces") could be confusing for this scenario.
This is an obscure edge case — the typical project_id use case is cross-workspace cloud disambiguation. Local users with no cloud credentials are correctly handled by test_get_project_client_with_project_id_routes_locally_without_cloud. For local projects, project=<name> remains the correct call pattern and continues to work unchanged.
🟡 entries_by_external_id uses silent last-writer-wins on UUID collision
_build_workspace_project_index line 361: by_external_id[entry.project.external_id] = entry overwrites silently on collision. UUIDs should be globally unique, but a logger.warning on duplicate would surface data-integrity bugs visibly rather than silently (consistent with how ambiguous permalink collisions are handled via setdefault). Minor — not a correctness issue.
🟡 Trailing blank lines in test_tool_telemetry.py
Five test dicts have a trailing blank line where "workspace_id": None, was removed (e.g., lines 66, 111, 159, 212, 264 in the diff). Syntactically fine, cosmetically inconsistent. This was noted in the first review round and remains unaddressed.
Checklist
| Item | Status |
|---|---|
| Follows Basic Memory's coding conventions (CLAUDE.md) | ✅ Literate style, decision-point comments (Trigger/Why/Outcome), section headers |
| Python 3.12+ type annotations and async patterns | ✅ |
| SQLAlchemy 2.0 best practices | ✅ (no DB changes) |
| FastAPI and Typer conventions followed | ✅ |
| 100-character line length | ✅ |
| No commented-out code blocks | ✅ |
| Unit tests for new functions/methods | ✅ UUID fast-path fully covered (4 tests); CLI passthrough tested |
| Integration tests for new MCP tools | ✅ test_read_note_by_project_id + test_read_note_project_id_takes_precedence_over_name in test-int/ |
| Test coverage for edge cases | ✅ UUID normalization variants, local-mode regression, precedence, create paths |
| 100% test coverage maintained | ✅ |
| Documentation/docstrings updated | ✅ All 14 tool docstrings, get_project_client, resolve_workspace_project_identifier, assistant guide, list_memory_projects docstring points LLMs to external_id |
| CLAUDE.md updated if conventions change | N/A |
| MCP tools follow atomic, composable design | ✅ |
| Database changes include Alembic migrations | N/A (no schema changes) |
| Preserves local-first architecture principles | ✅ Pure-local users: zero behavior change |
| Knowledge graph operations maintain consistency | ✅ |
| No hardcoded secrets or credentials | ✅ |
| Input validation for MCP tools | ✅ UUID() in try/except |
| Proper error handling and logging | ✅ |
| No sensitive data in logs | ✅ (requested_project_id is an opaque UUID, not PII) |
| Windows-compatible file path comparisons | N/A |
| No emojis/unicode in console/log output | ✅ |
Positive highlights
- Complete workspace abstraction: removing
workspacefrom all 15 MCP tool signatures AND all 8 CLI tool subcommands is the right long-term direction — workspaces are internal routing detail. - End-to-end UUID routing:
list_memory_projects()shows UUIDs → caller picks one → passes asproject_id→ fast-path lookup resolves unambiguously → workspace-correct API call. The loop is fully closed. - Local-mode regression pinned:
test_get_project_client_with_project_id_routes_locally_without_cloudprevents the false-cloud-routing bug from regressing silently. - Backward compatible: callers still using only
project=<name>keep working unchanged. _fetch_cloud_projectsremoval: consolidates the listing code path, reducing complexity.
3 participants
Summary
Lets MCP tools address projects unambiguously across cloud workspaces, without requiring callers to know about workspaces. Removes the
workspaceparameter from MCP tools entirely and adds an optionalproject_id(UUID) parameter to every project-scoped tool.Closes basicmachines-co/basic-memory-cloud#673
What changed
1. Project resolution by
external_id(UUID)resolve_workspace_project_identifier()accepts a UUID and resolves it via a fast-path lookup against the workspace/project index — bypassing name/permalink resolution and workspace ambiguity handling.str(UUID(value))before lookup, so uppercase hex, brace-wrapped ({...}),urn:uuid:..., and unhyphenated forms all map to the same key. (Codex P2)2.
workspaceparameter removed from MCP toolsWorkspaces are now purely an implementation detail. When a project name exists in multiple workspaces and no UUID disambiguator is given, resolution falls back to the default workspace (instead of erroring with "exists in multiple workspaces").
3. Optional
project_idparameter on all 14 project-scoped MCP toolsread_note,write_note,search_notes,edit_note,delete_note,view_note,read_content,move_note,list_directory,canvas,recent_activity,build_context,schema_validate/infer/diff,search_notes_ui,read_note_ui.project_idtakes precedence overprojectand routes to a specific project unambiguously.list_memory_projects(), which now includesexternal_idin its output (and the docstring points there).project(the existing pattern) keep working unchanged.4.
get_project_client()routes UUIDs correctly in both modesThe local config keys projects by name only, so
get_project_mode(uuid)would have falsely defaulted to CLOUD and broken pure-local routing for any UUID identifier. Fixed by:project_idis set, skip per-project mode lookup inget_client()— local mode routes everything through the same ASGI client; the API resolves the UUID via/v2/projects/resolve.5. Internal tool→tool calls preserve workspace selection
read_note._search_candidates(the title/text fallback path) now passesproject_id=active_project.external_idtosearch_notes. Without this, the inner re-resolution could select a different tenant via the default-workspace fallback when context was unavailable (CLI /context=None). (Codex P1, solved more thoroughly than the original suggestion — preserving routing without breaking local mode.)Tools that delegate (
view_note→read_note,ui_sdk→search_notes/read_note) propagateproject_idthrough.Tools calling
resolve_project_and_path()(read_note,read_content,search_notes) now passactive_project.name(the resolved canonical name) instead of the rawprojectargument, so the innerget_active_project()cache hits even whenproject_idwas used orprojectwas wrong.Why this approach
The earlier draft had
read_note._search_candidatesswapproject=nameforproject=external_id. That broke pure-local mode because the local config indexes by name only — passing a UUID hit the CLOUD-by-default branch inget_project_mode()and errored without cloud credentials.Adding a separate
project_idparameter:list_memory_projects().Test plan
project_idend-to-end through MCP transport passrequested_project_idspan attributeNew tests
tests/mcp/test_project_context.py:test_resolve_workspace_project_identifier_resolves_by_external_id— direct UUID lookup picks the right workspacetest_resolve_workspace_project_identifier_normalizes_uuid_forms— uppercase/brace/urn/unhyphenated forms canonicalizetest_get_project_client_prefers_project_id_over_project_name— UUID precedencetest_get_project_client_with_project_id_routes_locally_without_cloud— regression for the local-mode UUID routing fixtest-int/mcp/test_read_note_integration.py:test_read_note_by_project_id— end-to-end read using onlyproject_idtest_read_note_project_id_takes_precedence_over_name— wrong name + correct UUID still resolves correctly🤖 Generated with Claude Code