fix(cli): address mount PR review feedback

From the Codex and claude-review automated reviews on PR #920:

- Route the cloud project lookup through the resolved workspace (Codex P2):
  _get_cloud_project now takes workspace_id and passes it to get_client, so the
  project metadata is read from the same tenant the bucket/remote target. Without
  this, --workspace (or a non-default config) could read a different tenant's
  project and transfer against the wrong bucket.
- Validate workspace slugs before splicing into a remote name (claude-review,
  security): remote_name_for_workspace rejects slugs with characters invalid in
  an rclone remote section, failing fast instead of writing a broken rclone.conf.
- Fix the now-stale rclone_commands module docstring (single-remote bullet).
- Clarify that generate_mount_credentials' tenant_id is the X-Workspace-ID
  routing key.
- Note (TODO #919) why sync/bisync/check still use default-tenant mount info.
- Tests: pull --workspace routing, slug validation rejects unsafe slugs.

Signed-off-by: phernandez <paul@basicmachines.co>

Author: phernandez

src/basic_memory/cli/commands/cloud/bisync_commands.py

@@ -77,6 +77,8 @@ async def generate_mount_credentials(tenant_id: str) -> MountCredentials:
         config = config_manager.config
         host_url = config.cloud_host.rstrip("/")
 
+        # The mount endpoints resolve X-Workspace-ID by matching the workspace's
+        # tenant_id, so passing a tenant_id here is the correct routing key.
         response = await make_api_request(
             method="POST",
             url=f"{host_url}/tenant/mount/credentials",

src/basic_memory/cli/commands/cloud/project_sync.py

@@ -186,9 +186,15 @@ def _require_personal_workspace(
     return workspace
 
 
-async def _get_cloud_project(name: str) -> ProjectItem | None:
-    """Fetch a project by name from the cloud API."""
-    async with get_client(project_name=name) as client:
+async def _get_cloud_project(name: str, *, workspace_id: str | None = None) -> ProjectItem | None:
+    """Fetch a project by name from the cloud API.
+
+    ``workspace_id`` routes the lookup to a specific tenant so the project
+    metadata comes from the same workspace the transfer targets (otherwise
+    get_client would resolve the workspace from config/default and could read a
+    different tenant — see #920 review).
+    """
+    async with get_client(project_name=name, workspace=workspace_id) as client:
         projects_list = await ProjectClient(client).list_projects()
         for proj in projects_list.projects:
             if generate_permalink(proj.name) == generate_permalink(name):
@@ -252,7 +258,10 @@ def sync_project_command(
     _require_personal_workspace(name, config, unsupported_message=TEAM_WORKSPACE_SYNC_UNSUPPORTED)
 
     try:
-        # Get tenant info for bucket name
+        # Get tenant info for bucket name.
+        # TODO(#919): scope to the project's workspace like push/pull. Safe for now
+        # because these mirror commands are gated to the (default-tenant) Personal
+        # workspace, so the default mount info is correct.
         tenant_info = run_with_cleanup(get_mount_info())
         bucket_name = tenant_info.bucket_name
 
@@ -353,9 +362,12 @@ def _run_directional_transfer(
         tenant_info = run_with_cleanup(get_mount_info(workspace_id=target_workspace.tenant_id))
         bucket_name = tenant_info.bucket_name
 
-        # Get project info
+        # Get project info from the same workspace we resolved above, so the
+        # project path and the bucket/remote all refer to one tenant.
         with force_routing(cloud=True):
-            project_data = run_with_cleanup(_get_cloud_project(name))
+            project_data = run_with_cleanup(
+                _get_cloud_project(name, workspace_id=target_workspace.tenant_id)
+            )
         if not project_data:
             console.print(f"[red]Error: Project '{name}' not found[/red]")
             raise typer.Exit(1)
@@ -512,7 +524,10 @@ def bisync_project_command(
     _require_personal_workspace(name, config)
 
     try:
-        # Get tenant info for bucket name
+        # Get tenant info for bucket name.
+        # TODO(#919): scope to the project's workspace like push/pull. Safe for now
+        # because these mirror commands are gated to the (default-tenant) Personal
+        # workspace, so the default mount info is correct.
         tenant_info = run_with_cleanup(get_mount_info())
         bucket_name = tenant_info.bucket_name
 
@@ -570,7 +585,10 @@ def check_project_command(
     _require_cloud_credentials(config)
 
     try:
-        # Get tenant info for bucket name
+        # Get tenant info for bucket name.
+        # TODO(#919): scope to the project's workspace like push/pull. Safe for now
+        # because these mirror commands are gated to the (default-tenant) Personal
+        # workspace, so the default mount info is correct.
         tenant_info = run_with_cleanup(get_mount_info())
         bucket_name = tenant_info.bucket_name
 

src/basic_memory/cli/commands/cloud/rclone_commands.py

@@ -2,7 +2,8 @@
 
 This module provides simplified, project-scoped rclone operations:
 - Each project syncs independently
-- Uses single "basic-memory-cloud" remote (not tenant-specific)
+- Routes through the project's tenant-scoped remote (SyncProject.remote_name);
+  the default tenant keeps "basic-memory-cloud", others use their own (see #919)
 - Balanced defaults from SPEC-8 Phase 4 testing
 - Per-project bisync state tracking
 

src/basic_memory/cli/commands/cloud/rclone_config.py

@@ -6,6 +6,7 @@
 
 import configparser
 import os
+import re
 import shutil
 from pathlib import Path
 from typing import Optional
@@ -67,14 +68,29 @@ def save_rclone_config(config: configparser.ConfigParser) -> None:
 DEFAULT_RCLONE_REMOTE = "basic-memory-cloud"
 
 
+# rclone remote section names allow letters, digits, hyphens, and underscores.
+# The slug comes from the cloud API (a trust boundary), so validate it before
+# splicing it into a remote name to avoid a broken/unusable rclone.conf section.
+_SAFE_SLUG = re.compile(r"^[A-Za-z0-9_-]+$")
+
+
 def remote_name_for_workspace(slug: str | None, *, is_default: bool) -> str:
     """Return the rclone remote name for a workspace.
 
     The default workspace keeps the legacy ``basic-memory-cloud`` remote so
     existing setups keep working; other workspaces get ``basic-memory-cloud-<slug>``.
+
+    Raises:
+        RcloneConfigError: If a non-default workspace slug contains characters
+            that are not valid in an rclone remote name.
     """
     if is_default or not slug:
         return DEFAULT_RCLONE_REMOTE
+    if not _SAFE_SLUG.match(slug):
+        raise RcloneConfigError(
+            f"Workspace slug '{slug}' cannot be used as an rclone remote name "
+            "(allowed: letters, digits, hyphens, underscores)."
+        )
     return f"{DEFAULT_RCLONE_REMOTE}-{slug}"
 
 

tests/cli/cloud/test_project_sync_command.py

@@ -409,7 +409,7 @@ def _stub_transfer_env(
     monkeypatch.setattr(
         module,
         "_get_cloud_project",
-        lambda _name: _async_value(SimpleNamespace(name="research", path="research")),
+        lambda _name, **_kwargs: _async_value(SimpleNamespace(name="research", path="research")),
     )
     monkeypatch.setattr(
         module,
@@ -540,6 +540,29 @@ def test_cloud_push_allows_organization_workspace(monkeypatch, config_manager):
     assert recorder["args"][0].remote_name == "basic-memory-cloud-acme"
 
 
+def test_cloud_pull_workspace_override_routes_through_workspace_remote(monkeypatch, config_manager):
+    """pull --workspace routes through the named workspace's own remote and bucket."""
+    module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync")
+
+    org_ws = _workspace("team-tenant", "organization", "acme", is_default=False)
+    plan = TransferPlan(new=["new.md"], conflicts=[], dest_only=[], errors=[])
+    recorder: dict = {}
+
+    # _get_workspace_for_project must receive the override and return the org workspace.
+    def _resolve(_name, _config, *, workspace_override=None):
+        assert workspace_override == "acme"
+        return _async_value(org_ws)
+
+    _stub_transfer_env(monkeypatch, module, plan=plan, recorder=recorder, workspace=org_ws)
+    monkeypatch.setattr(module, "_get_workspace_for_project", _resolve)
+
+    result = runner.invoke(app, ["cloud", "pull", "--name", "research", "--workspace", "acme"])
+
+    assert result.exit_code == 0, result.output
+    assert recorder["args"][0].remote_name == "basic-memory-cloud-acme"
+    assert recorder["args"][2] == "pull"
+
+
 def test_cloud_push_errors_when_workspace_remote_not_set_up(monkeypatch, config_manager):
     """If the workspace's remote isn't configured, push stops with the setup command."""
     module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync")

tests/cli/cloud/test_rclone_config_and_bmignore_filters.py

@@ -1,7 +1,10 @@
 import time
 
+import pytest
+
 from basic_memory.cli.commands.cloud.bisync_commands import convert_bmignore_to_rclone_filters
 from basic_memory.cli.commands.cloud.rclone_config import (
+    RcloneConfigError,
     configure_rclone_remote,
     get_rclone_config_path,
     rclone_remote_exists,
@@ -123,6 +126,14 @@ def test_remote_name_for_workspace():
     assert remote_name_for_workspace("acme", is_default=False) == "basic-memory-cloud-acme"
 
 
+def test_remote_name_for_workspace_rejects_unsafe_slug():
+    # A slug from the API with characters invalid in an rclone remote name must
+    # fail fast rather than write a broken rclone.conf section.
+    for bad in ["a/b", "has space", "dot.dot", "weird:name"]:
+        with pytest.raises(RcloneConfigError):
+            remote_name_for_workspace(bad, is_default=False)
+
+
 def test_configure_rclone_remote_named_workspace_remote(config_home):
     remote = configure_rclone_remote(
         access_key="ak", secret_key="sk", remote_name="basic-memory-cloud-acme"