fix(cli): address push/pull review feedback (fatal-error guard, UX, tests)

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

- project_diff: fail fast on fatal `rclone check` errors. A non-zero exit with
  no combined listing (auth/network/missing-remote) previously produced an empty
  plan, so the transfer ran as a no-op and reported success. Now raises
  RcloneError with rclone's stderr. (Codex P2 / claude-review #2)
- sync-setup "Next steps": stop pointing every user at the now-Personal-only
  `bm cloud sync`; lead with the Team-safe `pull`/`push` and note bisync is
  Personal-only. (claude-review #1)
- Cosmetic: capitalize the push/pull abort headlines for consistency; document
  the two ConflictStrategy definitions (Typer enum vs engine Literal). (nits)
- Tests: cover keep-local+pull and keep-cloud+push (preserve-destination cases),
  project_transfer aborting on a mid-loop conflict-copy failure, and project_diff
  raising on a fatal check error vs not raising on a normal differences exit.

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

Author: phernandez

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

@@ -57,6 +57,10 @@ class ConflictStrategy(str, Enum):
     Default is ``fail``: surface the conflicts and abort before transferring,
     leaving the user to re-run with an explicit resolution — like git refusing
     to clobber local changes.
+
+    This is the Typer-facing enum; the engine in ``rclone_commands`` accepts the
+    same values as a ``ConflictStrategy`` Literal. ``_run_directional_transfer``
+    bridges the two by passing ``on_conflict.value``. Keep the values in sync.
     """
 
     fail = "fail"
@@ -235,7 +239,7 @@ def sync_project_command(
 def _print_conflict_abort(name: str, direction: TransferDirection, plan: TransferPlan) -> None:
     """Explain a conflict abort and how to resolve it (git-pull style)."""
     console.print(
-        f"[red]{direction} aborted: {len(plan.conflicts)} file(s) differ between "
+        f"[red]{direction.capitalize()} aborted: {len(plan.conflicts)} file(s) differ between "
         f"local and cloud.[/red]"
     )
     for path in plan.conflicts:
@@ -287,7 +291,7 @@ def _run_directional_transfer(
         # Outcome: abort before moving any bytes.
         if plan.errors:
             console.print(
-                f"[red]{direction} aborted: rclone could not compare "
+                f"[red]{direction.capitalize()} aborted: rclone could not compare "
                 f"{len(plan.errors)} file(s)[/red]"
             )
             for path in plan.errors:
@@ -602,9 +606,15 @@ async def _create_local_project():
 
         console.print(f"[green]Sync configured for project '{name}'[/green]")
         console.print(f"\nLocal sync path: {resolved_path}")
+        # Lead with the Team-safe additive commands (work on any workspace); the
+        # `sync`/`bisync` mirrors are Personal-workspace-only.
         console.print("\nNext steps:")
-        console.print(f"  1. Preview: bm cloud sync --name {name} --dry-run")
-        console.print(f"  2. Sync: bm cloud sync --name {name}")
+        console.print(f"  1. Preview a pull: bm cloud pull --name {name} --dry-run")
+        console.print(f"  2. Fetch from cloud: bm cloud pull --name {name}")
+        console.print(f"  3. Upload local changes: bm cloud push --name {name}")
+        console.print(
+            f"  Personal workspaces can also mirror with: bm cloud bisync --name {name} --resync"
+        )
     except Exception as e:
         console.print(f"[red]Error configuring sync: {str(e)}[/red]")
         raise typer.Exit(1)

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

@@ -42,6 +42,7 @@
 class RunResult(Protocol):
     returncode: int
     stdout: str
+    stderr: str
 
 
 RunFunc = Callable[..., RunResult]
@@ -384,7 +385,19 @@ def project_diff(
     # 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)
+    plan = _parse_check_combined(result.stdout)
+
+    # Trigger: non-zero exit AND the combined listing produced no entries at all.
+    # Why: a difference always yields +/-/*/! lines, so an empty listing on a
+    # non-zero exit means the check itself failed (auth, missing remote, network,
+    # bad filter) rather than finding zero differences. Without this guard the
+    # caller would see an empty plan, transfer nothing, and report success.
+    # Outcome: fail fast with rclone's stderr instead of a silent no-op.
+    if result.returncode != 0 and not (plan.new or plan.conflicts or plan.dest_only or plan.errors):
+        detail = result.stderr.strip() or f"rclone check exited with code {result.returncode}"
+        raise RcloneError(f"Failed to compare {project.name} with cloud: {detail}")
+
+    return plan
 
 
 def project_copy(

tests/test_rclone_commands.py

@@ -31,20 +31,22 @@
 
 
 class _RunResult:
-    def __init__(self, returncode: int = 0, stdout: str = ""):
+    def __init__(self, returncode: int = 0, stdout: str = "", stderr: str = ""):
         self.returncode = returncode
         self.stdout = stdout
+        self.stderr = stderr
 
 
 class _Runner:
-    def __init__(self, *, returncode: int = 0, stdout: str = ""):
+    def __init__(self, *, returncode: int = 0, stdout: str = "", stderr: str = ""):
         self.calls: list[tuple[list[str], dict]] = []
         self._returncode = returncode
         self._stdout = stdout
+        self._stderr = stderr
 
     def __call__(self, cmd: list[str], **kwargs):
         self.calls.append((cmd, kwargs))
-        return _RunResult(returncode=self._returncode, stdout=self._stdout)
+        return _RunResult(returncode=self._returncode, stdout=self._stdout, stderr=self._stderr)
 
 
 def _assert_has_consistency_headers(cmd: list[str]) -> None:
@@ -809,3 +811,110 @@ def test_project_diff_requires_local_path():
     project = SyncProject(name="research", path="/research")
     with pytest.raises(RcloneError):
         project_diff(project, "my-bucket", "pull", is_installed=lambda: True)
+
+
+def test_project_diff_raises_on_fatal_check_error(tmp_path):
+    """A non-zero exit with no combined listing means the check failed, not 'no diffs'."""
+    runner = _Runner(returncode=7, stdout="", stderr="Failed to create file system: AccessDenied")
+    filter_path = _write_filter_file(tmp_path)
+    project = SyncProject(name="research", path="/research", local_sync_path="/tmp/research")
+
+    with pytest.raises(RcloneError) as exc_info:
+        project_diff(
+            project,
+            "my-bucket",
+            "pull",
+            run=runner,
+            is_installed=lambda: True,
+            filter_path=filter_path,
+        )
+
+    assert "AccessDenied" in str(exc_info.value)
+
+
+def test_project_diff_nonzero_with_differences_does_not_raise(tmp_path):
+    """Differences make rclone check exit non-zero, but that is expected — no raise."""
+    runner = _Runner(returncode=1, stdout="* changed.md\n")
+    filter_path = _write_filter_file(tmp_path)
+    project = SyncProject(name="research", path="/research", local_sync_path="/tmp/research")
+
+    plan = project_diff(
+        project,
+        "my-bucket",
+        "pull",
+        run=runner,
+        is_installed=lambda: True,
+        filter_path=filter_path,
+    )
+
+    assert plan.conflicts == ["changed.md"]
+
+
+def test_project_transfer_keep_local_on_pull_preserves_local(tmp_path):
+    """keep-local + pull → local is the destination and must be preserved (--ignore-existing)."""
+    runner = _Runner(returncode=0)
+    filter_path = _write_filter_file(tmp_path)
+    project = SyncProject(name="research", path="/research", local_sync_path="/tmp/research")
+    plan = TransferPlan(new=[], conflicts=["dup.md"], dest_only=[], errors=[])
+
+    project_transfer(
+        project,
+        "my-bucket",
+        "pull",
+        plan,
+        strategy="keep-local",
+        run=runner,
+        is_installed=lambda: True,
+        filter_path=filter_path,
+    )
+
+    cmd, _ = runner.calls[0]
+    assert "--ignore-existing" in cmd
+    assert "--checksum" not in cmd
+
+
+def test_project_transfer_keep_cloud_on_push_preserves_cloud(tmp_path):
+    """keep-cloud + push → cloud is the destination and must be preserved (--ignore-existing)."""
+    runner = _Runner(returncode=0)
+    filter_path = _write_filter_file(tmp_path)
+    project = SyncProject(name="research", path="/research", local_sync_path="/tmp/research")
+    plan = TransferPlan(new=[], conflicts=["dup.md"], dest_only=[], errors=[])
+
+    project_transfer(
+        project,
+        "my-bucket",
+        "push",
+        plan,
+        strategy="keep-cloud",
+        run=runner,
+        is_installed=lambda: True,
+        filter_path=filter_path,
+    )
+
+    cmd, _ = runner.calls[0]
+    assert "--ignore-existing" in cmd
+
+
+def test_project_transfer_keep_both_returns_false_on_copy_failure(tmp_path):
+    """A failed conflict-copy aborts the transfer before the additive pass."""
+    runner = _Runner(returncode=1)  # every rclone invocation fails
+    filter_path = _write_filter_file(tmp_path)
+    project = SyncProject(name="research", path="/research", local_sync_path="/tmp/research")
+    plan = TransferPlan(new=["a.md"], conflicts=["dup.md"], dest_only=[], errors=[])
+
+    result = project_transfer(
+        project,
+        "my-bucket",
+        "pull",
+        plan,
+        strategy="keep-both",
+        conflict_suffix="S",
+        run=runner,
+        is_installed=lambda: True,
+        filter_path=filter_path,
+    )
+
+    assert result is False
+    # Stopped after the first failed copyto — the additive copy never ran.
+    assert len(runner.calls) == 1
+    assert runner.calls[0][0][:2] == ["rclone", "copyto"]