fix(mcp): list factory projects across workspaces#778
Conversation
|
Claude finished @phernandez's task in 1m 44s —— View job PR Review: List factory MCP projects across workspaces
SummaryThis is a clean, well-targeted fix. The factory mode path was doing a one-off Code Quality & Standards Checklist
What Changed
The factory-mode block in Before: Called After: if is_factory_mode():
workspace_index = await ensure_workspace_project_index(context=context)
merged = _merge_workspace_projects(None, workspace_index.entries)
default_project = next(
(entry.project.name for entry in workspace_index.entries
if entry.workspace.is_default and entry.project.is_default),
None,
)This is the correct approach — reusing the already-correct multi-workspace index path rather than duplicating a broken subset of it. Fail-fast is the right call here. Factory mode runs in a cloud app context where there's no local fallback and no partial list is better than a complete one. Surfacing discovery errors to the caller (rather than silently returning an empty/incomplete list) matches the existing behavior of the non-factory path and gives callers actionable signal.
Testing
Three factory-mode tests are updated/added:
Minor Observations (Non-blocking)1. Unused-in-factory-path In from basic_memory.mcp.clients import ProjectClientThis deferred import runs before the 2. Pre-existing dead code in the non-factory At line 338, when merged = _merge_projects(local_list, cloud_list, cloud_workspace_name=cloud_ws_name, ...)
Verdict✅ LGTM. The change is minimal, correct, and well-tested. The fail-fast behavior in factory mode is the right design choice. No blocking issues. |
Signed-off-by: phernandez <paul@basicmachines.co>
4c9fc28 to
014cbd3
Compare
|
Claude finished @phernandez's task in 1m 52s —— View job PR Review: List factory MCP projects across workspaces
SummaryThis PR fixes a real bug: in factory/cloud mode, Code Quality & Standards
The comment header on the factory block was correctly updated to reflect the new intent. The Architecture & CorrectnessFactory path now (correct): workspace_index = await ensure_workspace_project_index(context=context)
merged = _merge_workspace_projects(None, workspace_index.entries)Factory path before (incorrect): async with get_client() as client:
project_list = await ProjectClient(client).list_projects()
# then a best-effort workspace lookup that could silently miss org workspacesThe new path calls the same
Testing & Documentation
The test helpers One gap: no test for factory mode + Security & Compatibility
VerdictApprove. The change is minimal, well-scoped, and correct. The factory path now mirrors the already-tested multi-workspace discovery path instead of reinventing a one-workspace subset. Tests are solid. The behavior change in error handling (fail-fast vs. silent degradation) is intentional and appropriate for factory mode. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c9fc28d9a
ℹ️ 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".
| workspace_index = await ensure_workspace_project_index(context=context) | ||
| merged = _merge_workspace_projects(None, workspace_index.entries) |
There was a problem hiding this comment.
Surface partial workspace discovery failures
Handle the failed_workspaces case before formatting results: ensure_workspace_project_index() can return a partial index when one or more workspace project fetches fail, and this factory-mode path currently renders workspace_index.entries as if discovery were complete. In that scenario, list_memory_projects silently omits projects from failed workspaces, so callers can miss external_ids and make incorrect routing decisions even though the tool appears successful.
Useful? React with 👍 / 👎.
Why
Cloud MCP factory mode currently lists projects by calling the default workspace client once. That hides projects in team/organization workspaces, so callers cannot discover the
external_idUUIDs they need for cross-workspace project routing.What Changed
list_memory_projectsfactory mode to use the existing workspace project index._merge_workspace_projects.default_projectonly when it belongs to the default workspace.Implementation Details
The factory path now mirrors the already-correct multi-workspace discovery path instead of doing a one-off
ProjectClient.list_projects()call and best-effort workspace metadata lookup. Workspace/project discovery failures now surface to the caller instead of silently returning an incomplete list.Testing
uv run pytest tests/mcp/test_tool_project_management.py -q-> 17 passeduv run ruff check src/basic_memory/mcp/tools/project_management.py tests/mcp/test_tool_project_management.py-> passeduv run ruff format --check src/basic_memory/mcp/tools/project_management.py tests/mcp/test_tool_project_management.py-> passedjust fast-check-> lint, format, and typecheck passed; testmon ran 2794 tests successfully but failed 14 Docker/testcontainers-backed semantic fixtures because the local Docker Unix socket was unavailable (FileNotFoundErrorwhile probing Docker/version).Risks / Follow-ups
The cloud repo still needs to pin
basic-memoryto this commit so issue basicmachines-co/basic-memory-cloud#684 is fixed in web-v2/cloud deployments.