feat: auto-resolve init-name conflicts during syncback

When a child instance has a Roblox name that would produce a filesystem
name of "init" (case-insensitive), syncback now automatically prefixes
it with '_' (e.g. "Init" → "_Init.luau") instead of erroring. The
corresponding meta.json writes the original name via the `name` property
so Rojo can restore it on the next snapshot.

The sibling dedup check is updated to use actual on-disk names for
existing children and the resolved (init-prefixed) name for new ones,
so genuine collisions still error while false positives from the `name`
property are avoided.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
2026-02-24 01:05:31 +01:00
parent a6e9939d6c
commit 95fe993de3
4 changed files with 323 additions and 22 deletions

View File

@@ -8,7 +8,7 @@ use memofs::{DirEntry, Vfs};
use crate::{ use crate::{
snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource}, snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource},
syncback::{hash_instance, FsSnapshot, SyncbackReturn, SyncbackSnapshot}, syncback::{hash_instance, slugify_name, FsSnapshot, SyncbackReturn, SyncbackSnapshot},
}; };
use super::{meta_file::DirectoryMetadata, snapshot_from_vfs}; use super::{meta_file::DirectoryMetadata, snapshot_from_vfs};
@@ -134,12 +134,39 @@ pub fn syncback_dir_no_meta<'sync>(
let mut children = Vec::new(); let mut children = Vec::new();
let mut removed_children = Vec::new(); let mut removed_children = Vec::new();
// We have to enforce unique child names for the file system. // Build the old child map early so it can be used for deduplication below.
let mut child_names = HashSet::with_capacity(new_inst.children().len()); let mut old_child_map = HashMap::new();
if let Some(old_inst) = snapshot.old_inst() {
for child in old_inst.children() {
let inst = snapshot.get_old_instance(*child).unwrap();
old_child_map.insert(inst.name(), inst);
}
}
// Enforce unique filesystem names. Uses actual on-disk names for existing
// children and resolved names (with init-prefix) for new ones.
let mut fs_child_names = HashSet::with_capacity(new_inst.children().len());
let mut duplicate_set = HashSet::new(); let mut duplicate_set = HashSet::new();
for child_ref in new_inst.children() { for child_ref in new_inst.children() {
let child = snapshot.get_new_instance(*child_ref).unwrap(); let child = snapshot.get_new_instance(*child_ref).unwrap();
if !child_names.insert(child.name.to_lowercase()) { let fs_name = old_child_map
.get(child.name.as_str())
.and_then(|old| old.metadata().relevant_paths.first())
.and_then(|p| p.file_name())
.and_then(|n| n.to_str())
.map(|s| s.to_lowercase())
.unwrap_or_else(|| {
let slug = slugify_name(&child.name);
let slug_lower = slug.to_lowercase();
// Mirror name_for_inst's init-prefix.
if slug_lower == "init" {
format!("_{slug_lower}")
} else {
slug_lower
}
});
if !fs_child_names.insert(fs_name) {
duplicate_set.insert(child.name.as_str()); duplicate_set.insert(child.name.as_str());
} }
} }
@@ -153,13 +180,7 @@ pub fn syncback_dir_no_meta<'sync>(
anyhow::bail!("Instance has more than 25 children with duplicate names"); anyhow::bail!("Instance has more than 25 children with duplicate names");
} }
if let Some(old_inst) = snapshot.old_inst() { if snapshot.old_inst().is_some() {
let mut old_child_map = HashMap::with_capacity(old_inst.children().len());
for child in old_inst.children() {
let inst = snapshot.get_old_instance(*child).unwrap();
old_child_map.insert(inst.name(), inst);
}
for new_child_ref in new_inst.children() { for new_child_ref in new_inst.children() {
let new_child = snapshot.get_new_instance(*new_child_ref).unwrap(); let new_child = snapshot.get_new_instance(*new_child_ref).unwrap();
if let Some(old_child) = old_child_map.remove(new_child.name.as_str()) { if let Some(old_child) = old_child_map.remove(new_child.name.as_str()) {
@@ -225,6 +246,12 @@ pub fn syncback_dir_no_meta<'sync>(
mod test { mod test {
use super::*; use super::*;
use std::path::PathBuf;
use crate::{
snapshot::{InstanceMetadata, InstanceSnapshot},
Project, RojoTree, SyncbackData, SyncbackSnapshot,
};
use memofs::{InMemoryFs, VfsSnapshot}; use memofs::{InMemoryFs, VfsSnapshot};
#[test] #[test]
@@ -261,4 +288,237 @@ mod test {
insta::assert_yaml_snapshot!(instance_snapshot); insta::assert_yaml_snapshot!(instance_snapshot);
} }
fn make_project() -> Project {
serde_json::from_str(r#"{"tree": {"$className": "DataModel"}}"#).unwrap()
}
fn make_vfs() -> Vfs {
let mut imfs = InMemoryFs::new();
imfs.load_snapshot("/root", VfsSnapshot::empty_dir()).unwrap();
Vfs::new(imfs)
}
/// Two children whose Roblox names are identical when lowercased ("Alpha"
/// and "alpha") but live at different filesystem paths because of the
/// `name` property ("Beta/" and "Alpha/" respectively). The dedup check
/// must use the actual filesystem paths, not the raw Roblox names, to
/// avoid a false-positive duplicate error.
#[test]
fn syncback_no_false_duplicate_with_name_prop() {
use rbx_dom_weak::{InstanceBuilder, WeakDom};
// Old child A: Roblox name "Alpha", on disk at "/root/Beta"
// (name property maps "Alpha" → "Beta" on the filesystem)
let old_child_a = InstanceSnapshot::new()
.name("Alpha")
.class_name("Folder")
.metadata(
InstanceMetadata::new()
.instigating_source(PathBuf::from("/root/Beta"))
.relevant_paths(vec![PathBuf::from("/root/Beta")]),
);
// Old child B: Roblox name "alpha", on disk at "/root/Alpha"
let old_child_b = InstanceSnapshot::new()
.name("alpha")
.class_name("Folder")
.metadata(
InstanceMetadata::new()
.instigating_source(PathBuf::from("/root/Alpha"))
.relevant_paths(vec![PathBuf::from("/root/Alpha")]),
);
let old_parent = InstanceSnapshot::new()
.name("Parent")
.class_name("Folder")
.children(vec![old_child_a, old_child_b])
.metadata(
InstanceMetadata::new()
.instigating_source(PathBuf::from("/root"))
.relevant_paths(vec![PathBuf::from("/root")]),
);
let old_tree = RojoTree::new(old_parent);
// New state: same two children in Roblox.
let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT"));
let new_parent = new_tree.insert(
new_tree.root_ref(),
InstanceBuilder::new("Folder").with_name("Parent"),
);
new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Alpha"));
new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("alpha"));
let vfs = make_vfs();
let project = make_project();
let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project);
let snapshot = SyncbackSnapshot {
data,
old: Some(old_tree.get_root_id()),
new: new_parent,
path: PathBuf::from("/root"),
middleware: None,
};
let result = syncback_dir_no_meta(&snapshot);
assert!(
result.is_ok(),
"should not error when two children have the same lowercased Roblox \
name but map to distinct filesystem paths: {result:?}",
);
}
/// Two completely new children with the same non-init name would produce
/// the same filesystem entry and must be detected as a duplicate.
#[test]
fn syncback_detects_sibling_duplicate_names() {
use rbx_dom_weak::{InstanceBuilder, WeakDom};
let old_parent = InstanceSnapshot::new()
.name("Parent")
.class_name("Folder")
.metadata(
InstanceMetadata::new()
.instigating_source(PathBuf::from("/root"))
.relevant_paths(vec![PathBuf::from("/root")]),
);
let old_tree = RojoTree::new(old_parent);
let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT"));
let new_parent = new_tree.insert(
new_tree.root_ref(),
InstanceBuilder::new("Folder").with_name("Parent"),
);
// "Foo" is not a reserved name but two siblings named "Foo" still
// collide on disk.
new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Foo"));
new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Foo"));
let vfs = make_vfs();
let project = make_project();
let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project);
let snapshot = SyncbackSnapshot {
data,
old: Some(old_tree.get_root_id()),
new: new_parent,
path: PathBuf::from("/root"),
middleware: None,
};
let result = syncback_dir_no_meta(&snapshot);
assert!(
result.is_err(),
"should error when two new children would produce the same filesystem name",
);
}
/// A new child named "Init" (as a ModuleScript) would naively become
/// "Init.luau", which case-insensitively matches the parent's reserved
/// "init.luau". Syncback must resolve this automatically by prefixing the
/// filesystem name with '_' (→ "_Init.luau") rather than erroring.
#[test]
fn syncback_resolves_init_name_conflict() {
use rbx_dom_weak::{InstanceBuilder, WeakDom};
let old_parent = InstanceSnapshot::new()
.name("Parent")
.class_name("Folder")
.metadata(
InstanceMetadata::new()
.instigating_source(PathBuf::from("/root"))
.relevant_paths(vec![PathBuf::from("/root")]),
);
let old_tree = RojoTree::new(old_parent);
let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT"));
let new_parent = new_tree.insert(
new_tree.root_ref(),
InstanceBuilder::new("Folder").with_name("Parent"),
);
new_tree.insert(
new_parent,
InstanceBuilder::new("ModuleScript").with_name("Init"),
);
let vfs = make_vfs();
let project = make_project();
let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project);
let snapshot = SyncbackSnapshot {
data,
old: Some(old_tree.get_root_id()),
new: new_parent,
path: PathBuf::from("/root"),
middleware: None,
};
let result = syncback_dir_no_meta(&snapshot);
assert!(
result.is_ok(),
"should resolve init-name conflict by prefixing '_', not error: {result:?}",
);
// The child should have been placed at "_Init.luau", not "Init.luau".
let child_file_name = result
.unwrap()
.children
.into_iter()
.next()
.and_then(|c| c.path.file_name().map(|n| n.to_string_lossy().into_owned()))
.unwrap_or_default();
assert!(
child_file_name.starts_with('_'),
"child filesystem name should start with '_' to avoid init collision, \
got: {child_file_name}",
);
}
/// A child whose filesystem name is stored with a slugified prefix (e.g.
/// "_Init") must NOT be blocked — only the bare "init" stem is reserved.
#[test]
fn syncback_allows_slugified_init_name() {
use rbx_dom_weak::{InstanceBuilder, WeakDom};
// Existing child: on disk as "_Init" (slugified from a name with an
// illegal character), its stem is "_init" which is not reserved.
let old_child = InstanceSnapshot::new()
.name("Init")
.class_name("Folder")
.metadata(
InstanceMetadata::new()
.instigating_source(PathBuf::from("/root/_Init"))
.relevant_paths(vec![PathBuf::from("/root/_Init")]),
);
let old_parent = InstanceSnapshot::new()
.name("Parent")
.class_name("Folder")
.children(vec![old_child])
.metadata(
InstanceMetadata::new()
.instigating_source(PathBuf::from("/root"))
.relevant_paths(vec![PathBuf::from("/root")]),
);
let old_tree = RojoTree::new(old_parent);
let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT"));
let new_parent = new_tree.insert(
new_tree.root_ref(),
InstanceBuilder::new("Folder").with_name("Parent"),
);
new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Init"));
let vfs = make_vfs();
let project = make_project();
let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project);
let snapshot = SyncbackSnapshot {
data,
old: Some(old_tree.get_root_id()),
new: new_parent,
path: PathBuf::from("/root"),
middleware: None,
};
let result = syncback_dir_no_meta(&snapshot);
assert!(
result.is_ok(),
"should allow a child whose filesystem name is slugified away from \
the reserved 'init' stem: {result:?}",
);
}
} }

View File

@@ -154,11 +154,18 @@ impl AdjacentMetadata {
.old_inst() .old_inst()
.and_then(|inst| inst.metadata().specified_name.clone()) .and_then(|inst| inst.metadata().specified_name.clone())
.or_else(|| { .or_else(|| {
// If this is a new instance and its name is invalid for the filesystem, // Write name when the filesystem path doesn't match the
// we need to specify the name in meta.json so it can be preserved // instance name (invalid chars or init-prefix).
if snapshot.old_inst().is_none() { if snapshot.old_inst().is_none() {
let instance_name = &snapshot.new_inst().name; let instance_name = &snapshot.new_inst().name;
if validate_file_name(instance_name).is_err() { let fs_stem = path
.file_name()
.and_then(|n| n.to_str())
.map(|s| s.split('.').next().unwrap_or(s))
.unwrap_or("");
if validate_file_name(instance_name).is_err()
|| fs_stem != instance_name.as_str()
{
Some(instance_name.clone()) Some(instance_name.clone())
} else { } else {
None None
@@ -421,11 +428,17 @@ impl DirectoryMetadata {
.old_inst() .old_inst()
.and_then(|inst| inst.metadata().specified_name.clone()) .and_then(|inst| inst.metadata().specified_name.clone())
.or_else(|| { .or_else(|| {
// If this is a new instance and its name is invalid for the filesystem, // Write name when the directory name doesn't match the
// we need to specify the name in meta.json so it can be preserved // instance name (invalid chars or init-prefix).
if snapshot.old_inst().is_none() { if snapshot.old_inst().is_none() {
let instance_name = &snapshot.new_inst().name; let instance_name = &snapshot.new_inst().name;
if validate_file_name(instance_name).is_err() { let fs_name = path
.file_name()
.and_then(|n| n.to_str())
.unwrap_or("");
if validate_file_name(instance_name).is_err()
|| fs_name != instance_name.as_str()
{
Some(instance_name.clone()) Some(instance_name.clone())
} else { } else {
None None

View File

@@ -36,23 +36,33 @@ pub fn name_for_inst<'a>(
| Middleware::ServerScriptDir | Middleware::ServerScriptDir
| Middleware::ClientScriptDir | Middleware::ClientScriptDir
| Middleware::ModuleScriptDir => { | Middleware::ModuleScriptDir => {
if validate_file_name(&new_inst.name).is_err() { let name = if validate_file_name(&new_inst.name).is_err() {
Cow::Owned(slugify_name(&new_inst.name)) Cow::Owned(slugify_name(&new_inst.name))
} else { } else {
Cow::Borrowed(&new_inst.name) Cow::Borrowed(new_inst.name.as_str())
};
// Prefix "init" to avoid colliding with reserved init files.
if name.to_lowercase() == "init" {
Cow::Owned(format!("_{name}"))
} else {
name
} }
} }
_ => { _ => {
let extension = extension_for_middleware(middleware); let extension = extension_for_middleware(middleware);
let slugified; let slugified;
let final_name = if validate_file_name(&new_inst.name).is_err() { let stem: &str = if validate_file_name(&new_inst.name).is_err() {
slugified = slugify_name(&new_inst.name); slugified = slugify_name(&new_inst.name);
&slugified &slugified
} else { } else {
&new_inst.name &new_inst.name
}; };
// Prefix "init" stems to avoid colliding with reserved init files.
Cow::Owned(format!("{final_name}.{extension}")) if stem.to_lowercase() == "init" {
Cow::Owned(format!("_{stem}.{extension}"))
} else {
Cow::Owned(format!("{stem}.{extension}"))
}
} }
}) })
} }

View File

@@ -237,6 +237,24 @@ pub fn inst_path(dom: &WeakDom, referent: Ref) -> String {
path.join("/") path.join("/")
} }
impl<'sync> SyncbackData<'sync> {
/// Constructs a `SyncbackData` for use in unit tests.
#[cfg(test)]
pub fn for_test(
vfs: &'sync Vfs,
old_tree: &'sync RojoTree,
new_tree: &'sync WeakDom,
project: &'sync Project,
) -> Self {
Self {
vfs,
old_tree,
new_tree,
project,
}
}
}
#[cfg(test)] #[cfg(test)]
mod test { mod test {
use rbx_dom_weak::{InstanceBuilder, WeakDom}; use rbx_dom_weak::{InstanceBuilder, WeakDom};