diff --git a/client-tauri/src-tauri/src/sync.rs b/client-tauri/src-tauri/src/sync.rs index d2d3b2d..01c197a 100644 --- a/client-tauri/src-tauri/src/sync.rs +++ b/client-tauri/src-tauri/src/sync.rs @@ -22,7 +22,7 @@ struct SyncStateFile { fn is_ignored(path_str: &str) -> bool { let normalized = path_str.replace('\\', "/"); - if normalized.contains("/.git/") || normalized.starts_with(".git/") { + if normalized.contains("/.git/") || normalized.starts_with(".git/") || normalized == ".git" || normalized.ends_with("/.git") { return true; } if normalized.contains("/.brandybox/") || normalized.starts_with(".brandybox/") || normalized == ".brandybox" { @@ -554,6 +554,7 @@ pub fn run_sync(client: &mut ApiClient, local_root: &Path) -> Result<(u64, u64, }; let skip = prev_downloaded.contains(path); if skip && local_path.exists() && local_path.is_file() { + completed_downloads.insert(path.clone()); done += 1; continue; } @@ -562,6 +563,7 @@ pub fn run_sync(client: &mut ApiClient, local_root: &Path) -> Result<(u64, u64, if let Some(local_hash) = compute_file_hash(&local_path) { if local_hash == *hash { state.file_hashes.insert(path.clone(), hash.clone()); + completed_downloads.insert(path.clone()); done += 1; continue; } @@ -1108,5 +1110,101 @@ mod tests { state.file_hashes.remove("deleted.txt"); assert_eq!(state.file_hashes.len(), 0); } + + #[test] + fn test_is_ignored_bare_git_files() { + assert!(is_ignored(".git")); + assert!(is_ignored("sub/.git")); + assert!(is_ignored("deeply/nested/repo/.git")); + assert!(is_ignored(".git/config")); + assert!(is_ignored("sub/.git/HEAD")); + } + + #[test] + fn test_skipped_download_matching_hash_retained_in_synced_state() { + let temp_dir = std::env::temp_dir().join(format!("test_skip_match_hash_{}", uuid::Uuid::new_v4())); + let _ = std::fs::create_dir_all(&temp_dir); + let file_path = temp_dir.join("existing.txt"); + std::fs::write(&file_path, "identical content").unwrap(); + + let path = "existing.txt".to_string(); + let local_hash = compute_file_hash(&file_path).unwrap(); + let mut remote_hashes = HashMap::new(); + remote_hashes.insert(path.clone(), local_hash.clone()); + + let mut state = SyncStateFile::default(); + let mut completed_downloads = HashSet::new(); + + // Simulate the download loop logic for hash match + if let Some(hash) = remote_hashes.get(&path) { + if file_path.exists() && file_path.is_file() { + if let Some(lh) = compute_file_hash(&file_path) { + if lh == *hash { + state.file_hashes.insert(path.clone(), hash.clone()); + completed_downloads.insert(path.clone()); + } + } + } + } + + assert!(completed_downloads.contains("existing.txt")); + assert_eq!(state.file_hashes.get("existing.txt"), Some(&local_hash)); + + let base_synced = HashSet::new(); + let completed_uploads = HashSet::new(); + let new_synced: HashSet = base_synced + .union(&completed_downloads) + .cloned() + .chain(completed_uploads.iter().cloned()) + .collect(); + state.paths = new_synced.into_iter().collect(); + + assert!(state.paths.contains(&"existing.txt".to_string())); + + // Ensure next cycle does not resurrect if deleted locally + let last_synced: HashSet = state.paths.into_iter().collect(); + let current_local: HashSet = HashSet::new(); // User deleted locally + let to_delete_remote: HashSet = last_synced.difference(¤t_local).cloned().collect(); + assert!( + to_delete_remote.contains("existing.txt"), + "Deleted file must be deleted on server, not resurrected" + ); + + let _ = std::fs::remove_dir_all(&temp_dir); + } + + #[test] + fn test_resumed_download_retained_in_synced_state() { + let temp_dir = std::env::temp_dir().join(format!("test_resumed_download_{}", uuid::Uuid::new_v4())); + let _ = std::fs::create_dir_all(&temp_dir); + let file_path = temp_dir.join("partial.txt"); + std::fs::write(&file_path, "resumed file content").unwrap(); + + let path = "partial.txt".to_string(); + let prev_downloaded: HashSet = [path.clone()].into_iter().collect(); + let mut completed_downloads = HashSet::new(); + + // Simulate resume logic + let skip = prev_downloaded.contains(&path); + if skip && file_path.exists() && file_path.is_file() { + completed_downloads.insert(path.clone()); + } + + assert!(completed_downloads.contains("partial.txt")); + + let base_synced = HashSet::new(); + let completed_uploads = HashSet::new(); + let new_synced: HashSet = base_synced + .union(&completed_downloads) + .cloned() + .chain(completed_uploads.iter().cloned()) + .collect(); + + let mut state = SyncStateFile::default(); + state.paths = new_synced.into_iter().collect(); + assert!(state.paths.contains(&"partial.txt".to_string())); + + let _ = std::fs::remove_dir_all(&temp_dir); + } } diff --git a/openspec/changes/fix-sync-matching-download-state-retention/.openspec.yaml b/openspec/changes/fix-sync-matching-download-state-retention/.openspec.yaml new file mode 100644 index 0000000..b9ebfc7 --- /dev/null +++ b/openspec/changes/fix-sync-matching-download-state-retention/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-10-12 diff --git a/openspec/changes/fix-sync-matching-download-state-retention/design.md b/openspec/changes/fix-sync-matching-download-state-retention/design.md new file mode 100644 index 0000000..982f027 --- /dev/null +++ b/openspec/changes/fix-sync-matching-download-state-retention/design.md @@ -0,0 +1,37 @@ +## Context + +In `client-tauri/src-tauri/src/sync.rs`, the synchronization engine constructs `base_synced` by filtering out all paths scheduled in `to_download_set` and `to_upload_set` (to ensure failed transfers are excluded from synchronized state). The persisted synchronized paths list (`state.paths`) is then constructed by `base_synced.union(completed_downloads).chain(completed_uploads)`. + +In the download execution loop, when a file transfer is skipped because the local file's computed SHA-256 hash already matches the server's hash (`local_hash == *hash`) or because it was completed in an earlier interrupted run (`prev_downloaded`), the loop executes `continue;` without adding `path` to `completed_downloads`. Consequently, the file is dropped from `state.paths` (`sync_state.json`), leading to resurrection on subsequent sync cycles if deleted locally. + +In addition, `is_ignored` checks `.git/` directory prefixes (`contains("/.git/")` and `starts_with(".git/")`) but fails to match bare `.git` files (`.git` or `*/.git`) produced by git submodules and worktrees. + +## Goals / Non-Goals + +**Goals:** +- Ensure files whose content is verified to match the server hash are recorded in `completed_downloads` and persisted in `state.paths`. +- Ensure previously completed downloads in resumed cycles are recorded in `completed_downloads` and persisted in `state.paths`. +- Keep failed or skipped (due to error/unsafe path) transfers excluded from `state.paths`. +- Update `is_ignored` to recognize bare `.git` files at root and within subdirectories. +- Add regression tests covering hash-matched state retention and `.git` path filtering. + +**Non-Goals:** +- Modifying the 3-way hash reconciliation or clock skew resolution logic. +- Changing API network protocols or server storage handling. + +## Decisions + +### Decision 1: Register hash-matched and resumed files into `completed_downloads` +- **Choice**: Call `completed_downloads.insert(path.clone())` before `continue;` in both the `prev_downloaded` and `local_hash == *hash` branches in the download execution loop. +- **Rationale**: An existing local file whose full SHA-256 hash matches the server's remote hash is fully synchronized. Inserting it into `completed_downloads` ensures it is restored to `state.paths` when merged with `base_synced`. +- **Alternatives considered**: Including matching files in `base_synced` during planning: Rejected because local file hashes may not be available or readable at initial scan time; the download loop provides the authoritative point of disk content verification. + +### Decision 2: Match bare `.git` files in `is_ignored` +- **Choice**: Add `|| normalized == ".git" || normalized.ends_with("/.git")` to the `.git` condition in `is_ignored`. +- **Rationale**: Follows the convention already established for `.brandybox` and `.uploads`, correctly ignoring git submodule and worktree pointer files. +- **Alternatives considered**: Adding `.git` to `SYNC_IGNORE`: `SYNC_IGNORE` matches `Path::file_name()`, which would work for filenames, but keeping directory and file exclusions together in the prefix/normalized check maintains explicit consistency. + +## Risks / Trade-offs + +- **[Risk]** Does inserting into `completed_downloads` risk marking a failed download as successful? + - *Mitigation*: No. Downloads that fail (network timeout, HTTP error, permission denied) route to the `Err` branch and `skipped_downloads`, leaving `completed_downloads` untouched. diff --git a/openspec/changes/fix-sync-matching-download-state-retention/proposal.md b/openspec/changes/fix-sync-matching-download-state-retention/proposal.md new file mode 100644 index 0000000..5b06beb --- /dev/null +++ b/openspec/changes/fix-sync-matching-download-state-retention/proposal.md @@ -0,0 +1,21 @@ +## Why + +When a file scheduled in `to_download` already matches the server's content hash on local disk (`local_hash == *hash`) or was completed in a resumed cycle (`prev_downloaded`), the desktop sync engine skips transferring the payload over the network. However, because `to_download` candidates are filtered out of `base_synced` and only paths registered in `completed_downloads` are saved as synchronized, the engine omits these verified files from `state.paths` (`sync_state.json`). On subsequent sync cycles, deleting such a file locally fails to propagate a remote deletion (because `last_synced` lacks the path), causing the engine to instead treat the file as missing locally and re-download it from the server, resurrecting the deleted file. Furthermore, `is_ignored` in `sync.rs` checks only for `.git/` directory prefixes and fails to match bare `.git` files or submodule/worktree files (`.git` and `*/.git`). + +## What Changes + +- In `client-tauri/src-tauri/src/sync.rs`, insert `path` into `completed_downloads` when a download is skipped due to matching content hash (`local_hash == *hash`) or previous download completion (`skip && local_path.exists() && local_path.is_file()`), ensuring hash-verified files are preserved in `state.paths` (`sync_state.json`). +- In `client-tauri/src-tauri/src/sync.rs`, update `is_ignored` to ignore bare `.git` files and subdirectory submodule/worktree `.git` files (`normalized == ".git"` or `normalized.ends_with("/.git")`). +- Add regression unit tests in `sync.rs` asserting that hash-matched downloads are retained in `new_synced` and that `.git` files are ignored. + +## Capabilities + +### Modified Capabilities +- `desktop-client`: Require the desktop client sync engine to retain hash-verified and previously downloaded files in the persistent synchronized state (`sync_state.json`), and ignore `.git` repository and worktree files. + +## Impact + +- **Affected code**: `client-tauri/src-tauri/src/sync.rs` +- **APIs**: No API changes. +- **Dependencies**: No new dependencies. +- **Behavior**: Local deletions of hash-matched files propagate correctly without resurrection; git worktree / submodule `.git` files are properly ignored. diff --git a/openspec/changes/fix-sync-matching-download-state-retention/specs/desktop-client/spec.md b/openspec/changes/fix-sync-matching-download-state-retention/specs/desktop-client/spec.md new file mode 100644 index 0000000..ce23b67 --- /dev/null +++ b/openspec/changes/fix-sync-matching-download-state-retention/specs/desktop-client/spec.md @@ -0,0 +1,19 @@ +## ADDED Requirements + +### Requirement: Retention of Hash-Verified and Resumed Downloads in Synced State +The desktop client synchronization engine SHALL retain files whose content is verified identical to the remote server or whose download was completed in a previous cycle in the persistent synchronized paths list (`sync_state.json`), ensuring they are recorded as synchronized and preventing false download resurrection on subsequent cycles. + +#### Scenario: Download skipped due to matching content hash +- **WHEN** a file scheduled in `to_download` has an existing local file whose computed hash matches the remote server hash +- **THEN** the download payload transfer is skipped, the file hash is recorded in `state.file_hashes`, and the file path is added to `completed_downloads` so that it is preserved in `state.paths` (`sync_state.json`) + +#### Scenario: Download skipped due to previous download completion +- **WHEN** a file scheduled in `to_download` was already downloaded in an interrupted sync cycle and exists on disk +- **THEN** the download is skipped and the file path is added to `completed_downloads` so that it is preserved in `state.paths` (`sync_state.json`) + +### Requirement: Exclusion of Git Repository Metadata and Worktree Files +The desktop client synchronization engine SHALL ignore `.git` directory trees, bare `.git` files, and worktree or submodule pointer files across all directory levels. + +#### Scenario: Bare .git file at sync folder root or subdirectories +- **WHEN** a file named `.git` exists in the sync root or in any subdirectory +- **THEN** `is_ignored` returns true, excluding the file from local file scans, uploads, and downloads diff --git a/openspec/changes/fix-sync-matching-download-state-retention/tasks.md b/openspec/changes/fix-sync-matching-download-state-retention/tasks.md new file mode 100644 index 0000000..ccd0cf6 --- /dev/null +++ b/openspec/changes/fix-sync-matching-download-state-retention/tasks.md @@ -0,0 +1,14 @@ +## 1. Unit Testing and Reproduction + +- [x] 1.1 Add regression test in `client-tauri/src-tauri/src/sync.rs` asserting that hash-matched and resumed downloads are retained in `completed_downloads` and saved in `state.paths` +- [x] 1.2 Add regression test in `client-tauri/src-tauri/src/sync.rs` asserting that bare `.git` files and `sub/.git` files are ignored by `is_ignored` + +## 2. Desktop Client Implementation + +- [x] 2.1 Update the download loop in `client-tauri/src-tauri/src/sync.rs` to register hash-matched and resumed paths into `completed_downloads` before continuing +- [x] 2.2 Update `is_ignored` in `client-tauri/src-tauri/src/sync.rs` to match bare `.git` files (`normalized == ".git"` and `normalized.ends_with("/.git")`) + +## 3. Verification and QA + +- [x] 3.1 Run `cargo test` in `client-tauri/src-tauri` and verify all tests pass +- [x] 3.2 Run full QA suite (`pytest backend` and `npm run build` in `client-tauri`) to verify no regressions