test: Phase 7 — test coverage (+24 tests, 64 → 88 total)

test_backup.py (9 new):
  - backup_file path structure, rotation (3 keep), exact keep_count enforcement
  - list_backups newest-first ordering, empty when no dir
  - restore_backup single file, all files, unknown-id error
  - restore_backup path-traversal rejection (validates Phase 1.4)

test_cli.py (7 new, via typer CliRunner):
  - push updates manifest on success; skips on errors (validates Phase 2.3)
  - push cleans up sanitized temp even when engine raises
  - pull skips merge on empty remote .claude.json; merges when non-empty
  - remote add rejects address without @
  - diff includes per-project files (validates Phase 3.1)

test_engine.py (4 new):
  - push with project paths calls rsync per-project
  - get_remote_file_hashes raises on invalid JSON (validates Phase 2.1)
  - _rsync_project aggregates all failures (validates Phase 3.2)
  - check_connection handles TimeoutExpired (validates Phase 2.2)

test_sanitize.py (4 new):
  - strips primaryApiKey and hasCompletedOnboarding
  - merge raises ValueError on corrupted pulled JSON (validates Phase 1.2)
  - merge raises ValueError on corrupted local JSON (validates Phase 1.2)

cli.py: restore Optional[str] type hint for backup_restore (typer needs it
  on Python 3.9 — get_type_hints doesn't evaluate str|None syntax at runtime)

Author: jyshnkr

src/claudesync/cli.py

@@ -7,6 +7,7 @@
 import typer
 from rich.console import Console
 from rich.table import Table
+from typing import Optional
 
 from .backup import list_backups, restore_backup
 from .config import Config, Remote, SyncSettings, load_config, save_config
@@ -314,7 +315,7 @@ def backup_list() -> None:
 @backup_app.command("restore")
 def backup_restore(
     backup_id: str = typer.Argument(..., help="Backup ID to restore"),
-    original_path: str | None = typer.Argument(None, help="Specific file to restore (or all files if omitted)"),
+    original_path: Optional[str] = typer.Argument(None, help="Specific file to restore (or all files if omitted)"),
 ) -> None:
     """Restore a backed-up file."""
     try:

tests/test_backup.py

@@ -0,0 +1,123 @@
+"""Tests for backup management."""
+import pytest
+from pathlib import Path
+from unittest.mock import patch
+
+from claudesync.backup import backup_file, list_backups, restore_backup
+
+
+@pytest.fixture
+def backup_dir(tmp_path, monkeypatch):
+    """Redirect BACKUP_DIR to an isolated temp directory and return it."""
+    bd = tmp_path / "backups"
+    monkeypatch.setattr("claudesync.backup.BACKUP_DIR", bd)
+    return bd
+
+
+def _ts_backup(src: Path, ts: str, **kwargs) -> Path:
+    """Call backup_file with a fixed mock timestamp."""
+    with patch("claudesync.backup.datetime") as mock_dt:
+        mock_dt.now.return_value.strftime.return_value = ts
+        return backup_file(src, **kwargs)
+
+
+def _get_backup_id(dest: Path, backup_dir: Path) -> str:
+    """Extract the timestamp directory name (backup_id) from a dest path."""
+    return dest.relative_to(backup_dir).parts[0]
+
+
+def test_backup_file_creates_expected_path_structure(tmp_path, backup_dir):
+    src = tmp_path / "settings.json"
+    src.write_text("content")
+
+    dest = _ts_backup(src, "20260101T000000")
+
+    assert dest.exists()
+    assert dest.is_relative_to(backup_dir)
+    assert dest.name == "settings.json"
+
+
+def test_backup_file_rotates_when_over_limit(tmp_path, backup_dir):
+    """Verify rotation removes oldest entries when over keep_count."""
+    src = tmp_path / "file.txt"
+    src.write_text("data")
+
+    timestamps = [f"2026010{i}T000000" for i in range(5)]
+    for ts in timestamps:
+        _ts_backup(src, ts, keep_count=3)
+
+    ts_dirs = sorted([d for d in backup_dir.iterdir() if d.is_dir()])
+    assert len(ts_dirs) == 3
+    # Newest 3 kept: indices 2,3,4
+    assert ts_dirs[0].name == "20260102T000000"
+    assert ts_dirs[-1].name == "20260104T000000"
+
+
+def test_backup_file_keeps_exactly_keep_count_entries(tmp_path, backup_dir):
+    src = tmp_path / "f.txt"
+    src.write_text("x")
+
+    for i in range(10):
+        _ts_backup(src, f"202601{i:02d}T000000", keep_count=5)
+
+    ts_dirs = [d for d in backup_dir.iterdir() if d.is_dir()]
+    assert len(ts_dirs) == 5
+
+
+def test_list_backups_returns_newest_first(tmp_path, backup_dir):
+    src = tmp_path / "f.txt"
+    src.write_text("x")
+
+    _ts_backup(src, "20260101T000000")
+    _ts_backup(src, "20260102T000000")
+
+    entries = list_backups()
+    assert len(entries) >= 2
+    ids = [e.backup_id for e in entries]
+    assert ids == sorted(ids, reverse=True)
+
+
+def test_list_backups_empty_when_no_backup_dir(tmp_path, monkeypatch):
+    monkeypatch.setattr("claudesync.backup.BACKUP_DIR", tmp_path / "nonexistent")
+    assert list_backups() == []
+
+
+def test_restore_backup_single_file(tmp_path, backup_dir, monkeypatch):
+    # Patch Path.home() so the restore guard accepts tmp_path
+    monkeypatch.setattr(Path, "home", lambda: tmp_path)
+
+    src = tmp_path / "restore_target.txt"
+    src.write_text("original")
+    dest = _ts_backup(src, "20260101T120000")
+    backup_id = _get_backup_id(dest, backup_dir)
+
+    src.write_text("overwritten")
+    restored = restore_backup(backup_id, str(src))
+
+    assert len(restored) == 1
+    assert src.read_text() == "original"
+
+
+def test_restore_backup_all_files(tmp_path, backup_dir):
+    src = tmp_path / "all_files.txt"
+    src.write_text("data")
+    dest = _ts_backup(src, "20260101T130000")
+    backup_id = _get_backup_id(dest, backup_dir)
+
+    restored = restore_backup(backup_id)
+    assert len(restored) >= 1
+
+
+def test_restore_backup_raises_on_unknown_backup_id(backup_dir):
+    with pytest.raises(ValueError, match="not found"):
+        restore_backup("nonexistent_id")
+
+
+def test_restore_backup_rejects_path_traversal(tmp_path, backup_dir):
+    """restore_backup must reject paths that escape the backup directory."""
+    ts_dir = backup_dir / "20260101T000000"
+    ts_dir.mkdir(parents=True)
+    (ts_dir / "innocent.txt").write_text("ok")
+
+    with pytest.raises(ValueError, match="traversal"):
+        restore_backup("20260101T000000", "/../../../etc/passwd")

tests/test_cli.py

@@ -0,0 +1,165 @@
+"""Tests for ClaudeSync CLI commands."""
+import json
+import pytest
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+from typer.testing import CliRunner
+
+from claudesync.cli import app
+from claudesync.config import Config, Remote, SyncSettings
+from claudesync.engine import SyncError, SyncSummary
+
+
+runner = CliRunner()
+
+REMOTE_NAME = "home"
+
+
+@pytest.fixture
+def mock_config(tmp_path):
+    """A Config with one remote and no projects."""
+    remote = Remote(host="192.168.1.1", user="alice", ssh_key="~/.ssh/id_ed25519",
+                    remote_home="/home/alice")
+    config = Config(remotes={REMOTE_NAME: remote}, projects=[], sync=SyncSettings())
+    return config
+
+
+@pytest.fixture
+def connected_engine():
+    """An Engine mock that always reports connection success."""
+    engine = MagicMock()
+    engine.check_connection.return_value = True
+    engine.get_remote_file_hashes.return_value = {}
+    engine.push.return_value = SyncSummary(files_transferred=1, errors=[])
+    engine.pull.return_value = SyncSummary(files_transferred=0, errors=[])
+    return engine
+
+
+# ---------------------------------------------------------------------------
+# push
+# ---------------------------------------------------------------------------
+
+def test_push_updates_manifest_after_successful_sync(mock_config, connected_engine):
+    with patch("claudesync.cli.load_config", return_value=mock_config), \
+         patch("claudesync.cli.Engine", return_value=connected_engine), \
+         patch("claudesync.cli.get_global_include_paths", return_value=[]), \
+         patch("claudesync.cli.build_local_manifest", return_value={}), \
+         patch("claudesync.cli.get_remote_manifest", return_value={}), \
+         patch("claudesync.cli.update_manifest_for_remote") as mock_update, \
+         patch("claudesync.cli.write_sanitized_temp", return_value=Path("/tmp/sanitized.json")), \
+         patch("pathlib.Path.unlink"):
+        result = runner.invoke(app, ["push", REMOTE_NAME])
+
+    assert result.exit_code == 0
+    mock_update.assert_called_once()
+
+
+def test_push_skips_manifest_update_on_errors(mock_config, connected_engine):
+    connected_engine.push.return_value = SyncSummary(files_transferred=0, errors=["rsync failed"])
+
+    with patch("claudesync.cli.load_config", return_value=mock_config), \
+         patch("claudesync.cli.Engine", return_value=connected_engine), \
+         patch("claudesync.cli.get_global_include_paths", return_value=[]), \
+         patch("claudesync.cli.build_local_manifest", return_value={}), \
+         patch("claudesync.cli.get_remote_manifest", return_value={}), \
+         patch("claudesync.cli.update_manifest_for_remote") as mock_update, \
+         patch("claudesync.cli.write_sanitized_temp", return_value=Path("/tmp/sanitized.json")), \
+         patch("pathlib.Path.unlink"):
+        result = runner.invoke(app, ["push", REMOTE_NAME])
+
+    assert result.exit_code == 0
+    mock_update.assert_not_called()
+
+
+def test_push_cleans_up_sanitized_temp_on_engine_exception(mock_config, connected_engine):
+    tmp_file = MagicMock(spec=Path)
+    connected_engine.push.side_effect = RuntimeError("rsync crashed")
+
+    with patch("claudesync.cli.load_config", return_value=mock_config), \
+         patch("claudesync.cli.Engine", return_value=connected_engine), \
+         patch("claudesync.cli.get_global_include_paths", return_value=[]), \
+         patch("claudesync.cli.build_local_manifest", return_value={}), \
+         patch("claudesync.cli.get_remote_manifest", return_value={}), \
+         patch("claudesync.cli.write_sanitized_temp", return_value=tmp_file):
+        result = runner.invoke(app, ["push", REMOTE_NAME])
+
+    # Even on exception, unlink should have been called
+    tmp_file.unlink.assert_called_once()
+
+
+# ---------------------------------------------------------------------------
+# pull
+# ---------------------------------------------------------------------------
+
+def test_pull_skips_merge_on_empty_remote_claude_json(mock_config, connected_engine, tmp_path):
+    empty_tmp = tmp_path / "empty.json"
+    empty_tmp.write_text("")  # zero bytes — merge should be skipped
+
+    with patch("claudesync.cli.load_config", return_value=mock_config), \
+         patch("claudesync.cli.Engine", return_value=connected_engine), \
+         patch("claudesync.cli.get_global_include_paths", return_value=[]), \
+         patch("claudesync.cli.build_local_manifest", return_value={}), \
+         patch("claudesync.cli.get_remote_manifest", return_value={}), \
+         patch("claudesync.cli.update_manifest_for_remote"), \
+         patch("claudesync.cli.merge_pulled_claude_json") as mock_merge, \
+         patch("tempfile.NamedTemporaryFile") as mock_ntf:
+        mock_ntf.return_value.__enter__.return_value.name = str(empty_tmp)
+        result = runner.invoke(app, ["pull", REMOTE_NAME])
+
+    mock_merge.assert_not_called()
+
+
+def test_pull_merges_when_remote_claude_json_nonempty(mock_config, connected_engine, tmp_path):
+    nonempty_tmp = tmp_path / "nonempty.json"
+    nonempty_tmp.write_text('{"key": "value"}')
+
+    with patch("claudesync.cli.load_config", return_value=mock_config), \
+         patch("claudesync.cli.Engine", return_value=connected_engine), \
+         patch("claudesync.cli.get_global_include_paths", return_value=[]), \
+         patch("claudesync.cli.build_local_manifest", return_value={}), \
+         patch("claudesync.cli.get_remote_manifest", return_value={}), \
+         patch("claudesync.cli.update_manifest_for_remote"), \
+         patch("claudesync.cli.merge_pulled_claude_json") as mock_merge, \
+         patch("tempfile.NamedTemporaryFile") as mock_ntf:
+        mock_ntf.return_value.__enter__.return_value.name = str(nonempty_tmp)
+        result = runner.invoke(app, ["pull", REMOTE_NAME])
+
+    mock_merge.assert_called_once()
+
+
+# ---------------------------------------------------------------------------
+# remote add
+# ---------------------------------------------------------------------------
+
+def test_remote_add_rejects_missing_at_symbol(mock_config):
+    with patch("claudesync.cli.load_config", return_value=mock_config):
+        result = runner.invoke(app, ["remote", "add", "work", "nousernamehost"])
+
+    assert result.exit_code != 0
+    assert "user@host" in result.output
+
+
+# ---------------------------------------------------------------------------
+# diff
+# ---------------------------------------------------------------------------
+
+def test_diff_includes_project_files(mock_config, connected_engine, tmp_path):
+    """diff should discover per-project files, not just global ones."""
+    proj = tmp_path / "MyProject"
+    proj.mkdir()
+    claude_md = proj / "CLAUDE.md"
+    claude_md.write_text("# My project")
+
+    mock_config.projects = [str(proj)]
+
+    with patch("claudesync.cli.load_config", return_value=mock_config), \
+         patch("claudesync.cli.Engine", return_value=connected_engine), \
+         patch("claudesync.cli.get_global_include_paths", return_value=[]), \
+         patch("claudesync.cli.get_remote_manifest", return_value={}), \
+         patch("claudesync.cli.build_local_manifest", return_value={}) as mock_build:
+        result = runner.invoke(app, ["diff", REMOTE_NAME])
+
+    # build_local_manifest should have received CLAUDE.md in the file list
+    call_args = mock_build.call_args[0][0]
+    assert str(claude_md) in call_args

tests/test_engine.py

@@ -143,3 +143,63 @@ def test_empty_result():
     r = _empty_result()
     assert r.returncode == 0
     assert r.stdout == ""
+
+
+def test_push_with_project_paths_calls_rsync_per_project(engine, tmp_path):
+    """Engine should call rsync once per PROJECT_SYNC_ITEM per project."""
+    proj = tmp_path / "MyProject"
+    proj.mkdir()
+    (proj / "CLAUDE.md").write_text("# project")
+
+    mock_result = MagicMock()
+    mock_result.returncode = 0
+    mock_result.stdout = ""
+    mock_result.stderr = ""
+
+    with patch("claudesync.engine.subprocess.run", return_value=mock_result) as mock_run:
+        engine.push([proj])
+
+    # At minimum: 1 global call + >=1 project call
+    assert mock_run.call_count >= 2
+
+
+def test_get_remote_file_hashes_raises_on_invalid_json(engine):
+    """If SSH stdout is not JSON (e.g. login banner), SyncError is raised."""
+    mock_result = MagicMock()
+    mock_result.returncode = 0
+    mock_result.stdout = "Welcome to server!\nLast login: ..."
+
+    with patch("claudesync.engine.subprocess.run", return_value=mock_result):
+        with pytest.raises(SyncError, match="parse"):
+            engine.get_remote_file_hashes(["/some/file"])
+
+
+def test_rsync_project_aggregates_all_failures(engine, tmp_path):
+    """All per-item rsync failures are aggregated, not just the first."""
+    proj = tmp_path / "Proj"
+    proj.mkdir()
+    # Create all three items so all rsync calls are attempted on push
+    (proj / ".claude").mkdir()
+    (proj / ".claude" / "settings.json").write_text("{}")
+    (proj / "CLAUDE.md").write_text("# x")
+    (proj / ".mcp.json").write_text("{}")
+
+    fail_result = MagicMock()
+    fail_result.returncode = 1
+    fail_result.stdout = ""
+    fail_result.stderr = "error"
+
+    with patch("claudesync.engine.subprocess.run", return_value=fail_result):
+        combined = engine._rsync_project(proj, direction="push", dry_run=False)
+
+    assert combined.returncode != 0
+    # Combined stderr should contain errors from all 3 items
+    assert combined.stderr.count("error") >= 2
+
+
+def test_check_connection_handles_timeout(engine):
+    """TimeoutExpired during SSH connection check returns False."""
+    import subprocess
+    with patch("claudesync.engine.subprocess.run",
+               side_effect=subprocess.TimeoutExpired(cmd="ssh", timeout=10)):
+        assert engine.check_connection() is False