diff --git a/.cursor/plans/audit_slugify_pass_3_b11f65a1.plan.md b/.cursor/plans/audit_slugify_pass_3_b11f65a1.plan.md new file mode 100644 index 00000000..417549d5 --- /dev/null +++ b/.cursor/plans/audit_slugify_pass_3_b11f65a1.plan.md @@ -0,0 +1,162 @@ +--- +name: Audit slugify pass 3 +overview: Holistic audit of the entire `slugify` branch diff against master (110 files, +6315/-1188 lines, 21 commits). Found 1 critical bug, 2 correctness issues, and 8 test gaps. All prior plan fixes verified in code. Legacy cleanup confirmed complete. +todos: + - id: c1-model-rename + content: "CRITICAL: Fix model JSON rename in change_processor.rs -- add .model to suffix list, update name inside model file instead of adjacent meta" + status: completed + - id: cc1-model-update-name + content: Fix syncback_update_existing_instance (api.rs:1093) dropping name field when rewriting .model.json5 files, and write_init_meta_json (api.rs:1075) dropping name field for directories + status: completed + - id: cc2-dir-removal-meta + content: Fix remove_instance_at_path directory case to clean up grandparent-level DirName.meta.json5 + status: completed + - id: test-roundtrip-slugified + content: "Add roundtrip test: instances with forbidden chars through syncback -> rebuild -> compare" + status: completed + - id: test-model-rename + content: "Add two-way sync test: rename a .model.json5 instance, verify compound extension preserved and name updated" + status: completed + - id: test-idempotency + content: "Add syncback idempotency integration test: syncback twice = zero changes" + status: completed + - id: test-3-collision + content: Add 3+ instance collision test (X/Y, X:Y, X|Y all slug to X_Y) + status: completed + - id: test-reserved-names + content: Add Windows reserved name integration test (CON, PRN round-trip) + status: completed + - id: test-dir-rename + content: "Add two-way sync test: directory/init-file rename with forbidden chars" + status: completed + - id: test-case-collision + content: Add case-insensitive collision integration test (Foo vs foo) + status: completed + - id: test-meta-empty-deletion + content: Strengthen rename_slugified_to_clean test to verify meta file is deleted (not just name removed) when it becomes empty + status: completed +isProject: false +--- + +# Audit: slugify Branch -- Pass 3 (Holistic) + +## Scope + +Full diff against `master`: 110 files, +6,315 / -1,188 lines across 21 commits. Every changed file reviewed. All 4 prior plan files cross-referenced against codebase. + +## Quality Standard + +> **Round-trip identity**: Syncback (or two-way sync) writes a directory tree. Building an rbxl from that directory tree and forward-syncing it back must produce a **bit-identical instance tree**. Any deviation is a bug. + +--- + +## Prior Plan Verification + +All claims from prior plans verified in code -- no discrepancies: + +- **fix_audit_findings**: C1 (meta gating), C2 (removal meta path), C3 (deny_unknown_fields), CC1 (determinism), CC2 (init removal), DR1 (shared meta helper), DR2 (meta path helper) -- all confirmed in code. +- **fix_audit_pass_2**: CC2 refactor (`remove_instance_at_path`), preseed fix, rename dedup, dir sort, csv instigating_source, over-suppression, comment fix -- all confirmed. +- **fix_stem-level_dedup**: `return-dedup-key`, `fix-dir`, `fix-project`, `fix-api` -- all confirmed. Note: `fix-tests` is still marked `in_progress` in the plan but tests do exist; the plan status is stale. + +--- + +## Areas Confirmed CLEAN + +Thoroughly re-audited via the full diff and found correct: + +- **Forward sync** (all middleware): `decode_path_name` fully removed. All middleware correctly resolves names from metadata `name` field or filename stem. `specified_name` set on `InstanceMetadata` in every middleware. `is_empty()` updated for `name`. `instigating_source` preserved in `csv.rs`. +- **Reverse sync / syncback**: `slugify_name()`, `deduplicate_name()`, `name_for_inst()` all correct. Pre-seeding uses `relevant_paths` + `strip_middleware_extension()`. Child ordering sorted alphabetically in both `dir.rs` and `project.rs`. `dedup_key.to_lowercase()` applied at ALL insertion sites. +- **Two-way sync adds**: `syncback_added_instance` groups adds by parent, seeds `sibling_slugs` from tree children via `instigating_source` paths, uses `strip_middleware_extension`. Meta files written when slug differs. Meta path matches script file path via `adjacent_meta_path()`. +- **Two-way sync removals**: `remove_instance_at_path` handles directories, init files (parent dir removal + grandparent meta), and regular files (adjacent meta). Old dead `syncback_removed_instance` deleted. +- **Meta lifecycle**: C1 fix in place -- meta updates OUTSIDE `if new_path != *path` for both init and regular paths. Shared module (`syncback::meta`) used by `change_processor.rs`. `suppress_path_remove` exists. `RemoveNameOutcome` handled correctly with `(1, 0)` counts for `FileDeleted`. +- **Legacy cleanup**: Zero references to old encoding system in source, tests, plugin, schema, or snapshots. `src/path_encoding.rs` deleted. `pub mod path_encoding` removed from `lib.rs`. `decode_windows_invalid_chars` removed from `InstanceContext` and `serve_session.rs`. +- **Dedup key consistency**: All insertion sites use `.to_lowercase()`. Cross-format collision handled. +- **SyncbackRules**: No `deny_unknown_fields`. +- **JSON schema**: `encodeWindowsInvalidChars` removed from `vscode-rojo/schemas/`. +- **Snapshot files**: All `.snap` files updated. No stale `decode_windows_invalid_chars` or `%ENCODED%` patterns. +- `**json_model_legacy_name` snapshot change**: Instance name changed from `Expected Name` to `Overridden Name` -- correct, this is the intended behavior change (the `Name`/`name` field is now respected instead of ignored). +- **Test fixture migration**: All `%ENCODED%` filenames in `syncback_encoded_names/` correctly replaced with slugified names + meta files. +- **Existing test updates**: All ~12 two-way sync tests with encoded patterns correctly updated to slugified equivalents. + +--- + +## CRITICAL: Model JSON rename loses `.model` compound extension + +**File**: [src/change_processor.rs](src/change_processor.rs) lines 786-868 + +The rename handler's suffix list: + +```786:789:src/change_processor.rs +let known_suffixes = [ + ".server", ".client", ".plugin", ".local", + ".legacy", +]; +``` + +Does NOT include `.model`. For `What_Model.model.json5` renamed to `Why?Model`: + +- `stem` = `"What_Model.model"`, `extension` = `"json5"`, `script_suffix` = `""` (no match) +- `old_base` = `"What_Model.model"` (includes `.model`) +- New filename = `"Why_Model.json5"` -- `.model` LOST + +**Impact**: File type changes from `.model.json5` (JSON instance) to `.json5` (data module). Instance class, properties, hierarchy silently lost. The meta update then writes to `Why_Model.meta.json5` (wrong location for model files). Direct round-trip violation. + +**Fix**: Add `.model` to the suffix list. Then detect `.model.json5`/`.model.json` files and update the `name` field INSIDE the model file (like `syncback::meta::upsert_meta_name` but targeting the JSON root's `name` key) instead of writing adjacent `.meta.json5`. + +--- + +## CORRECTNESS: `syncback_update_existing_instance` drops `name` field + +**File**: [src/web/api.rs](src/web/api.rs) lines 1091-1098 + +When updating an existing `.model.json5` via two-way sync (plugin sends updated properties), the code rewrites the entire model file: + +```1093:1093:src/web/api.rs +let content = self.serialize_instance_to_model_json(added, None)?; +``` + +`None` for `instance_name` means the `name` field is NOT written. If the model file previously had `"name": "Hey/Bro"`, the name is silently lost on rewrite. Same issue exists at line 1075 for directory updates via `write_init_meta_json(existing_path, added, None)`. + +**Fix**: Compute `meta_name_field` by comparing the slug of the existing filesystem name against `added.name`. If they differ, pass `Some(&added.name)`. This matches the logic in `syncback_instance_to_path_with_stats`. + +--- + +## CORRECTNESS: `remove_instance_at_path` directory case skips grandparent meta + +**File**: [src/web/api.rs](src/web/api.rs) lines 1389-1396 + +When `path.is_dir()` (plain Folder), `remove_dir_all` is called but no adjacent `DirName.meta.json5` at the parent level is cleaned up. The init-file case (lines 1423-1438) correctly handles this. + +**Severity**: LOW. Grandparent-level `DirName.meta.json5` only exists from manual creation or the change_processor rename path (which renames it). Standard syncback uses `init.meta.json5` inside the directory. + +**Fix**: After `remove_dir_all`, add grandparent meta cleanup matching the init-file path. + +--- + +## TEST COVERAGE GAPS + +Prioritized by risk: + +1. **Roundtrip test with slugified names** -- `syncback_roundtrip.rs` exists but no fixtures exercise slugified names. None of `meta_name_override`, `model_json_name_override`, `dedup_suffix_with_meta` are in the roundtrip test list. +2. **Model file rename two-way sync** -- No test for renaming a `.model.json5` instance. Would catch the critical bug above. +3. **Syncback idempotency integration** -- Run syncback twice, assert zero changes on second run. Unit tests exist but no full-pipeline test. +4. **3+ instance collision** -- Only 2-instance collision tested (`add_two_colliding_instances_deduplicates`). Need `X/Y`, `X:Y`, `X|Y` -> `X_Y`, `X_Y~1`, `X_Y~2`. +5. **Windows reserved name integration** -- Unit tests for `CON`/`PRN` slugification exist. No build or syncback integration test. +6. **Directory rename with forbidden chars** -- Two-way sync tests cover file renames but not init-file/directory renames. +7. **Case-insensitive collision** -- Code handles it (`.to_lowercase()` everywhere). No integration test. +8. **Meta file empty deletion** -- `rename_slugified_to_clean` test checks meta doesn't have `name` field but doesn't verify the meta file itself is deleted when it becomes empty (the `RemoveNameOutcome::FileDeleted` path). + +--- + +## Summary + + +| Category | Count | Details | +| ------------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------- | +| Critical bugs | 1 | Model JSON rename loses `.model` extension | +| Correctness | 2 | `syncback_update_existing_instance` drops `name` field; directory removal skips grandparent meta | +| Missing tests | 8 | Roundtrip, model rename, idempotency, 3+ collision, reserved names, dir rename, case collision, meta empty deletion | +| Prior plans | OK | All claims verified in code, no discrepancies | +| Clean areas | 10 | Forward sync, reverse sync, two-way adds/removals, meta lifecycle, legacy cleanup, dedup consistency, schema, snapshots, test fixtures, test updates | + + diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__reserved_name_override.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__reserved_name_override.snap new file mode 100644 index 00000000..e86a16a8 --- /dev/null +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__reserved_name_override.snap @@ -0,0 +1,30 @@ +--- +source: tests/tests/build.rs +expression: contents +--- + + + + reserved_name_override + + + + ReplicatedStorage + + + + CON + + + + + + PRN + + + + + + diff --git a/rojo-test/build-tests/reserved_name_override/default.project.json5 b/rojo-test/build-tests/reserved_name_override/default.project.json5 new file mode 100644 index 00000000..54ea895e --- /dev/null +++ b/rojo-test/build-tests/reserved_name_override/default.project.json5 @@ -0,0 +1,10 @@ +{ + "name": "reserved_name_override", + "tree": { + "$className": "DataModel", + "ReplicatedStorage": { + "$className": "ReplicatedStorage", + "$path": "src" + } + } +} diff --git a/rojo-test/build-tests/reserved_name_override/src/CON_.luau b/rojo-test/build-tests/reserved_name_override/src/CON_.luau new file mode 100644 index 00000000..aafb0926 --- /dev/null +++ b/rojo-test/build-tests/reserved_name_override/src/CON_.luau @@ -0,0 +1 @@ +return "I am CON" diff --git a/rojo-test/build-tests/reserved_name_override/src/CON_.meta.json5 b/rojo-test/build-tests/reserved_name_override/src/CON_.meta.json5 new file mode 100644 index 00000000..0a7f5e98 --- /dev/null +++ b/rojo-test/build-tests/reserved_name_override/src/CON_.meta.json5 @@ -0,0 +1,3 @@ +{ + "name": "CON" +} diff --git a/rojo-test/build-tests/reserved_name_override/src/PRN_.luau b/rojo-test/build-tests/reserved_name_override/src/PRN_.luau new file mode 100644 index 00000000..78e22614 --- /dev/null +++ b/rojo-test/build-tests/reserved_name_override/src/PRN_.luau @@ -0,0 +1 @@ +return "I am PRN" diff --git a/rojo-test/build-tests/reserved_name_override/src/PRN_.meta.json5 b/rojo-test/build-tests/reserved_name_override/src/PRN_.meta.json5 new file mode 100644 index 00000000..696bbcf8 --- /dev/null +++ b/rojo-test/build-tests/reserved_name_override/src/PRN_.meta.json5 @@ -0,0 +1,3 @@ +{ + "name": "PRN" +} diff --git a/src/change_processor.rs b/src/change_processor.rs index 784415d5..a71d294b 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -247,6 +247,49 @@ impl JobThreadContext { } } + /// Upsert the `name` field inside a `.model.json5` / `.model.json` file, + /// suppressing filesystem events. + fn upsert_model_name_field(&self, model_path: &Path, real_name: &str) { + self.suppress_path(model_path); + if let Err(err) = crate::syncback::meta::upsert_model_name(model_path, real_name) { + self.unsuppress_path(model_path); + log::error!( + "Failed to upsert name in model file {}: {}", + model_path.display(), + err + ); + } + } + + /// Remove the `name` field from a `.model.json5` / `.model.json` file, + /// suppressing filesystem events. + fn remove_model_name_field(&self, model_path: &Path) { + use crate::syncback::meta::RemoveNameOutcome; + self.suppress_path(model_path); + match crate::syncback::meta::remove_model_name(model_path) { + Ok(RemoveNameOutcome::NoOp) => { + self.unsuppress_path(model_path); + } + Ok(RemoveNameOutcome::FieldRemoved) => { + // File was rewritten — suppress_path already covers it. + } + Ok(RemoveNameOutcome::FileDeleted) => { + // Model files shouldn't be deleted (they have className etc), + // but handle for completeness. + self.unsuppress_path(model_path); + self.suppress_path_remove(model_path); + } + Err(err) => { + self.unsuppress_path(model_path); + log::error!( + "Failed to remove name from model file {}: {}", + model_path.display(), + err + ); + } + } + } + /// Remove the `name` field from a `.meta.json5` file, suppressing filesystem /// events. If the file becomes empty after removal, deletes it entirely. fn remove_meta_name_field(&self, meta_path: &Path) { @@ -785,7 +828,7 @@ impl JobThreadContext { .unwrap_or(""); let known_suffixes = [ ".server", ".client", ".plugin", ".local", - ".legacy", + ".legacy", ".model", ]; let script_suffix = known_suffixes .iter() @@ -912,22 +955,39 @@ impl JobThreadContext { effective_meta_base = old_base; } - // Always update the meta name field when the + // Always update the name field when the // instance name changed, even if the path didn't // change (e.g. "Foo/Bar" → "Foo|Bar" both slugify // to "Foo_Bar"). Use deduped_new_name because // dedup may have appended ~N. - let current_meta = parent.join(format!( - "{}.meta.json5", - effective_meta_base - )); - if deduped_new_name != *new_name { - self.upsert_meta_name_field( - ¤t_meta, - new_name, - ); + // + // For .model.json5/.model.json files, the name + // field lives INSIDE the model file, not in + // adjacent .meta.json5. + if script_suffix == ".model" { + let model_file = overridden_source_path + .as_deref() + .unwrap_or(path.as_path()); + if deduped_new_name != *new_name { + self.upsert_model_name_field( + model_file, new_name, + ); + } else { + self.remove_model_name_field(model_file); + } } else { - self.remove_meta_name_field(¤t_meta); + let current_meta = parent.join(format!( + "{}.meta.json5", + effective_meta_base + )); + if deduped_new_name != *new_name { + self.upsert_meta_name_field( + ¤t_meta, + new_name, + ); + } else { + self.remove_meta_name_field(¤t_meta); + } } } } diff --git a/src/syncback/file_names.rs b/src/syncback/file_names.rs index fe491bf5..9f8e1b43 100644 --- a/src/syncback/file_names.rs +++ b/src/syncback/file_names.rs @@ -323,7 +323,9 @@ pub fn validate_file_name>(name: S) -> anyhow::Result<()> { /// Known script suffixes that appear between the base name and file extension. /// For example, in `MyScript.server.luau`, `.server` is the suffix. -const KNOWN_SCRIPT_SUFFIXES: &[&str] = &[".server", ".client", ".plugin", ".local", ".legacy"]; +const KNOWN_SCRIPT_SUFFIXES: &[&str] = &[ + ".server", ".client", ".plugin", ".local", ".legacy", ".model", +]; /// Strips a known script suffix from a file stem. /// diff --git a/src/syncback/meta.rs b/src/syncback/meta.rs index 1165aac4..20134a93 100644 --- a/src/syncback/meta.rs +++ b/src/syncback/meta.rs @@ -46,6 +46,50 @@ pub enum RemoveNameOutcome { FileDeleted, } +/// Upsert the `name` field inside a `.model.json5` / `.model.json` file. +/// +/// Parses the existing JSON, sets/replaces the `name` key, and rewrites. +/// Unlike `upsert_meta_name`, this modifies the model file in-place. +pub fn upsert_model_name(model_path: &Path, real_name: &str) -> anyhow::Result<()> { + let bytes = fs::read(model_path)?; + let mut obj = match crate::json::from_slice::(&bytes) { + Ok(serde_json::Value::Object(map)) => map, + _ => anyhow::bail!("model file is not a JSON object: {}", model_path.display()), + }; + obj.insert( + "name".to_string(), + serde_json::Value::String(real_name.to_string()), + ); + let content = crate::json::to_vec_pretty_sorted(&serde_json::Value::Object(obj))?; + fs::write(model_path, &content)?; + Ok(()) +} + +/// Remove the `name` field from a `.model.json5` / `.model.json` file. +/// +/// Unlike meta files, model files are never deleted when they become "empty" +/// (they always have at least `className`). Returns `RemoveNameOutcome` for +/// consistency with the meta helpers. +pub fn remove_model_name(model_path: &Path) -> anyhow::Result { + if !model_path.exists() { + return Ok(RemoveNameOutcome::NoOp); + } + let bytes = match fs::read(model_path) { + Ok(b) => b, + Err(_) => return Ok(RemoveNameOutcome::NoOp), + }; + let mut obj = match crate::json::from_slice::(&bytes) { + Ok(serde_json::Value::Object(map)) => map, + _ => return Ok(RemoveNameOutcome::NoOp), + }; + if obj.remove("name").is_none() { + return Ok(RemoveNameOutcome::NoOp); + } + let content = crate::json::to_vec_pretty_sorted(&serde_json::Value::Object(obj))?; + fs::write(model_path, &content)?; + Ok(RemoveNameOutcome::FieldRemoved) +} + /// Remove the `name` field from a `.meta.json5` file. /// /// If the file becomes an empty object after removal, deletes it entirely. diff --git a/src/web/api.rs b/src/web/api.rs index d5ddb02d..c29c715a 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -1072,7 +1072,18 @@ impl ApiService { if existing_path.is_dir() { // Update init.meta.json5 if needed if !added.properties.is_empty() { - self.write_init_meta_json(existing_path, added, None)?; + // Preserve the name field if the directory slug differs + // from the instance name (due to slugification/dedup). + let dir_name = existing_path + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(""); + let instance_name = if dir_name != added.name { + Some(added.name.as_str()) + } else { + None + }; + self.write_init_meta_json(existing_path, added, instance_name)?; log::info!( "Syncback: Updated existing {} at {}/init.meta.json5", class_name, @@ -1090,7 +1101,19 @@ impl ApiService { } } else { // It's a standalone file (e.g., .model.json5) - let content = self.serialize_instance_to_model_json(added, None)?; + // Preserve the name field if the filesystem slug differs + // from the instance name (due to slugification/dedup). + let stem = existing_path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or(""); + let base = crate::syncback::strip_script_suffix(stem); + let instance_name = if base != added.name { + Some(added.name.as_str()) + } else { + None + }; + let content = self.serialize_instance_to_model_json(added, instance_name)?; self.suppress_path(existing_path); fs::write(existing_path, &content).with_context(|| { format!("Failed to write file: {}", existing_path.display()) @@ -1393,6 +1416,26 @@ impl ApiService { return false; } log::info!("Syncback: Removed directory at {}", path.display()); + + // Also remove adjacent dir-level meta in the parent if it + // exists (e.g. parent/DirName.meta.json5), matching the + // init-file case below. + if let Some(parent) = path.parent() { + if let Some(dir_name) = path.file_name().and_then(|f| f.to_str()) { + let dir_meta = parent.join(format!("{}.meta.json5", dir_name)); + if dir_meta.exists() { + self.suppress_path_remove(&dir_meta); + if let Err(err) = fs::remove_file(&dir_meta) { + log::warn!( + "Failed to remove dir-level meta file {}: {}", + dir_meta.display(), + err + ); + } + } + } + } + return true; } diff --git a/tests/tests/build.rs b/tests/tests/build.rs index 6a936170..26666f00 100644 --- a/tests/tests/build.rs +++ b/tests/tests/build.rs @@ -69,6 +69,7 @@ gen_build_tests! { model_json_name_override, dedup_suffix_with_meta, init_meta_name_override, + reserved_name_override, } fn run_build_test(test_name: &str) { diff --git a/tests/tests/syncback_roundtrip.rs b/tests/tests/syncback_roundtrip.rs index b1e50c18..31d85893 100644 --- a/tests/tests/syncback_roundtrip.rs +++ b/tests/tests/syncback_roundtrip.rs @@ -8,6 +8,7 @@ //! Produces semantically identical output. This validates that the syncback //! system correctly reconstructs project structure from binary place files. +use std::collections::HashMap; use std::{fs, path::Path}; use rbx_dom_weak::{ustr, WeakDom}; @@ -284,6 +285,11 @@ roundtrip_tests! { // Issue regression tests issue_546, + + // Slugified names and metadata overrides + meta_name_override, + model_json_name_override, + dedup_suffix_with_meta, } // ============================================================================= @@ -337,3 +343,111 @@ fn infer_service_name() { fn rbxmx_ref() { run_roundtrip_test("rbxmx_ref"); } + +// ============================================================================= +// SYNCBACK IDEMPOTENCY TEST +// ============================================================================= + +/// Build -> syncback -> syncback again. The second syncback must produce +/// zero filesystem changes (i.e., the result is identical to the first). +/// This validates that syncback output is stable and self-consistent. +#[test] +fn syncback_idempotency() { + let _ = env_logger::try_init(); + + // Use dedup_suffix_with_meta as it exercises slugified names + meta + let project_path = Path::new(BUILD_TESTS_PATH).join("dedup_suffix_with_meta"); + + // 1. Build original -> rbxm + let (_tmp1, original_rbxm) = run_rojo_build(&project_path, "original.rbxm"); + + // 2. First syncback + let first_dir = tempdir().expect("Failed to create temp dir"); + copy_project_dir(&project_path, first_dir.path()); + assert!( + run_rojo_syncback_clean(first_dir.path(), &original_rbxm), + "First syncback should succeed" + ); + ensure_project_dirs_exist(first_dir.path()); + + // Snapshot all files after first syncback + let first_snapshot = snapshot_dir(first_dir.path()); + + // 3. Build from first syncback result -> rbxm + let (_tmp2, first_rbxm) = run_rojo_build(first_dir.path(), "first.rbxm"); + + // 4. Second syncback (into a copy of the first syncback result) + let second_dir = tempdir().expect("Failed to create temp dir"); + copy_project_dir(first_dir.path(), second_dir.path()); + assert!( + run_rojo_syncback_clean(second_dir.path(), &first_rbxm), + "Second syncback should succeed" + ); + ensure_project_dirs_exist(second_dir.path()); + + // 5. Snapshot all files after second syncback + let second_snapshot = snapshot_dir(second_dir.path()); + + // 6. Compare snapshots — must be identical + assert_eq!( + first_snapshot.len(), + second_snapshot.len(), + "File count should be identical between first and second syncback.\n\ + First: {:?}\nSecond: {:?}", + first_snapshot.keys().collect::>(), + second_snapshot.keys().collect::>(), + ); + + for (path, first_content) in &first_snapshot { + let second_content = second_snapshot.get(path).unwrap_or_else(|| { + panic!( + "File {:?} exists after first syncback but not after second", + path + ) + }); + assert_eq!( + first_content, second_content, + "File {:?} changed between first and second syncback", + path + ); + } +} + +/// Snapshot all files under a directory into a map of relative_path -> contents. +fn snapshot_dir(root: &Path) -> HashMap> { + let mut result = HashMap::new(); + snapshot_dir_inner(root, root, &mut result); + result +} + +fn snapshot_dir_inner(root: &Path, current: &Path, map: &mut HashMap>) { + if let Ok(entries) = fs::read_dir(current) { + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + snapshot_dir_inner(root, &path, map); + } else { + let relative = path + .strip_prefix(root) + .unwrap() + .to_string_lossy() + .replace('\\', "/"); + if let Ok(content) = fs::read(&path) { + map.insert(relative, content); + } + } + } + } +} + +// ============================================================================= +// WINDOWS RESERVED NAME ROUNDTRIP TEST +// ============================================================================= + +/// Instances named after Windows reserved names (CON, PRN) should round-trip +/// correctly: slugified on disk, real name in meta, rebuild produces correct +/// instance names. +#[test] +fn reserved_name_roundtrip() { + run_roundtrip_test("reserved_name_override"); +} diff --git a/tests/tests/two_way_sync.rs b/tests/tests/two_way_sync.rs index b549b0a9..9561381b 100644 --- a/tests/tests/two_way_sync.rs +++ b/tests/tests/two_way_sync.rs @@ -5197,7 +5197,8 @@ fn rename_clean_to_slugified() { } /// Renaming a slugified instance back to a clean name should remove the -/// meta `name` field (or the entire meta file if it had no other fields). +/// meta `name` field. When the meta file had no other fields, the file +/// itself must be deleted (the `RemoveNameOutcome::FileDeleted` path). #[test] fn rename_slugified_to_clean() { run_serve_test("syncback_encoded_names", |session, _redactions| { @@ -5231,15 +5232,360 @@ fn rename_slugified_to_clean() { "What_Module.meta.json5 should be gone after rename", ); - // The new meta file should either not exist (clean name needs no - // name override) or, if it exists, should NOT contain a "name" field. - if new_meta.exists() { - let meta_content = fs::read_to_string(&new_meta).unwrap(); + // The meta file had only a "name" field, so it should be deleted + // entirely (not just emptied). This validates the FileDeleted path. + poll_not_exists( + &new_meta, + "CleanName.meta.json5 should NOT exist (name-only meta → deleted)", + ); + }); +} + +// --------------------------------------------------------------------------- +// Tests: Model JSON rename (compound extension preservation) +// --------------------------------------------------------------------------- + +/// Renaming a .model.json5 instance must preserve the `.model` compound +/// extension and update the `name` field inside the model file (not in +/// adjacent .meta.json5). +#[test] +fn rename_model_json_preserves_compound_extension() { + run_serve_test("syncback_encoded_names", |session, _redactions| { + let (session_id, model_id) = get_encoded_names_instance(&session, "What?Model"); + + let src = session.path().join("src"); + let old_path = src.join("What_Model.model.json5"); + assert_file_exists(&old_path, "What_Model.model.json5 before rename"); + + // Rename to another name needing slugification + send_update( + &session, + &session_id, + InstanceUpdate { + id: model_id, + changed_name: Some("Why?Model".to_string()), + changed_class_name: None, + changed_properties: UstrMap::default(), + changed_metadata: None, + }, + ); + + // The .model compound extension MUST be preserved + let new_path = src.join("Why_Model.model.json5"); + poll_file_exists(&new_path, "Why_Model.model.json5 should exist after rename"); + poll_not_exists( + &old_path, + "What_Model.model.json5 should be gone after rename", + ); + + // The WRONG path (lost .model extension) must NOT exist + let wrong_path = src.join("Why_Model.json5"); + assert_not_exists( + &wrong_path, + "Why_Model.json5 (without .model) must NOT exist", + ); + + // The name field must be INSIDE the model file, not in adjacent meta. + // JSON5 serialization uses unquoted keys, so check for key without + // surrounding double-quotes. + let model_content = fs::read_to_string(&new_path).unwrap(); + assert!( + model_content.contains("name") && model_content.contains("Why?Model"), + "Model file should contain a name field with \"Why?Model\", got: {}", + model_content + ); + + // No adjacent .meta.json5 should be created for model files + let wrong_meta = src.join("Why_Model.meta.json5"); + assert_not_exists( + &wrong_meta, + "Adjacent .meta.json5 should NOT be created for model files", + ); + }); +} + +/// Renaming a .model.json5 instance to a clean name should remove the +/// `name` field from inside the model file. +#[test] +fn rename_model_json_to_clean_name() { + run_serve_test("syncback_encoded_names", |session, _redactions| { + let (session_id, model_id) = get_encoded_names_instance(&session, "What?Model"); + + let src = session.path().join("src"); + let old_path = src.join("What_Model.model.json5"); + assert_file_exists(&old_path, "What_Model.model.json5 before rename"); + + send_update( + &session, + &session_id, + InstanceUpdate { + id: model_id, + changed_name: Some("CleanModel".to_string()), + changed_class_name: None, + changed_properties: UstrMap::default(), + changed_metadata: None, + }, + ); + + let new_path = src.join("CleanModel.model.json5"); + poll_file_exists( + &new_path, + "CleanModel.model.json5 should exist after rename", + ); + poll_not_exists(&old_path, "What_Model.model.json5 should be gone"); + + // The model file should NOT have a "name" key when the + // filesystem name matches the instance name. + // JSON5 uses unquoted keys (`name:` not `"name":`), so we + // check for the key pattern at line start. + let model_content = fs::read_to_string(&new_path).unwrap(); + let has_name_key = model_content + .lines() + .any(|l| l.trim().starts_with("name:") || l.trim().starts_with("\"name\":")); + assert!( + !has_name_key, + "CleanModel.model.json5 should not have a name key, got: {}", + model_content + ); + let has_classname = model_content + .lines() + .any(|l| l.trim().starts_with("className:") || l.trim().starts_with("\"className\":")); + assert!( + has_classname, + "Model file should still have className, got: {}", + model_content + ); + }); +} + +// --------------------------------------------------------------------------- +// Tests: 3+ instance collision deduplication +// --------------------------------------------------------------------------- + +/// Adding 3 instances that all slug to the same name (X/Y, X:Y, X|Y → X_Y) +/// should produce X_Y.luau, X_Y~1.luau, X_Y~2.luau with correct meta files. +#[test] +fn add_three_colliding_instances_deduplicates() { + run_serve_test("syncback_encoded_names", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + let root_read = session.get_api_read(info.root_instance_id).unwrap(); + let (rs_id, _) = find_by_class(&root_read.instances, "ReplicatedStorage"); + + let names = ["X/Y", "X:Y", "X|Y"]; + let mut added_map = HashMap::new(); + for (i, name) in names.iter().enumerate() { + let mut props = HashMap::new(); + props.insert( + "Source".to_string(), + Variant::String(format!("-- instance {}", i + 1)), + ); + added_map.insert( + Ref::new(), + AddedInstance { + parent: Some(rs_id), + name: name.to_string(), + class_name: "ModuleScript".to_string(), + properties: props, + children: vec![], + }, + ); + } + let write_request = WriteRequest { + session_id: info.session_id, + removed: vec![], + added: added_map, + updated: vec![], + }; + session.post_api_write(&write_request).unwrap(); + thread::sleep(Duration::from_millis(500)); + + let src = session.path().join("src"); + let base = src.join("X_Y.luau"); + let dedup1 = src.join("X_Y~1.luau"); + let dedup2 = src.join("X_Y~2.luau"); + + assert!( + base.exists() && dedup1.exists() && dedup2.exists(), + "All three slugified files should exist.\n\ + X_Y.luau: {}, X_Y~1.luau: {}, X_Y~2.luau: {}", + base.exists(), + dedup1.exists(), + dedup2.exists() + ); + + // All three should have meta files with the original names + let base_meta = src.join("X_Y.meta.json5"); + let dedup1_meta = src.join("X_Y~1.meta.json5"); + let dedup2_meta = src.join("X_Y~2.meta.json5"); + assert_file_exists(&base_meta, "Meta for base slug"); + assert_file_exists(&dedup1_meta, "Meta for ~1 dedup"); + assert_file_exists(&dedup2_meta, "Meta for ~2 dedup"); + + // Each meta should contain one of the original names + let all_metas = [ + fs::read_to_string(&base_meta).unwrap(), + fs::read_to_string(&dedup1_meta).unwrap(), + fs::read_to_string(&dedup2_meta).unwrap(), + ]; + for meta in &all_metas { + let has_name = + meta.contains("\"X/Y\"") || meta.contains("\"X:Y\"") || meta.contains("\"X|Y\""); assert!( - !meta_content.contains("\"name\""), - "CleanName.meta.json5 should not have a \"name\" field, got: {}", - meta_content + has_name, + "Meta should contain one of X/Y, X:Y, X|Y, got: {}", + meta ); } }); } + +// --------------------------------------------------------------------------- +// Tests: Directory rename with forbidden chars +// --------------------------------------------------------------------------- + +/// Renaming a directory-format script to a name with forbidden chars should +/// rename the directory using the slugified name and write a meta file with +/// the real name. +#[test] +fn rename_directory_format_to_slugified_name() { + run_serve_test("syncback_format_transitions", |session, _redactions| { + let (session_id, dir_module_id) = + get_format_transitions_instance(&session, "DirModuleWithChildren"); + + let src = session.path().join("src"); + let old_dir = src.join("DirModuleWithChildren"); + assert!(old_dir.is_dir(), "Old directory should exist before rename"); + + // Rename to a name with forbidden chars + send_update( + &session, + &session_id, + InstanceUpdate { + id: dir_module_id, + changed_name: Some("Dir/With:Chars".to_string()), + changed_class_name: None, + changed_properties: UstrMap::default(), + changed_metadata: None, + }, + ); + + let new_dir = src.join("Dir_With_Chars"); + poll_not_exists(&old_dir, "Old directory should be gone after rename"); + + // Poll for the new directory + let start = Instant::now(); + loop { + if new_dir.is_dir() { + break; + } + if start.elapsed() > Duration::from_millis(API_POLL_TIMEOUT_MS) { + panic!( + "New directory Dir_With_Chars should exist after rename (timed out after {}ms)", + API_POLL_TIMEOUT_MS + ); + } + thread::sleep(Duration::from_millis(50)); + } + + // init.luau should be inside the renamed directory + let init_file = new_dir.join("init.luau"); + assert_file_exists(&init_file, "init.luau inside renamed directory"); + + // Children should be present in the renamed directory + let child_a = new_dir.join("ChildA.luau"); + assert_file_exists(&child_a, "ChildA.luau inside renamed directory"); + + // For init-file directories, the name is stored inside the + // directory in init.meta.json5, not adjacent to it. + let init_meta = new_dir.join("init.meta.json5"); + poll_file_exists( + &init_meta, + "init.meta.json5 inside Dir_With_Chars should exist with name override", + ); + let meta_content = fs::read_to_string(&init_meta).unwrap(); + assert!( + meta_content.contains("Dir/With:Chars"), + "init.meta.json5 should contain real name \"Dir/With:Chars\", got: {}", + meta_content + ); + }); +} + +// --------------------------------------------------------------------------- +// Tests: Case-insensitive collision +// --------------------------------------------------------------------------- + +/// Adding two instances whose names differ only in case (Foo and foo) should +/// be deduplicated, since they slug to the same lowercase key. +#[test] +fn add_case_insensitive_colliding_instances() { + run_serve_test("syncback_encoded_names", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + let root_read = session.get_api_read(info.root_instance_id).unwrap(); + let (rs_id, _) = find_by_class(&root_read.instances, "ReplicatedStorage"); + + let mut added_map = HashMap::new(); + let mut props1 = HashMap::new(); + props1.insert( + "Source".to_string(), + Variant::String("-- upper".to_string()), + ); + added_map.insert( + Ref::new(), + AddedInstance { + parent: Some(rs_id), + name: "CaseTest".to_string(), + class_name: "ModuleScript".to_string(), + properties: props1, + children: vec![], + }, + ); + let mut props2 = HashMap::new(); + props2.insert( + "Source".to_string(), + Variant::String("-- lower".to_string()), + ); + added_map.insert( + Ref::new(), + AddedInstance { + parent: Some(rs_id), + name: "casetest".to_string(), + class_name: "ModuleScript".to_string(), + properties: props2, + children: vec![], + }, + ); + + let write_request = WriteRequest { + session_id: info.session_id, + removed: vec![], + added: added_map, + updated: vec![], + }; + session.post_api_write(&write_request).unwrap(); + thread::sleep(Duration::from_millis(500)); + + let src = session.path().join("src"); + + // One should get the base name, the other should get ~1 + // The exact casing depends on processing order, but both files + // must exist with distinct names. + let base = src.join("CaseTest.luau"); + let base_lower = src.join("casetest.luau"); + let dedup = src.join("CaseTest~1.luau"); + let dedup_lower = src.join("casetest~1.luau"); + + let base_exists = base.exists() || base_lower.exists(); + let dedup_exists = dedup.exists() || dedup_lower.exists(); + + assert!( + base_exists && dedup_exists, + "Both case-insensitive colliding instances should exist as separate files.\n\ + CaseTest.luau: {}, casetest.luau: {}, CaseTest~1.luau: {}, casetest~1.luau: {}", + base.exists(), + base_lower.exists(), + dedup.exists(), + dedup_lower.exists() + ); + }); +}