diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 597646d4..60227d33 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -374,7 +374,8 @@ pub fn syncback_project<'sync>( if old_inst.class_name() != new_inst.class { // In clean mode, allow recoverable class transitions // (e.g., Folder -> ModuleScript by creating init.luau) - let can_recover = can_transition_class(old_inst.class_name().as_str(), new_inst.class.as_str()); + let can_recover = + can_transition_class(old_inst.class_name().as_str(), new_inst.class.as_str()); if !snapshot.data.is_incremental() && can_recover { log::debug!( "Clean mode: allowing class transition {} -> {} for {}", @@ -490,6 +491,31 @@ pub fn syncback_project<'sync>( ); middleware = Middleware::Dir; } + + // Case 3: Filesystem has a script-dir but new instance is a DIFFERENT script type. + // e.g., ModuleScriptDir (init.luau) but new instance is Script → use ServerScriptDir. + // Without this, changing a ModuleScript to Script in Studio would silently keep + // the old init.luau file, causing the script to be read as ModuleScript on next build. + if is_script_dir_middleware { + let correct_middleware = match new_inst.class.as_str() { + "ModuleScript" => Some(Middleware::ModuleScriptDir), + "Script" => Some(Middleware::ServerScriptDir), + "LocalScript" => Some(Middleware::LocalScriptDir), + _ => None, + }; + if let Some(new_middleware) = correct_middleware { + if middleware != new_middleware { + log::debug!( + "Clean mode: overriding {:?}->{:?} middleware for {} (class {} changed)", + middleware, + new_middleware, + full_path.display(), + new_inst.class + ); + middleware = new_middleware; + } + } + } } descendant_snapshots.push( diff --git a/src/snapshot_middleware/rbxm.rs b/src/snapshot_middleware/rbxm.rs index 77647472..1d81b934 100644 --- a/src/snapshot_middleware/rbxm.rs +++ b/src/snapshot_middleware/rbxm.rs @@ -74,7 +74,8 @@ pub fn syncback_rbxm<'sync>( /// Clones a subtree from the source WeakDom, filtering out instances that match /// the provided ignoreTrees glob patterns. -pub fn clone_tree_filtered(source: &WeakDom, root_ref: Ref, ignore_globs: &[Glob]) -> WeakDom { +/// Each entry contains both the compiled Glob and the original pattern string. +pub fn clone_tree_filtered(source: &WeakDom, root_ref: Ref, ignore_globs: &[(Glob, String)]) -> WeakDom { let root_inst = source.get_by_ref(root_ref).expect("root ref should exist"); // Create a new tree with the root instance @@ -93,7 +94,7 @@ fn add_children_filtered( source: &WeakDom, parent_ref: Ref, builder: &mut InstanceBuilder, - ignore_globs: &[Glob], + ignore_globs: &[(Glob, String)], ) { let parent = source .get_by_ref(parent_ref) @@ -103,7 +104,7 @@ fn add_children_filtered( let child_path = inst_path(source, child_ref); // Check if this child should be ignored - let should_ignore = ignore_globs.iter().any(|glob| glob.is_match(&child_path)); + let should_ignore = ignore_globs.iter().any(|(glob, _)| glob.is_match(&child_path)); if should_ignore { log::debug!("Filtering out {child_path} from rbxm due to ignoreTrees pattern"); diff --git a/src/syncback/fs_snapshot.rs b/src/syncback/fs_snapshot.rs index 4f714807..216a2023 100644 --- a/src/syncback/fs_snapshot.rs +++ b/src/syncback/fs_snapshot.rs @@ -126,10 +126,7 @@ impl FsSnapshot { // Also handle the case where the same file might be listed twice (absolute vs relative path) // by gracefully handling "file not found" errors. for path in &self.removed_files { - let is_inside_removed_dir = self - .removed_dirs - .iter() - .any(|dir| path.starts_with(dir)); + let is_inside_removed_dir = self.removed_dirs.iter().any(|dir| path.starts_with(dir)); if is_inside_removed_dir { continue; } @@ -158,11 +155,7 @@ impl FsSnapshot { let files_inside_dirs = self .removed_files .iter() - .filter(|path| { - self.removed_dirs - .iter() - .any(|dir| path.starts_with(dir)) - }) + .filter(|path| self.removed_dirs.iter().any(|dir| path.starts_with(dir))) .count(); log::debug!( "Removed {} directories and {} files from the file system ({} files skipped, inside removed dirs)", diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index d69394c5..aa617bfa 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -229,7 +229,7 @@ pub fn syncback_loop_with_stats( // 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 DIRECTORY entries from project tree // Only directories need to be scanned for orphan detection; single files // don't contain orphans that need to be removed. @@ -252,10 +252,10 @@ pub fn syncback_loop_with_stats( 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 scan the project folder itself - files like src.luau that exist // alongside default.project.json5 should be considered for orphan removal let project_path_buf = project_path.to_path_buf(); @@ -263,11 +263,15 @@ pub fn syncback_loop_with_stats( log::trace!("Adding project folder to scan: {}", project_path.display()); dirs_to_scan.push(project_path_buf); } - + // 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()); + 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 @@ -280,7 +284,7 @@ pub fn syncback_loop_with_stats( } } } - + // Check children for additional paths for ref_id in old_tree.inner().root().children() { if let Some(inst) = old_tree.get_instance(*ref_id) { @@ -288,11 +292,9 @@ pub fn syncback_loop_with_stats( let path = source.path(); 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()) { + && path.exists() && !dirs_to_scan.contains(&path.to_path_buf()) { dirs_to_scan.push(path.to_path_buf()); } - } } } } @@ -341,7 +343,10 @@ pub fn syncback_loop_with_stats( log::trace!( "dirs_to_scan: {:?}", - dirs_to_scan.iter().map(|p| p.display().to_string()).collect::>() + dirs_to_scan + .iter() + .map(|p| p.display().to_string()) + .collect::>() ); for dir in &dirs_to_scan { @@ -405,7 +410,7 @@ pub fn syncback_loop_with_stats( } // Check ignoreTrees with glob pattern support if let Some(ref globs) = tree_globs { - for glob in globs { + for (glob, _pattern) in globs { if glob.is_match(&inst_path) { log::debug!("Tree {inst_path} is blocked by ignoreTrees glob pattern"); continue 'syncback; @@ -477,7 +482,7 @@ pub fn syncback_loop_with_stats( } // Check ignoreTrees with glob pattern support if let Some(ref globs) = tree_globs { - for glob in globs { + for (glob, _pattern) in globs { if glob.is_match(&inst_path) { log::debug!("Skipping removing {inst_path} because its path is blocked by ignoreTrees glob pattern"); continue 'remove; @@ -584,6 +589,67 @@ pub fn syncback_loop_with_stats( // Get the project file path to exclude it from removal let project_file = normalize_existing_path(&project.file_location); + // Collect ALL paths explicitly referenced via $path in the project. + // These paths should NOT be removed during orphan cleanup because they + // are explicitly part of the project structure. + let mut protected_paths: HashSet = HashSet::new(); + + // Also build a mapping from filesystem path prefix to instance path prefix. + // This is needed to convert filesystem paths to instance paths for ignoreTrees checking. + // e.g., if project has "$path": "src" on "ReplicatedStorage", then + // filesystem "src/Foo" maps to instance "ReplicatedStorage/Foo" + let mut path_to_instance_prefix: Vec<(PathBuf, String)> = Vec::new(); + + fn collect_all_path_refs_and_mappings( + node: &crate::project::ProjectNode, + base_path: &Path, + instance_path: &str, + protected: &mut HashSet, + mappings: &mut Vec<(PathBuf, String)>, + ) { + if let Some(ref path_node) = node.path { + let resolved = base_path.join(path_node.path()); + let canonical = std::fs::canonicalize(&resolved) + .map(strip_unc_prefix) + .unwrap_or_else(|_| resolved.clone()); + protected.insert(canonical.clone()); + // Add mapping from filesystem path to instance path + if !instance_path.is_empty() { + mappings.push((canonical, instance_path.to_string())); + } + } + for (name, child) in &node.children { + let child_inst_path = if instance_path.is_empty() { + name.clone() + } else { + format!("{}/{}", instance_path, name) + }; + collect_all_path_refs_and_mappings(child, base_path, &child_inst_path, protected, mappings); + } + } + collect_all_path_refs_and_mappings(&project.tree, project_path, "", &mut protected_paths, &mut path_to_instance_prefix); + log::trace!("Protected $path references: {:?}", protected_paths); + log::trace!("Path to instance mappings: {:?}", path_to_instance_prefix); + + // Helper to convert a filesystem path to an instance path using the mappings + let fs_path_to_instance_path = |fs_path: &Path| -> Option { + for (fs_prefix, inst_prefix) in &path_to_instance_prefix { + if let Ok(relative) = fs_path.strip_prefix(fs_prefix) { + let relative_str = relative + .components() + .filter_map(|c| c.as_os_str().to_str()) + .collect::>() + .join("/"); + if relative_str.is_empty() { + return Some(inst_prefix.clone()); + } else { + return Some(format!("{}/{}", inst_prefix, relative_str)); + } + } + } + None + }; + // First pass: collect normalized paths to remove (deduplicated) let mut paths_to_remove: HashSet = HashSet::new(); for old_path in &existing_paths { @@ -594,25 +660,76 @@ pub fn syncback_loop_with_stats( log::trace!("Skipping root project file: {}", old_path.display()); continue; } - // In clean mode, orphan nested project files ARE removed. - // Clean mode represents "delete everything and rebuild from rbxm" - - // orphan project files that aren't referenced by the main project - // should not survive. Only the root project file is protected. - // + + // Never remove paths that are explicitly referenced via $path in the project + if protected_paths.contains(&old_path_norm) { + log::trace!( + "Skipping {} (explicitly referenced via $path)", + old_path.display() + ); + continue; + } + + // Check ignoreTrees with glob pattern support. + // Convert filesystem path to instance path using the project structure mappings. + if let Some(ref globs) = tree_globs { + if let Some(inst_path_str) = fs_path_to_instance_path(&old_path_norm) { + let mut is_ignored = false; + for (glob, pattern_str) in globs { + // Check if this path matches the ignore pattern + if glob.is_match(&inst_path_str) { + log::trace!( + "Skipping {} (matches ignoreTrees pattern, inst_path: {})", + old_path.display(), + inst_path_str + ); + is_ignored = true; + break; + } + // Also check if this is a directory that is an ANCESTOR of an ignored path. + // e.g., if "ReplicatedStorage/KeepMe" is ignored, then "ReplicatedStorage" (src/) + // should not be removed as it contains ignored content. + if old_path_norm.is_dir() { + // Check if the pattern starts with the instance path (meaning it's inside this directory) + if pattern_str.starts_with(&inst_path_str) + && pattern_str.len() > inst_path_str.len() + && pattern_str.as_bytes().get(inst_path_str.len()) == Some(&b'/') + { + log::trace!( + "Skipping {} (ancestor of ignoreTrees path, inst_path: {})", + old_path.display(), + inst_path_str + ); + is_ignored = true; + break; + } + } + } + if is_ignored { + continue; + } + } + } + // Skip if this exact path is being written to if added_paths.contains(&old_path_norm) { - log::trace!("Skipping {} (exact path in added_paths)", old_path.display()); + 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) // 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; - } + if old_path_norm.is_dir() + && 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()); @@ -804,14 +921,15 @@ impl SyncbackRules { /// Compiles the ignoreTrees patterns into glob matchers. /// Supports glob patterns like `**/Abc/Script` for flexible matching. - pub fn compile_tree_globs(&self) -> anyhow::Result> { + /// Returns both the compiled Glob and the original pattern string for each entry. + pub fn compile_tree_globs(&self) -> anyhow::Result> { let mut globs = Vec::with_capacity(self.ignore_trees.len()); for pattern in &self.ignore_trees { let glob = Glob::new(pattern).with_context(|| { format!("the pattern '{pattern}' is not a valid ignoreTrees glob") })?; - globs.push(glob); + globs.push((glob, pattern.clone())); } Ok(globs) diff --git a/src/syncback/snapshot.rs b/src/syncback/snapshot.rs index a4bb17b0..f77580d7 100644 --- a/src/syncback/snapshot.rs +++ b/src/syncback/snapshot.rs @@ -309,7 +309,8 @@ impl<'sync> SyncbackSnapshot<'sync> { /// Compiles the ignoreTrees patterns into globs. /// Returns an empty Vec if no patterns are configured. - pub fn compile_tree_globs(&self) -> Vec { + /// Each entry contains both the compiled Glob and the original pattern string. + pub fn compile_tree_globs(&self) -> Vec<(Glob, String)> { self.data .project .syncback_rules diff --git a/src/web/api.rs b/src/web/api.rs index d4600ca1..66299c8b 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -625,14 +625,15 @@ impl ApiService { // We need to convert from standalone (e.g., MyScript.server.luau) // to directory format (e.g., MyScript/init.server.luau). let script_name = added.name.as_str(); - let parent_dir = existing_path - .parent() - .unwrap_or(existing_path); + let parent_dir = existing_path.parent().unwrap_or(existing_path); let new_dir = parent_dir.join(script_name); // Create the directory fs::create_dir_all(&new_dir).with_context(|| { - format!("Failed to create directory for script with children: {}", new_dir.display()) + format!( + "Failed to create directory for script with children: {}", + new_dir.display() + ) })?; // Determine the init file name based on class @@ -652,7 +653,10 @@ impl ApiService { // Remove the old standalone file if existing_path.exists() && existing_path != init_path { fs::remove_file(existing_path).with_context(|| { - format!("Failed to remove old standalone script: {}", existing_path.display()) + format!( + "Failed to remove old standalone script: {}", + existing_path.display() + ) })?; } diff --git a/tests/rojo_test/roundtrip_util.rs b/tests/rojo_test/roundtrip_util.rs index 0053ac49..8a0d1f4a 100644 --- a/tests/rojo_test/roundtrip_util.rs +++ b/tests/rojo_test/roundtrip_util.rs @@ -53,10 +53,7 @@ pub fn run_rojo_build(project_path: &Path, output_name: &str) -> (TempDir, PathB pub fn run_rojo_syncback_clean(project_path: &Path, input_path: &Path) -> bool { // Verify input file exists before calling if !input_path.exists() { - eprintln!( - "ERROR: Input file does not exist: {}", - input_path.display() - ); + eprintln!("ERROR: Input file does not exist: {}", input_path.display()); return false; } @@ -319,7 +316,10 @@ pub fn apply_mutation(dir: &Path, mutation: &Mutation) { fs::remove_dir_all(&path).expect("Failed to delete directory"); } } - Mutation::ConvertDirToFile { dir: dir_path, file_content } => { + Mutation::ConvertDirToFile { + dir: dir_path, + file_content, + } => { let path = dir.join(dir_path); if path.exists() && path.is_dir() { fs::remove_dir_all(&path).expect("Failed to remove directory"); diff --git a/tests/tests/clean_mode_stress.rs b/tests/tests/clean_mode_stress.rs index 597bad9d..73fd80fd 100644 --- a/tests/tests/clean_mode_stress.rs +++ b/tests/tests/clean_mode_stress.rs @@ -521,12 +521,10 @@ fn clean_handles_empty_orphan_directory() { // with just adding an empty-ish directory structure clean_equals_fresh( "deep_nesting", - &[ - Mutation::AddOrphanFile { - relative_path: "src/empty_dir/.gitkeep", - content: "", - }, - ], + &[Mutation::AddOrphanFile { + relative_path: "src/empty_dir/.gitkeep", + content: "", + }], ); } diff --git a/tests/tests/syncback_format_transitions.rs b/tests/tests/syncback_format_transitions.rs index 02ea73f8..1acacdc9 100644 --- a/tests/tests/syncback_format_transitions.rs +++ b/tests/tests/syncback_format_transitions.rs @@ -335,10 +335,16 @@ fn standalone_module_sync_adding_children_converts_to_directory() { send_write_request(&session, &info.session_id, rs_id, added); // Standalone file should be removed (converted to directory) - assert_not_exists(&standalone_file, "Standalone file should be removed after conversion"); + assert_not_exists( + &standalone_file, + "Standalone file should be removed after conversion", + ); // Directory should be created with init file - assert_directory_exists(&dir_path, "Directory should be created for script with children"); + assert_directory_exists( + &dir_path, + "Directory should be created for script with children", + ); let init_file = dir_path.join("init.luau"); assert_file_exists(&init_file, "init.luau should exist"); @@ -388,10 +394,16 @@ fn standalone_script_sync_adding_children_converts_to_directory() { send_write_request(&session, &info.session_id, rs_id, added); // Standalone file should be removed (converted to directory) - assert_not_exists(&standalone_file, "Standalone file should be removed after conversion"); + assert_not_exists( + &standalone_file, + "Standalone file should be removed after conversion", + ); // Directory should be created with init file - assert_directory_exists(&dir_path, "Directory should be created for script with children"); + assert_directory_exists( + &dir_path, + "Directory should be created for script with children", + ); let init_file = dir_path.join("init.server.luau"); assert_file_exists(&init_file, "init.server.luau should exist"); @@ -442,10 +454,16 @@ fn standalone_localscript_sync_adding_children_converts_to_directory() { send_write_request(&session, &info.session_id, rs_id, added); // Standalone file should be removed (converted to directory) - assert_not_exists(&standalone_file, "Standalone file should be removed after conversion"); + assert_not_exists( + &standalone_file, + "Standalone file should be removed after conversion", + ); // Directory should be created with init file - assert_directory_exists(&dir_path, "Directory should be created for script with children"); + assert_directory_exists( + &dir_path, + "Directory should be created for script with children", + ); let init_file = dir_path.join("init.local.luau"); assert_file_exists(&init_file, "init.local.luau should exist"); @@ -2066,7 +2084,10 @@ fn sync_standalone_then_directory_format() { send_write_request(&session, &info.session_id, rs_id, added1); // Standalone should still exist after sync without children - assert_file_exists(&standalone, "Standalone preserved after sync without children"); + assert_file_exists( + &standalone, + "Standalone preserved after sync without children", + ); // Second sync - with children (converts to directory) let added2 = make_added_instance( diff --git a/tests/tests/syncback_roundtrip.rs b/tests/tests/syncback_roundtrip.rs index 13c31cec..b1e50c18 100644 --- a/tests/tests/syncback_roundtrip.rs +++ b/tests/tests/syncback_roundtrip.rs @@ -75,11 +75,16 @@ fn assert_rbx_equal(file_a: &Path, file_b: &Path, test_name: &str) { let data_b = fs::read(file_b).expect("Failed to read roundtrip rbxl"); let dom_a = rbx_binary::from_reader(data_a.as_slice()).expect("Failed to parse original rbxl"); - let dom_b = - rbx_binary::from_reader(data_b.as_slice()).expect("Failed to parse roundtrip rbxl"); + let dom_b = rbx_binary::from_reader(data_b.as_slice()).expect("Failed to parse roundtrip rbxl"); // Compare the root's children (DataModel children) - compare_children(&dom_a, dom_a.root_ref(), &dom_b, dom_b.root_ref(), test_name); + compare_children( + &dom_a, + dom_a.root_ref(), + &dom_b, + dom_b.root_ref(), + test_name, + ); } /// Recursively compare children of two instances.