feat(cli): add Team-safe cloud push/pull and gate sync to Personal workspaces

[phernandez]

Summary

Adds bm cloud push and bm cloud pull as git-style, fail-safe transfer primitives that are usable on Team workspaces, and restricts the destructive bm cloud sync mirror to Personal workspaces.

Closes #858. Design discussion: #858 comment. The longer-term three-way reconciler remains tracked separately in #862.

Why

• bm cloud sync is a destructive local→cloud mirror; on a shared Team bucket it can delete a teammate's files. The only pull path was two-way bisync, which is already Personal-only (Block rclone sync/bisync commands for Team workspaces with clear errors #849).
• Teams need a safe way to fetch teammates' notes and add their own without one stale local tree becoming authoritative for shared cloud state.

What

Command	rclone	Destructive on	Personal	Team
bm cloud pull --name X	copy cloud → local	never deletes local	✅	✅
bm cloud push --name X	copy local → cloud	never deletes cloud	✅	✅
bm cloud sync --name X	sync local → cloud mirror	shared bucket	✅	❌ (now gated)
bm cloud bisync --name X	bisync two-way	both sides	✅	❌ (already gated, #849)

• push/pull are additive (rclone copy, never delete on the destination), so neither can damage shared cloud state — they're ungated and Team-safe.
• Conflicts abort by default, git-style: a file that differs on both sides causes the command to list the paths and exit without transferring, like git pull refusing to clobber local changes or git push being rejected when the remote is ahead.
• --on-conflict {fail|keep-local|keep-cloud|keep-both} lets the user decide — no vague --force, no silently-picked winner. keep-both writes the incoming version beside the existing one as name.conflict-<date>.md.
• sync now requires a Personal workspace; its guard message (and the bisync guard) point Team users at push/pull.

Limitations (surfaced in --help + command output; tracked by #862)

Without a sync baseline, deletions are not propagated and every divergence is treated as a conflict rather than auto-resolved. Real three-way merge (auto-resolving unambiguous cases, propagating deletes) needs the per-client manifest + Tigris snapshot baseline designed in #862. Conflict-aware editing continues to flow through MCP/API.

Implementation

• rclone_commands.py — shared _build_transfer_cmd/_transfer_endpoints; refactored project_sync onto them (behavior unchanged); added project_diff (conflict detection via rclone check --combined), project_copy, project_copy_file, and project_transfer (strategy dispatch).
• project_sync.py — new push/pull commands with --on-conflict/--dry-run/--verbose; gated sync to Personal via a per-command guard message.
• AGENTS.md — documented the new commands in the cloud command reference.

Test plan

• tests/test_rclone_commands.py — argv-level: conflict-output parsing, source/dest ordering per direction, copy/--ignore-existing per strategy, copyto conflict-copy, keep-both ordering.
• tests/cli/cloud/test_project_sync_command.py — command-level: abort-on-conflict, abort-on-compare-error, each --on-conflict strategy, push/pull allowed for Team workspaces, sync blocked for Team / allowed for Personal.
• ruff check + ruff format clean; ty check clean on changed source.
• bm cloud pull/push --help render correctly.

🤖 Generated with Claude Code

[github-actions]

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

---

PR Review: feat(cli): add Team-safe cloud push/pull and gate sync to Personal workspaces

Todo List

• Gather context and understand the PR
• Read changed files: rclone_commands.py, project_sync.py
• Read test files: test_rclone_commands.py, test_project_sync_command.py
• Check AGENTS.md changes
• Review against team checklist
• Post final review

---

Summary

This is a well-structured PR that introduces additive push/pull primitives for Team workspace safety, backed by solid tests and a clean rclone copy-based design. Overall quality is high. A few findings below.

---

Checklist Results

Item	Status	Notes
Coding conventions (CLAUDE.md)	✅	Clean structure, section headers, decision-point comments
Python 3.12+ types / async patterns	✅	Proper Literal, dataclasses, str | None, Protocol
SQLAlchemy 2.0	✅ N/A	No DB changes
FastAPI / Typer conventions	✅	Typer Option parameters, str.Enum for conflict strategy
100-char line limit	✅	All lines within limit
No commented-out code	✅
Unit tests for new functions	✅	Good argv-level coverage in test_rclone_commands.py
Integration tests	✅ N/A	No MCP tools changed
Edge case coverage	⚠️	See gaps below
100% test coverage	⚠️	Untested branches noted below
Documentation updated	✅	AGENTS.md updated; CLI --help docstrings clear
MCP atomic/composable design	✅ N/A
DB changes include migrations	✅ N/A
Local-first principles	✅
No hardcoded secrets	✅
Input validation	✅	_transfer_endpoints, _require_personal_workspace guard correctly
Error handling	⚠️	One gap (fatal rclone errors — see below)
Logging	✅	loguru used appropriately
Windows path compatibility	⚠️	One string-concat path (see below)
No emojis/unicode in console output	✅	→ only in code comments, not output

---

Issues Found

1. setup_project_sync "Next steps" directs Team users into the now-gated sync command

project_sync.py:605-607 — After bm cloud sync-setup, the command prints:

Next steps:
  1. Preview: bm cloud sync --name <name> --dry-run
  2. Sync: bm cloud sync --name <name>

Team workspace users who follow these instructions will immediately hit the new access-denied guard. Since sync-setup is valid for both workspace types, the next-steps guidance should be workspace-aware or at minimum mention both paths.

Fix this →

2. project_diff silently succeeds on fatal rclone errors

rclone_commands.py:385-387:

result = run(cmd, capture_output=True, text=True)
return _parse_check_combined(result.stdout)

rclone check exits 1 when files differ (expected) and exits with higher codes on fatal errors (bad credentials, no network, bucket not found). Both cases are ignored. On a fatal error, result.stdout is empty, _parse_check_combined returns an empty TransferPlan, and _run_directional_transfer proceeds to call project_copy with nothing to transfer — printing "completed successfully" to the user.

A minimal guard: check result.returncode > 1 and include result.stderr in the abort message. rclone exit code 1 = "differences found" (normal), > 1 = fatal error.

Fix this →

3. Windows path separator in project_copy_file

rclone_commands.py:469-472:

cmd = [
    "rclone", "copyto",
    f"{source_root}/{source_rel_path}",
    f"{dest_root}/{dest_rel_path}",

dest_root is derived from str(Path(project.local_sync_path).expanduser()) which on Windows returns C:\Users\.... The / join produces C:\Users\...\notes/dup.conflict-S.md — rclone generally accepts this, but it's inconsistent. Use Path(dest_root) / dest_rel_path (or the PurePosixPath equivalent consistently) to be safe.

---

Test Coverage Gaps

Two symmetric cases in _strategy_overwrites_dest are not tested — the "wrong direction" protection that guards against accidentally overwriting the wrong side:

• keep-local + pull: Cloud is the source, but user says keep-local → should preserve local (--ignore-existing). Not tested.
• keep-cloud + push: Local is the source, but user says keep-cloud → should preserve cloud (--ignore-existing). Not tested.

The existing tests cover keep-cloud + pull (overwrite) and keep-local + push (overwrite), but not their inverses. These are the cases most likely to confuse users about which side "wins."

Fix this →%20test_project_transfer_keep_local_on_pull_preserves_local%3A%20direction%3Dpull%2C%20strategy%3Dkeep-local%20should%20result%20in%20--ignore-existing%20(local%20wins%20and%20is%20the%20destination)%3B%202)%20test_project_transfer_keep_cloud_on_push_preserves_cloud%3A%20direction%3Dpush%2C%20strategy%3Dkeep-cloud%20should%20result%20in%20--ignore-existing%20(cloud%20wins%20and%20is%20the%20destination).%20Branch%3A%20feat%2Fcloud-push-pull-858&repo=basicmachines-co/basic-memory)

Also missing: a test for project_transfer returning False when project_copy_file fails mid-loop (the if not copied: return False branch at rclone_commands.py:533-534).

---

Minor / Nits

• ConflictStrategy defined twice: once as a Literal in rclone_commands.py (for the function signatures) and once as a str.Enum in project_sync.py (for Typer). The current .value bridge in _run_directional_transfer works fine, but a comment linking them would help a future reader understand the split.

• _print_conflict_abort says {direction} aborted (line 237 of project_sync.py): direction is "push" or "pull" (lowercase). Output is readable, just ensure the casing is intentional — "push aborted: 1 file(s) differ..." vs "Pull aborted: ..." higher up in the same function. The capitalized direction.capitalize() at line 306 suggests there's slight inconsistency.

---

Positives Worth Calling Out

• The _build_transfer_cmd refactor cleanly centralises the shared flags (Tigris headers, filter, --local-no-preallocate) so project_sync and project_copy cannot drift.
• The project_diff → project_transfer split makes the two phases testable in isolation, which the tests exploit nicely.
• The --on-conflict UX mirrors git semantics precisely and the abort message shows exactly what to re-run — excellent user experience.
• The keep-both implementation correctly preserves the destination's copy first, then writes the incoming version as .conflict-<date> — no bytes lost.
• Team workspace guard runs before the mount/bucket lookup (verified in tests with pytest.fail sentinels) — good fail-fast ordering.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e23363606a

ℹ️ 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]

Resolve mount info for the project workspace

For projects attached to a non-default Team workspace, this fetches mount info without the same workspace context used by _get_cloud_project below, so the rclone bucket can be the user's default/personal tenant while the project metadata came from the Team tenant. In that configuration bm cloud pull/push --name ... either reads the wrong bucket or uploads the local Team files into the wrong workspace while reporting success; pass the resolved project workspace as an X-Workspace-ID header when obtaining mount info/credentials.

Useful? React with 👍 / 👎.

[phernandez]

Good catch, and confirmed: get_mount_info() (and generate_mount_credentials) hit /tenant/mount/info with no workspace context, so bucket_name is the account-default tenant's, while _get_cloud_project resolves the project across workspaces. For a project on a non-default Team workspace with its own bucket, the rclone remote could be wrong.

This is a pre-existing pattern (the existing sync/bisync use get_mount_info() identically), but push/pull are the first commands intended to run against a Team bucket, so the PR newly exposes it. The correct fix needs the backend to support a workspace-scoped mount-info/credentials endpoint (e.g. honoring an X-Workspace-ID header), so I don't want to fake it client-side here. Flagging to @phernandez to decide between (a) backend workspace-scoped mount info, or (b) resolving + asserting the project's workspace tenant in _run_directional_transfer until then. Tracking separately rather than blocking this PR.

[chatgpt-codex-connector]

Abort when rclone check itself fails

This ignores rclone check's return code entirely; although differences can produce a non-zero status, rclone's documented CLI contract is still that operational errors are reported via non-zero exits, and fatal failures may have only stderr rather than --combined entries. When the check cannot list/compare the remote (auth, missing remote, network, bad filter, etc.), this returns an empty plan and the caller proceeds to transfer without the conflict-detection gate that makes push/pull safe; treat non-zero with empty/invalid combined output as a RcloneError instead of assuming there are no conflicts.

Useful? React with 👍 / 👎.

[phernandez]

Fixed in c850450 — project_diff now raises RcloneError (with rclone's stderr) when rclone check exits non-zero and produces no combined listing, so a fatal auth/network/missing-remote failure aborts instead of running a no-op transfer that reports success. A normal 'differences found' non-zero exit still parses fine.

[phernandez]

Code review

Ran a high-recall multi-agent review of the diff (7 finder angles → recall-biased verify). One confirmed finding, now fixed in 9dbe313:

• project_copy_file omitted --local-no-preallocate — on pull --on-conflict keep-both the rclone copyto writes the conflict copy to the local filesystem, the exact case that flag guards (NUL-byte padding on virtual filesystems like Google Drive File Stream). Brought it in line with the shared _build_transfer_cmd helper.

Everything else surfaced (the project_sync refactor onto the shared builder, the centralized local_sync_path guard, the reworded bisync message, the new sync Personal-only gate) was verified as intentional and correct. The detection-by-content vs transfer-by-modtime nuance and the "check runs before every transfer" cost were considered and deemed acceptable for this interim primitive (the real reconciler is #862).

[github-actions]

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

---

PR Review: feat(cli): add Team-safe cloud push/pull and gate sync to Personal workspaces

Todo List

• Load tools
• Read changed files: rclone_commands.py, project_sync.py
• Check latest commit (9dbe313) diff
• Review test files
• Cross-check open issues from prior review
• Post final review

---

What the latest commit (9dbe313) fixed

project_copy_file now includes --local-no-preallocate in its rclone copyto command, bringing it in line with the shared _build_transfer_cmd helper. The fix is correct and the new assertion in test_project_copy_file_pull_copyto_renames_on_dest covers it.

---

Checklist Results

Item	Status	Notes
Coding conventions (CLAUDE.md)	✅	Section headers, decision-point comments, literate style
Python 3.12+ types / async patterns	✅	Literal, dataclasses, str | None, Protocol
FastAPI / Typer conventions	✅	Typer Option, str.Enum for conflict strategy
100-char line limit	✅	All lines within limit
No commented-out code	✅
Unit tests for new functions	✅	Good argv-level coverage in test_rclone_commands.py
Edge case coverage	⚠️	Two inverse conflict-strategy cases untested (see below)
100% test coverage	⚠️	See gaps below
Documentation updated	✅	AGENTS.md updated, --help docstrings are clear
No hardcoded secrets	✅
Input validation	✅	_transfer_endpoints, _require_personal_workspace guard correctly
Error handling	⚠️	project_diff fatal-error gap (see below)
Windows path compatibility	⚠️	String-concat paths in project_copy_file (see below)
No emojis/unicode in console output	✅

---

Issues Remaining

1. setup_project_sync "Next steps" still directs Team users to the gated sync command

project_sync.py:605-607 — After bm cloud sync-setup, the command prints:

Next steps:
  1. Preview: bm cloud sync --name <name> --dry-run
  2. Sync: bm cloud sync --name <name>

Team workspace users who follow these instructions will immediately hit the Personal-only guard that was just added. Since sync-setup is valid for both workspace types, this guidance should at least mention the Team-safe path.

Fix this →

2. project_diff silently succeeds on fatal rclone errors

rclone_commands.py:384-387:

# rclone check exits non-zero when files differ — that's expected here, so we
# parse the combined listing rather than trusting the return code.
result = run(cmd, capture_output=True, text=True)
return _parse_check_combined(result.stdout)

rclone check exits 1 when files differ (expected) but exits >1 on fatal errors (bad credentials, no network, bucket not found). Currently both cases are treated identically — empty stdout on a fatal error produces an empty TransferPlan, _run_directional_transfer sees no conflicts, prints "completed successfully", and no data actually moved. The comment at line 385 is correct for rc=1 but the guard needs to distinguish fatal errors.

Minimal fix: if result.returncode > 1: raise RcloneError(f"rclone check failed: {result.stderr}").

Fix this →

3. Windows path separator in project_copy_file

rclone_commands.py:471-472:

f"{source_root}/{source_rel_path}",
f"{dest_root}/{dest_rel_path}",

dest_root is derived from str(Path(...).expanduser()) which on Windows returns C:\Users\.... The / join produces C:\Users\...\notes/dup.conflict-S.md — an inconsistent path. Use Path(dest_root) / dest_rel_path (or str(Path(dest_root) / dest_rel_path)) to be consistent.

Fix this →

---

Test Coverage Gaps

Two symmetric "wrong direction" conflict strategy cases are not tested. The existing tests cover keep-cloud + pull (overwrite) and keep-local + push (overwrite), but not their inverses — these are the cases most likely to confuse users:

• keep-local + pull: local is the destination, user says keep-local → --ignore-existing should be present (local wins)
• keep-cloud + push: cloud is the destination, user says keep-cloud → --ignore-existing should be present (cloud wins)

These directly exercise _strategy_overwrites_dest returning False for the "wrong direction" cases and are critical for confidence that users can't accidentally overwrite the wrong side.

Fix this →%20test_project_transfer_keep_local_on_pull_preserves_local%3A%20direction%3D%22pull%22%2C%20strategy%3D%22keep-local%22%20should%20have%20--ignore-existing%20in%20the%20copy%20command%20(local%20is%20the%20destination%20and%20wins)%3B%202)%20test_project_transfer_keep_cloud_on_push_preserves_cloud%3A%20direction%3D%22push%22%2C%20strategy%3D%22keep-cloud%22%20should%20have%20--ignore-existing%20in%20the%20copy%20command%20(cloud%20is%20the%20destination%20and%20wins).%20Branch%3A%20feat%2Fcloud-push-pull-858&repo=basicmachines-co/basic-memory)

---

Minor Notes

• ConflictStrategy defined twice: as Literal in rclone_commands.py (line 199) and str.Enum in project_sync.py (line 54). The .value bridge at project_sync.py:311 works fine; a short comment linking the two would help a future reader understand the intentional split.

• Capitalization inconsistency in _print_conflict_abort: direction is passed lowercase ("push"/"pull") so the message reads "push aborted: 1 file(s)..." (line 239), but direction.capitalize() is used elsewhere (line 306). Minor but inconsistent.

---

Positives

The overall design is excellent. A few things worth calling out explicitly:

• _build_transfer_cmd centralising shared flags (Tigris headers, filter, --local-no-preallocate) means project_sync and project_copy cannot drift — the latest fix (9dbe313) is proof this abstraction is paying off.
• The --on-conflict UX precisely mirrors git semantics and the abort message shows exactly what to re-run.
• keep-both correctly preserves the destination's copy first, then writes the incoming version as .conflict-<date> — no bytes lost.
• Team workspace guard runs before mount/bucket lookup, verified with pytest.fail sentinels in the tests — good fail-fast ordering.
• The project_diff → project_transfer split makes the two phases testable in isolation, which the tests exploit cleanly.

[phernandez]

Code review — full results

The review completed with 5 findings (correction to my earlier comment, which was posted before the full run finished). Resolutions:

#	Verdict	Finding	Status
1–2	CONFIRMED / PLAUSIBLE	project_copy_file omitted --local-no-preallocate (conflict copy writes locally on pull)	Fixed — 9dbe313
3	PLAUSIBLE	Detection/action comparison mismatch: project_diff uses rclone check (content/hash) but overwrite project_copy used default size+modtime, so a keep-cloud/keep-local overwrite could silently skip a flagged conflict (same size, dest not older)	Fixed — a7937d4 (add --checksum to overwrite copy)
4–5	PLAUSIBLE	keep-both per-file copyto loop isn't atomic: a mid-loop failure leaves some .conflict-<date> sidecars written	Accepted — additive/non-destructive, safe to re-run; the verifier rated harm negligible. Transactional rollback across independent rclone subprocesses would be over-engineering for an additive op.

Everything else (the project_sync refactor onto the shared builder, the centralized local_sync_path guard, the reworded bisync message, the new sync Personal-only gate) was verified as intentional and correct.

All 78 tests, ty, ruff check, and ruff format pass after the fixes.

[github-actions]

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

---

PR Review: feat(cli): add Team-safe cloud push/pull and gate sync to Personal workspaces

Todo List

• Load tools
• Read changed files: rclone_commands.py, project_sync.py
• Read test files: test_rclone_commands.py, test_project_sync_command.py
• Check latest commits (a7937d4, 9dbe313) for fixes
• Verify remaining issues from prior reviews
• Post final review

---

Fixes confirmed in latest commits

Commit	Finding	Status
9dbe313	project_copy_file omitted --local-no-preallocate for conflict-copy writes	✅ Fixed
a7937d4	Overwrite copy used size+modtime instead of --checksum, could silently skip a flagged conflict	✅ Fixed — extra_flags = ("--checksum",) if overwrite else ("--ignore-existing",) at rclone_commands.py:426

Both fixes are correct and targeted.

---

Checklist Results

Item	Status	Notes
Coding conventions (CLAUDE.md)	✅	Section headers, decision-point comments, literate style
Python 3.12+ types / async patterns	✅	Literal, dataclasses, str | None, Protocol
FastAPI / Typer conventions	✅	Typer Option, str.Enum for conflict strategy
100-char line limit	✅
No commented-out code	✅
Unit tests for new functions	✅	Good argv-level coverage
Edge case coverage	⚠️	Two inverse conflict-strategy cases untested (see below)
100% test coverage	⚠️	See gaps below
Documentation updated	✅	AGENTS.md updated, --help docstrings clear
No hardcoded secrets	✅
Input validation	✅	_transfer_endpoints, _require_personal_workspace guard correctly
Error handling	⚠️	project_diff fatal-error gap still open (see below)
Windows path compatibility	⚠️	String-concat paths in project_copy_file still open (see below)
No emojis/unicode in console output	✅

---

Issues Remaining

1. setup_project_sync "Next steps" still directs Team users to the gated sync command

project_sync.py:605-607:

console.print(f"  1. Preview: bm cloud sync --name {name} --dry-run")
console.print(f"  2. Sync: bm cloud sync --name {name}")

sync-setup is valid for both workspace types, but after it completes, Team users who follow the "Next steps" output will immediately hit the Personal-only guard added by this PR. The guidance should at least mention the Team-safe path.

Fix this →

2. project_diff silently succeeds on fatal rclone errors

rclone_commands.py:384-387:

# rclone check exits non-zero when files differ — that's expected here, so we
# parse the combined listing rather than trusting the return code.
result = run(cmd, capture_output=True, text=True)
return _parse_check_combined(result.stdout)

rclone check exits 1 when files differ (expected) but exits >1 on fatal errors (bad credentials, network failure, bucket not found). Both cases are currently treated identically — an empty stdout on a fatal error produces an empty TransferPlan, _run_directional_transfer sees no conflicts and no errors, prints "completed successfully", and zero bytes actually moved.

Minimal fix: if result.returncode > 1: raise RcloneError(f"rclone check failed: {result.stderr}").

Fix this →

3. Windows path separator in project_copy_file

rclone_commands.py:478-479:

f"{source_root}/{source_rel_path}",
f"{dest_root}/{dest_rel_path}",

dest_root is derived from str(Path(...).expanduser()) which on Windows returns C:\Users\.... The / join produces a mixed-separator path. Use str(Path(dest_root) / dest_rel_path) for consistency.

Note: source_root on push is a remote rclone path (basic-memory-cloud:bucket/...), which must keep its / separator regardless. Only the local side (dest_root on pull, source_root on push) needs Path join.

Fix this →

---

Test Coverage Gaps

Two "wrong direction" conflict strategy cases remain untested. The existing tests cover keep-cloud + pull (overwrite) and keep-local + push (overwrite), but not their inverses — these are the cases most likely to confuse users:

• keep-local + pull: local is the destination, user says keep-local → --ignore-existing should be present (local wins, cloud does not overwrite)
• keep-cloud + push: cloud is the destination, user says keep-cloud → --ignore-existing should be present (cloud wins, local does not overwrite)

These directly exercise _strategy_overwrites_dest returning False for the "wrong direction" cases.

Fix this →%20test_project_transfer_keep_local_on_pull_preserves_local%3A%20direction%3D%22pull%22%2C%20strategy%3D%22keep-local%22%20should%20have%20--ignore-existing%20in%20the%20copy%20command%20(local%20is%20the%20destination%20and%20wins)%3B%202)%20test_project_transfer_keep_cloud_on_push_preserves_cloud%3A%20direction%3D%22push%22%2C%20strategy%3D%22keep-cloud%22%20should%20have%20--ignore-existing%20in%20the%20copy%20command%20(cloud%20is%20the%20destination%20and%20wins).%20Branch%3A%20feat%2Fcloud-push-pull-858&repo=basicmachines-co/basic-memory)

---

On the Codex P1 finding (workspace routing)

Codex flagged that get_mount_info() in _run_directional_transfer does not pass workspace context, while _get_cloud_project(name) uses per-project routing — so on a non-default Team workspace the bucket could come from a different tenant. However, the same pattern exists verbatim in the pre-existing sync_project_command and bisync_project_command. This is a pre-existing gap not introduced by this PR and is out of scope here.

---

Minor Notes

• ConflictStrategy defined twice: as Literal in rclone_commands.py:199 and str.Enum in project_sync.py:54. The .value bridge at project_sync.py:314 works fine; a short comment linking the two would help a future reader.
• Capitalization inconsistency: _print_conflict_abort uses lowercase direction in the abort message ("push aborted: ...") but direction.capitalize() appears one line above in _run_directional_transfer:306. Minor but inconsistent.

---

Summary

The core design is excellent and the two latest fixes (9dbe313, a7937d4) addressed real correctness issues. Three issues remain unaddressed from the prior reviews: the sync-setup Next steps guidance misleads Team users (issue 1), a fatal rclone error in project_diff produces a silent false-success (issue 2), and Windows path compatibility in project_copy_file (issue 3). The two missing inverse conflict-strategy tests (issue 4) leave _strategy_overwrites_dest's "wrong direction" branches uncovered. Issues 1–2 are functional bugs; 3–4 are correctness/coverage gaps worth addressing before merge.

[github-actions]

Claude finished @phernandez's task in 4m 15s —— View job

---

PR Review: feat(cli): add Team-safe cloud push/pull and gate sync to Personal workspaces

Todo List

• Load tools
• Read changed files: rclone_commands.py, project_sync.py
• Read test files: test_rclone_commands.py, test_project_sync_command.py
• Fix issue 1: setup_project_sync "Next steps" directs Team users to gated sync
• Fix issue 2: project_diff silent success on fatal rclone errors
• Fix issue 3: Windows path separator in project_copy_file
• Add missing inverse conflict-strategy tests + fatal error test
• Commit changes (commit b3e319e)
• Push to remote — blocked: github-actions[bot] lacks write access to this repo

---

Changes implemented in commit b3e319e

All four issues from the prior reviews are addressed. Changes are committed but not yet pushed due to the permissions issue noted at the bottom.

Fix 1: setup_project_sync "Next steps" (project_sync.py:605-610)

Team users following sync-setup output were pointed straight at the Personal-only sync command. Now shows both paths:

Next steps — Team workspaces (additive, never deletes):
  Fetch cloud changes: bm cloud pull --name <name>
  Upload local changes: bm cloud push --name <name>

Next steps — Personal workspaces only (mirror):
  Preview: bm cloud sync --name <name> --dry-run
  Sync:    bm cloud sync --name <name>

Fix 2: project_diff fatal rclone errors (rclone_commands.py:390-391)

rclone check exits 1 for differences (expected) but exits >1 for fatal errors (bad credentials, no network, bucket not found). An empty stdout on a fatal error was returning an empty TransferPlan, which caused _run_directional_transfer to print "completed successfully" with zero bytes transferred. Added the guard and extended the RunResult Protocol with stderr:

result = run(cmd, capture_output=True, text=True)
if result.returncode > 1:
    raise RcloneError(f"rclone check failed: {result.stderr}")
return _parse_check_combined(result.stdout)

Fix 3: Windows path separator in project_copy_file (rclone_commands.py:483-492)

Replaced f-string / joins with Path() on the local side to avoid mixed-separator paths on Windows (C:\Users\...\notes/file.conflict-S.md). Cloud remote paths keep their forward-slash joins:

source_arg = (
    f"{source_root}/{source_rel_path}" if direction == "pull"
    else str(Path(source_root) / source_rel_path)
)
dest_arg = (
    str(Path(dest_root) / dest_rel_path) if direction == "pull"
    else f"{dest_root}/{dest_rel_path}"
)

Fix 4: Missing tests (tests/test_rclone_commands.py)

Four new tests added:

• test_project_diff_raises_on_fatal_rclone_error — exit code 2 raises RcloneError
• test_project_diff_does_not_raise_on_differences — exit code 1 parses normally (regression guard)
• test_project_transfer_keep_local_on_pull_preserves_local — keep-local + pull → --ignore-existing (local destination is preserved)
• test_project_transfer_keep_cloud_on_push_preserves_cloud — keep-cloud + push → --ignore-existing (cloud destination is preserved)

_RunResult and _Runner test helpers updated to carry stderr.

---

Push blocked by GitHub permissions

The commit is on the local checkout but git push returned 403 — github-actions[bot] does not have write access to this repository. To land the commit, either:

1. Repo settings: Settings → Actions → General → Workflow permissions → set to "Read and write permissions"
2. Manual cherry-pick: git fetch && git cherry-pick b3e319e && git push origin feat/cloud-push-pull-858

[github-actions]

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

---

PR Review: feat(cli): add Team-safe cloud push/pull and gate sync to Personal workspaces

Todo List

• Load tools
• Read changed files (rclone_commands.py, project_sync.py)
• Read test files (test_rclone_commands.py, test_project_sync_command.py)
• Verify fixes from prior reviews against commit c850450
• Post final review

---

What commit c850450 fixed

All four issues carried over from the previous reviews are now resolved:

#	Finding	Status
1	sync-setup "Next steps" pointed Team users at the Personal-only sync command	✅ Fixed — project_sync.py:609-617 now leads with pull/push and notes bisync is Personal-only
2	project_diff silent success on fatal rclone errors	✅ Fixed — rclone_commands.py:396-398 raises RcloneError on non-zero exit with empty combined listing
3	Missing inverse conflict-strategy tests (keep-local+pull, keep-cloud+push)	✅ Fixed — four new tests added, including midloop-abort and fatal-check-error coverage
4	Nits: capitalization inconsistency in _print_conflict_abort, undocumented ConflictStrategy duplication	✅ Fixed — direction.capitalize() used consistently; linking comment added to the Typer enum

The fatal-error guard in project_diff is slightly more robust than the suggested returncode > 1: using returncode != 0 AND empty plan also catches a returncode=1 with no combined output (an edge case where rclone check exits with "differences" code but produced nothing — which cannot happen legitimately, so raising is correct).

---

Checklist Results

Item	Status	Notes
Coding conventions (CLAUDE.md)	✅	Section headers, decision-point comments, literate style
Python 3.12+ types / async patterns	✅	Literal, dataclasses, str | None, Protocol
FastAPI / Typer conventions	✅	Typer Option, str.Enum for conflict strategy
100-char line limit	✅
No commented-out code	✅
Unit tests for new functions	✅	Strong argv-level coverage including all strategy/direction combinations
Edge case coverage	✅	Fatal error guard, midloop abort, both "wrong direction" preserve cases
100% test coverage	✅	All identified gaps now covered
Documentation updated	✅	AGENTS.md, docs/cloud-cli.md, --help docstrings
No hardcoded secrets	✅
Input validation	✅	_transfer_endpoints, _require_personal_workspace guard correctly
Error handling	✅	Fatal rclone error now surfaced; conflict abort before transfer
Windows path compatibility	⚠️	One minor gap remains (see below)
No emojis/unicode in console output	✅

---

Remaining Issue: Windows path separator in project_copy_file

rclone_commands.py:491-492 still uses f-string / joins for the copyto paths:

f"{source_root}/{source_rel_path}",
f"{dest_root}/{dest_rel_path}",

On Windows, the local root (local_path) is returned by str(Path(...).expanduser()) as C:\Users\.... The / join produces C:\Users\...\research/notes/dup.conflict-S.md — a mixed-separator path that rclone generally tolerates but that is inconsistent with project convention.

The fix is direction-aware (cloud remote paths must stay with /):

• On pull: local is dest_root → str(Path(dest_root) / dest_rel_path); remote is source_root → keep f-string
• On push: local is source_root → str(Path(source_root) / source_rel_path); remote is dest_root → keep f-string

This is a minor platform-compatibility issue that won't affect Linux/macOS users. It is the only outstanding finding from the full review cycle.

Fix this →

---

Summary

The overall design is solid and the three prior review rounds converged well. Commits 9dbe313 → a7937d4 → c850450 resolved all meaningful correctness and UX issues: --local-no-preallocate on conflict copies, checksum-based overwrite comparison, fatal rclone error surfacing, sync-setup Next steps guidance, and the two "wrong direction" test gaps. The single remaining item is the Windows path separator nit in project_copy_file — low risk, Linux/macOS unaffected. The PR is otherwise ready to merge.

[phernandez]

Responses to automated review (Codex + claude-review)

Thanks both. Triage and resolutions (pushed in c850450d, on top of the earlier 9dbe313 / a7937d4):

Fixed

• Fatal rclone check errors silently succeeding (Codex P2 / claude-review Memory json import #2) — project_diff now raises RcloneError with rclone's stderr when check exits non-zero and produces no combined listing (auth/network/missing-remote). A normal "differences found" non-zero exit still parses. Tests cover both paths.
• sync-setup "Next steps" pointed at the now-gated sync (claude-review Pre release #1) — now leads with the Team-safe pull/push and notes bisync is Personal-only.
• Conflict-copy copyto missing --local-no-preallocate (earlier review) — fixed in 9dbe313.
• Detection/action comparison mismatch (earlier review) — overwrite copy now uses --checksum to match the hash-based project_diff (a7937d4).
• Nits — capitalized the push/pull abort headlines; documented the two ConflictStrategy definitions (Typer enum ↔ engine Literal).
• Test gaps (claude-review) — added keep-local + pull and keep-cloud + push (preserve-destination) cases, and a project_transfer mid-loop conflict-copy failure test.

Surfaced, not auto-fixed

• Workspace-scoped mount info (Codex P1) — real exposure for projects on a non-default Team workspace, but get_mount_info() is workspace-unaware today and the correct fix needs backend support (workspace-scoped mount-info/credentials, e.g. X-Workspace-ID). Replied inline; flagged to @phernandez to decide on the backend path vs an interim client-side workspace assertion. Not blocking this PR.

Accepted as-is

• keep-both partial state (earlier review) — the per-file copyto loop isn't transactional, but it's additive/non-destructive and safe to re-run; transactional rollback across independent rclone subprocesses would be over-engineering.
• Windows / join in copyto (claude-review fix: fix permalink uniqueness violations on create/update/sync #3) — cosmetic; rclone accepts forward slashes on Windows, and one endpoint is always an rclone remote (forward-slash) while the other is a local path. Left as-is to avoid a riskier path-normalization change.

All 83 unit tests + ty + ruff pass locally.

[github-actions]

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

---

PR Review: feat(cli): add Team-safe cloud push/pull and gate sync to Personal workspaces

Todo List

• Load tools
• Read changed files (rclone_commands.py, project_sync.py)
• Read test files (test_rclone_commands.py, test_project_sync_command.py)
• Review against team checklist
• Post final review

---

Summary

This is a well-executed PR that has converged through multiple review rounds. All previously raised functional issues are resolved in c850450. The design is solid, the test coverage is thorough, and the UX matches git semantics precisely.

---

Checklist Results

Item	Status	Notes
Coding conventions (CLAUDE.md)	✅	Section headers, decision-point Trigger/Why/Outcome comments throughout
Python 3.12+ types / async patterns	✅	Literal, dataclasses, str | None, Protocol — all correct
SQLAlchemy 2.0	✅ N/A	No DB changes
FastAPI / Typer conventions	✅	Typer.Option, str.Enum for ConflictStrategy, proper exit codes
100-char line limit	✅	All lines within limit
No commented-out code	✅
Unit tests for new functions	✅	Strong argv-level coverage in test_rclone_commands.py
Integration tests for new MCP tools	✅ N/A	No MCP tools changed
Edge case coverage	✅	Fatal-error guard, mid-loop abort, all strategy/direction combos
100% test coverage	✅	All previously identified gaps now covered
Documentation updated	✅	AGENTS.md and docs/cloud-cli.md updated; --help docstrings are clear
MCP atomic/composable design	✅ N/A
DB changes include migrations	✅ N/A
Local-first principles	✅	Push/pull are additive; no cloud state destroyed
No hardcoded secrets	✅
Input validation	✅	_transfer_endpoints and _require_personal_workspace guard correctly
Error handling	✅	Fatal rclone error now surfaced via RcloneError; conflict abort precedes transfer
Logging	✅	loguru used appropriately
Windows path compatibility	⚠️	One accepted nit remains — see below
No emojis/unicode in console output	✅	→ only in code comments, not printed output

---

Fixes Confirmed from Prior Review Rounds

Commit	Finding	Status
9dbe313	project_copy_file missing --local-no-preallocate for conflict-copy	✅ Fixed
a7937d4	Overwrite copy used size+modtime instead of --checksum, could silently skip a flagged conflict	✅ Fixed
c850450	project_diff silent success on fatal rclone errors	✅ Fixed — now raises RcloneError on non-zero exit with empty combined listing
c850450	sync-setup "Next steps" pointed Team users at the Personal-only sync command	✅ Fixed — project_sync.py:609-617 now leads with pull/push
c850450	Missing inverse conflict-strategy tests (keep-local+pull, keep-cloud+push)	✅ Fixed — plus mid-loop abort and fatal-check-error coverage
c850450	Nits: direction.capitalize() inconsistency; undocumented ConflictStrategy duplication	✅ Fixed

The fatal-error guard (rclone_commands.py:396-398) is more robust than the suggested returncode > 1 — it uses returncode != 0 AND empty plan, which also catches the edge case where rc=1 produces no combined output (which cannot happen legitimately). Good defensive choice.

---

Remaining Item (Accepted by Author)

Windows path separator in project_copy_file (rclone_commands.py:491-492):

f"{source_root}/{source_rel_path}",
f"{dest_root}/{dest_rel_path}",

On Windows dest_root is C:\Users\..., producing mixed-separator paths. The author accepted this as-is since rclone handles forward slashes on Windows, and the tests now use Path() comparisons (e8cda55) to hold correctly on all platforms. This is a cosmetic gap with no functional impact on Linux/macOS.

---

What's Working Well

• _build_transfer_cmd centralisation — all shared flags (Tigris headers, filter, --local-no-preallocate) flow through one builder, preventing project_sync and project_copy from drifting. The 9dbe313 and a7937d4 fixes both prove this abstraction is paying off.
• --on-conflict UX precisely mirrors git semantics; the abort message shows exactly which paths conflict and the exact --on-conflict flag to re-run with.
• keep-both correctly preserves the destination copy first, then writes the incoming version as .conflict-<date> — no bytes lost.
• Team workspace guard runs before mount/bucket lookup, verified with pytest.fail sentinels — correct fail-fast ordering.
• project_diff → project_transfer split makes the detection and execution phases independently testable, which the test suite exploits cleanly.
• ConflictStrategy duplication is now self-documented (lines 63-64): the Typer enum explicitly references the engine Literal and the .value bridge, so a future reader understands the intentional split.

---

Pre-existing Issue (Not Introduced by This PR)

The Codex P1 finding (workspace-scoped mount info) is a real exposure for projects on a non-default Team workspace, but get_mount_info() is workspace-unaware in the existing sync/bisync commands too. Push/pull are the first commands targeting Team buckets so the gap is newly relevant, but the correct fix requires a backend workspace-scoped credentials endpoint. Tracked separately — not blocking this PR.

---

Verdict: Ready to merge. All functional correctness issues are resolved, test coverage is complete, and the design is clean.