diff --git a/Audit.md b/Audit.md index 0204e0b..977a2c7 100644 --- a/Audit.md +++ b/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: diff --git a/crates/onyx-core/src/storage.rs b/crates/onyx-core/src/storage.rs index 380e757..7f8bc8b 100644 --- a/crates/onyx-core/src/storage.rs +++ b/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 { diff --git a/crates/onyx-core/src/sync.rs b/crates/onyx-core/src/sync.rs index 6eb2b57..a21cf9d 100644 --- a/crates/onyx-core/src/sync.rs +++ b/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![