diff --git a/.cursor/plans/fix_stem-level_dedup_8fa9a015.plan.md b/.cursor/plans/fix_stem-level_dedup_8fa9a015.plan.md new file mode 100644 index 00000000..545cbf80 --- /dev/null +++ b/.cursor/plans/fix_stem-level_dedup_8fa9a015.plan.md @@ -0,0 +1,124 @@ +--- +name: Fix stem-level dedup +overview: "Fix the real defect where deduplicate_name compares bare slugs against full filenames (with extensions), making it blind to file-format collisions. The fix: return the dedup key (bare slug) from name_for_inst so callers never touch extensions, and seed from tree children's slugified names instead of disk filenames." +todos: + - id: return-dedup-key + content: Change name_for_inst to return (filename, needs_meta, dedup_key) — the bare slug that goes into taken_names + status: completed + - id: fix-dir + content: "dir.rs: remove disk seeding, insert dedup_key (not filename) into taken_names" + status: completed + - id: fix-project + content: "project.rs: seed from slugified old-tree children, insert dedup_key into taken_names" + status: completed + - id: fix-api + content: "api.rs: seed taken_names from tree siblings' slugified names instead of fs::read_dir" + status: completed + - id: fix-tests + content: Update file_names.rs and api.rs tests to assert correct file-format dedup + status: in_progress +isProject: false +--- + +# Fix dedup key propagation (no extension awareness outside file_names.rs) + +## The Defect + +`deduplicate_name` compares bare slugs against `taken_names`. But callers populate `taken_names` from filesystem entries (`vfs.read_dir` / `fs::read_dir`), which have extensions. Bare slug `"helper"` does not match `"helper.luau"`, so file-format collisions go undetected. + +```mermaid +sequenceDiagram + participant Caller as dir.rs / api.rs + participant NFI as name_for_inst + participant Dedup as deduplicate_name + + Caller->>Caller: seed taken = {"helper.luau"} from disk + Caller->>NFI: name_for_inst("Helper", taken) + NFI->>NFI: slug = "Helper" + NFI->>Dedup: deduplicate_name("Helper", {"helper.luau"}) + Dedup-->>NFI: "Helper" (no match!) + NFI-->>Caller: "Helper.luau" + Caller->>Caller: OVERWRITES existing Helper.luau +``` + +## Root Cause + +The callers violate the `taken_names` contract. Per the original plan: `taken_names` should contain **claimed names** (bare slugs), not filenames with extensions. The defect is in what goes INTO the set, not in `deduplicate_name` itself. + +## The Fix (zero extension awareness outside file_names.rs) + +Two changes, both inside the existing architecture: + +### 1. `name_for_inst` returns the dedup key + +`name_for_inst` already computes the bare slug (`deduped`) internally. For file middleware, it appends the extension AFTER dedup to produce the filename. We expose the pre-extension slug as a third return value so callers can insert it into `taken_names`. + +``` +Current: (filename, needs_meta) +Proposed: (filename, needs_meta, dedup_key) +``` + +- **Old inst path:** `dedup_key` = the slug portion of the existing filename. `name_for_inst` already receives `middleware`, so it can strip `extension_for_middleware(middleware)` from the filename. This is not "extension stripping scattered around" -- it is inside the one function that owns extension logic, using `extension_for_middleware` which already exists. +- **New Dir inst:** `dedup_key` = `deduped` (same as filename, no extension) +- **New File inst:** `dedup_key` = `deduped` (the slug before `.{extension}` was appended) + +Callers insert `dedup_key.to_lowercase()` into `taken_names`. They never see or touch an extension. + +### 2. Replace disk seeding with tree-based seeding + +Instead of reading filenames from disk (which contain extensions), seed `taken_names` from the **existing tree's children** by slugifying their instance names: + +```rust +// Seed from old tree children's slugified instance names +for child_ref in parent.children() { + let child = get_instance(child_ref); + let slug = if name_needs_slugify(&child.name) { + slugify_name(&child.name).to_lowercase() + } else { + child.name.to_lowercase() + }; + taken_names.insert(slug); +} +``` + +This is architecturally correct -- instance names are the authoritative source, and slugifying them produces the same bare slugs that `name_for_inst` would produce. No filesystem reads, no extensions, no parsing. + +**What about orphaned files?** Files on disk that are not in the tree (manually added, from other tools) will not be in `taken_names`. A new instance could overwrite them. This is acceptable -- those files are not part of Rojo's model. The old `encode_path_name` system had the same behavior, and upstream PR #1187 does not do disk seeding either. + +## Affected Files + +### [src/syncback/file_names.rs](src/syncback/file_names.rs) + +- Change `name_for_inst` return type to include `dedup_key: String` +- Old inst path: derive `dedup_key` by stripping `.{extension_for_middleware(middleware)}` suffix from the filename (single strip, inside the function that already owns extensions) +- New inst paths: `dedup_key` = the `deduped` variable (already computed, just return it) + +### [src/syncback/snapshot.rs](src/syncback/snapshot.rs) + +- `with_joined_path` and `with_base_path` call `name_for_inst` and return the result. Update them to propagate the `dedup_key` so callers in dir.rs/project.rs can use it. + +### [src/snapshot_middleware/dir.rs](src/snapshot_middleware/dir.rs) + +- Remove `vfs.read_dir` seeding entirely +- The loop already processes old+new children interleaved; old children go through `name_for_inst`'s old-inst path, which returns the dedup_key. Accumulate `dedup_key` instead of `file_name()` +- New children's dedup_keys are also accumulated + +### [src/snapshot_middleware/project.rs](src/snapshot_middleware/project.rs) + +- Remove `vfs.read_dir` seeding in `or_insert_with` +- Accumulate `dedup_key` from `with_base_path` results instead of `file_name()` + +### [src/web/api.rs](src/web/api.rs) + +- Replace `fs::read_dir` seeding with tree-sibling slugified names +- `parent_instance.children()` gives child refs, `tree.get_instance(ref)` gives instance names, slugify them into `taken_names` + +### Tests + +- Fix the "limitation documentation" tests to assert correct dedup behavior for file-format instances +- The nightmare tests that use `process_new_children_with_disk_seed` switch to tree-style seeding with bare slugs +- The `twoway_sync_dedup` tests test file-format collisions properly + +## Edge Case: Old inst with unknown middleware + +For old instances, `name_for_inst` receives `middleware` as a parameter (determined by `get_best_middleware`). It uses `extension_for_middleware(middleware)` to derive the dedup_key. For Dir middleware, extension is empty, so dedup_key = filename. For all file middleware, the extension is known and strippable. There is no "unknown extension" case. diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index aa1bc860..2ab10ba7 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -138,24 +138,12 @@ pub fn syncback_dir_no_meta<'sync>( let mut children = Vec::new(); let mut removed_children = Vec::new(); - // Seed taken_names with existing files on disk so new children don't - // collide with files already present in the directory. - let mut taken_names: HashSet = snapshot - .vfs() - .read_dir(&snapshot.path) - .ok() - .map(|entries| { - entries - .filter_map(|e| e.ok()) - .filter_map(|e| { - e.path() - .file_name() - .and_then(|n| n.to_str()) - .map(|n| n.to_lowercase()) - }) - .collect() - }) - .unwrap_or_default(); + // taken_names tracks claimed bare slugs (without extensions) for dedup. + // It is NOT seeded from disk filenames — those contain extensions that + // would cause mismatches with bare-slug comparisons. Instead, old + // children's dedup_keys are accumulated as they are processed in the + // loop below (via name_for_inst's third return value). + let mut taken_names: HashSet = HashSet::new(); // Detect duplicate child names (case-insensitive for file system safety). // We skip duplicates instead of failing, tracking them in stats. @@ -239,22 +227,18 @@ pub fn syncback_dir_no_meta<'sync>( continue; } // This child exists in both doms. Pass it on. - let (child_snap, _needs_meta) = snapshot.with_joined_path( + let (child_snap, _needs_meta, dedup_key) = snapshot.with_joined_path( *new_child_ref, Some(old_child.id()), &taken_names, )?; - if let Some(name) = child_snap.path.file_name().and_then(|n| n.to_str()) { - taken_names.insert(name.to_lowercase()); - } + taken_names.insert(dedup_key.to_lowercase()); children.push(child_snap); } else { // The child only exists in the the new dom - let (child_snap, _needs_meta) = + let (child_snap, _needs_meta, dedup_key) = snapshot.with_joined_path(*new_child_ref, None, &taken_names)?; - if let Some(name) = child_snap.path.file_name().and_then(|n| n.to_str()) { - taken_names.insert(name.to_lowercase()); - } + taken_names.insert(dedup_key.to_lowercase()); children.push(child_snap); } } @@ -299,11 +283,9 @@ pub fn syncback_dir_no_meta<'sync>( if snapshot.should_ignore_tree(*new_child_ref) { continue; } - let (child_snap, _needs_meta) = + let (child_snap, _needs_meta, dedup_key) = snapshot.with_joined_path(*new_child_ref, None, &taken_names)?; - if let Some(name) = child_snap.path.file_name().and_then(|n| n.to_str()) { - taken_names.insert(name.to_lowercase()); - } + taken_names.insert(dedup_key.to_lowercase()); children.push(child_snap); } } diff --git a/src/snapshot_middleware/json_model.rs b/src/snapshot_middleware/json_model.rs index 96056e46..8b2284fd 100644 --- a/src/snapshot_middleware/json_model.rs +++ b/src/snapshot_middleware/json_model.rs @@ -77,6 +77,11 @@ pub fn syncback_json_model<'sync>( // could have a performance cost. model.schema = old_inst.metadata().schema.clone(); model.name = old_inst.metadata().specified_name.clone(); + } else if snapshot.needs_meta_name { + // New instance whose filesystem name differs from the instance name + // (due to slugification or deduplication). Store the real name so it + // survives a round-trip through the filesystem. + model.name = Some(snapshot.new_inst().name.clone()); } let serialized = match crate::json::to_vec_pretty_sorted(&model) { diff --git a/src/snapshot_middleware/meta_file.rs b/src/snapshot_middleware/meta_file.rs index 6a1ca9e2..c03a55c0 100644 --- a/src/snapshot_middleware/meta_file.rs +++ b/src/snapshot_middleware/meta_file.rs @@ -13,7 +13,7 @@ use crate::{ json, resolution::UnresolvedValue, snapshot::InstanceSnapshot, - syncback::{name_needs_slugify, SyncbackSnapshot}, + syncback::SyncbackSnapshot, RojoRef, }; @@ -165,15 +165,11 @@ impl AdjacentMetadata { .old_inst() .and_then(|inst| inst.metadata().specified_name.clone()) .or_else(|| { - // If this is a new instance and its name is invalid for the filesystem, - // we need to specify the name in meta.json so it can be preserved - if snapshot.old_inst().is_none() { - let instance_name = &snapshot.new_inst().name; - if name_needs_slugify(instance_name) { - Some(instance_name.clone()) - } else { - None - } + // If this is a new instance and its filesystem name differs from + // the instance name (due to slugification or deduplication), we + // need to specify the name in meta.json so it can be preserved. + if snapshot.old_inst().is_none() && snapshot.needs_meta_name { + Some(snapshot.new_inst().name.clone()) } else { None } @@ -431,15 +427,11 @@ impl DirectoryMetadata { .old_inst() .and_then(|inst| inst.metadata().specified_name.clone()) .or_else(|| { - // If this is a new instance and its name is invalid for the filesystem, - // we need to specify the name in meta.json so it can be preserved - if snapshot.old_inst().is_none() { - let instance_name = &snapshot.new_inst().name; - if name_needs_slugify(instance_name) { - Some(instance_name.clone()) - } else { - None - } + // If this is a new instance and its filesystem name differs from + // the instance name (due to slugification or deduplication), we + // need to specify the name in meta.json so it can be preserved. + if snapshot.old_inst().is_none() && snapshot.needs_meta_name { + Some(snapshot.new_inst().name.clone()) } else { None } diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index bee9b971..9f04fc0e 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -648,8 +648,8 @@ pub fn syncback_project<'sync>( // All of the children in this loop are by their nature not in the // project, so we just need to run syncback on them. // - // Track taken names per parent directory so siblings under the same - // parent deduplicate against each other (and against existing files). + // Track taken names (bare slugs) per parent directory so siblings + // under the same parent deduplicate against each other. let mut taken_names_per_dir: std::collections::HashMap< std::path::PathBuf, std::collections::HashSet, @@ -690,35 +690,14 @@ pub fn syncback_project<'sync>( if parent_middleware != Middleware::Project { let taken_names = taken_names_per_dir .entry(parent_path.clone()) - .or_insert_with(|| { - // Seed with existing files on disk so new children - // don't collide with files already present. - vfs.read_dir(&parent_path) - .ok() - .map(|entries| { - entries - .filter_map(|e| e.ok()) - .filter_map(|e| { - e.path() - .file_name() - .and_then(|n| n.to_str()) - .map(|n| n.to_lowercase()) - }) - .collect() - }) - .unwrap_or_default() - }); - let (child_snap, _needs_meta) = snapshot.with_base_path( + .or_insert_with(std::collections::HashSet::new); + let (child_snap, _needs_meta, dedup_key) = snapshot.with_base_path( &parent_path, new_child.referent(), None, taken_names, )?; - if let Some(file_name) = - child_snap.path.file_name().and_then(|n| n.to_str()) - { - taken_names.insert(file_name.to_lowercase()); - } + taken_names.insert(dedup_key.to_lowercase()); descendant_snapshots.push(child_snap); } } diff --git a/src/syncback/file_names.rs b/src/syncback/file_names.rs index 09258883..c9955126 100644 --- a/src/syncback/file_names.rs +++ b/src/syncback/file_names.rs @@ -9,25 +9,37 @@ use rbx_dom_weak::Instance; use crate::{snapshot::InstanceWithMeta, snapshot_middleware::Middleware}; -/// Generates a filesystem name for an instance. Returns `(filename, needs_meta_name)`. +/// Generates a filesystem name for an instance. +/// Returns `(filename, needs_meta_name, dedup_key)`. /// -/// - If `old_inst` exists, its existing path is preserved (incremental mode). -/// - For new instances, names with forbidden chars are slugified and deduplicated -/// against `taken_names`. The `bool` is `true` when the filesystem name differs -/// from the instance name (meaning a `name` field must be written in metadata). +/// - `filename`: The full filesystem name (including extension for file middleware). +/// - `needs_meta_name`: `true` when the filesystem name differs from the instance +/// name (meaning a `name` field must be written in metadata). +/// - `dedup_key`: The bare slug (without extension) that callers must insert into +/// `taken_names`. This is the name-level identifier used for collision detection. +/// Callers should **always** use `dedup_key` (not `filename`) when accumulating +/// into `taken_names`. +/// +/// If `old_inst` exists, its existing path is preserved (incremental mode). +/// For new instances, names with forbidden chars are slugified and deduplicated +/// against `taken_names`. pub fn name_for_inst<'a>( middleware: Middleware, new_inst: &'a Instance, old_inst: Option>, taken_names: &HashSet, -) -> anyhow::Result<(Cow<'a, str>, bool)> { +) -> anyhow::Result<(Cow<'a, str>, bool, String)> { if let Some(old_inst) = old_inst { if let Some(source) = old_inst.metadata().relevant_paths.first() { let name = source .file_name() .and_then(|s| s.to_str()) .context("sources on the file system should be valid unicode and not be stubs")?; - Ok((Cow::Borrowed(name), false)) + // Derive dedup_key by stripping the middleware extension from the + // filename. For Dir middleware the extension is empty so this is a + // no-op. This keeps extension logic inside this function. + let dedup_key = strip_middleware_extension(name, middleware); + Ok((Cow::Borrowed(name), false, dedup_key)) } else { anyhow::bail!( "members of 'old' trees should have an instigating source. Somehow, {} did not.", @@ -54,18 +66,44 @@ pub fn name_for_inst<'a>( | Middleware::LegacyScriptDir => { let deduped = deduplicate_name(&base, taken_names); let needs_meta = needs_slugify || deduped != base; - Ok((Cow::Owned(deduped), needs_meta)) + let dedup_key = deduped.clone(); + Ok((Cow::Owned(deduped), needs_meta, dedup_key)) } _ => { let extension = extension_for_middleware(middleware); let deduped = deduplicate_name(&base, taken_names); let needs_meta = needs_slugify || deduped != base; - Ok((Cow::Owned(format!("{deduped}.{extension}")), needs_meta)) + let dedup_key = deduped.clone(); + Ok((Cow::Owned(format!("{deduped}.{extension}")), needs_meta, dedup_key)) } } } } +/// Strips the middleware extension from a filename to recover the bare slug. +/// Used internally by `name_for_inst` to derive the dedup key for old instances. +fn strip_middleware_extension(filename: &str, middleware: Middleware) -> String { + // Dir middleware has no extension — filename IS the slug + match middleware { + Middleware::Dir + | Middleware::CsvDir + | Middleware::ServerScriptDir + | Middleware::ClientScriptDir + | Middleware::ModuleScriptDir + | Middleware::PluginScriptDir + | Middleware::LocalScriptDir + | Middleware::LegacyScriptDir => filename.to_string(), + _ => { + let ext = extension_for_middleware(middleware); + let suffix = format!(".{ext}"); + filename + .strip_suffix(&suffix) + .unwrap_or(filename) + .to_string() + } + } +} + /// Returns the extension a provided piece of middleware is supposed to use. pub fn extension_for_middleware(middleware: Middleware) -> &'static str { match middleware { @@ -652,7 +690,7 @@ mod tests { let child = dom.get_by_ref(child_ref).unwrap(); let taken = HashSet::new(); - let (filename, needs_meta) = + let (filename, needs_meta, _dk) = name_for_inst(Middleware::ModuleScript, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "MyModule.luau"); assert!(!needs_meta); @@ -665,7 +703,7 @@ mod tests { let child = dom.get_by_ref(child_ref).unwrap(); let taken = HashSet::new(); - let (filename, needs_meta) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap(); + let (filename, needs_meta, _dk) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "MyFolder"); assert!(!needs_meta); } @@ -677,7 +715,7 @@ mod tests { let child = dom.get_by_ref(child_ref).unwrap(); let taken = HashSet::new(); - let (filename, needs_meta) = + let (filename, needs_meta, _dk) = name_for_inst(Middleware::ModuleScript, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "Hey_Bro.luau"); assert!(needs_meta, "slug differs from real name, needs meta"); @@ -690,7 +728,7 @@ mod tests { let child = dom.get_by_ref(child_ref).unwrap(); let taken = HashSet::new(); - let (filename, needs_meta) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap(); + let (filename, needs_meta, _dk) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "Hey_Bro"); assert!(needs_meta); } @@ -702,7 +740,7 @@ mod tests { let child = dom.get_by_ref(child_ref).unwrap(); let taken: HashSet = ["foo".to_string()].into_iter().collect(); - let (filename, needs_meta) = + let (filename, needs_meta, _dk) = name_for_inst(Middleware::ModuleScript, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "Foo~1.luau"); assert!(needs_meta, "deduped name differs from original"); @@ -715,7 +753,7 @@ mod tests { let child = dom.get_by_ref(child_ref).unwrap(); let taken: HashSet = ["stuff".to_string()].into_iter().collect(); - let (filename, needs_meta) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap(); + let (filename, needs_meta, _dk) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "Stuff~1"); assert!(needs_meta); } @@ -728,7 +766,7 @@ mod tests { let child = dom.get_by_ref(child_ref).unwrap(); let taken: HashSet = ["hey_bro".to_string()].into_iter().collect(); - let (filename, needs_meta) = + let (filename, needs_meta, _dk) = name_for_inst(Middleware::ModuleScript, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "Hey_Bro~1.luau"); assert!(needs_meta); @@ -741,7 +779,7 @@ mod tests { let child = dom.get_by_ref(child_ref).unwrap(); let taken = HashSet::new(); - let (filename, needs_meta) = + let (filename, needs_meta, _dk) = name_for_inst(Middleware::ServerScript, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "Main.server.luau"); assert!(!needs_meta); @@ -754,7 +792,7 @@ mod tests { let child = dom.get_by_ref(child_ref).unwrap(); let taken = HashSet::new(); - let (filename, needs_meta) = + let (filename, needs_meta, _dk) = name_for_inst(Middleware::ClientScript, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "Client.client.luau"); assert!(!needs_meta); @@ -767,7 +805,7 @@ mod tests { let child = dom.get_by_ref(child_ref).unwrap(); let taken = HashSet::new(); - let (filename, needs_meta) = name_for_inst(Middleware::Text, child, None, &taken).unwrap(); + let (filename, needs_meta, _dk) = name_for_inst(Middleware::Text, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "Readme.txt"); assert!(!needs_meta); } @@ -788,7 +826,7 @@ mod tests { let child_ref = dom.root().children()[0]; let child = dom.get_by_ref(child_ref).unwrap(); let taken_set: HashSet = taken.iter().map(|s| s.to_string()).collect(); - let (filename, needs_meta) = name_for_inst(mw, child, None, &taken_set).unwrap(); + let (filename, needs_meta, _dk) = name_for_inst(mw, child, None, &taken_set).unwrap(); (filename.into_owned(), needs_meta) } @@ -1081,9 +1119,9 @@ mod tests { let dom = make_inst(name, "Folder"); let child_ref = dom.root().children()[0]; let child = dom.get_by_ref(child_ref).unwrap(); - let (filename, needs_meta) = + let (filename, needs_meta, dedup_key) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap(); - taken.insert(filename.to_lowercase().to_string()); + taken.insert(dedup_key.to_lowercase()); results.push((name.to_string(), filename.into_owned(), needs_meta)); } results @@ -1411,7 +1449,7 @@ mod tests { disk_entries: &[&str], new_children: &[(&str, Middleware)], ) -> Vec<(String, String, bool)> { - // Seed from "disk" (lowercased, as the fix does) + // Seed from bare slugs (simulates tree-based seeding) let mut taken: HashSet = disk_entries .iter() .map(|e| e.to_lowercase()) @@ -1421,9 +1459,9 @@ mod tests { let dom = make_inst(name, "ModuleScript"); let child_ref = dom.root().children()[0]; let child = dom.get_by_ref(child_ref).unwrap(); - let (filename, needs_meta) = + let (filename, needs_meta, dedup_key) = name_for_inst(mw, child, None, &taken).unwrap(); - taken.insert(filename.to_lowercase().to_string()); + taken.insert(dedup_key.to_lowercase()); results.push((name.to_string(), filename.into_owned(), needs_meta)); } results @@ -1497,46 +1535,30 @@ mod tests { } #[test] - fn disk_seed_file_middleware_dedup_compares_bare_slugs() { - // IMPORTANT: For file middleware, deduplicate_name compares the - // bare slug against taken_names. Disk entries with extensions - // (e.g., "mymodule.luau") do NOT collide with the bare slug - // "MyModule" because "mymodule" ≠ "mymodule.luau". - // - // This is by design — duplicate-named children are already skipped - // by dir.rs's duplicate detection before reaching name_for_inst. - // File middleware dedup only catches slug collisions between - // DIFFERENT names (e.g., "A/B" and "A:B" both slugify to "A_B"). + fn disk_seed_file_middleware_collides_with_bare_slug() { + // taken_names now uses bare slugs (dedup_keys), so file-format + // dedup works correctly. "mymodule" in taken matches slug "MyModule". let r = process_new_children_with_disk_seed( - &["mymodule.luau"], + &["mymodule"], &[("MyModule", Middleware::ModuleScript)], ); - // Bare slug "mymodule" ≠ "mymodule.luau" in taken_names. - // No collision detected — this is the expected behavior. - assert_eq!(r[0].1, "MyModule.luau"); - assert!(!r[0].2); + assert_eq!(r[0].1, "MyModule~1.luau"); + assert!(r[0].2, "dedup suffix means needs meta"); } #[test] - fn disk_seed_file_middleware_sibling_slug_collision_works() { + fn disk_seed_file_middleware_sibling_slug_collision_deduped() { // Two file-middleware children whose names slugify to the same base. - // The FIRST child's full filename gets added to taken_names, - // and the second child's bare slug doesn't match that filename. - // However, bare slugs DO collide with each other through accumulation: - // First: slug "A_B" → not in taken → filename "A_B.luau" → taken gets "a_b.luau" - // Second: slug "A_B" → "a_b" not in taken (has "a_b.luau") → filename "A_B.luau" - // This demonstrates that file-middleware sibling dedup relies on the - // duplicate-name pre-filter in dir.rs (which skips same-named children). + // Since taken_names accumulates dedup_keys (bare slugs), the second + // child correctly detects the collision and gets ~1. let r = process_new_children_with_disk_seed( &[], &[("A/B", Middleware::ModuleScript), ("A:B", Middleware::ModuleScript)], ); - // Both get the same filename because bare slug dedup doesn't - // catch full-filename entries. This is acceptable because in real - // dir.rs, children with different instance names but same slug - // would need the full duplicate-name resolution system (Phase 2). assert_eq!(r[0].1, "A_B.luau"); - assert_eq!(r[1].1, "A_B.luau"); + assert!(r[0].2); + assert_eq!(r[1].1, "A_B~1.luau"); + assert!(r[1].2); } #[test] @@ -1619,9 +1641,9 @@ mod tests { let dom = make_inst(name, "ModuleScript"); let child_ref = dom.root().children()[0]; let child = dom.get_by_ref(child_ref).unwrap(); - let (filename, needs_meta) = + let (filename, needs_meta, dedup_key) = name_for_inst(mw, child, None, &taken).unwrap(); - taken.insert(filename.to_lowercase().to_string()); + taken.insert(dedup_key.to_lowercase()); results.push((name.to_string(), filename.into_owned(), needs_meta)); } results @@ -1889,9 +1911,9 @@ mod tests { #[test] fn nightmare_mixed_middleware_file_then_dir() { // Reverse order: ModuleScript first, Dir second. - // ModuleScript "Shared" → filename "Shared.luau" → taken gets "shared.luau" - // Dir "Shared" → slug "Shared" → dedup checks "shared" → NOT in taken - // ("shared" ≠ "shared.luau") → filename "Shared" → taken gets "shared" + // ModuleScript "Shared" → dedup_key "shared" → taken gets "shared" + // Dir "Shared" → slug "Shared" → dedup checks "shared" → MATCH + // (dedup_key-based taken catches this correctly) → "Shared~1" let r = process_new_children_with_disk_seed( &[], &[ @@ -1901,9 +1923,9 @@ mod tests { ); assert_eq!(r[0].1, "Shared.luau"); assert!(!r[0].2); - // Dir slug "Shared" doesn't collide with "shared.luau" in taken - assert_eq!(r[1].1, "Shared"); - assert!(!r[1].2); + // Dir slug "Shared" collides with dedup_key "shared" from the ModuleScript + assert_eq!(r[1].1, "Shared~1"); + assert!(r[1].2, "dedup suffix means different from instance name"); } #[test] @@ -2100,7 +2122,7 @@ mod tests { let child_ref = dom.root().children()[0]; let child = dom.get_by_ref(child_ref).unwrap(); let taken = HashSet::new(); - let (filename, needs_meta) = + let (filename, needs_meta, _dk) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap(); if needs_meta { assert_ne!( diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 27651213..bb1eb019 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -451,6 +451,7 @@ pub fn syncback_loop_with_stats( new: new_tree.root_ref(), path: project.file_location.clone(), middleware: Some(Middleware::Project), + needs_meta_name: false, }]; let mut fs_snapshot = FsSnapshot::new(); diff --git a/src/syncback/snapshot.rs b/src/syncback/snapshot.rs index 3c7179d6..e2cd018b 100644 --- a/src/syncback/snapshot.rs +++ b/src/syncback/snapshot.rs @@ -47,6 +47,10 @@ pub struct SyncbackSnapshot<'sync> { pub new: Ref, pub path: PathBuf, pub middleware: Option, + /// When `true`, the filesystem name differs from the instance name + /// (due to slugification or deduplication) and a `name` field must be + /// written in the metadata file to preserve the real instance name. + pub needs_meta_name: bool, } impl<'sync> SyncbackSnapshot<'sync> { @@ -62,7 +66,7 @@ impl<'sync> SyncbackSnapshot<'sync> { new_ref: Ref, old_ref: Option, taken_names: &HashSet, - ) -> anyhow::Result<(Self, bool)> { + ) -> anyhow::Result<(Self, bool, String)> { // In clean mode, ignore old_ref to ensure fresh structure let effective_old_ref = if self.data.incremental { old_ref } else { None }; let mut snapshot = Self { @@ -71,17 +75,19 @@ impl<'sync> SyncbackSnapshot<'sync> { new: new_ref, path: PathBuf::new(), middleware: None, + needs_meta_name: false, }; let middleware = get_best_middleware(&snapshot); - let (name, needs_meta_name) = name_for_inst( + let (name, needs_meta_name, dedup_key) = name_for_inst( middleware, snapshot.new_inst(), snapshot.old_inst(), taken_names, )?; snapshot.path = self.path.join(&*name); + snapshot.needs_meta_name = needs_meta_name; - Ok((snapshot, needs_meta_name)) + Ok((snapshot, needs_meta_name, dedup_key)) } /// Constructs a SyncbackSnapshot from the provided refs and a base path, @@ -99,7 +105,7 @@ impl<'sync> SyncbackSnapshot<'sync> { new_ref: Ref, old_ref: Option, taken_names: &HashSet, - ) -> anyhow::Result<(Self, bool)> { + ) -> anyhow::Result<(Self, bool, String)> { // In clean mode, ignore old_ref to ensure fresh structure let effective_old_ref = if self.data.incremental { old_ref } else { None }; let mut snapshot = Self { @@ -108,17 +114,19 @@ impl<'sync> SyncbackSnapshot<'sync> { new: new_ref, path: PathBuf::new(), middleware: None, + needs_meta_name: false, }; let middleware = get_best_middleware(&snapshot); - let (name, needs_meta_name) = name_for_inst( + let (name, needs_meta_name, dedup_key) = name_for_inst( middleware, snapshot.new_inst(), snapshot.old_inst(), taken_names, )?; snapshot.path = base_path.join(&*name); + snapshot.needs_meta_name = needs_meta_name; - Ok((snapshot, needs_meta_name)) + Ok((snapshot, needs_meta_name, dedup_key)) } /// Constructs a SyncbackSnapshot with the provided path and refs while @@ -131,6 +139,7 @@ impl<'sync> SyncbackSnapshot<'sync> { new: new_ref, path, middleware: None, + needs_meta_name: false, } } diff --git a/src/web/api.rs b/src/web/api.rs index c96d6e86..995df0d9 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -899,8 +899,28 @@ impl ApiService { } }; + // Seed sibling slugs from the tree's existing children of the parent. + // This gives us bare instance-name-level slugs (no extensions) for + // accurate dedup against existing siblings. + let sibling_slugs: HashSet = { + use crate::syncback::name_needs_slugify; + parent_instance + .children() + .iter() + .filter_map(|r| tree.get_instance(*r)) + .map(|inst| { + let name = inst.name(); + if name_needs_slugify(name) { + slugify_name(name).to_lowercase() + } else { + name.to_lowercase() + } + }) + .collect() + }; + // Create the instance at the resolved path - self.syncback_instance_to_path_with_stats(added, &parent_dir, stats) + self.syncback_instance_to_path_with_stats(added, &parent_dir, stats, &sibling_slugs) } /// Update an existing instance in place instead of creating new files. @@ -1056,8 +1076,9 @@ impl ApiService { // Process children using normal syncback path // This will use detect_existing_script_format to check existing files + let child_slugs = Self::compute_sibling_slugs(&unique_children); for child in &unique_children { - self.syncback_instance_to_path_with_stats(child, &children_dir, stats)?; + self.syncback_instance_to_path_with_stats(child, &children_dir, stats, &child_slugs)?; } } } @@ -1081,8 +1102,9 @@ impl ApiService { let unique_children = self.filter_duplicate_children(&added.children, &inst_path, stats); + let child_slugs = Self::compute_sibling_slugs(&unique_children); for child in &unique_children { - self.syncback_instance_to_path_with_stats(child, existing_path, stats)?; + self.syncback_instance_to_path_with_stats(child, existing_path, stats, &child_slugs)?; } } } else { @@ -1584,17 +1606,42 @@ impl ApiService { ) -> anyhow::Result<()> { // Use a default stats tracker for backwards compatibility let stats = crate::syncback::SyncbackStats::new(); - self.syncback_instance_to_path_with_stats(added, parent_dir, &stats) + let sibling_slugs = HashSet::new(); + self.syncback_instance_to_path_with_stats(added, parent_dir, &stats, &sibling_slugs) + } + + /// Computes slugified sibling names from a slice of `AddedInstance` references. + /// Returns a `HashSet` of lowercased bare slugs suitable for `deduplicate_name`. + fn compute_sibling_slugs( + children: &[&crate::web::interface::AddedInstance], + ) -> HashSet { + use crate::syncback::name_needs_slugify; + children + .iter() + .map(|c| { + if name_needs_slugify(&c.name) { + slugify_name(&c.name).to_lowercase() + } else { + c.name.to_lowercase() + } + }) + .collect() } /// Recursively syncback an instance and its children to the filesystem. /// This is the internal implementation that handles the actual file creation. /// Uses the provided stats tracker for recording issues. + /// + /// `sibling_slugs` contains the slugified names of all siblings in the + /// parent directory (bare slugs, lowercased). This is used by + /// `deduplicate_name` to avoid collisions. Callers compute this from + /// tree children or `AddedInstance` siblings — never from disk filenames. fn syncback_instance_to_path_with_stats( &self, added: &crate::web::interface::AddedInstance, parent_dir: &std::path::Path, stats: &crate::syncback::SyncbackStats, + sibling_slugs: &HashSet, ) -> anyhow::Result<()> { use crate::syncback::{deduplicate_name, name_needs_slugify}; use anyhow::Context; @@ -1608,24 +1655,10 @@ impl ApiService { added.name.clone() }; - // Deduplicate against existing files in the parent directory to prevent - // collisions (e.g., two instances whose slugified names match, or a new - // instance whose name matches an existing file). - let taken_names: HashSet = if parent_dir.is_dir() { - fs::read_dir(parent_dir) - .ok() - .map(|entries| { - entries - .filter_map(|e| e.ok()) - .filter_map(|e| e.file_name().into_string().ok()) - .map(|n| n.to_lowercase()) - .collect() - }) - .unwrap_or_default() - } else { - HashSet::new() - }; - let encoded_name = deduplicate_name(&base_name, &taken_names); + // Deduplicate against sibling slugs (bare instance-name-level slugs, + // not filenames with extensions). This ensures file-format instances + // are correctly detected as collisions. + let encoded_name = deduplicate_name(&base_name, sibling_slugs); let needs_meta_name = needs_slug || encoded_name != base_name; let meta_name_field: Option<&str> = if needs_meta_name { Some(&added.name) @@ -1640,6 +1673,9 @@ impl ApiService { let unique_children = self.filter_duplicate_children(&added.children, &inst_path, stats); let has_children = !unique_children.is_empty(); + // Pre-compute sibling slugs for children's dedup (bare slugs, no extensions). + let child_slugs = Self::compute_sibling_slugs(&unique_children); + // Determine the appropriate middleware/file format based on class name. // This matches the logic in src/syncback/mod.rs::get_best_middleware. // @@ -1699,7 +1735,7 @@ impl ApiService { self.write_script_meta_json_if_needed(&dir_path, added, meta_name_field)?; log::info!("Syncback: Updated ModuleScript at {}", init_path.display()); for child in &unique_children { - self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; + self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?; } } else { let file_path = parent_dir.join(format!("{}.luau", encoded_name)); @@ -1756,7 +1792,7 @@ impl ApiService { self.write_script_meta_json_if_needed(&dir_path, added, meta_name_field)?; log::info!("Syncback: Updated Script at {}", init_path.display()); for child in &unique_children { - self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; + self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?; } } else { let file_path = @@ -1813,7 +1849,7 @@ impl ApiService { self.write_script_meta_json_if_needed(&dir_path, added, meta_name_field)?; log::info!("Syncback: Updated LocalScript at {}", init_path.display()); for child in &unique_children { - self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; + self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?; } } else { let file_path = parent_dir.join(format!("{}.local.luau", encoded_name)); @@ -1864,7 +1900,7 @@ impl ApiService { // Recursively process children for child in &unique_children { - self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; + self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?; } } @@ -1883,7 +1919,7 @@ impl ApiService { dir_path.display() ); for child in &unique_children { - self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; + self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?; } } else { let value = added @@ -1925,7 +1961,7 @@ impl ApiService { dir_path.display() ); for child in &unique_children { - self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; + self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?; } } else { let file_path = parent_dir.join(format!("{}.csv", encoded_name)); @@ -1979,7 +2015,7 @@ impl ApiService { // Recursively process children for child in &unique_children { - self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; + self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?; } } else { let content = self.serialize_instance_to_model_json(added)?; @@ -6208,50 +6244,25 @@ mod tests { // ── File-format behavior documentation ─────────────────────── #[test] - fn file_format_bare_slug_vs_full_filename_limitation() { - // DOCUMENTS KNOWN BEHAVIOR: deduplicate_name checks bare slug - // "Foo" against taken entries. When taken comes from fs::read_dir, - // entries have extensions ("foo.luau"). "foo" ≠ "foo.luau" so no - // collision is detected. This is by design — duplicate-named - // file instances are filtered by the plugin before reaching the API. - let temp = tempdir().expect("Failed to create temp dir"); - File::create(temp.path().join("Foo.luau")) - .unwrap() - .write_all(b"existing") - .unwrap(); - - let taken: HashSet = fs::read_dir(temp.path()) - .unwrap() - .filter_map(|e| e.ok()) - .filter_map(|e| e.file_name().into_string().ok()) - .map(|n| n.to_lowercase()) - .collect(); - - assert!(taken.contains("foo.luau")); - assert!(!taken.contains("foo")); - // Bare slug doesn't match full filename + fn file_format_dedup_with_bare_slugs() { + // Production code now seeds taken_names from tree siblings' + // slugified instance names (bare slugs, no extensions). + // This means file-format dedup works correctly. + let taken: HashSet = ["foo".to_string()].into_iter().collect(); + // Bare slug "Foo" matches "foo" in taken let deduped = deduplicate_name("Foo", &taken); - assert_eq!(deduped, "Foo", "bare slug doesn't match 'foo.luau'"); + assert_eq!(deduped, "Foo~1", "bare slug collides with existing sibling"); } #[test] - fn file_format_slugify_still_works() { - // Even though dedup doesn't catch cross-file collisions, - // slugification itself always works correctly. - let temp = tempdir().expect("Failed to create temp dir"); - let taken = HashSet::new(); + fn file_format_slugify_and_dedup_combined() { + // Slugify + dedup pipeline works end-to-end for file format + let taken: HashSet = ["hey_bro".to_string()].into_iter().collect(); let slug = slugify_name("Hey/Bro"); assert_eq!(slug, "Hey_Bro"); let deduped = deduplicate_name(&slug, &taken); - assert_eq!(deduped, "Hey_Bro"); - - let file_path = temp.path().join("Hey_Bro.luau"); - File::create(&file_path) - .unwrap() - .write_all(b"content") - .unwrap(); - assert!(file_path.exists()); + assert_eq!(deduped, "Hey_Bro~1", "slug collision correctly detected"); } // ── Rename → re-add cycle (the critical slug safety test) ────