From 2e6f909a7185c7395186e1bc7a1a4678b79d5ea4 Mon Sep 17 00:00:00 2001 From: Joe Date: Sun, 1 Feb 2026 19:27:53 -0600 Subject: [PATCH] Fix syncback process to handle directory paths and class transitions in clean mode - Resolved an issue where syncback failed for projects with `$path` pointing to single files by ensuring only directory paths are scanned for orphans. - Implemented a function to create missing directories in clean mode, allowing restoration of deleted directories. - Enhanced class transition handling in clean mode to allow recoverable transitions between Folder and Script types. - Updated logging and error handling to improve clarity during the syncback process. --- .../syncback_roundtrip_tests_f980a65b.plan.md | 27 +++- src/cli/syncback.rs | 52 ++++++++ src/snapshot_middleware/project.rs | 115 +++++++++++++++--- src/syncback/mod.rs | 17 ++- src/syncback/snapshot.rs | 10 ++ 5 files changed, 194 insertions(+), 27 deletions(-) diff --git a/.cursor/plans/syncback_roundtrip_tests_f980a65b.plan.md b/.cursor/plans/syncback_roundtrip_tests_f980a65b.plan.md index 74bb674e..2f4d2188 100644 --- a/.cursor/plans/syncback_roundtrip_tests_f980a65b.plan.md +++ b/.cursor/plans/syncback_roundtrip_tests_f980a65b.plan.md @@ -94,13 +94,34 @@ This skipped ANY file whose parent directory was in `added_paths`, but adding a **Affected tests**: `clean_removes_duplicate_extensions`, `clean_removes_multiple_duplicates` +### Bug 5: File Paths Added to Directory Scan (FIXED) + +**Symptom**: Projects with `$path` pointing to single files (not directories) were failing syncback. + +**Root cause**: The `collect_paths_from_project` function was adding all `$path` entries to the scan list, but only directories should be scanned for orphans. + +**Fix applied**: Changed to only add `$path` entries that are directories (`resolved.is_dir()`). + --- -## Test Results Summary (Current) +## Test Results Summary (Final) -**Passing: 15 tests** - orphan removal, basic clean mode behavior +**Full test suite: 179 passed, 16 failed** -**Failing: 11 tests** - class change scenarios, ambiguous files, meta files, combination tests +**Clean Mode Stress Tests: 15 passed, 11 failed** + +- Passing: All orphan removal tests, basic clean mode behavior +- Failing: Class change scenarios (3), ambiguous files (2), meta/extension issues (3), combination tests (3) + +**Pre-existing Failures (not from this work): 5 tests** + +- `syncback_roundtrip::infer_service_name` +- `syncback_roundtrip::init_meta_class_name` +- `syncback_roundtrip::init_meta_properties` +- `syncback_roundtrip::rbxmx_ref` +- `syncback::ignore_trees_removing` + +These were failing before the current changes and are separate issues. --- diff --git a/src/cli/syncback.rs b/src/cli/syncback.rs index 627a4a94..acfbf394 100644 --- a/src/cli/syncback.rs +++ b/src/cli/syncback.rs @@ -28,6 +28,52 @@ use super::{ GlobalOptions, }; +/// In clean mode, ensures all $path directories exist before loading the project. +/// This allows syncback to restore deleted directories. +fn ensure_project_paths_exist(project_path: &Path, vfs: &Vfs) -> anyhow::Result<()> { + // Load the project file (handles both file and directory paths) + let project = Project::load_initial_project(vfs, project_path) + .with_context(|| format!("Failed to load project file: {}", project_path.display()))?; + + let base_path = project.folder_location(); + + // Recursively find all $path entries in the project tree + fn collect_paths(node: &crate::project::ProjectNode, base: &Path, paths: &mut Vec) { + if let Some(ref path_node) = node.path { + let full_path = if path_node.path().is_absolute() { + path_node.path().to_path_buf() + } else { + base.join(path_node.path()) + }; + paths.push(full_path); + } + for child in node.children.values() { + collect_paths(child, base, paths); + } + } + + let mut paths_to_check = Vec::new(); + collect_paths(&project.tree, base_path, &mut paths_to_check); + + // Create missing directories + for path in paths_to_check { + if !path.exists() { + log::debug!( + "Clean mode: creating missing $path directory: {}", + path.display() + ); + std::fs::create_dir_all(&path).with_context(|| { + format!( + "Failed to create directory for $path: {}", + path.display() + ) + })?; + } + } + + Ok(()) +} + const UNKNOWN_INPUT_KIND_ERR: &str = "Could not detect what kind of file was inputted. \ Expected input file to end in .rbxl, .rbxlx, .rbxm, or .rbxmx."; @@ -139,6 +185,12 @@ impl SyncbackCommand { // watcher errors shouldn't terminate the process let vfs = Vfs::new_oneshot(); + // In clean mode, ensure all $path directories exist before loading + // the project. This allows syncback to restore deleted directories. + if !self.incremental { + ensure_project_paths_exist(&path_old, &vfs)?; + } + let project_start_timer = Instant::now(); let session_old = ServeSession::new(vfs, path_old.clone())?; log::debug!( diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index b08488c9..874aa06b 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -27,6 +27,34 @@ use crate::{ use super::snapshot_from_vfs; +/// Checks if a class transition is recoverable in clean mode. +/// +/// Recoverable transitions: +/// - Folder -> Script (creates init.luau) +/// - Script -> Folder (removes init.luau) +/// - Script -> Script (changes init.server/client.luau type) +fn can_transition_class(old_class: &str, new_class: &str) -> bool { + let is_script = |c: &str| matches!(c, "ModuleScript" | "Script" | "LocalScript"); + let is_folder = |c: &str| c == "Folder"; + + // Folder can become a script (by creating init file) + if is_folder(old_class) && is_script(new_class) { + return true; + } + + // Script can become Folder (by removing init file - handled by orphan removal) + if is_script(old_class) && is_folder(new_class) { + return true; + } + + // Script can change type (e.g., ModuleScript -> LocalScript) + if is_script(old_class) && is_script(new_class) { + return true; + } + + false +} + pub fn snapshot_project( context: &InstanceContext, vfs: &Vfs, @@ -344,14 +372,26 @@ pub fn syncback_project<'sync>( while let Some((node, old_inst, new_inst)) = node_queue.pop_front() { log::debug!("Processing node {}", old_inst.name()); if old_inst.class_name() != new_inst.class { - anyhow::bail!( - "Cannot change the class of {} in project file {}.\n\ - Current class is {}, it is a {} in the input file.", - old_inst.name(), - project_path.display(), - 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()); + if !snapshot.data.is_incremental() && can_recover { + log::debug!( + "Clean mode: allowing class transition {} -> {} for {}", + old_inst.class_name(), + new_inst.class, + old_inst.name() + ); + } else { + anyhow::bail!( + "Cannot change the class of {} in project file {}.\n\ + Current class is {}, it is a {} in the input file.", + old_inst.name(), + project_path.display(), + old_inst.class_name(), + new_inst.class + ); + } } // TODO handle meta.json5 files in this branch. Right now, we perform @@ -372,20 +412,65 @@ pub fn syncback_project<'sync>( base_path.join(node_path) }; - let middleware = match Middleware::middleware_for_path( + let mut middleware = match Middleware::middleware_for_path( snapshot.vfs(), &project.sync_rules, &full_path, )? { Some(middleware) => middleware, - // The only way this can happen at this point is if the path does - // not exist on the file system or there's no middleware for it. - None => anyhow::bail!( - "path does not exist or could not be turned into a file Rojo understands: {}", - full_path.display() - ), + None => { + // Path doesn't exist on filesystem. In clean mode, we can + // determine the middleware from the new instance and let + // syncback create the necessary files/directories. + if !snapshot.data.is_incremental() { + // Determine middleware based on new instance class + let inferred_middleware = match new_inst.class.as_str() { + "ModuleScript" => Middleware::ModuleScriptDir, + "Script" => Middleware::ServerScriptDir, + "LocalScript" => Middleware::ClientScriptDir, + "Folder" => Middleware::Dir, + // For other classes, default to Dir + _ => Middleware::Dir, + }; + log::debug!( + "Clean mode: path {} doesn't exist, inferring {:?} middleware for class {}", + full_path.display(), + inferred_middleware, + new_inst.class + ); + inferred_middleware + } else { + anyhow::bail!( + "Rojo project referred to a file using $path that could not be turned \ + into a Roblox Instance by Rojo.\n\ + File $path: {}", + full_path.display() + ) + } + } }; + // In clean mode, when the filesystem has a directory (Dir middleware) + // but the new instance is a script, we need to use the appropriate + // script-dir middleware to create the init file and restore the script. + if !snapshot.data.is_incremental() && middleware == Middleware::Dir { + let script_middleware = match new_inst.class.as_str() { + "ModuleScript" => Some(Middleware::ModuleScriptDir), + "Script" => Some(Middleware::ServerScriptDir), + "LocalScript" => Some(Middleware::ClientScriptDir), + _ => None, + }; + if let Some(new_middleware) = script_middleware { + log::debug!( + "Clean mode: overriding Dir->{:?} middleware for {} (class {})", + new_middleware, + full_path.display(), + new_inst.class + ); + middleware = new_middleware; + } + } + descendant_snapshots.push( snapshot .with_new_path(full_path.clone(), new_inst.referent(), Some(old_inst.id())) diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 1de12431..b188eba1 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -230,17 +230,20 @@ pub fn syncback_loop_with_stats( // 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 + // 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. fn collect_paths_from_project( node: &crate::project::ProjectNode, base_path: &Path, paths: &mut Vec, ) { - // If this node has a $path, add it + // If this node has a $path that points to a directory, 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) { + // Only add directories - single files don't need orphan scanning + if resolved.is_dir() && !paths.contains(&resolved) { + log::trace!("Found $path directory in project: {}", resolved.display()); paths.push(resolved); } } @@ -336,12 +339,8 @@ pub fn syncback_loop_with_stats( for dir in &dirs_to_scan { if dir.is_dir() { scan_directory(dir, &mut paths, &ignore_patterns, project_path); - } else if dir.exists() { - // It's a file, add it directly - if is_valid_path(&ignore_patterns, project_path, dir) { - paths.insert(dir.clone()); - } } + // Note: We only add directories to dirs_to_scan now, so no else branch needed } log::debug!("Scanned {} existing paths from filesystem", paths.len()); diff --git a/src/syncback/snapshot.rs b/src/syncback/snapshot.rs index c56ee2a5..a4bb17b0 100644 --- a/src/syncback/snapshot.rs +++ b/src/syncback/snapshot.rs @@ -30,6 +30,16 @@ pub struct SyncbackData<'sync> { pub(super) stats: &'sync SyncbackStats, } +impl<'sync> SyncbackData<'sync> { + /// Returns whether syncback is running in incremental mode. + /// In incremental mode, existing file structure is preserved. + /// In clean mode (non-incremental), all children are treated as new. + #[inline] + pub fn is_incremental(&self) -> bool { + self.incremental + } +} + pub struct SyncbackSnapshot<'sync> { pub data: SyncbackData<'sync>, pub old: Option,