Merge: atomic transfers, signed-release CI, backup-safe defaults, security hardening, full test suite

- Atomic local/WebDAV/SFTP transfers (no truncation on interrupted sync) - Direction-aware delete default (Upload-only => KEEP; backups not wiped) - Path-traversal guard against hostile remotes - ARCHIVE delete fix (create _Deleted base) - CI: run tests on every push, signed release on tags - 40 JVM tests + 14 on-device Nextcloud integration tests
2026-06-05 10:25:32 +00:00
parent dbd317624d c1b7221324
commit 402d0447a0
14 changed files with 790 additions and 54 deletions
@@ -2,8 +2,10 @@ name: Build & Release APK

 on:
  push:
+    branches: ['**']
    tags:
      - 'v*'
+  pull_request:

 jobs:
  build:
@@ -20,10 +22,29 @@ jobs:

      - uses: android-actions/setup-android@v3

-      - name: Build debug APK
+      - name: Run unit tests
        run: |
          chmod +x gradlew
-          ./gradlew assembleDebug --no-daemon
+          ./gradlew testDebugUnitTest --no-daemon
+
+      - name: Decode release keystore
+        env:
+          KEYSTORE_BASE64: ${{ secrets.KEYSTORE_BASE64 }}
+        run: |
+          if [ -n "$KEYSTORE_BASE64" ]; then
+            echo "$KEYSTORE_BASE64" | base64 -d > "$RUNNER_TEMP/release.keystore"
+            echo "KEYSTORE_PATH=$RUNNER_TEMP/release.keystore" >> "$GITHUB_ENV"
+            echo "Release keystore decoded — building signed release."
+          else
+            echo "::warning::KEYSTORE_BASE64 secret not set — release APK will be debug-signed."
+          fi
+
+      - name: Build release APK
+        env:
+          KEYSTORE_PASSWORD: ${{ secrets.KEYSTORE_PASSWORD }}
+          KEY_ALIAS: ${{ secrets.KEY_ALIAS }}
+          KEY_PASSWORD: ${{ secrets.KEY_PASSWORD }}
+        run: ./gradlew assembleRelease --no-daemon

      - name: Get version name
        id: ver
@@ -32,10 +53,11 @@ jobs:
      - name: Rename APK
        run: |
          mkdir dist
-          cp app/build/outputs/apk/debug/app-debug.apk \
+          cp app/build/outputs/apk/release/app-release.apk \
             dist/SyncFlow-v${{ steps.ver.outputs.name }}.apk

      - name: Attach APK to release
+        if: startsWith(github.ref, 'refs/tags/v')
        env:
          TOKEN: ${{ secrets.GITHUB_TOKEN }}
          TAG: ${{ github.ref_name }}
@@ -18,9 +18,15 @@ val localProps = Properties().apply {
    if (f.exists()) load(f.inputStream())
 }

+// Release signing is read from local.properties (local builds) or environment variables
+// (CI). When no keystore is available the release build falls back to the debug key so the
+// build still succeeds — it just isn't a distributable, properly-signed APK.
+val keystorePath = (localProps["KEYSTORE_PATH"] as String?) ?: System.getenv("KEYSTORE_PATH")
+val hasReleaseKeystore = keystorePath != null && file(keystorePath).exists()
+
 android {
    namespace = "com.syncflow"
-    compileSdk = 34
+    compileSdk = 35

    defaultConfig {
        applicationId = "com.syncflow"
@@ -38,19 +44,28 @@ android {

    signingConfigs {
        create("release") {
-            storeFile = localProps["KEYSTORE_PATH"]?.toString()?.let { file(it) }
-            storePassword = localProps["KEYSTORE_PASSWORD"]?.toString()
-            keyAlias = localProps["KEY_ALIAS"]?.toString()
-            keyPassword = localProps["KEY_PASSWORD"]?.toString()
+            if (hasReleaseKeystore) {
+                storeFile = file(keystorePath!!)
+                storePassword = (localProps["KEYSTORE_PASSWORD"] as String?) ?: System.getenv("KEYSTORE_PASSWORD")
+                keyAlias = (localProps["KEY_ALIAS"] as String?) ?: System.getenv("KEY_ALIAS")
+                keyPassword = (localProps["KEY_PASSWORD"] as String?) ?: System.getenv("KEY_PASSWORD")
+            }
        }
    }

    buildTypes {
        release {
-            isMinifyEnabled = true
-            isShrinkResources = true
+            // R8/minify has never been exercised by CI (it only built debug), so leave it off
+            // to keep the signed release behaving identically to the debug builds in use today.
+            // Re-enable with proper keep rules and an on-device smoke test if APK size matters.
+            isMinifyEnabled = false
+            isShrinkResources = false
            proguardFiles(getDefaultProguardFile("proguard-android-optimize.txt"), "proguard-rules.pro")
-            signingConfig = signingConfigs.getByName("release")
+            signingConfig = if (hasReleaseKeystore) {
+                signingConfigs.getByName("release")
+            } else {
+                signingConfigs.getByName("debug")
+            }
        }
    }

@@ -0,0 +1,246 @@
+package com.syncflow
+
+import androidx.room.Room
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.test.platform.app.InstrumentationRegistry
+import com.syncflow.data.db.SyncDatabase
+import com.syncflow.data.db.entities.CloudAccountEntity
+import com.syncflow.data.db.entities.SyncPairEntity
+import com.syncflow.data.db.entities.toDomain
+import com.syncflow.data.providers.nextcloud.NextcloudProvider
+import com.syncflow.domain.model.*
+import com.syncflow.domain.sync.SyncEngine
+import kotlinx.coroutines.runBlocking
+import org.junit.After
+import org.junit.Assert.*
+import org.junit.Assume.assumeTrue
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+import java.io.ByteArrayInputStream
+import java.io.ByteArrayOutputStream
+import java.io.File
+import java.time.Instant
+
+/**
+ * Full-matrix end-to-end test of the REAL SyncEngine against a live Nextcloud, on the device:
+ * in-memory Room DB, a real local folder per test, and the real NextcloudProvider over TLS.
+ * Covers every direction, every delete behavior, updates, nested/recursive, filters, and conflicts.
+ *
+ * Creds via instrumentation args: -e ncUrl ... -e ncUser ... -e ncPass ...
+ */
+@RunWith(AndroidJUnit4::class)
+class FullSyncEngineTest {
+
+    private val ctx = InstrumentationRegistry.getInstrumentation().targetContext
+    private val args = InstrumentationRegistry.getArguments()
+    private val url get() = args.getString("ncUrl")
+    private val user get() = args.getString("ncUser")
+    private val pass get() = args.getString("ncPass")
+
+    private lateinit var db: SyncDatabase
+    private lateinit var engine: SyncEngine
+    private lateinit var provider: NextcloudProvider
+    private var accountId = 0L
+    private val remoteRoot = "SyncFlowFull_${System.currentTimeMillis()}"
+    private val localDirs = mutableListOf<File>()
+
+    @Before fun setUp() = runBlocking {
+        assumeTrue("ncUrl/ncUser/ncPass required", url != null && user != null && pass != null)
+        db = Room.inMemoryDatabaseBuilder(ctx, SyncDatabase::class.java).build()
+        val account = CloudAccount(0, "IT", user, ProviderType.NEXTCLOUD,
+            """{"username":"$user","password":"$pass"}""", url, null)
+        accountId = db.cloudAccountDao().insert(
+            CloudAccountEntity(0, account.displayName, account.email, account.providerType,
+                account.credentialJson, account.serverUrl, account.port))
+        provider = NextcloudProvider(account)
+        engine = SyncEngine(db.syncPairDao(), db.syncFileStateDao(), db.syncConflictDao(), db.syncEventDao(), ctx)
+        provider.createDirectory(remoteRoot).getOrThrow()
+    }
+
+    @After fun tearDown() = runBlocking {
+        runCatching { provider.deleteFile(remoteRoot) }
+        localDirs.forEach { it.deleteRecursively() }
+        if (::db.isInitialized) db.close()
+    }
+
+    // ── helpers ──────────────────────────────────────────────────────────────
+
+    private suspend fun newPair(
+        name: String,
+        dir: SyncDirection = SyncDirection.TWO_WAY,
+        delete: DeleteBehavior = DeleteBehavior.MIRROR,
+        conflict: ConflictStrategy = ConflictStrategy.KEEP_NEWEST,
+        recursive: Boolean = true,
+        excludeExtensions: String = "",
+        excludePatterns: String = "",
+        skipHidden: Boolean = false,
+        maxKb: Long = 0L,
+    ): Triple<SyncPair, File, String> {
+        val local = File(ctx.cacheDir, "synctest_${name}_${System.currentTimeMillis()}").apply { mkdirs() }
+        localDirs += local
+        val remote = "$remoteRoot/$name"
+        provider.createDirectory(remote).getOrThrow()
+        val id = db.syncPairDao().insert(SyncPairEntity(
+            id = 0, name = name, localPath = local.absolutePath, remotePath = remote, accountId = accountId,
+            syncDirection = dir, conflictStrategy = conflict, deleteBehavior = delete, recursive = recursive,
+            scheduleType = ScheduleType.MANUAL, scheduleIntervalMinutes = 30, scheduleDailyTime = null, scheduleWeekdays = 0,
+            wifiOnly = false, wifiSsid = "", chargingOnly = false, minBatteryPct = 0,
+            excludePatterns = excludePatterns, includeExtensions = "", excludeExtensions = excludeExtensions,
+            skipHiddenFiles = skipHidden, minFileSizeKb = 0, maxFileSizeKb = maxKb,
+            notifyOnComplete = false, notifyOnError = false,
+            isEnabled = true, lastSyncAt = null, lastSyncResult = SyncStatus.IDLE, pendingConflicts = 0,
+        ))
+        return Triple(db.syncPairDao().getById(id)!!.toDomain(), local, remote)
+    }
+
+    private suspend fun sync(pair: SyncPair) = engine.sync(pair, provider)
+    private fun write(dir: File, rel: String, content: String) =
+        File(dir, rel).apply { parentFile?.mkdirs() }.writeText(content)
+    private suspend fun remoteNames(remote: String) =
+        provider.listFiles(remote).getOrThrow().map { it.name }
+    private suspend fun remoteText(path: String): String {
+        val out = ByteArrayOutputStream(); provider.downloadFile(path, out).getOrThrow(); return out.toString("UTF-8")
+    }
+    private suspend fun putRemote(remote: String, name: String, content: String) {
+        val b = content.toByteArray()
+        provider.uploadFile(ByteArrayInputStream(b), "$remote/$name", b.size.toLong()).getOrThrow()
+    }
+
+    // ── 1. Upload-only backup: delete on phone keeps the cloud copy (KEEP) ────
+    @Test fun uploadOnly_keep_localDeleteKeepsCloud() = runBlocking {
+        val (pair, local, remote) = newPair("ul_keep", SyncDirection.UPLOAD_ONLY, DeleteBehavior.KEEP)
+        write(local, "a.txt", "hello")
+        assertEquals(1, sync(pair).uploaded)
+        assertTrue("a.txt" in remoteNames(remote))
+        File(local, "a.txt").delete()
+        val r = sync(pair)
+        assertEquals("KEEP must not delete remotely", 0, r.deleted)
+        assertTrue("cloud copy must survive", "a.txt" in remoteNames(remote))
+    }
+
+    // ── 2. Upload-only + MIRROR: local delete removes the cloud copy ──────────
+    @Test fun uploadOnly_mirror_localDeleteRemovesCloud() = runBlocking {
+        val (pair, local, remote) = newPair("ul_mirror", SyncDirection.UPLOAD_ONLY, DeleteBehavior.MIRROR)
+        write(local, "a.txt", "hello"); assertEquals(1, sync(pair).uploaded)
+        File(local, "a.txt").delete()
+        assertEquals(1, sync(pair).deleted)
+        assertFalse("a.txt" in remoteNames(remote))
+    }
+
+    // ── 3. Upload-only + ARCHIVE: deleted file moved to _Deleted/ ─────────────
+    @Test fun uploadOnly_archive_movesToDeleted() = runBlocking {
+        val (pair, local, remote) = newPair("ul_archive", SyncDirection.UPLOAD_ONLY, DeleteBehavior.ARCHIVE)
+        write(local, "a.txt", "keepme"); sync(pair)
+        File(local, "a.txt").delete(); sync(pair)
+        assertFalse("a.txt" in remoteNames(remote))
+        assertTrue("archived copy expected", "a.txt" in remoteNames("$remote/_Deleted"))
+    }
+
+    // ── 4. Two-way initial sync: each side gets the other's files ─────────────
+    @Test fun twoWay_initial_mergesBothSides() = runBlocking {
+        val (pair, local, remote) = newPair("tw_init", SyncDirection.TWO_WAY)
+        write(local, "local.txt", "L")
+        putRemote(remote, "remote.txt", "R")
+        val r = sync(pair)
+        assertEquals(1, r.uploaded); assertEquals(1, r.downloaded)
+        assertTrue(File(local, "remote.txt").exists())
+        assertTrue("local.txt" in remoteNames(remote) && "remote.txt" in remoteNames(remote))
+    }
+
+    // ── 5. Two-way: a local edit propagates to the remote ─────────────────────
+    @Test fun twoWay_localEdit_updatesRemote() = runBlocking {
+        val (pair, local, remote) = newPair("tw_edit", SyncDirection.TWO_WAY)
+        write(local, "f.txt", "v1"); sync(pair)
+        Thread.sleep(1100) // cross the 1s mtime resolution
+        write(local, "f.txt", "v2-updated"); val r = sync(pair)
+        assertEquals(1, r.uploaded)
+        assertEquals("v2-updated", remoteText("$remote/f.txt"))
+    }
+
+    // ── 6. Two-way + MIRROR: deleting locally removes it remotely ─────────────
+    @Test fun twoWay_mirror_localDeletePropagates() = runBlocking {
+        val (pair, local, remote) = newPair("tw_mirror", SyncDirection.TWO_WAY, DeleteBehavior.MIRROR)
+        write(local, "f.txt", "x"); sync(pair)
+        File(local, "f.txt").delete()
+        assertEquals(1, sync(pair).deleted)
+        assertFalse("f.txt" in remoteNames(remote))
+    }
+
+    // ── 7. Download-only: pulls remote, never uploads local-only files ────────
+    @Test fun downloadOnly_pullsRemoteIgnoresLocal() = runBlocking {
+        val (pair, local, remote) = newPair("dl_only", SyncDirection.DOWNLOAD_ONLY)
+        putRemote(remote, "cloud.txt", "from-cloud")
+        write(local, "phoneonly.txt", "P")
+        val r = sync(pair)
+        assertEquals(1, r.downloaded); assertEquals(0, r.uploaded)
+        assertEquals("from-cloud", File(local, "cloud.txt").readText())
+        assertFalse("local-only file must NOT upload", "phoneonly.txt" in remoteNames(remote))
+    }
+
+    // ── 8. Recursive: nested directory structure is preserved ─────────────────
+    @Test fun recursive_uploadsNestedTree() = runBlocking {
+        val (pair, local, remote) = newPair("rec_on", SyncDirection.UPLOAD_ONLY, recursive = true)
+        write(local, "sub/deep/n.txt", "nested")
+        assertEquals(1, sync(pair).uploaded)
+        assertTrue("n.txt" in remoteNames("$remote/sub/deep"))
+    }
+
+    // ── 9. recursive=false: subfolders are skipped ────────────────────────────
+    @Test fun nonRecursive_skipsSubfolders() = runBlocking {
+        val (pair, local, remote) = newPair("rec_off", SyncDirection.UPLOAD_ONLY, recursive = false)
+        write(local, "top.txt", "T")
+        write(local, "sub/deep.txt", "D")
+        assertEquals(1, sync(pair).uploaded)
+        assertTrue("top.txt" in remoteNames(remote))
+        assertTrue("subfolder must be skipped", remoteNames(remote).none { it == "sub" })
+    }
+
+    // ── 10. Filters: excluded extension + hidden file are not uploaded ─────────
+    @Test fun filters_excludeExtensionAndHidden() = runBlocking {
+        val (pair, local, remote) = newPair("filters", SyncDirection.UPLOAD_ONLY,
+            excludeExtensions = "tmp", skipHidden = true)
+        write(local, "keep.txt", "k")
+        write(local, "skip.tmp", "s")
+        write(local, ".hidden", "h")
+        sync(pair)
+        val names = remoteNames(remote)
+        assertTrue("keep.txt" in names)
+        assertFalse("skip.tmp" in names)
+        assertFalse(".hidden" in names)
+    }
+
+    // ── 11. Conflict: both sides changed (ASK) → conflict recorded, no clobber ─
+    @Test fun twoWay_bothChanged_recordsConflict() = runBlocking {
+        val (pair, local, remote) = newPair("conflict", SyncDirection.TWO_WAY, conflict = ConflictStrategy.ASK)
+        write(local, "c.txt", "base"); sync(pair)                  // upload
+        sync(pair)                                                 // reconcile: record remote baseline (etag/mtime)
+        Thread.sleep(1100)
+        write(local, "c.txt", "LOCAL-change")                       // change local
+        putRemote(remote, "c.txt", "REMOTE-change")                // change remote out-of-band
+        val r = sync(pair)
+        assertEquals("a conflict must be detected", 1, r.conflicts)
+        // ASK must not silently overwrite either side
+        assertEquals("LOCAL-change", File(local, "c.txt").readText())
+        assertEquals("REMOTE-change", remoteText("$remote/c.txt"))
+    }
+
+    // ── 12. Conflict KEEP_NEWEST: newer local wins and uploads ────────────────
+    @Test fun twoWay_keepNewest_newerLocalWins() = runBlocking {
+        val (pair, local, remote) = newPair("newest", SyncDirection.TWO_WAY, conflict = ConflictStrategy.KEEP_NEWEST)
+        write(local, "n.txt", "base"); sync(pair)
+        putRemote(remote, "n.txt", "remote-older")
+        Thread.sleep(1100)
+        write(local, "n.txt", "local-newer")                       // local is newer than remote
+        sync(pair)
+        assertEquals("newer local must win", "local-newer", remoteText("$remote/n.txt"))
+    }
+
+    // ── 13. Content integrity: binary-ish bytes round-trip exactly ────────────
+    @Test fun contentIntegrity_roundTrip() = runBlocking {
+        val (pair, local, remote) = newPair("integrity", SyncDirection.TWO_WAY)
+        val payload = (0..5000).joinToString("") { "Ω$it·" }
+        write(local, "big.txt", payload); sync(pair)
+        assertEquals(payload, remoteText("$remote/big.txt"))
+    }
+}
@@ -0,0 +1,117 @@
+package com.syncflow
+
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.test.platform.app.InstrumentationRegistry
+import com.syncflow.data.db.entities.SyncFileStateEntity
+import com.syncflow.data.providers.nextcloud.NextcloudProvider
+import com.syncflow.domain.model.CloudAccount
+import com.syncflow.domain.model.ConflictStrategy
+import com.syncflow.domain.model.DeleteBehavior
+import com.syncflow.domain.model.ProviderType
+import com.syncflow.domain.model.SyncDirection
+import com.syncflow.domain.sync.LocalFileInfo
+import com.syncflow.domain.sync.SyncDecision
+import com.syncflow.domain.sync.syncDecide
+import kotlinx.coroutines.runBlocking
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertTrue
+import org.junit.Assume.assumeTrue
+import org.junit.Test
+import org.junit.runner.RunWith
+import java.io.ByteArrayInputStream
+import java.io.ByteArrayOutputStream
+import java.time.Instant
+
+/**
+ * Real end-to-end test against a live Nextcloud, run ON the device. Exercises the actual
+ * NextcloudProvider (WebDAV over real TLS, including the atomic temp+MOVE upload) and proves
+ * the backup guarantee: with Upload-only + KEEP, "deleted on phone" leaves the cloud copy.
+ *
+ * Credentials are passed as instrumentation args (never committed):
+ *   adb shell am instrument -w \
+ *     -e ncUrl https://nextcloud.khodak.me -e ncUser syncflow-test -e ncPass <pw> \
+ *     com.syncflow.test/androidx.test.runner.AndroidJUnitRunner
+ */
+@RunWith(AndroidJUnit4::class)
+class NextcloudIntegrationTest {
+
+    private val args = InstrumentationRegistry.getArguments()
+    private val url = args.getString("ncUrl")
+    private val user = args.getString("ncUser")
+    private val pass = args.getString("ncPass")
+
+    private fun provider(): NextcloudProvider {
+        val account = CloudAccount(
+            id = 1L,
+            displayName = "IT",
+            email = user,                       // Nextcloud dav path uses this
+            providerType = ProviderType.NEXTCLOUD,
+            credentialJson = """{"username":"$user","password":"$pass"}""",
+            serverUrl = url,
+            port = null,
+        )
+        return NextcloudProvider(account)
+    }
+
+    @Test
+    fun fullBackupRoundTrip() = runBlocking {
+        assumeTrue("ncUrl/ncUser/ncPass instrumentation args required", url != null && user != null && pass != null)
+        val p = provider()
+        val dir = "SyncFlowITest_${System.currentTimeMillis()}"
+        val remoteFile = "$dir/hello.txt"
+        val content = "SyncFlow integration test — 0 to 100 — ${System.currentTimeMillis()}".toByteArray()
+
+        try {
+            // 1. Connect
+            assertTrue("testConnection failed", p.testConnection().isSuccess)
+
+            // 2. Create the backup folder
+            assertTrue("createDirectory failed", p.createDirectory(dir).isSuccess)
+
+            // 3. Upload (exercises atomic temp-file + MOVE)
+            val uploaded = p.uploadFile(ByteArrayInputStream(content), remoteFile, content.size.toLong())
+            assertTrue("upload failed: ${uploaded.exceptionOrNull()}", uploaded.isSuccess)
+
+            // 4. List — the file is on the cloud with the right size
+            val listed = p.listFiles(dir).getOrThrow()
+            val entry = listed.firstOrNull { it.name == "hello.txt" }
+            assertNotNull("uploaded file not found in listing", entry)
+            assertEquals("remote size mismatch", content.size.toLong(), entry!!.sizeBytes)
+
+            // 5. Download — bytes round-trip intact
+            val out = ByteArrayOutputStream()
+            assertTrue("download failed", p.downloadFile(remoteFile, out).isSuccess)
+            assertEquals("downloaded content mismatch", String(content), out.toString("UTF-8"))
+
+            // 6. THE backup guarantee. Phone copy deleted, state record exists, Upload-only + KEEP.
+            val known = SyncFileStateEntity(
+                syncPairId = 1L, relativePath = "hello.txt",
+                localModifiedAt = Instant.now(), localSizeBytes = content.size.toLong(), localHash = null,
+                remoteModifiedAt = entry.modifiedAt, remoteSizeBytes = entry.sizeBytes, remoteEtag = entry.etag,
+                lastSyncedAt = Instant.now(), syncedHash = null,
+            )
+            val keepDecision = syncDecide(
+                SyncDirection.UPLOAD_ONLY, ConflictStrategy.KEEP_NEWEST, DeleteBehavior.KEEP,
+                local = null, remote = entry, known = known, hasPriorSyncState = true,
+            )
+            assertEquals("KEEP must not delete the cloud copy", SyncDecision.SKIP, keepDecision)
+
+            // ...and the engine would do nothing, so the file is verifiably STILL on the cloud:
+            val stillThere = p.listFiles(dir).getOrThrow().any { it.name == "hello.txt" }
+            assertTrue("cloud copy must survive a local delete under KEEP", stillThere)
+
+            // 7. Contrast: MIRROR would delete it — prove the real DELETE works (also cleanup).
+            val mirrorDecision = syncDecide(
+                SyncDirection.UPLOAD_ONLY, ConflictStrategy.KEEP_NEWEST, DeleteBehavior.MIRROR,
+                local = null, remote = entry, known = known, hasPriorSyncState = true,
+            )
+            assertEquals(SyncDecision.DELETE_REMOTE, mirrorDecision)
+            assertTrue("deleteFile failed", p.deleteFile(remoteFile).isSuccess)
+            val goneAfterDelete = p.listFiles(dir).getOrThrow().none { it.name == "hello.txt" }
+            assertTrue("file should be gone after explicit remote delete", goneAfterDelete)
+        } finally {
+            runCatching { p.deleteFile(dir) } // best-effort cleanup of the test folder
+        }
+    }
+}
@@ -43,7 +43,11 @@ class SftpProvider(private val account: CloudAccount, private val credentialStor

    override suspend fun listFiles(remotePath: String): Result<List<RemoteFile>> = runCatching {
        withSftp { sftp ->
-            sftp.ls(remotePath).map { entry ->
+            sftp.ls(remotePath)
+                // Drop "."/".." and any name with a path separator so a hostile server can't
+                // smuggle a traversal segment into a local/remote path.
+                .filter { it.name != "." && it.name != ".." && !it.name.contains('/') && !it.name.contains('\\') }
+                .map { entry ->
                    RemoteFile(
                        path = "$remotePath/${entry.name}".replace("//", "/"),
                        name = entry.name,
@@ -63,12 +67,25 @@ class SftpProvider(private val account: CloudAccount, private val credentialStor
        sizeBytes: Long,
        onProgress: (Long) -> Unit,
    ): Result<RemoteFile> = runCatching {
+        // Upload to a hidden temp sibling, then rename onto the destination so an interrupted
+        // transfer never leaves a truncated file at the real path.
+        val dir = remotePath.substringBeforeLast('/', "")
+        val name = remotePath.substringAfterLast('/')
+        val tmpPath = if (dir.isEmpty()) ".$name.sfpart" else "$dir/.$name.sfpart"
        withSftp { sftp ->
            sftp.put(object : InMemorySourceFile() {
-                override fun getName() = remotePath.substringAfterLast('/')
+                override fun getName() = name
                override fun getLength() = sizeBytes
                override fun getInputStream() = localStream
-            }, remotePath)
+            }, tmpPath)
+            // SFTP rename fails if the target exists on servers without the POSIX-rename
+            // extension, so fall back to removing the destination first.
+            try {
+                sftp.rename(tmpPath, remotePath)
+            } catch (e: Exception) {
+                runCatching { sftp.rm(remotePath) }
+                sftp.rename(tmpPath, remotePath)
+            }
        }
        getFileMetadata(remotePath).getOrThrow()
    }
@@ -93,15 +93,30 @@ open class WebDavProvider(protected val account: CloudAccount) : CloudProvider {
                    localStream.source().use { source -> sink.writeAll(source) }
                }
            }
-            val req = Request.Builder().url(url(remotePath)).put(body).build()
+            // Upload to a hidden temp sibling first, then MOVE it onto the destination. A
+            // failed PUT leaves the real file untouched instead of overwriting it with a
+            // truncated body; the MOVE is a server-side atomic-ish swap.
+            val tmpPath = tempPathFor(remotePath)
+            val req = Request.Builder().url(url(tmpPath)).put(body).build()
            client.newCall(req).execute().use { resp ->
                if (!resp.isSuccessful) throw Exception("Upload HTTP ${resp.code}")
            }
+            moveFile(tmpPath, remotePath).getOrElse { e ->
+                runCatching { deleteFile(tmpPath) }
+                throw e
+            }
            onProgress(sizeBytes)
            getFileMetadata(remotePath).getOrThrow()
        }
    }

+    private fun tempPathFor(remotePath: String): String {
+        val dir = remotePath.substringBeforeLast('/', "")
+        val name = remotePath.substringAfterLast('/')
+        val tmp = ".$name.sfpart"
+        return if (dir.isEmpty()) tmp else "$dir/$tmp"
+    }
+
    override suspend fun downloadFile(remotePath: String, destination: OutputStream, onProgress: (Long) -> Unit): Result<Unit> = runCatching {
        withContext(Dispatchers.IO) {
            val req = Request.Builder().url(url(remotePath)).get().build()
@@ -5,6 +5,8 @@ import android.net.Uri
 import android.provider.DocumentsContract
 import com.syncflow.domain.model.SyncPair
 import java.io.File
+import java.io.FileOutputStream
+import java.io.IOException
 import java.io.InputStream
 import java.io.OutputStream

@@ -14,10 +16,17 @@ sealed class LocalAccessor {

    abstract fun walkFiles(pair: SyncPair): Map<String, LocalFileInfo>
    abstract fun openInputStream(relativePath: String): InputStream?
-    abstract fun createOutputStream(relativePath: String): OutputStream?
    abstract fun delete(relativePath: String): Boolean
    abstract fun lastModifiedMs(relativePath: String): Long

+    /**
+     * Write [relativePath] atomically: stream into a temp sibling first, then swap it into
+     * place only after [write] completes without throwing. An interrupted transfer (network
+     * drop, process death) leaves the existing destination untouched instead of truncating it.
+     * On failure the temp is removed and the exception is rethrown.
+     */
+    abstract suspend fun writeAtomically(relativePath: String, write: suspend (OutputStream) -> Unit)
+
    // ── java.io.File backend (regular /storage/... paths) ────────────────────

    class JavaFile(private val root: File) : LocalAccessor() {
@@ -48,10 +57,30 @@ sealed class LocalAccessor {
        override fun openInputStream(relativePath: String): InputStream =
            File(root, relativePath).inputStream()

-        override fun createOutputStream(relativePath: String): OutputStream {
+        override suspend fun writeAtomically(relativePath: String, write: suspend (OutputStream) -> Unit) {
            val dest = File(root, relativePath)
            dest.parentFile?.mkdirs()
-            return dest.outputStream()
+            val tmp = File(dest.parentFile, ".${dest.name}.sfpart")
+            try {
+                FileOutputStream(tmp).use { os ->
+                    write(os)
+                    os.flush()
+                    os.fd.sync() // durably persist bytes before the rename swaps the file in
+                }
+            } catch (e: Throwable) {
+                tmp.delete()
+                throw e
+            }
+            // Same-directory rename is atomic on POSIX/Android and replaces the destination.
+            if (!tmp.renameTo(dest)) {
+                try {
+                    tmp.copyTo(dest, overwrite = true)
+                } catch (e: Throwable) {
+                    tmp.delete()
+                    throw e
+                }
+                tmp.delete()
+            }
        }

        override fun delete(relativePath: String): Boolean = File(root, relativePath).delete()
@@ -131,7 +160,7 @@ sealed class LocalAccessor {
            return resolver.openInputStream(docUri)
        }

-        override fun createOutputStream(relativePath: String): OutputStream? {
+        override suspend fun writeAtomically(relativePath: String, write: suspend (OutputStream) -> Unit) {
            val parts = relativePath.replace('\\', '/').split('/')
            var currentId = DocumentsContract.getTreeDocumentId(treeUri)

@@ -141,7 +170,7 @@ sealed class LocalAccessor {
                    val parentUri = DocumentsContract.buildDocumentUriUsingTree(treeUri, currentId)
                    val newDir = DocumentsContract.createDocument(
                        resolver, parentUri, DocumentsContract.Document.MIME_TYPE_DIR, parts[i]
-                    ) ?: return null
+                    ) ?: throw IOException("Cannot create directory ${parts[i]} for $relativePath")
                    DocumentsContract.getDocumentId(newDir)
                }
            }
@@ -149,19 +178,47 @@ sealed class LocalAccessor {
            val fileName = parts.last()
            val childrenUri = DocumentsContract.buildChildDocumentsUriUsingTree(treeUri, currentId)
            val parentUri = DocumentsContract.buildDocumentUriUsingTree(treeUri, currentId)
+            val tmpName = ".$fileName.sfpart"

-            // Delete existing to allow overwrite
-            findChildId(childrenUri, fileName)?.let { existingId ->
+            // Clear any leftover temp document from a previously interrupted write.
+            findChildId(childrenUri, tmpName)?.let { staleId ->
+                runCatching {
                    DocumentsContract.deleteDocument(
-                    resolver,
-                    DocumentsContract.buildDocumentUriUsingTree(treeUri, existingId)
+                        resolver, DocumentsContract.buildDocumentUriUsingTree(treeUri, staleId)
                    )
                }
+            }

-            val newUri = DocumentsContract.createDocument(
-                resolver, parentUri, "application/octet-stream", fileName
-            ) ?: return null
-            return resolver.openOutputStream(newUri)
+            val tmpUri = DocumentsContract.createDocument(
+                resolver, parentUri, "application/octet-stream", tmpName
+            ) ?: throw IOException("Cannot create temp document for $relativePath")
+
+            try {
+                (resolver.openOutputStream(tmpUri)
+                    ?: throw IOException("Cannot open temp stream for $relativePath")).use { os ->
+                    write(os)
+                    os.flush()
+                }
+            } catch (e: Throwable) {
+                runCatching { DocumentsContract.deleteDocument(resolver, tmpUri) }
+                throw e
+            }
+
+            // Commit: remove the existing destination, then rename the fully-written temp into
+            // place. If interrupted between the two steps the temp still holds the complete data
+            // (recoverable by hand), which is strictly safer than truncating the destination.
+            findChildId(childrenUri, fileName)?.let { existingId ->
+                DocumentsContract.deleteDocument(
+                    resolver, DocumentsContract.buildDocumentUriUsingTree(treeUri, existingId)
+                )
+            }
+            val renamed = DocumentsContract.renameDocument(resolver, tmpUri, fileName)
+            if (renamed == null) {
+                runCatching { DocumentsContract.deleteDocument(resolver, tmpUri) }
+                throw IOException("Cannot finalize $relativePath")
+            }
+            // Drop the stale cache entry so the next read re-resolves the new document id.
+            docIdCache.remove(relativePath)
        }

        override fun delete(relativePath: String): Boolean {
@@ -111,6 +111,14 @@ class SyncEngine @Inject constructor(
            allPaths.map { rel ->
                async {
                    semaphore.withPermit {
+                        // Defense-in-depth against a malicious/compromised remote returning a
+                        // path that escapes the sync root (e.g. "../../evil"). Skip rather than
+                        // write outside pair.localPath / pair.remotePath.
+                        if (isUnsafeSyncPath(rel)) {
+                            Timber.w("SyncEngine: skipping unsafe path for pair ${pair.id}: $rel")
+                            logEvent(pair.id, SyncEventType.FILE_SKIPPED, rel, "unsafe path", 0)
+                            return@withPermit FileOutcome(skipped = 1)
+                        }
                        val local = localFiles[rel]
                        val remote = remoteFiles[rel]
                        val known = knownStates[rel]
@@ -142,8 +150,8 @@ class SyncEngine @Inject constructor(
                            }
                            SyncDecision.DOWNLOAD -> {
                                val bytes = runCatching {
-                                    accessor.createOutputStream(rel)?.use { stream ->
-                                        provider.downloadFile("${pair.remotePath}/$rel", stream) { }
+                                    accessor.writeAtomically(rel) { stream ->
+                                        provider.downloadFile("${pair.remotePath}/$rel", stream) { }.getOrThrow()
                                    }
                                    remote!!.sizeBytes
                                }.getOrElse { e ->
@@ -178,6 +186,10 @@ class SyncEngine @Inject constructor(
                                if (pair.deleteBehavior == DeleteBehavior.ARCHIVE) {
                                    val archivePath = "${pair.remotePath}/_Deleted/$rel"
                                    runCatching {
+                                        // Create the _Deleted base itself first — ensureRemoteDirs only
+                                        // makes sub-parents of rel, so for a top-level file the MOVE
+                                        // would otherwise fail with a missing-parent error.
+                                        provider.createDirectory("${pair.remotePath}/_Deleted")
                                        ensureRemoteDirs(provider, "${pair.remotePath}/_Deleted", rel)
                                        provider.moveFile("${pair.remotePath}/$rel", archivePath).getOrThrow()
                                    }.onFailure { e -> Timber.e(e, "SyncEngine: ARCHIVE failed for $rel") }
@@ -363,6 +375,19 @@ internal fun syncDecide(

 enum class SyncDecision { UPLOAD, DOWNLOAD, DELETE_LOCAL, DELETE_REMOTE, CONFLICT, SKIP }

+/**
+ * True if a relative sync path is unsafe to act on — empty, absolute, or containing a ".."
+ * segment that would let a hostile remote escape the sync root via path traversal. Applied to
+ * every path before any file operation as defense-in-depth (WebDAV already filters names at the
+ * parser; SFTP and any future provider are covered here).
+ */
+internal fun isUnsafeSyncPath(rel: String): Boolean {
+    if (rel.isBlank()) return true
+    val normalized = rel.replace('\\', '/')
+    if (normalized.startsWith("/")) return true
+    return normalized.split('/').any { it == ".." }
+}
+
 data class SyncResult(
    val uploaded: Int = 0,
    val downloaded: Int = 0,
@@ -5,6 +5,8 @@ import androidx.activity.compose.rememberLauncherForActivityResult
 import androidx.activity.result.contract.ActivityResultContracts
 import androidx.compose.animation.AnimatedVisibility
 import androidx.compose.foundation.clickable
+import androidx.compose.foundation.selection.selectable
+import androidx.compose.ui.semantics.Role
 import androidx.compose.foundation.layout.*
 import androidx.compose.foundation.rememberScrollState
 import androidx.compose.foundation.text.KeyboardOptions
@@ -163,7 +165,7 @@ fun AddPairScreen(onDone: () -> Unit, vm: AddPairViewModel = hiltViewModel()) {
                    label = "Direction",
                    options = SyncDirection.entries,
                    selected = s.syncDirection,
-                    onSelect = { vm.update { copy(syncDirection = it) } },
+                    onSelect = { vm.setDirection(it) },
                    itemLabel = { "${it.label}  —  ${it.description}" },
                )
                Spacer(Modifier.height(8.dp))
@@ -179,7 +181,7 @@ fun AddPairScreen(onDone: () -> Unit, vm: AddPairViewModel = hiltViewModel()) {
                    label = "Deletion behaviour",
                    options = DeleteBehavior.entries,
                    selected = s.deleteBehavior,
-                    onSelect = { vm.update { copy(deleteBehavior = it) } },
+                    onSelect = { vm.setDeleteBehavior(it) },
                    itemLabel = { "${it.label}  —  ${it.description}" },
                )
            }
@@ -382,10 +384,17 @@ private fun <T> RadioGroup(
        }
        options.forEach { option ->
            Row(
-                modifier = Modifier.fillMaxWidth(),
+                modifier = Modifier
+                    .fillMaxWidth()
+                    .selectable(
+                        selected = option == selected,
+                        role = Role.RadioButton,
+                        onClick = { onSelect(option) },
+                    ),
                verticalAlignment = Alignment.CenterVertically,
            ) {
-                RadioButton(selected = option == selected, onClick = { onSelect(option) })
+                // onClick = null: the whole row handles selection (bigger tap target + a11y).
+                RadioButton(selected = option == selected, onClick = null)
                Text(itemLabel(option), style = MaterialTheme.typography.bodyMedium)
            }
        }
@@ -28,7 +28,10 @@ data class AddPairUiState(
    // ── Sync type ────────────────────────────────────────────────────────────
    val syncDirection: SyncDirection = SyncDirection.TWO_WAY,
    val conflictStrategy: ConflictStrategy = ConflictStrategy.KEEP_NEWEST,
-    val deleteBehavior: DeleteBehavior = DeleteBehavior.MIRROR,
+    val deleteBehavior: DeleteBehavior = recommendedDeleteBehavior(SyncDirection.TWO_WAY),
+    // True once the user explicitly picks a deletion behaviour, so changing direction stops
+    // auto-overriding their choice.
+    val deleteBehaviorTouched: Boolean = false,
    val recursive: Boolean = true,
    // ── Schedule ─────────────────────────────────────────────────────────────
    val scheduleType: ScheduleType = ScheduleType.INTERVAL,
@@ -56,6 +59,16 @@ data class AddPairUiState(
    val done: Boolean = false,
 )

+/**
+ * Safe default deletion behaviour for a given direction. One-way backups must NOT propagate a
+ * local deletion to the cloud (the whole point of a backup), so they default to KEEP; two-way
+ * sync defaults to MIRROR. The user can always override — all three options stay selectable.
+ */
+internal fun recommendedDeleteBehavior(direction: SyncDirection): DeleteBehavior = when (direction) {
+    SyncDirection.UPLOAD_ONLY, SyncDirection.DOWNLOAD_ONLY -> DeleteBehavior.KEEP
+    SyncDirection.TWO_WAY -> DeleteBehavior.MIRROR
+}
+
@HiltViewModel
 class AddPairViewModel @Inject constructor(
    private val syncPairDao: SyncPairDao,
@@ -93,6 +106,7 @@ class AddPairViewModel @Inject constructor(
                            syncDirection = pair.syncDirection,
                            conflictStrategy = pair.conflictStrategy,
                            deleteBehavior = pair.deleteBehavior,
+                            deleteBehaviorTouched = true, // preserve the saved choice when editing
                            recursive = pair.recursive,
                            scheduleType = pair.scheduleType,
                            intervalMinutes = pair.scheduleIntervalMinutes,
@@ -119,6 +133,18 @@ class AddPairViewModel @Inject constructor(

    fun update(transform: AddPairUiState.() -> AddPairUiState) = _state.update(transform)

+    /** Changing direction re-applies the safe deletion default unless the user already chose one. */
+    fun setDirection(direction: SyncDirection) = _state.update { s ->
+        s.copy(
+            syncDirection = direction,
+            deleteBehavior = if (s.deleteBehaviorTouched) s.deleteBehavior else recommendedDeleteBehavior(direction),
+        )
+    }
+
+    fun setDeleteBehavior(behavior: DeleteBehavior) = _state.update {
+        it.copy(deleteBehavior = behavior, deleteBehaviorTouched = true)
+    }
+
    fun save() {
        val s = _state.value
        val errors = buildList {
@@ -0,0 +1,36 @@
+package com.syncflow.domain.sync
+
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+/**
+ * Path-traversal guard: a hostile/compromised remote must not be able to make the engine read
+ * or write outside the sync root via "..", absolute, or separator-smuggled paths.
+ */
+class PathSafetyTest {
+
+    @Test fun `normal relative paths are allowed`() {
+        assertFalse(isUnsafeSyncPath("photo.jpg"))
+        assertFalse(isUnsafeSyncPath("sub/dir/photo.jpg"))
+        assertFalse(isUnsafeSyncPath("a.b..c/file.txt")) // ".." only inside a name, not a segment
+    }
+
+    @Test fun `parent-dir traversal is rejected`() {
+        assertTrue(isUnsafeSyncPath(".."))
+        assertTrue(isUnsafeSyncPath("../evil"))
+        assertTrue(isUnsafeSyncPath("a/../../etc/passwd"))
+        assertTrue(isUnsafeSyncPath("sub/../../escape"))
+    }
+
+    @Test fun `backslash traversal is rejected`() {
+        assertTrue(isUnsafeSyncPath("..\\evil"))
+        assertTrue(isUnsafeSyncPath("a\\..\\..\\escape"))
+    }
+
+    @Test fun `absolute and empty paths are rejected`() {
+        assertTrue(isUnsafeSyncPath("/etc/passwd"))
+        assertTrue(isUnsafeSyncPath(""))
+        assertTrue(isUnsafeSyncPath("   "))
+    }
+}
@@ -91,10 +91,19 @@ class SyncDecideTest {
            decide(local(ts), remote(ts, etag = "e"), state(localMs = ts, remoteMs = ts, etag = "e")))
    }

-    @Test fun `1ms difference detected as local change`() {
+    @Test fun `sub-second mtime difference treated as unchanged`() {
+        // Second-precision comparison is intentional: FAT32 has 2s mtime resolution and WebDAV
+        // 1s, so sub-second deltas are phantom changes that caused rewrite loops. A 1ms diff
+        // within the same second must NOT be treated as a change.
+        val ts = 1_716_393_136_789L
+        assertEquals(SyncDecision.SKIP,
+            decide(local(ts + 1), remote(ts, etag = "e"), state(localMs = ts, remoteMs = ts, etag = "e")))
+    }
+
+    @Test fun `mtime change of a full second detected as local change`() {
        val ts = 1_716_393_136_789L
        assertEquals(SyncDecision.UPLOAD,
-            decide(local(ts + 1), remote(ts, etag = "e"), state(localMs = ts, remoteMs = ts, etag = "e")))
+            decide(local(ts + 1000), remote(ts, etag = "e"), state(localMs = ts, remoteMs = ts, etag = "e")))
    }

    @Test fun `epoch-second stored value differs from millis comparison`() {
@@ -127,12 +136,14 @@ class SyncDecideTest {
        assertEquals(SyncDecision.SKIP,
            decide(null, remote(), state(), dir = SyncDirection.DOWNLOAD_ONLY))

-    // ── local deleted, no state record (uploaded in broken version) ──────────
+    // ── remote exists, no state record: never delete on ambiguity ────────────

-    @Test fun `local deleted no known state but pair has prior history deletes remote`() =
-        // hasPriorState=true means the pair has been synced before; file has no state
-        // because it was uploaded when getFileMetadata was broken. Should still mirror deletion.
-        assertEquals(SyncDecision.DELETE_REMOTE,
+    @Test fun `remote exists with no state record downloads rather than deleting`() =
+        // known=null can mean a brand-new remote file OR one whose state was lost. The engine
+        // cannot tell them apart, so it downloads rather than risk deleting a real file —
+        // worst case is a re-downloaded file, never a lost one. A file the user genuinely
+        // deleted locally still has its state record, which routes to DELETE_REMOTE.
+        assertEquals(SyncDecision.DOWNLOAD,
            decide(null, remote(), known = null, delete = DeleteBehavior.MIRROR, hasPriorState = true))

    @Test fun `initial sync remote only no prior state downloads`() =
@@ -0,0 +1,117 @@
+package com.syncflow.domain.sync
+
+import com.syncflow.data.db.entities.SyncFileStateEntity
+import com.syncflow.domain.model.ConflictStrategy
+import com.syncflow.domain.model.DeleteBehavior
+import com.syncflow.domain.model.RemoteFile
+import com.syncflow.domain.model.SyncDirection
+import org.junit.Assert.assertEquals
+import org.junit.Test
+import java.time.Instant
+
+/**
+ * End-to-end decision lifecycle for the backup scenario:
+ *
+ *   "I back up my phone to the cloud, then I delete the file on the phone.
+ *    It must stay in the cloud."
+ *
+ * These walk the exact multi-cycle state the SyncEngine produces:
+ *   - a successful UPLOAD saves state with the local mtime known and remote metadata null
+ *     (SyncEngine.buildState(..., remoteAfterTransfer = null)),
+ *   - the next sync sees both sides present and unchanged, returns SKIP, and the SKIP branch
+ *     reconciles the record by filling in the remote metadata,
+ *   - then the local file is deleted and we assert what happens to the cloud copy.
+ *
+ * The decision is driven entirely by deleteBehavior, so each terminal case is asserted for
+ * KEEP, MIRROR, and ARCHIVE.
+ */
+class UploadBackupLifecycleTest {
+
+    private val T0 = 1_716_393_136_000L // exact second boundary
+
+    private fun local(ms: Long = T0, size: Long = 100L) =
+        LocalFileInfo("photo.jpg", size, ms)
+
+    private fun remote(ms: Long = T0, etag: String? = "etag1", size: Long = 100L) =
+        RemoteFile("backup/photo.jpg", "photo.jpg", false, size, Instant.ofEpochMilli(ms), etag, null)
+
+    /** Mirrors SyncEngine.buildState right after a successful UPLOAD: remote metadata still null. */
+    private fun stateAfterUpload(ms: Long = T0) = SyncFileStateEntity(
+        syncPairId = 1L, relativePath = "photo.jpg",
+        localModifiedAt = Instant.ofEpochMilli(ms), localSizeBytes = 100L, localHash = null,
+        remoteModifiedAt = null, remoteSizeBytes = 0L, remoteEtag = null,
+        lastSyncedAt = Instant.now(), syncedHash = null,
+    )
+
+    /** Mirrors the record after the next sync's SKIP reconciliation fills in remote metadata. */
+    private fun stateReconciled(ms: Long = T0, etag: String? = "etag1") = SyncFileStateEntity(
+        syncPairId = 1L, relativePath = "photo.jpg",
+        localModifiedAt = Instant.ofEpochMilli(ms), localSizeBytes = 100L, localHash = null,
+        remoteModifiedAt = Instant.ofEpochMilli(ms), remoteSizeBytes = 100L, remoteEtag = etag,
+        lastSyncedAt = Instant.now(), syncedHash = null,
+    )
+
+    private fun decide(
+        local: LocalFileInfo?,
+        remote: RemoteFile?,
+        known: SyncFileStateEntity?,
+        delete: DeleteBehavior,
+    ) = syncDecide(
+        SyncDirection.UPLOAD_ONLY, ConflictStrategy.KEEP_NEWEST, delete,
+        local, remote, known, hasPriorSyncState = known != null,
+    )
+
+    // ── Cycle 1: first backup uploads the file ───────────────────────────────
+
+    @Test fun `cycle 1 - first backup uploads regardless of delete behavior`() {
+        assertEquals(SyncDecision.UPLOAD, decide(local(), null, null, DeleteBehavior.KEEP))
+        assertEquals(SyncDecision.UPLOAD, decide(local(), null, null, DeleteBehavior.MIRROR))
+        assertEquals(SyncDecision.UPLOAD, decide(local(), null, null, DeleteBehavior.ARCHIVE))
+    }
+
+    // ── Cycle 2: file present on both sides, unchanged -> SKIP (no deletion) ──
+
+    @Test fun `cycle 2 - unchanged file skips right after upload (remote metadata still null)`() {
+        assertEquals(SyncDecision.SKIP, decide(local(), remote(), stateAfterUpload(), DeleteBehavior.KEEP))
+        assertEquals(SyncDecision.SKIP, decide(local(), remote(), stateAfterUpload(), DeleteBehavior.MIRROR))
+    }
+
+    @Test fun `cycle 2 - unchanged file skips once state is reconciled`() {
+        assertEquals(SyncDecision.SKIP, decide(local(), remote(), stateReconciled(), DeleteBehavior.KEEP))
+        assertEquals(SyncDecision.SKIP, decide(local(), remote(), stateReconciled(), DeleteBehavior.MIRROR))
+    }
+
+    // ── Cycle 3: deleted on the phone — THE scenario ─────────────────────────
+
+    @Test fun `KEEP - deleting on phone leaves the cloud copy (correct backup behavior)`() {
+        // Reconciled steady state:
+        assertEquals(SyncDecision.SKIP, decide(null, remote(), stateReconciled(), DeleteBehavior.KEEP))
+        // And even if deletion happens before the reconcile pass ran:
+        assertEquals(SyncDecision.SKIP, decide(null, remote(), stateAfterUpload(), DeleteBehavior.KEEP))
+    }
+
+    @Test fun `MIRROR - deleting on phone DELETES the cloud copy (wrong for a backup)`() {
+        assertEquals(SyncDecision.DELETE_REMOTE, decide(null, remote(), stateReconciled(), DeleteBehavior.MIRROR))
+        assertEquals(SyncDecision.DELETE_REMOTE, decide(null, remote(), stateAfterUpload(), DeleteBehavior.MIRROR))
+    }
+
+    @Test fun `ARCHIVE - deleting on phone moves the cloud copy to _Deleted (preserved)`() {
+        // syncDecide returns DELETE_REMOTE; the engine's DELETE_REMOTE branch MOVEs the file to
+        // <remote>/_Deleted/ instead of removing it when deleteBehavior == ARCHIVE.
+        assertEquals(SyncDecision.DELETE_REMOTE, decide(null, remote(), stateReconciled(), DeleteBehavior.ARCHIVE))
+    }
+
+    // ── After deletion: a brand-new remote file is NOT pulled down (upload-only) ─
+
+    @Test fun `KEEP - a new remote file never comes down to the phone (upload-only)`() {
+        // Remote-only, no state record: in upload-only this must SKIP, not DOWNLOAD.
+        assertEquals(SyncDecision.SKIP, decide(null, remote(), null, DeleteBehavior.KEEP))
+    }
+
+    // ── Re-adding / changing a file after a KEEP deletion still uploads ───────
+
+    @Test fun `KEEP - modifying the file locally still uploads the change`() {
+        val newer = local(T0 + 5_000)
+        assertEquals(SyncDecision.UPLOAD, decide(newer, remote(), stateReconciled(), DeleteBehavior.KEEP))
+    }
+}
@@ -0,0 +1,23 @@
+package com.syncflow.ui.addpair
+
+import com.syncflow.domain.model.DeleteBehavior
+import com.syncflow.domain.model.SyncDirection
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+/**
+ * The Add-Pair screen's default deletion behaviour must never wipe a backup. One-way directions
+ * default to KEEP so deleting a file on the phone leaves the cloud copy intact; two-way defaults
+ * to MIRROR. (The user can still override to any of the three options.)
+ */
+class RecommendedDeleteBehaviorTest {
+
+    @Test fun `upload-only defaults to KEEP so backups are never deleted`() =
+        assertEquals(DeleteBehavior.KEEP, recommendedDeleteBehavior(SyncDirection.UPLOAD_ONLY))
+
+    @Test fun `download-only defaults to KEEP`() =
+        assertEquals(DeleteBehavior.KEEP, recommendedDeleteBehavior(SyncDirection.DOWNLOAD_ONLY))
+
+    @Test fun `two-way defaults to MIRROR`() =
+        assertEquals(DeleteBehavior.MIRROR, recommendedDeleteBehavior(SyncDirection.TWO_WAY))
+}