Replace timestamp LWW with checksum-based conflict resolution

Rework sync conflict handling to stop using timestamp-based
last-write-wins and use a single Conflict action. The conflict handler
now downloads the remote file and compares SHA-256 checksums: identical
content is treated as a false conflict and skipped; when different, the
remote version wins and the local task file is recovered as a duplicate
with a new UUID and a "[RECOVERED FROM CONFLICT]" prefix. Duplicates are
inserted adjacent to the original in .listdata.json; non-task files
still use remote-wins without duplication. Removed local_wins() and its
tests, simplified action variants to a single Conflict, and updated
conflict-related tests and reporting accordingly.

crates/onyx-core/src/sync.rs

@@ -3,7 +3,9 @@ use std::path::Path;
 use chrono::{DateTime, Utc};
 use serde::{Deserialize, Serialize};
 use sha2::{Sha256, Digest};
+use uuid::Uuid;
 use crate::error::{Error, Result};
+use crate::storage::{ListMetadata, TaskFrontmatter};
 use crate::webdav::WebDavClient;
 
 // --- Sync State ---
@@ -32,8 +34,7 @@ pub enum SyncAction {
     Download { path: String },
     DeleteLocal { path: String },
     DeleteRemote { path: String },
-    ConflictLocalWins { path: String },
+    Conflict { path: String },
-    ConflictRemoteWins { path: String },
 }
 
 impl SyncAction {
@@ -43,8 +44,7 @@ impl SyncAction {
             | SyncAction::Download { path }
             | SyncAction::DeleteLocal { path }
             | SyncAction::DeleteRemote { path }
-            | SyncAction::ConflictLocalWins { path }
+            | SyncAction::Conflict { path } => path,
-            | SyncAction::ConflictRemoteWins { path } => path,
         }
     }
 }
@@ -137,12 +137,7 @@ pub fn compute_sync_actions(
                     (true, false) => actions.push(SyncAction::Upload { path: path.to_string() }),
                     (false, true) => actions.push(SyncAction::Download { path: path.to_string() }),
                     (true, true) => {
-                        // Both modified: last-write-wins based on timestamps
+                        actions.push(SyncAction::Conflict { path: path.to_string() });
-                        if local_wins(l.modified_at.as_deref(), r.last_modified.as_deref()) {
-                            actions.push(SyncAction::ConflictLocalWins { path: path.to_string() });
-                        } else {
-                            actions.push(SyncAction::ConflictRemoteWins { path: path.to_string() });
-                        }
                     }
                 }
             }
@@ -157,13 +152,9 @@ pub fn compute_sync_actions(
                 actions.push(SyncAction::Download { path: path.to_string() });
             }
 
-            // Both present, no base (both added): last-write-wins
+            // Both present, no base (both added): conflict
-            (Some(l), Some(r), None) => {
+            (Some(_), Some(_), None) => {
-                if local_wins(l.modified_at.as_deref(), r.last_modified.as_deref()) {
+                actions.push(SyncAction::Conflict { path: path.to_string() });
-                    actions.push(SyncAction::ConflictLocalWins { path: path.to_string() });
-                } else {
-                    actions.push(SyncAction::ConflictRemoteWins { path: path.to_string() });
-                }
             }
 
             // Local present, remote gone, base known: remote was deleted
@@ -221,19 +212,6 @@ fn timestamps_equal(a: Option<&str>, b: Option<&str>) -> bool {
     }
 }
 
-/// Determine if local wins based on timestamps. True means local wins.
-fn local_wins(local_modified: Option<&str>, remote_modified: Option<&str>) -> bool {
-    // Try parsing both; if we can't parse, local wins by default
-    let local_ts = local_modified.and_then(parse_timestamp);
-    let remote_ts = remote_modified.and_then(parse_timestamp);
-    match (local_ts, remote_ts) {
-        (Some(l), Some(r)) => l >= r,
-        (Some(_), None) => true,
-        (None, Some(_)) => false,
-        (None, None) => true, // Default to local
-    }
-}
-
 /// Parse a timestamp string (ISO 8601 or HTTP date format).
 fn parse_timestamp(s: &str) -> Option<DateTime<Utc>> {
     // Try ISO 8601 / RFC 3339
@@ -342,8 +320,7 @@ fn queued_op_to_action(op: &QueuedOperation) -> Option<SyncAction> {
         "download" => Some(SyncAction::Download { path }),
         "delete_local" => Some(SyncAction::DeleteLocal { path }),
         "delete_remote" => Some(SyncAction::DeleteRemote { path }),
-        "conflict_local_wins" => Some(SyncAction::ConflictLocalWins { path }),
+        "conflict" => Some(SyncAction::Conflict { path }),
-        "conflict_remote_wins" => Some(SyncAction::ConflictRemoteWins { path }),
         _ => None,
     }
 }
@@ -354,8 +331,7 @@ fn action_to_queued_op(action: &SyncAction) -> QueuedOperation {
         SyncAction::Download { path } => ("download", path),
         SyncAction::DeleteLocal { path } => ("delete_local", path),
         SyncAction::DeleteRemote { path } => ("delete_remote", path),
-        SyncAction::ConflictLocalWins { path } => ("conflict_local_wins", path),
+        SyncAction::Conflict { path } => ("conflict", path),
-        SyncAction::ConflictRemoteWins { path } => ("conflict_remote_wins", path),
     };
     QueuedOperation {
         action_type: action_type.to_string(),
@@ -567,11 +543,11 @@ async fn sync_workspace_inner(
     // Merge with offline queue
     let all_actions = queue.merge_with_actions(fresh_actions);
 
-    // Filter by sync mode
+    // Filter by sync mode (conflicts always run in any mode since they need both sides)
     let actions: Vec<SyncAction> = all_actions.into_iter().filter(|a| match mode {
         SyncMode::Full => true,
-        SyncMode::Push => matches!(a, SyncAction::Upload { .. } | SyncAction::DeleteRemote { .. } | SyncAction::ConflictLocalWins { .. }),
+        SyncMode::Push => matches!(a, SyncAction::Upload { .. } | SyncAction::DeleteRemote { .. } | SyncAction::Conflict { .. }),
-        SyncMode::Pull => matches!(a, SyncAction::Download { .. } | SyncAction::DeleteLocal { .. } | SyncAction::ConflictRemoteWins { .. }),
+        SyncMode::Pull => matches!(a, SyncAction::Download { .. } | SyncAction::DeleteLocal { .. } | SyncAction::Conflict { .. }),
     }).collect();
 
     // Execute actions, collecting failures for the queue
@@ -581,22 +557,17 @@ async fn sync_workspace_inner(
         match execute_action(&client, workspace_path, action, &mut sync_state, &report).await {
             Ok(()) => {
                 match action {
-                    SyncAction::Upload { .. } | SyncAction::ConflictLocalWins { .. } => result.uploaded += 1,
+                    SyncAction::Upload { .. } => result.uploaded += 1,
-                    SyncAction::Download { .. } | SyncAction::ConflictRemoteWins { .. } => result.downloaded += 1,
+                    SyncAction::Download { .. } => result.downloaded += 1,
                     SyncAction::DeleteLocal { .. } => result.deleted_local += 1,
                     SyncAction::DeleteRemote { .. } => result.deleted_remote += 1,
+                    SyncAction::Conflict { .. } => result.conflicts += 1,
                 }
             }
             Err(e) => {
                 let msg = format!("Failed {}: {}", action.path(), e);
                 report(&format!("  ! {}", msg));
                 result.errors.push(msg);
-                if matches!(action,
-                    SyncAction::Upload { .. } | SyncAction::Download { .. }
-                    | SyncAction::ConflictLocalWins { .. } | SyncAction::ConflictRemoteWins { .. }
-                ) {
-                    result.conflicts += 1;
-                }
                 failed_actions.push(action.clone());
             }
         }
@@ -643,22 +614,80 @@ async fn execute_action(
             sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64);
         }
 
-        SyncAction::ConflictLocalWins { path } => {
+        SyncAction::Conflict { path } => {
             let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR));
-            let data = std::fs::read(&local_path)?;
+            let local_data = std::fs::read(&local_path)?;
-            let checksum = compute_checksum(&data);
+            let local_checksum = compute_checksum(&local_data);
 
-            if let Some(parent) = path_parent(path) {
+            let remote_data = client.get_file(path).await?;
-                client.ensure_dir(parent).await?;
+            let remote_checksum = compute_checksum(&remote_data);
-            }
-
-            report(&format!("  ^ Conflict: uploading local version of {}", path));
-            client.put_file(path, data.clone()).await?;
 
+            // If checksums match, it's a false conflict — both sides made the same edit
+            if local_checksum == remote_checksum {
+                report(&format!("  = Conflict resolved: identical content for {}", path));
                 let modified = std::fs::metadata(&local_path).ok()
                     .and_then(|m| m.modified().ok())
                     .map(|t| { let dt: DateTime<Utc> = t.into(); dt.to_rfc3339() });
-            sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64);
+                sync_state.record_file(path, &local_checksum, modified.as_deref(), local_data.len() as u64);
+            } else {
+                report(&format!("  ! Conflict: remote wins for {}, recovering local as duplicate", path));
+
+                // Remote wins: overwrite local with remote content
+                std::fs::write(&local_path, &remote_data)?;
+                let modified = std::fs::metadata(&local_path).ok()
+                    .and_then(|m| m.modified().ok())
+                    .map(|t| { let dt: DateTime<Utc> = t.into(); dt.to_rfc3339() });
+                sync_state.record_file(path, &remote_checksum, modified.as_deref(), remote_data.len() as u64);
+
+                // For .md task files inside a list dir, create a duplicate of the local version
+                let parts: Vec<&str> = path.split('/').collect();
+                if parts.len() == 2 && parts[1].ends_with(".md") && parts[1] != ".listdata.json" {
+                    let local_content = String::from_utf8_lossy(&local_data);
+                    if let Ok((frontmatter, description)) = parse_frontmatter_for_conflict(&local_content) {
+                        let original_id = frontmatter.id;
+                        let new_id = Uuid::new_v4();
+                        let prefixed_desc = if description.is_empty() {
+                            "[RECOVERED FROM CONFLICT]".to_string()
+                        } else {
+                            format!("[RECOVERED FROM CONFLICT]\n{}", description)
+                        };
+
+                        let new_frontmatter = TaskFrontmatter {
+                            id: new_id,
+                            ..frontmatter
+                        };
+                        let yaml = serde_yaml::to_string(&new_frontmatter)
+                            .map_err(|e| Error::Sync(e.to_string()))?;
+                        let new_content = format!("---\n{}---\n\n{}", yaml, prefixed_desc);
+
+                        // Write the duplicate file using the new UUID as filename
+                        let list_dir = workspace_path.join(parts[0]);
+                        let dup_filename = format!("{}.md", new_id);
+                        let dup_path = list_dir.join(&dup_filename);
+                        std::fs::write(&dup_path, &new_content)?;
+
+                        // Insert new task adjacent to original in .listdata.json
+                        let listdata_path = list_dir.join(".listdata.json");
+                        if listdata_path.exists() {
+                            if let Ok(content) = std::fs::read_to_string(&listdata_path) {
+                                if let Ok(mut metadata) = serde_json::from_str::<ListMetadata>(&content) {
+                                    let insert_pos = metadata.task_order.iter()
+                                        .position(|id| *id == original_id)
+                                        .map(|p| p + 1)
+                                        .unwrap_or(metadata.task_order.len());
+                                    metadata.task_order.insert(insert_pos, new_id);
+                                    if let Ok(json) = serde_json::to_string_pretty(&metadata) {
+                                        let _ = std::fs::write(&listdata_path, json);
+                                    }
+                                }
+                            }
+                        }
+
+                        // Don't record the duplicate in sync state — next sync will see it
+                        // as "local added, remote absent" and upload it automatically.
+                    }
+                }
+            }
         }
 
         SyncAction::Download { path } => {
@@ -679,29 +708,6 @@ async fn execute_action(
             sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64);
         }
 
-        SyncAction::ConflictRemoteWins { path } => {
-            let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR));
-            // Back up local version before overwriting with remote
-            if local_path.exists() {
-                let backup_path = local_path.with_extension("conflict-backup");
-                let _ = std::fs::copy(&local_path, &backup_path);
-                report(&format!("  ! Backed up local version to {}", backup_path.display()));
-            }
-            report(&format!("  v Conflict: downloading remote version of {}", path));
-            let data = client.get_file(path).await?;
-            let checksum = compute_checksum(&data);
-
-            if let Some(parent) = local_path.parent() {
-                std::fs::create_dir_all(parent)?;
-            }
-            std::fs::write(&local_path, &data)?;
-
-            let modified = std::fs::metadata(&local_path).ok()
-                .and_then(|m| m.modified().ok())
-                .map(|t| { let dt: DateTime<Utc> = t.into(); dt.to_rfc3339() });
-            sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64);
-        }
-
         SyncAction::DeleteLocal { path } => {
             report(&format!("  x Deleting local {}", path));
             let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR));
@@ -720,6 +726,25 @@ async fn execute_action(
     Ok(())
 }
 
+/// Parse frontmatter and description from a markdown task file for conflict recovery.
+fn parse_frontmatter_for_conflict(content: &str) -> Result<(TaskFrontmatter, String)> {
+    let lines: Vec<&str> = content.lines().collect();
+    if lines.is_empty() || lines[0] != "---" {
+        return Err(Error::InvalidData("Missing frontmatter delimiter".to_string()));
+    }
+    let end_idx = lines[1..].iter().position(|&line| line == "---")
+        .ok_or_else(|| Error::InvalidData("Missing closing frontmatter delimiter".to_string()))?;
+    let frontmatter_str = lines[1..=end_idx].join("\n");
+    let frontmatter: TaskFrontmatter = serde_yaml::from_str(&frontmatter_str)
+        .map_err(|e| Error::Sync(format!("Failed to parse frontmatter: {}", e)))?;
+    let description = if end_idx + 2 < lines.len() {
+        lines[end_idx + 2..].join("\n").trim().to_string()
+    } else {
+        String::new()
+    };
+    Ok((frontmatter, description))
+}
+
 /// Get the parent path of a sync path (e.g., "My Tasks/file.md" -> "My Tasks").
 fn path_parent(path: &str) -> Option<&str> {
     path.rfind('/').map(|i| &path[..i])
@@ -896,11 +921,9 @@ mod tests {
     }
 
     #[test]
-    fn test_both_modified_local_newer() {
+    fn test_both_modified_emits_conflict() {
-        let mut local = make_local("file.md", "new_local");
+        let local = make_local("file.md", "new_local");
-        local.modified_at = Some("2026-03-15T12:00:00+00:00".to_string());
         let mut remote = make_remote("file.md");
-        remote.last_modified = Some("Mon, 01 Mar 2026 00:00:00 GMT".to_string());
         remote.size = 200;
 
         let mut state = SyncState::default();
@@ -908,23 +931,7 @@ mod tests {
 
         let actions = compute_sync_actions(&[local], &[remote], &state);
         assert_eq!(actions.len(), 1);
-        assert_eq!(actions[0], SyncAction::ConflictLocalWins { path: "file.md".to_string() });
+        assert_eq!(actions[0], SyncAction::Conflict { path: "file.md".to_string() });
-    }
-
-    #[test]
-    fn test_both_modified_remote_newer() {
-        let mut local = make_local("file.md", "new_local");
-        local.modified_at = Some("2026-01-01T00:00:00+00:00".to_string());
-        let mut remote = make_remote("file.md");
-        remote.last_modified = Some("Sun, 15 Mar 2026 12:00:00 GMT".to_string());
-        remote.size = 200;
-
-        let mut state = SyncState::default();
-        state.files.insert("file.md".to_string(), make_base("old_base"));
-
-        let actions = compute_sync_actions(&[local], &[remote], &state);
-        assert_eq!(actions.len(), 1);
-        assert_eq!(actions[0], SyncAction::ConflictRemoteWins { path: "file.md".to_string() });
     }
 
     #[test]
@@ -954,17 +961,15 @@ mod tests {
     }
 
     #[test]
-    fn test_both_added_local_newer() {
+    fn test_both_added_emits_conflict() {
-        let mut local = make_local("file.md", "local_content");
+        let local = make_local("file.md", "local_content");
-        local.modified_at = Some("2026-03-15T12:00:00+00:00".to_string());
+        let remote = make_remote("file.md");
-        let mut remote = make_remote("file.md");
-        remote.last_modified = Some("Mon, 01 Jan 2026 00:00:00 GMT".to_string());
 
         let state = SyncState::default(); // No base entry
 
         let actions = compute_sync_actions(&[local], &[remote], &state);
         assert_eq!(actions.len(), 1);
-        assert_eq!(actions[0], SyncAction::ConflictLocalWins { path: "file.md".to_string() });
+        assert_eq!(actions[0], SyncAction::Conflict { path: "file.md".to_string() });
     }
 
     #[test]
@@ -1203,22 +1208,6 @@ mod tests {
         assert!(result.is_none());
     }
 
-    #[test]
-    fn test_local_wins_local_newer() {
-        assert!(local_wins(
-            Some("2026-03-15T12:00:00+00:00"),
-            Some("Mon, 01 Jan 2026 00:00:00 GMT"),
-        ));
-    }
-
-    #[test]
-    fn test_local_wins_remote_newer() {
-        assert!(!local_wins(
-            Some("2026-01-01T00:00:00+00:00"),
-            Some("Sun, 15 Mar 2026 12:00:00 GMT"),
-        ));
-    }
-
     // --- path_parent ---
 
     #[test]