Merge pull request #60 from SteelDynamite/claude/serene-ride-1mX8o

Audit.md

@@ -1,5 +1,13 @@
 # Audit Log
 
+## 2026-04-24
+
+Found and fixed 3 issues:
+
+1. **Bug: orphan base entries never cleaned from sync state** (sync.rs) — when a file was deleted both locally and remotely, `compute_sync_actions` emitted no action (the `(None, None, Some(_))` arm), so the base entry in `.syncstate.json` persisted forever. On each subsequent sync the same no-op case fired and the state file grew. Added `prune_orphan_bases` pass in `sync_workspace_inner` that drops base entries not present in either scan.
+2. **Code quality: redundant is_some_and on already-matched Option** (sync.rs:208) — the `(None, Some(_), Some(b))` arm re-checked `remote` via `remote.is_some_and(|r| ...)` even though the pattern had just proven `remote` is `Some(_)`. Bound the inner value with `Some(r)` in the pattern and used `r` directly.
+3. **Code quality: single-caller sanitize_filename wrapper** (storage.rs) — `FileSystemStorage::sanitize_filename` was a one-line forwarder to `crate::sanitize_filename` with one call site. Inlined the crate call and removed the method.
+
 ## 2026-04-20
 
 Found and fixed 4 issues:

crates/onyx-core/src/storage.rs

@@ -236,12 +236,8 @@ impl FileSystemStorage {
         Ok(path)
     }
 
-    fn sanitize_filename(name: &str) -> String {
-        crate::sanitize_filename(name)
-    }
-
     fn task_file_path(&self, list_dir: &Path, task: &Task) -> PathBuf {
-        let safe_title = Self::sanitize_filename(&task.title);
+        let safe_title = crate::sanitize_filename(&task.title);
         let filename = if safe_title.is_empty() {
             task.id.to_string()
         } else {

crates/onyx-core/src/sync.rs

@@ -204,8 +204,9 @@ pub fn compute_sync_actions(
             }
 
             // Remote present, local gone, base known: local was deleted
-            (None, Some(_), Some(b)) => {
-                let remote_changed = remote.is_some_and(|r| r.size != b.size || !timestamps_equal(r.last_modified.as_deref(), b.modified_at.as_deref()));
+            (None, Some(r), Some(b)) => {
+                let remote_changed = r.size != b.size
+                    || !timestamps_equal(r.last_modified.as_deref(), b.modified_at.as_deref());
                 if remote_changed {
                     // deleted locally + modified remotely -> download (remote wins)
                     actions.push(SyncAction::Download { path: path.to_string() });
@@ -229,6 +230,22 @@ pub fn compute_sync_actions(
     actions
 }
 
+/// Remove base entries for files that are gone from both local and remote.
+/// `compute_sync_actions` emits no action for the both-deleted case, so without
+/// this pass those entries would persist in `.syncstate.json` indefinitely.
+fn prune_orphan_bases(
+    sync_state: &mut SyncState,
+    local_files: &[LocalFileInfo],
+    remote_files: &[RemoteFileSnapshot],
+) {
+    let live_paths: std::collections::HashSet<&str> = local_files
+        .iter()
+        .map(|f| f.path.as_str())
+        .chain(remote_files.iter().map(|f| f.path.as_str()))
+        .collect();
+    sync_state.files.retain(|p, _| live_paths.contains(p.as_str()));
+}
+
 /// Compare two timestamps for equality by parsing both, tolerating format differences.
 fn timestamps_equal(a: Option<&str>, b: Option<&str>) -> bool {
     match (a, b) {
@@ -604,6 +621,12 @@ async fn sync_workspace_inner(
         }
     };
 
+    // Purge orphan base entries: files we previously tracked that are now gone
+    // from both local and remote. Without this, `.syncstate.json` accumulates
+    // ghost entries forever because the both-deleted diff case emits no action
+    // and so nothing else would clean them.
+    prune_orphan_bases(&mut sync_state, &local_files, &remote_files);
+
     // Compute actions from three-way diff
     let fresh_actions = compute_sync_actions(&local_files, &remote_files, &sync_state);
 
@@ -1106,6 +1129,22 @@ mod tests {
         assert!(actions.is_empty());
     }
 
+    #[test]
+    fn test_prune_orphan_bases() {
+        let mut state = SyncState::default();
+        state.files.insert("kept_local.md".to_string(), make_base("a"));
+        state.files.insert("kept_remote.md".to_string(), make_base("b"));
+        state.files.insert("orphan.md".to_string(), make_base("c"));
+
+        let local = vec![make_local("kept_local.md", "a")];
+        let remote = vec![make_remote("kept_remote.md")];
+        prune_orphan_bases(&mut state, &local, &remote);
+
+        assert!(state.files.contains_key("kept_local.md"));
+        assert!(state.files.contains_key("kept_remote.md"));
+        assert!(!state.files.contains_key("orphan.md"));
+    }
+
     #[test]
     fn test_multiple_files_mixed() {
         let local = vec![