Fix bugs in syncback process and enhance error handling

- Resolved issues with nested orphan file removal, ensuring that both top-level and nested orphan files are correctly handled.
- Improved directory removal logic to gracefully handle cases where directories may have already been removed, preventing crashes.
- Enhanced the path scanning algorithm to accurately collect all source directories defined in the project, addressing missing paths in the syncback process.
- Updated logging to provide clearer debug information during directory scanning and error handling in the syncback operations.

.cursor/plans/comprehensive_plugin_tests_bdd5b47c.plan.md

@@ -110,12 +110,14 @@ The Rojo plugin/API interface is a critical weak layer that must handle:
 These are the types of bugs that stress tests should expose:
 
 **Duplicate/Ambiguous Path Handling**:
+
 - Instances with duplicate-named siblings cause ambiguous paths
 - Nested duplicates at any level of the hierarchy
 - Path uniqueness checks must walk entire ancestor chain
 - Tests should verify duplicate detection at every level
 
 **Property Equality Edge Cases**:
+
 - Floating-point comparison (epsilon = 0.0001)
 - Color3 RGB integer comparison vs float comparison
 - CFrame/Vector3/Vector2 precision
@@ -123,6 +125,7 @@ These are the types of bugs that stress tests should expose:
 - EnumItem vs number comparisons
 
 **Patch Application Failures**:
+
 - Partial application (some properties succeed, others fail)
 - Ref properties to non-existent instances
 - ClassName changes with child migration failures
@@ -130,12 +133,14 @@ These are the types of bugs that stress tests should expose:
 - Permission errors on protected properties
 
 **Concurrency Issues**:
+
 - Changes during confirmation dialogue
 - Rapid bidirectional changes
 - Instance destruction during patch application
 - WebSocket messages during patch processing
 
 **Memory and Scale**:
+
 - Large instance trees (1000+ instances)
 - Deep hierarchies (50+ levels)
 - Wide hierarchies (100+ siblings)
@@ -322,7 +327,7 @@ These are the types of bugs that stress tests should expose:
 
 ### 6. Scripts-Only vs Everything Mode Tests
 
-**`plugin/src/modes/scriptsOnly.spec.lua`** - Scripts-only mode specific tests
+`**plugin/src/modes/scriptsOnly.spec.lua**` - Scripts-only mode specific tests
 
 ```lua
 -- Key test scenarios:
@@ -337,7 +342,7 @@ These are the types of bugs that stress tests should expose:
 -- Format transitions (standalone <-> directory) for scripts only
 ```
 
-**`plugin/src/modes/everythingMode.spec.lua`** - Everything mode specific tests
+`**plugin/src/modes/everythingMode.spec.lua**` - Everything mode specific tests
 
 ```lua
 -- Key test scenarios:
@@ -353,7 +358,7 @@ These are the types of bugs that stress tests should expose:
 -- Mixed hierarchies (scripts and non-scripts)
 ```
 
-**`plugin/src/modes/modeTransitions.spec.lua`** - Switching between modes
+`**plugin/src/modes/modeTransitions.spec.lua**` - Switching between modes
 
 ```lua
 -- Key test scenarios:
@@ -366,7 +371,7 @@ These are the types of bugs that stress tests should expose:
 
 ### 7. Multi-Developer and Concurrent Scenarios
 
-**`plugin/src/concurrent/multiDeveloper.spec.lua`** - Simulating multiple developers
+`**plugin/src/concurrent/multiDeveloper.spec.lua**` - Simulating multiple developers
 
 ```lua
 -- Key test scenarios:
@@ -380,7 +385,7 @@ These are the types of bugs that stress tests should expose:
 -- Merge conflicts in PatchSet
 ```
 
-**`plugin/src/concurrent/raceConditions.spec.lua`** - Race condition detection
+`**plugin/src/concurrent/raceConditions.spec.lua**` - Race condition detection
 
 ```lua
 -- Key test scenarios:
@@ -497,6 +502,7 @@ These are the types of bugs that stress tests should expose:
 **Bug Tracking**:
 
 When a stress test fails, document:
+
 - The symptom (what failed)
 - The root cause (after investigation)
 - Whether it needs a patch or a redesign

.cursor/plans/syncback_roundtrip_tests_f980a65b.plan.md

@@ -40,25 +40,67 @@ isProject: false
 
 ## Current Status: Bugs Found by Stress Tests
 
-### Bug 1: Nested Orphan Files Not Removed (ACTIVE)
+### Bug 1: Nested Orphan Files Not Removed (FIXED)
 
-**Symptom**: Orphan files at the top level (`src/orphan.txt`) are removed, but nested orphan files (`src/level-1/orphan.luau`) are NOT removed.
+**Symptom**: Orphan files at the top level (`src/orphan.txt`) were removed, but nested orphan files (`src/level-1/orphan.luau`) were NOT removed.
 
-**Debug output**: `Scanned 2 existing paths from filesystem` - but should be finding many more paths including nested orphan files.
+**Root cause**: The orphan removal logic had a flawed check:
 
-**Root cause investigation needed**: The orphan scanning algorithm in `src/syncback/mod.rs` lines 220-292 may have a bug in:
+```rust
+if added_paths.iter().any(|added| old_path_norm.starts_with(added)) { continue; }
+```
 
-1. How `dirs_to_scan` is populated from root children
+This skipped ANY file whose parent directory was in `added_paths`, but adding a directory doesn't mean all files inside should be preserved.
-2. How the recursive `scan_directory` function traverses the filesystem
-3. Path normalization issues between scanned paths and added paths
 
-### Bug 2: Duplicate File Removal Crash (FIXED)
+**Fix applied**: Changed the logic to only skip DIRECTORIES that are ancestors of added paths, not files.
+
+### Bug 2: Missing $path Directories in Scan (FIXED)
+
+**Symptom**: `dirs_to_scan` only contained the project file, not the `$path` source directories.
+
+**Root cause**: The code relied on `instigating_source` metadata which points to the project file for root nodes, not the actual source directory defined by `$path`.
+
+**Fix applied**: Added `collect_paths_from_project()` to recursively extract `$path` entries from the project tree.
+
+### Bug 3: Duplicate File Removal Crash (FIXED)
 
 **Symptom**: `failed to remove file: The system cannot find the file specified`
 
 **Root cause**: Same file could be added to `removed_files` with both absolute and relative paths, causing double removal attempts.
 
-**Fix applied**: In `fs_snapshot.rs`, gracefully handle "file not found" errors during removal since the file may have already been deleted.
+**Fix applied**: In `fs_snapshot.rs`, gracefully handle "file not found" errors during removal.
+
+### Bug 4: Directory Double-Removal Crash (FIXED)
+
+**Symptom**: Syncback fails when a directory is marked for removal by both the syncback `removed_children` phase and the orphan removal phase.
+
+**Root cause**: `remove_dir_all` returns an error when the directory doesn't exist, and the same directory could be added twice.
+
+**Fix applied**: Handle "not found" errors gracefully in `fs_snapshot.rs` for directory removal.
+
+### Limitation 1: Class Change Not Supported (FUNDAMENTAL)
+
+**Symptom**: Deleting `init.luau` changes folder from ModuleScript to Folder. Syncback fails silently.
+
+**Root cause**: Syncback cannot change the class of instances defined in the project file. This is by design in `src/snapshot_middleware/project.rs`.
+
+**Affected tests**: `clean_restores_deleted_files`, `clean_restores_deleted_nested_files`, `clean_restores_dir_from_file`
+
+### Limitation 2: Ambiguous File Names Not Supported (FUNDAMENTAL)
+
+**Symptom**: Adding duplicate files with different extensions (e.g., `foo.luau` and `foo.lua`) causes syncback to fail.
+
+**Root cause**: Rojo can't handle ambiguous file names during project loading.
+
+**Affected tests**: `clean_removes_duplicate_extensions`, `clean_removes_multiple_duplicates`
+
+---
+
+## Test Results Summary (Current)
+
+**Passing: 15 tests** - orphan removal, basic clean mode behavior
+
+**Failing: 11 tests** - class change scenarios, ambiguous files, meta files, combination tests
 
 ---
 

src/syncback/fs_snapshot.rs

@@ -108,7 +108,18 @@ impl FsSnapshot {
             lock.write(base_path.join(path), contents)?;
         }
         for dir_path in &self.removed_dirs {
-            lock.remove_dir_all(base_path.join(dir_path))?;
+            let full_path = base_path.join(dir_path);
+            match lock.remove_dir_all(&full_path) {
+                Ok(()) => (),
+                // Directory might have already been removed (e.g., added twice via different code paths)
+                Err(err) if err.kind() == io::ErrorKind::NotFound => {
+                    log::debug!(
+                        "Directory already removed or doesn't exist: {}",
+                        full_path.display()
+                    );
+                }
+                Err(err) => return Err(err),
+            }
         }
         // Only remove files that aren't already inside a directory we're removing.
         // remove_dir_all already deleted those files recursively.

src/syncback/mod.rs

@@ -221,21 +221,66 @@ pub fn syncback_loop_with_stats(
     // not just what Rojo loaded into its tree. This ensures orphaned files
     // (like duplicates Rojo couldn't load) get cleaned up properly.
     // Clean mode is essentially: "delete everything and rebuild from Studio"
-    eprintln!("[DEBUG] incremental = {}", incremental);
     let existing_paths: HashSet<PathBuf> = if !incremental {
-        eprintln!("[DEBUG] Clean mode: scanning filesystem for existing paths");
         log::debug!("Clean mode: scanning filesystem for existing paths");
         let mut paths = HashSet::new();
 
         // Get the source directories from the project's tree structure.
-        // We scan these directories recursively to find all files on disk.
+        // We need to collect ALL $path directories defined in the project,
+        // not just instigating_source metadata (which may point to project file).
         let mut dirs_to_scan: Vec<PathBuf> = Vec::new();
+        
+        // Helper to recursively collect $path entries from project tree
+        fn collect_paths_from_project(
+            node: &crate::project::ProjectNode,
+            base_path: &Path,
+            paths: &mut Vec<PathBuf>,
+        ) {
+            // If this node has a $path, add it
+            if let Some(ref path_node) = node.path {
+                let resolved = base_path.join(path_node.path());
+                log::trace!("Found $path in project: {}", resolved.display());
+                if resolved.exists() && !paths.contains(&resolved) {
+                    paths.push(resolved);
+                }
+            }
+            // Recursively check children
+            for child in node.children.values() {
+                collect_paths_from_project(child, base_path, paths);
+            }
+        }
+        
+        // Collect paths from the project tree definition
+        collect_paths_from_project(&project.tree, project_path, &mut dirs_to_scan);
+        
+        // Also check instance metadata for paths that might not be in project
+        // (e.g., from nested project references)
+        let root = old_tree.root();
+        log::trace!("Root instance: name={}, class={}", root.name(), root.class_name());
+        if let Some(source) = &root.metadata().instigating_source {
+            let path = source.path();
+            // Skip project files - we want source directories
+            if !path.to_string_lossy().ends_with(".project.json5")
+                && !path.to_string_lossy().ends_with(".project.json")
+            {
+                log::trace!("Root has instigating_source: {}", path.display());
+                if path.exists() && !dirs_to_scan.contains(&path.to_path_buf()) {
+                    dirs_to_scan.push(path.to_path_buf());
+                }
+            }
+        }
+        
+        // Check children for additional paths
         for ref_id in old_tree.inner().root().children() {
             if let Some(inst) = old_tree.get_instance(*ref_id) {
                 if let Some(source) = &inst.metadata().instigating_source {
                     let path = source.path();
-                    if path.exists() {
+                    if !path.to_string_lossy().ends_with(".project.json5")
-                        dirs_to_scan.push(path.to_path_buf());
+                        && !path.to_string_lossy().ends_with(".project.json")
+                    {
+                        if path.exists() && !dirs_to_scan.contains(&path.to_path_buf()) {
+                            dirs_to_scan.push(path.to_path_buf());
+                        }
                     }
                 }
             }
@@ -248,11 +293,11 @@ pub fn syncback_loop_with_stats(
             ignore_patterns: &Option<Vec<Glob>>,
             project_path: &Path,
         ) {
-            eprintln!("[DEBUG] scan_directory: {}", dir.display());
+            log::trace!("Scanning directory: {}", dir.display());
             let entries = match std::fs::read_dir(dir) {
                 Ok(e) => e,
                 Err(e) => {
-                    eprintln!("[DEBUG] Failed to read directory {}: {}", dir.display(), e);
+                    log::trace!("Failed to read directory {}: {}", dir.display(), e);
                     return;
                 }
             };
@@ -262,19 +307,19 @@ pub fn syncback_loop_with_stats(
 
                 // Skip paths matching ignore patterns
                 if !is_valid_path(ignore_patterns, project_path, &path) {
-                    eprintln!("[DEBUG] Skipping {} (matches ignore pattern)", path.display());
+                    log::trace!("Skipping {} (matches ignore pattern)", path.display());
                     continue;
                 }
 
                 // Skip hidden files/directories (starting with .)
                 if let Some(name) = path.file_name().and_then(|n| n.to_str()) {
                     if name.starts_with('.') {
-                        eprintln!("[DEBUG] Skipping {} (hidden)", path.display());
+                        log::trace!("Skipping {} (hidden)", path.display());
                         continue;
                     }
                 }
 
-                eprintln!("[DEBUG] Found: {} (is_dir: {})", path.display(), path.is_dir());
+                log::trace!("Found path: {}", path.display());
                 paths.insert(path.clone());
 
                 if path.is_dir() {
@@ -283,33 +328,23 @@ pub fn syncback_loop_with_stats(
             }
         }
 
-        eprintln!(
+        log::trace!(
-            "[DEBUG] dirs_to_scan: {:?}",
+            "dirs_to_scan: {:?}",
             dirs_to_scan.iter().map(|p| p.display().to_string()).collect::<Vec<_>>()
         );
 
         for dir in &dirs_to_scan {
             if dir.is_dir() {
-                eprintln!("[DEBUG] Scanning directory: {}", dir.display());
                 scan_directory(dir, &mut paths, &ignore_patterns, project_path);
             } else if dir.exists() {
                 // It's a file, add it directly
-                eprintln!("[DEBUG] Adding file directly: {}", dir.display());
                 if is_valid_path(&ignore_patterns, project_path, dir) {
                     paths.insert(dir.clone());
                 }
-            } else {
-                eprintln!("[DEBUG] Path does not exist: {}", dir.display());
             }
         }
 
-        eprintln!(
+        log::debug!("Scanned {} existing paths from filesystem", paths.len());
-            "[DEBUG] Scanned {} existing paths from filesystem",
-            paths.len()
-        );
-        for p in &paths {
-            eprintln!("[DEBUG]   - {}", p.display());
-        }
         paths
     } else {
         HashSet::new()
@@ -549,38 +584,34 @@ pub fn syncback_loop_with_stats(
 
             // Never remove the project file itself
             if old_path_norm == project_file {
-                log::debug!("Skipping project file: {}", old_path.display());
+                log::trace!("Skipping project file: {}", old_path.display());
                 continue;
             }
             // Never remove any nested project files - they define project structure
             // and may be referenced via $path from other project files
             if let Some(file_name) = old_path_norm.file_name().and_then(|n| n.to_str()) {
                 if file_name.ends_with(".project.json5") || file_name.ends_with(".project.json") {
-                    log::debug!("Skipping nested project file: {}", old_path.display());
+                    log::trace!("Skipping nested project file: {}", old_path.display());
                     continue;
                 }
             }
             // Skip if this exact path is being written to
             if added_paths.contains(&old_path_norm) {
-                continue;
+                log::trace!("Skipping {} (exact path in added_paths)", old_path.display());
-            }
-            // Skip if a parent of this path is being added
-            // (e.g., the new structure will replace it)
-            if added_paths
-                .iter()
-                .any(|added| old_path_norm.starts_with(added))
-            {
                 continue;
             }
             // Skip if this path is an ancestor of something being written to
             // (e.g., a directory that will contain new files)
-            if added_paths
+            // NOTE: We DO NOT skip files just because their parent directory is in added_paths.
-                .iter()
+            // Adding a directory does not mean all orphan files inside should be preserved.
-                .any(|added| added.starts_with(&old_path_norm))
+            if old_path_norm.is_dir() {
-            {
+                if added_paths.iter().any(|added| added.starts_with(&old_path_norm)) {
-                continue;
+                    log::trace!("Skipping {} (ancestor of added path)", old_path.display());
+                    continue;
+                }
             }
             // Use normalized path to avoid duplicates from UNC vs regular paths
+            log::trace!("Marking for removal: {}", old_path_norm.display());
             paths_to_remove.insert(old_path_norm);
         }
 

tests/rojo_test/roundtrip_util.rs

@@ -63,7 +63,8 @@ pub fn run_rojo_syncback_clean(project_path: &Path, input_path: &Path) -> bool {
 
     if !output.status.success() {
         eprintln!(
-            "rojo syncback failed:\nstdout: {}\nstderr: {}",
+            "rojo syncback failed (exit code {:?}):\nstdout: {}\nstderr: {}",
+            output.status.code(),
             String::from_utf8_lossy(&output.stdout),
             String::from_utf8_lossy(&output.stderr)
         );