diff --git a/.cursor/plans/comprehensive_plugin_tests_bdd5b47c.plan.md b/.cursor/plans/comprehensive_plugin_tests_bdd5b47c.plan.md index 73dd73af..2d27147c 100644 --- a/.cursor/plans/comprehensive_plugin_tests_bdd5b47c.plan.md +++ b/.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 diff --git a/.cursor/plans/syncback_roundtrip_tests_f980a65b.plan.md b/.cursor/plans/syncback_roundtrip_tests_f980a65b.plan.md index 6e1f513d..74bb674e 100644 --- a/.cursor/plans/syncback_roundtrip_tests_f980a65b.plan.md +++ b/.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 -2. How the recursive `scan_directory` function traverses the filesystem -3. Path normalization issues between scanned paths and added paths +This skipped ANY file whose parent directory was in `added_paths`, but adding a directory doesn't mean all files inside should be preserved. -### 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 --- diff --git a/src/syncback/fs_snapshot.rs b/src/syncback/fs_snapshot.rs index 279fc8ec..4f714807 100644 --- a/src/syncback/fs_snapshot.rs +++ b/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. diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 5929cb79..1de12431 100644 --- a/src/syncback/mod.rs +++ b/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 = 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 = 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, + ) { + // 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() { - dirs_to_scan.push(path.to_path_buf()); + if !path.to_string_lossy().ends_with(".project.json5") + && !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>, 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!( - "[DEBUG] dirs_to_scan: {:?}", + log::trace!( + "dirs_to_scan: {:?}", dirs_to_scan.iter().map(|p| p.display().to_string()).collect::>() ); 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!( - "[DEBUG] Scanned {} existing paths from filesystem", - paths.len() - ); - for p in &paths { - eprintln!("[DEBUG] - {}", p.display()); - } + log::debug!("Scanned {} existing paths from filesystem", paths.len()); 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; - } - // 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)) - { + log::trace!("Skipping {} (exact path in added_paths)", old_path.display()); 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 - .iter() - .any(|added| added.starts_with(&old_path_norm)) - { - continue; + // NOTE: We DO NOT skip files just because their parent directory is in added_paths. + // Adding a directory does not mean all orphan files inside should be preserved. + if old_path_norm.is_dir() { + if added_paths.iter().any(|added| added.starts_with(&old_path_norm)) { + 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); } diff --git a/tests/rojo_test/roundtrip_util.rs b/tests/rojo_test/roundtrip_util.rs index 1ff62a5e..18c019c1 100644 --- a/tests/rojo_test/roundtrip_util.rs +++ b/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) );