feat(cli): per-workspace rclone remotes for Team push/pull

[phernandez]

Summary

Route bm cloud push/pull through the resolved workspace's own tenant-scoped rclone remote, so projects in non-default / Team workspaces read and write the correct Tigris bucket. Closes the Codex P1 review note on #917. Design + research: #919.

Keys stay tenant-scoped (option A); each accessible tenant gets its own remote.

Why

Tigris IAM keys are bucket/tenant-scoped (one tenant = one bucket — confirmed against the cloud DB model and the Tigris IAM docs / Fly Tigris docs). The CLI used a single basic-memory-cloud remote configured with the default tenant's credentials, and get_mount_info() was called with no workspace context — so push/pull against a Team-workspace project would target the wrong bucket. The cloud mount endpoints already honor X-Workspace-ID (resolve workspace → validate membership + subscription → return that tenant's bucket-scoped creds), so the fix is client-side.

What

• bisync_commands — get_mount_info(workspace_id=...) and generate_mount_credentials(tenant_id) send X-Workspace-ID (omitted → default tenant, unchanged).
• rclone_config — remote_name_for_workspace (default tenant keeps basic-memory-cloud; others get basic-memory-cloud-<slug>), rclone_remote_exists, and configure_rclone_remote(remote_name=...).
• rclone_commands — SyncProject.remote_name; get_project_remote routes through it.
• core_commands — bm cloud setup --workspace <slug|name|tenant_id> provisions a per-workspace remote. Provisioning is explicit and once — avoids re-minting (and leaking) keys for legacy tenants whose /tenant/mount/credentials mints a fresh key each call.
• project_sync — push/pull resolve the project's workspace (config, or a new --workspace override with disambiguation by tenant_id/slug), select that workspace's remote, error with the exact bm cloud setup --workspace … hint if it isn't configured, and scope mount info to the workspace's tenant.

$ bm cloud pull --name team-notes
Workspace 'acme' is not set up for sync.
  Run: bm cloud setup --workspace acme

$ bm cloud setup --workspace acme
Configured rclone remote: basic-memory-cloud-acme
$ bm cloud pull --name team-notes   # routes through basic-memory-cloud-acme → acme bucket

Scope / follow-ups

• sync/bisync/check still use the default remote (they're Personal-only); extending them to per-workspace remotes is a follow-up.
• Workspace resolution uses config + --workspace override (not a cross-workspace name search); --workspace covers same-name disambiguation.
• Backend hygiene (noted in CLI: per-workspace rclone remotes for Team push/pull (tenant-scoped keys) #919): tenant_mount_credentials.tenant_id lacks a FK to tenant(id) — add it in basic-memory-cloud.

Test plan

• rclone_config: remote_name_for_workspace, named-remote write, rclone_remote_exists.
• rclone_commands: get_project_remote honors SyncProject.remote_name; default remote unchanged.
• project_sync: push/pull route a Team workspace through basic-memory-cloud-<slug>; error + setup hint when the remote is missing; --workspace override resolution (match / no-match).
• core_commands: bm cloud setup --workspace acme configures basic-memory-cloud-acme.
• 117 unit tests pass; ty + ruff clean.

🤖 Generated with Claude Code

[github-actions]

Claude finished @phernandez's task in 2m 16s —— View job

---

PR Review: feat(cli): per-workspace rclone remotes for Team push/pull

Tasks

• Gather context and read changed files
• Review code quality and standards
• Review testing coverage
• Review architecture patterns
• Review security and performance
• Post final review

---

Summary

This PR correctly routes bm cloud push/pull through each workspace's own tenant-scoped rclone remote, fixing the root cause of Team-workspace bucket misrouting. The design is sound, the backward-compatibility story is clean, and the test coverage is good. A few minor issues worth addressing are noted below.

---

Code Quality & Standards

✅ Passed

• Python 3.12+ type annotations throughout
• Async patterns correct (keyword-only workspace_id param, proper run_with_cleanup usage)
• 100-character line length maintained
• Literate programming style followed: Trigger/Why/Outcome comments used appropriately
• No commented-out code blocks
• SyncProject.remote_name default keeps backward compat for all existing callers

⚠️ Issues

1. Stale module-level docstring in rclone_commands.py (line 8)

# This module provides simplified, project-scoped rclone operations:
# ...
# - Uses single "basic-memory-cloud" remote (not tenant-specific)

This bullet is now incorrect. The module does support per-workspace remotes via SyncProject.remote_name. Fix this →

2. Naming asymmetry in generate_mount_credentials (bisync_commands.py line 68–88)

The parameter is tenant_id: str, but it's routed via _workspace_id_header(tenant_id) — i.e., sent as X-Workspace-ID. The docstring says "Routed via X-Workspace-ID" which is correct, but the call site in setup() at core_commands.py:249 passes tenant_info.tenant_id as the argument. This is consistent — just slightly confusing that a tenant_id goes into a workspace-ID header. Low risk (backend accepts both), but worth a clarifying inline note.

3. sync/bisync/check commands do not propagate the resolved workspace to get_mount_info()

In sync_project_command, bisync_project_command, and check_project_command, the workspace is resolved via _require_personal_workspace(), but get_mount_info() is still called without workspace_id:

# sync_project_command (line 256)
workspace = _require_personal_workspace(name, config, ...)  # resolves workspace but result discarded for mount info
tenant_info = run_with_cleanup(get_mount_info())             # always uses default tenant

The PR description acknowledges this as intentional scope ("extending to per-workspace remotes is a follow-up"), and the Personal-only guard prevents misrouting in practice. A # TODO(#858-follow-up) comment here would prevent a future reader from wondering why this is inconsistent.

---

Testing & Coverage

✅ Passed

• remote_name_for_workspace — default/non-default cases covered (test_remote_name_for_workspace)
• rclone_remote_exists — exists/not-exists covered (test_rclone_remote_exists)
• Named-remote write (test_configure_rclone_remote_named_workspace_remote)
• get_project_remote honors SyncProject.remote_name (covered in rclone_commands tests)
• push/pull: remote-not-configured error + setup hint (test_cloud_push_errors_when_workspace_remote_not_set_up)
• push: Team workspace routed through basic-memory-cloud-acme (test_cloud_push_allows_organization_workspace)
• --workspace override: match / no-match cases (test_get_workspace_for_project_override_resolves/no_match_raises)
• bm cloud setup --workspace acme provisions basic-memory-cloud-acme (test_cloud_setup_workspace_configures_named_remote)

⚠️ Minor gap

No test for the --workspace override flag via CLI for pull — push is tested end-to-end via _stub_transfer_env, but pull --workspace only exercises the shared _run_directional_transfer path. The existing stub coverage is sufficient for confidence, but an explicit CLI test for pull --workspace acme (symmetric to test_cloud_push_allows_organization_workspace) would add clarity.

No test for workspace slug with edge-case characters — If a workspace slug contains characters that rclone doesn't allow in a remote name (e.g. dots, forward slashes), the remote name basic-memory-cloud-<slug> would silently fail at rclone parse time. Worth a note in the tests or a validation guard in remote_name_for_workspace.

---

Architecture

✅ Passed

• Clean separation: rclone_config owns remote naming; project_sync owns orchestration
• SyncProject.remote_name carries the routing decision into the rclone layer without leaking workspace concepts downward
• Explicit setup requirement (no surprise key-minting) is correct and the error message is actionable
• Backward compatibility: default workspace transparently keeps basic-memory-cloud — no migration needed

---

Security & Performance

✅ Passed

• No hardcoded credentials
• Workspace resolution validates membership + subscription server-side via X-Workspace-ID
• rclone_remote_exists reads config from disk (one configparser parse per push/pull — acceptable)
• backup_rclone_config() creates a backup on every configure_rclone_remote call, including re-setup. This is low-risk but could accumulate backup files over time (rclone.conf.backup-<pid>). Not a blocker.

⚠️ Workspace slug injection into remote names

remote_name_for_workspace builds the remote name from the API-supplied slug without sanitization:

# rclone_config.py:78
return f"{DEFAULT_RCLONE_REMOTE}-{slug}"

rclone remote names must match [a-zA-Z][\w-]* (letters, digits, hyphens, underscores). If a backend workspace slug contains spaces, slashes, or other special characters, the resulting configparser section name could break rclone config parsing or silently create an unusable remote. This is an external API trust boundary. Either validate/sanitize the slug here, or assert that the backend guarantees slug format. Fix this →

---

Compatibility

• ✅ No file path comparisons (Windows safe)
• ✅ No emoji or Unicode in console/log output

---

Checklist Result

Category	Status
Coding conventions / CLAUDE.md	✅
Python 3.12+ types & async	✅
FastAPI / Typer conventions	✅
100-char line limit	✅
Unit tests for new functions	✅ (minor gap: pull --workspace CLI test)
100% coverage	✅ (appears maintained)
Local-first / no DB changes	✅ (CLI-only change)
No hardcoded secrets	✅
Input validation	⚠️ (slug sanitization)
Windows compatibility	✅
No emoji in output	✅

Overall: Approve with suggestions. The slug-sanitization point is the most actionable item before merge; the rest are minor cleanup notes.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8b8e4008ec

ℹ️ 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".

[chatgpt-codex-connector]

Route the project lookup through the selected workspace

When bm cloud pull/push --workspace acme selects a workspace different from the project config or account default, this scopes the bucket/remote to target_workspace, but the subsequent _get_cloud_project(name) still calls get_client(project_name=name) without the selected workspace. Under force_routing(cloud=True), get_client resolves X-Workspace-ID from config/default, so it can fetch the project path from another tenant or report it missing, then transfer against the acme bucket with the wrong/missing project metadata. Pass target_workspace.tenant_id into the cloud project lookup (and ultimately get_client(..., workspace=...)) so all cloud reads use the same workspace selected here.

Useful? React with 👍 / 👎.

[phernandez]

Fixed in c99c3f7 — _get_cloud_project now takes workspace_id and passes it to get_client(..., workspace=...), so the project lookup uses the same tenant the bucket/remote target. Threaded target_workspace.tenant_id through from the resolver.

[github-actions]

Claude finished @phernandez's task in 1m 44s —— View job

---

PR Review: feat(cli): per-workspace rclone remotes for Team push/pull

Tasks

• Gather context and read changed files
• Review code quality and standards
• Review testing coverage
• Review architecture patterns
• Review security and performance
• Post final review

---

Summary

This PR is well-implemented and the latest commit (c99c3f7) addresses the key P2 issue from the Codex review. The workspace isolation for push/pull is correct and complete. A few minor items noted below.

---

Code Quality & Standards

✅ Passed

• Python 3.12+ type annotations throughout (str | None, keyword-only params)
• 100-character line length maintained
• Literate programming style: Trigger/Why/Outcome blocks used where branching materially changes behavior
• No commented-out code blocks
• rclone_commands.py module docstring updated (previous review flag addressed)
• SyncProject.remote_name defaults to legacy value — zero migration needed

⚠️ Minor: Stale docstring in rclone_config.py (line 4)

# rclone_config.py lines 1-5
"""rclone configuration management for Basic Memory Cloud.

This module provides simplified rclone configuration for SPEC-20.
Uses a single "basic-memory-cloud" remote for all operations.  # ← stale
"""

rclone_commands.py was updated (previous review flag) but rclone_config.py still says "Uses a single 'basic-memory-cloud' remote for all operations." The module now owns the per-workspace naming logic (remote_name_for_workspace, _SAFE_SLUG). Fix this →

---

Security & Performance

✅ Passed

• Slug sanitization implemented: _SAFE_SLUG = re.compile(r"^[A-Za-z0-9_-]+$") with RcloneConfigError raised on mismatch (previous P1 flag fully addressed)
• No hardcoded credentials
• rclone_remote_exists does one configparser read per push/pull — acceptable
• Workspace membership + subscription validated server-side via X-Workspace-ID

---

Architecture

✅ Passed

• Codex P2 fix verified: _get_cloud_project now accepts workspace_id and passes it to get_client(workspace=workspace_id) (c99c3f7). _run_directional_transfer passes target_workspace.tenant_id at line 369 — the project lookup uses the same tenant as the bucket/remote.
• sync, bisync, and check commands correctly skip workspace-scoped routing — each has a TODO(#919) comment explaining why (gated to Personal, so default mount info is correct).
• _get_cloud_project callers in sync_project_command, bisync_project_command, and check_project_command correctly omit workspace_id (defaults to None → default tenant); the Personal-only gate makes this safe.
• Clean layering: rclone_config owns naming, project_sync owns orchestration, rclone_commands.SyncProject.remote_name carries the decision across the boundary.

---

Testing & Coverage

✅ Passed

• test_remote_name_for_workspace — default/non-default cases
• test_remote_name_for_workspace_rejects_unsafe_slug — covers "a/b", "has space", "dot.dot", "weird:name" (new in this PR — covers the P1 security gap)
• test_rclone_remote_exists — exists/not-exists
• test_configure_rclone_remote_named_workspace_remote — named remote write
• test_get_project_remote_uses_workspace_remote — SyncProject.remote_name honored
• test_cloud_push_allows_organization_workspace — Team workspace routed through basic-memory-cloud-acme
• test_cloud_pull_workspace_override_routes_through_workspace_remote — pull --workspace end-to-end (addresses previous gap)
• test_cloud_push_errors_when_workspace_remote_not_set_up — setup hint surfaced
• test_get_workspace_for_project_override_resolves/no_match_raises — override resolution
• test_cloud_setup_workspace_configures_named_remote — setup flow

---

Checklist Results

Category	Status
Coding conventions / CLAUDE.md	✅
Python 3.12+ types & async	✅
Typer conventions	✅
100-char line limit	✅
Literate programming comments	✅
Unit tests for new functions	✅
100% coverage maintained	✅
Local-first / no DB changes	✅ (CLI-only)
No hardcoded secrets	✅
Input validation (slug sanitization)	✅
Codex P2 fix (workspace-scoped project lookup)	✅
Windows path compatibility	✅
No emoji in output	✅
Stale module docstring in rclone_config.py	⚠️ minor

Overall: Approve with minor suggestion. The stale docstring on rclone_config.py:4 is the only outstanding item — everything else is clean and the Codex P2 fix is correctly threaded through.

[phernandez]

Responses to automated review (Codex + claude-review)

Thanks both — pushed fixes in c99c3f78.

Fixed

• Project lookup not scoped to the selected workspace (Codex P2) — _get_cloud_project now takes workspace_id and passes it to get_client(..., workspace=...), so the project metadata is read from the same tenant the bucket/remote target. Threaded target_workspace.tenant_id through _run_directional_transfer.
• Workspace-slug injection into remote names (claude-review, security) — remote_name_for_workspace now validates the slug ([A-Za-z0-9_-]+) and raises RcloneConfigError instead of writing a broken rclone.conf section. Tested with /, space, dot, and : slugs.
• Stale module docstring in rclone_commands.py — the "single basic-memory-cloud remote" bullet now reflects per-workspace remotes.
• generate_mount_credentials naming note — added a comment that the tenant_id is the X-Workspace-ID routing key (the backend resolves the header by matching tenant_id).
• sync/bisync/check still default-scoped — added a TODO(#919) explaining why it's safe (those mirrors are gated to the default-tenant Personal workspace) and that per-workspace routing is the follow-up.
• Tests — added pull --workspace routing and slug-validation cases.

Acknowledged / not changed

• backup_rclone_config() accumulating rclone.conf.backup-<pid> files — pre-existing behavior, low risk; out of scope here.

All 114 unit tests + ty + ruff pass.

[github-actions]

Claude finished @phernandez's task in 2m 6s —— View job

---

PR Review: feat(cli): per-workspace rclone remotes for Team push/pull

Tasks

• Gather context and read changed files
• Review code quality and standards
• Review testing coverage
• Review architecture patterns
• Review security and performance
• Post final review

---

Summary

This PR correctly wires bm cloud push/pull through each workspace's own tenant-scoped rclone remote, fixing the root cause of Team-workspace bucket misrouting. All issues from prior review cycles have been addressed. One minor discrepancy is noted below.

---

Code Quality & Standards

✅ Passed

• Python 3.12+ type annotations throughout (str | None, *-keyword-only params)
• 100-character line length maintained
• Literate programming style: Trigger/Why/Outcome blocks used correctly throughout _run_directional_transfer and rclone_config.py
• rclone_config.py and rclone_commands.py module docstrings both updated and accurate
• _SAFE_SLUG regex (r"^[A-Za-z0-9_-]+$") with RcloneConfigError on violation — security gap from first review fully closed
• _get_cloud_project(workspace_id=...) scoped to the resolved tenant — Codex P2 threaded correctly
• TODO(#919) comments on sync/bisync/check explain the scope boundary clearly
• generate_mount_credentials comment clarifies the tenant_id / X-Workspace-ID naming asymmetry

⚠️ Minor: check_project_command TODO comment is slightly misleading (project_sync.py:591)

The TODO says "these mirror commands are gated to the (default-tenant) Personal workspace", but check_project_command (unlike sync and bisync) does not call _require_personal_workspace(). A Team workspace project can run bm cloud check and it will compare against the default tenant's bucket rather than the team bucket.

This is read-only (no data is modified), and the PR description notes check as follow-up scope alongside sync/bisync. But the comment should reflect the actual state rather than implying a guard that isn't there:

# TODO(#919): scope to the project's workspace like push/pull. check is read-only so
# a wrong-bucket comparison is tolerable (won't corrupt data), but results may be
# misleading for Team workspace projects until this is extended.

Fix this →

---

Testing & Coverage

✅ Passed

• test_remote_name_for_workspace — default/non-default naming
• test_remote_name_for_workspace_rejects_unsafe_slug — injection prevention ("a/b", "has space", "dot.dot", "weird:name")
• test_configure_rclone_remote_named_workspace_remote — named remote write
• test_rclone_remote_exists — exists/not-exists
• test_get_project_remote_uses_workspace_remote — SyncProject.remote_name honored
• test_cloud_push_allows_organization_workspace — Team workspace routed through basic-memory-cloud-acme
• test_cloud_pull_workspace_override_routes_through_workspace_remote — pull --workspace end-to-end
• test_cloud_push_errors_when_workspace_remote_not_set_up — actionable setup hint surfaced
• test_get_workspace_for_project_override_resolves / no_match_raises — override resolution
• test_cloud_setup_workspace_configures_named_remote — full setup flow for non-default workspace

No meaningful gaps; the _stub_transfer_env helper correctly uses **kwargs for _get_sync_project so older tests remain valid against the new signature.

---

Architecture

✅ Passed

• Clean separation: rclone_config owns naming and validation; project_sync owns orchestration; SyncProject.remote_name carries the routing decision down to the rclone layer without leaking workspace concepts
• _get_cloud_project(workspace_id=...) scoped to target_workspace.tenant_id — project metadata and bucket/remote all refer to the same tenant
• Explicit setup requirement (no surprise key-minting on push/pull) — rclone_remote_exists check with actionable error message is correct
• Backward compat: default workspace keeps basic-memory-cloud transparently — no migration needed for existing setups

---

Security & Performance

✅ Passed

• Slug sanitization in place — _SAFE_SLUG validates API-supplied slugs before writing rclone config sections
• No hardcoded credentials
• Workspace membership + subscription validated server-side via X-Workspace-ID
• rclone_remote_exists does one configparser parse per push/pull — acceptable overhead

---

Checklist

Category	Status
Coding conventions / CLAUDE.md	✅
Python 3.12+ types & async	✅
Typer conventions	✅
100-char line limit	✅
Literate programming comments	✅ (minor: misleading TODO in check)
Unit tests for new functions	✅
100% coverage maintained	✅
Local-first / no DB changes	✅ (CLI-only)
No hardcoded secrets	✅
Input validation (slug sanitization)	✅
Codex P2 (workspace-scoped project lookup)	✅
check_project_command workspace gate	⚠️ not gated (intentional, but comment is misleading)
Windows path compatibility	✅
No emoji/unicode in output	✅

Overall: Approved. The PR is correct and all prior P1/P2 issues are resolved. The one item above is a comment accuracy nit on check_project_command — not a behavioral regression since check is read-only.