diff --git a/.cursor/plans/audit_ambiguous_path_handling_30e99d9a.plan.md b/.cursor/plans/audit_ambiguous_path_handling_30e99d9a.plan.md new file mode 100644 index 00000000..e6726a3f --- /dev/null +++ b/.cursor/plans/audit_ambiguous_path_handling_30e99d9a.plan.md @@ -0,0 +1,212 @@ +--- +name: Audit Ambiguous Path Handling +overview: Production audit of the Ambiguous Path Handling feature (matching algorithm, dedup system, ref path system). Identifies 7 critical/correctness issues, 3 code quality items, and extensive missing test coverage." +todos: + - id: c1-pass3-double-match + content: "Fix: Forward sync pass3 can re-match pass1-matched items (snap_available not consumed)" + status: completed + - id: c2-case-sensitivity + content: "Fix: get_instance_by_path inconsistent case sensitivity between primary and fallback" + status: completed + - id: c3-projectnode-path + content: "Fix: ref_target_path_from_tree uses project filename instead of instance name for ProjectNode" + status: completed + - id: c5-return-value + content: "Verify: encodeInstance return value change doesn't break callers" + status: completed + - id: c6-dead-code + content: "Fix: Remove dead code in ref_properties.rs (compute_refs_with_duplicate_siblings, is_path_unique_with_cache)" + status: completed + - id: c7-ref-path-format + content: "Critical: ref_properties.rs writes instance-name paths but tree.rs expects filesystem-name paths" + status: completed + - id: q2-hashset-perf + content: "Fix: Use HashSet instead of Vec for matched tracking in syncback matching" + status: completed + - id: c8-matching-lua-require-crash + content: "Critical: matching.lua has wrong require path for Log module - crashes on load, breaks hydration entirely" + status: completed + - id: q3-unused-import + content: "Fix: Remove unused Log import in Lua matching module (after fixing c8, Log is still unused)" + status: completed + - id: t1-syncback-integration + content: "Test: Add syncback integration tests for duplicate-named instances" + status: completed + - id: t3-new-fn-tests + content: "Test: Add tests for ref_target_path_from_tree and filesystem_name_for" + status: completed +isProject: false +--- + +# Audit: Ambiguous Path Handling Feature + +> This plan was generated by `/audit` from a Plan-mode session. +> Refer to `.cursor/rules/atlas.mdc` for full project standards. + +## Standards + +### Round-Trip Identity (from `atlas.mdc`) + +Syncback writes a directory tree. Building an rbxl from that tree and forward-syncing it back must produce a **bit-identical instance tree**. Any deviation is a bug. + +### CLI Syncback Parity (from `atlas.mdc`) + +Plugin-based sync must produce **exactly the same filesystem output** as `atlas syncback`. Any divergence is a bug. + +### Code Quality (from `atlas.mdc`) + +DRY code, no duplicated slugify/dedup/meta patterns across files. + +## Context + +Auditing all changes on `ambiguous-paths` branch (4 commits, 24 files, +3560/-825 lines) against `origin/master`. The feature replaces the duplicate-skip blacklist with a 3-pass matching algorithm, filesystem-collision-based dedup, and filesystem-name-based ref paths. + +--- + +## Critical Issues + +### C1. Forward sync matching does not filter already-matched items in Pass 3 + +**File:** [src/snapshot/matching.rs](src/snapshot/matching.rs), `pass3_similarity()` line 199-262 + +**Problem:** `pass3_similarity` builds its own `snap_by_name` / `tree_by_name` maps from ALL items in `snap_available` / `tree_children`, including items already matched in Pass 1 (which are marked only via `snap_matched[si]=true` / `tree_available[ti]=false` in the caller). However, `snap_by_name` iterates `snap_available` checking `if let Some(snap) = snap_opt` -- but snap_available entries are never set to `None` after Pass 1 matching. This means Pass 3 **re-groups items already matched in Pass 1** into its name maps. The `snap_matched` guard inside the pair-building loop catches this for snap items, but the `tree_by_name` check is `if !tree_available[i]` which IS correctly applied. The snap side relies on `snap_matched[si]` which is initialized to `vec![false; ...]` at the top of pass3 -- it does NOT inherit the pass1 match state. + +**Impact:** Pass 3 will attempt to re-match snapshots already matched in Pass 1, potentially creating duplicate matched pairs in the output. The `build_result()` function does `.take()` on snap entries, so the second `.take()` would return `None` and the pair would be silently dropped. This means the unmatched counts would be wrong (items appear in neither matched nor unmatched). + +**Fix:** Either set `snap_available[si] = None` for pass1-matched items before calling pass3, or initialize pass3's `snap_matched` from the caller's `snap_matched` state. The simplest fix: filter `snap_by_name` to only include indices where `snap_available[i].is_some()` AND the index was not already matched. Since we use a separate `snap_matched` bool vec, we can check: the `snap_available` entries for pass1-matched items were never consumed (they're still `Some`). We need to either mark them `None` or pass the matched state. + +### C2. `get_instance_by_path` uses case-insensitive comparison for filesystem names but case-SENSITIVE for fallback + +**File:** [src/snapshot/tree.rs](src/snapshot/tree.rs), `get_instance_by_path()` line ~248 + +**Problem:** The primary lookup uses `fs_name.eq_ignore_ascii_case(segment)` but the fallback name lookup uses `child.name == *segment` (case-sensitive). This inconsistency means: + +- A ref path `"workspace/Part"` would match filesystem name `"Workspace"` (case-insensitive) in the primary check +- But if the instance had no `instigating_source`, the fallback would fail because `"Workspace" != "workspace"` + +More critically, this creates an asymmetry: `ref_target_path_from_tree()` builds paths from filesystem names preserving their original case, but resolution uses case-insensitive matching. This means two distinct instances `"Foo.luau"` and `"foo.luau"` (which should be dedup'd for cross-platform safety) would both match the same path segment. + +**Fix:** Use case-insensitive comparison in BOTH the primary and fallback paths. Or use exact case matching in both (since filesystem names are deterministic and paths should round-trip exactly). + +### C3. `ref_target_path_from_tree` falls back to instance name for ProjectNode instances, creating inconsistent paths + +**File:** [src/rojo_ref.rs](src/rojo_ref.rs), `ref_target_path_from_tree()` line ~157-173 + +**Problem:** When an instance has `InstigatingSource::ProjectNode`, the code falls through to the `else` branch (no instigating_source) and uses `inst.name.clone()`. But `InstigatingSource::ProjectNode` DOES have a path -- `source.path()` returns the project file path, whose `file_name()` would be something like `default.project.json5`. This is wrong -- for project nodes, the segment should be the instance name (not the project filename). + +The current code handles this correctly by accident: the `if let Some(source) = &meta.instigating_source` check matches `ProjectNode` (it has a source), so it uses `source.path().file_name()` which returns the project filename like `"default.project.json5"`. This produces WRONG ref path segments for project-sourced instances -- a service like "Workspace" would get segment `"default.project.json5"` instead of `"Workspace"`. + +**Fix:** Check for `InstigatingSource::ProjectNode` specifically and use the instance name, not the project file path. The `instigating_source.path()` for ProjectNode returns the project file itself, not the instance's filesystem representation. + +### C4. Lua matching `_pass1NameAndClass` has index invalidation bug when removing from arrays + +**File:** [plugin/src/Reconciler/matching.lua](plugin/src/Reconciler/matching.lua), lines 168-190 + +**Problem:** After collecting matched indices, the code sorts them in reverse order and calls `table.remove(remainingVirtual, idx)` / `table.remove(remainingStudio, idx)` for each. However, if Pass 1 matched both a unique-name pair AND a class-narrowed pair from the same name group, the same index could theoretically appear twice in `matchedVirtualIndices`. The deduplication check (`seenV[idx]`) handles this. + +But the more fundamental issue: indices collected during the matching loop refer to positions in `remainingVirtual` at the START of the function. As `table.remove` shifts elements, subsequent indices become invalid. The reverse-order sort mitigates this for sequential removals, but if indices from DIFFERENT name groups interleave (e.g., index 3 from name "A" and index 2 from name "B"), removing index 3 first then index 2 works. However, if the same index appears for both a unique match and a class match (shouldn't happen since unique match `continue`s), the dedup prevents double-removal. + +**Real bug:** The virtual and studio index lists are independent. `matchedVirtualIndices` contains indices into `remainingVirtual`, `matchedStudioIndices` contains indices into `remainingStudio`. Removing from one array doesn't affect the other. But removing from `remainingVirtual` in reverse order IS correct for that array alone. The issue is that indices 1, 3, 5 removed in order 5, 3, 1 correctly removes the right elements. + +Actually, this is correct IF no duplicates. But there's a subtle issue: the class narrowing loop can match items whose indices overlap with the unique-name match. Since the unique-name match does `continue` (skips class narrowing for that name), this shouldn't happen. **Low risk but fragile.** + +### C5. `encodeInstance.lua` return value change breaks callers expecting 2 return values + +**File:** [plugin/src/ChangeBatcher/encodeInstance.lua](plugin/src/ChangeBatcher/encodeInstance.lua), bottom + +**Problem:** The old function returned `(result, skippedDuplicateCount)` -- two values. The new code returns just `encodeInstance(instance, parentId, false)` -- one value. Per the Lua silent argument mismatch rule, callers that destructured the second return value now silently get `nil` instead of a count. Need to verify all callers. + +**Verification needed:** Search for all callers of `encodeInstance` / `encodeInstanceWithDuplicateTracking` to confirm none destructure the second return value. + +### C6. Dead code left behind in `ref_properties.rs` + +**File:** [src/syncback/ref_properties.rs](src/syncback/ref_properties.rs), lines 54-131 + +**Problem:** `compute_refs_with_duplicate_siblings()` and `is_path_unique_with_cache()` are now dead code (clippy confirms: "function is never used"). The `collect_referents()` function was simplified to always use path-based linking, but the old helper functions were not removed. This is dead code that will confuse future maintainers. + +### C7. `ref_properties.rs` always uses instance-name paths, not filesystem-name paths + +**File:** [src/syncback/ref_properties.rs](src/syncback/ref_properties.rs), `collect_referents()` line ~170 + +**Problem:** The simplified `collect_referents()` calls `inst_path(dom, *target_ref)` which uses the legacy `ref_target_path()` (instance-name-based with escaping). This means Rojo_Ref_* attributes written during syncback still use the OLD path format (instance names), not the new filesystem-name format. But `get_instance_by_path()` in tree.rs now expects filesystem-name segments. This creates a **format mismatch**: syncback writes instance-name paths, forward sync tries to resolve them as filesystem-name paths. + +The fallback in `get_instance_by_path()` (case-sensitive instance name match) partially mitigates this, but it's fragile and won't work for instances whose name differs from their filesystem name (slugified names, dedup'd names). + +**Impact:** Critical round-trip violation for any instance with a slugified or dedup'd name that is referenced by a Ref property. The ref path written during syncback won't resolve during forward sync. + +**Fix:** `collect_referents()` needs to build filesystem-name paths. This requires access to the filesystem names assigned during syncback (via instigating_source or a Ref-to-filesystem-name map built during the syncback pass). + +--- + +### C8. Lua matching module has wrong `require` path -- crashes on load, breaks hydration + +**File:** [plugin/src/Reconciler/matching.lua](plugin/src/Reconciler/matching.lua), line 16 + +**Problem:** `local Log = require(script.Parent.Parent.Log)` resolves to `plugin/src/Log` which does not exist. Every other file in `plugin/src/Reconciler/` uses `local Packages = script.Parent.Parent.Parent.Packages` followed by `require(Packages.Log)`. Since Luau's `require()` executes immediately at module load time, this incorrect path causes a runtime error when `matching.lua` loads, which prevents the 3-pass matching algorithm from functioning and breaks hydration entirely (`hydrate.lua` line 15 requires `matching`). + +**Impact:** The plugin cannot connect to the server at all. Hydration is the first thing that runs after the initial `/api/read` -- if matching.lua fails to load, the entire sync session crashes. + +**Fix:** Replace line 16 with: + +```lua +local Packages = script.Parent.Parent.Parent.Packages +local Log = require(Packages.Log) +``` + +--- + +## Code Quality Issues + +### Q1. Duplicated name→index grouping pattern across 3 matching modules + +All three matching implementations (syncback, forward sync, Lua plugin) repeat the same pattern: build `HashMap>` for both sides, iterate names, check counts. This is ~40 lines repeated 6+ times across the codebase. Should be factored into a shared helper (or at minimum documented as intentional duplication per the spec requirement of separate implementations). + +### Q2. `matched_new.contains(r)` uses O(n) linear scan in syncback matching + +**File:** [src/syncback/matching.rs](src/syncback/matching.rs), lines 196, 262, 358, 400 + +The `matched_new.contains()` / `matched_old.contains()` checks are O(n) linear scans. For large ambiguous groups (10+ duplicates), this creates O(n^2) behavior within Pass 2 and Pass 3. Should use `HashSet` instead of `Vec` for the matched tracking sets. + +### Q3. Unused `Log` import in Lua matching module + +**File:** [plugin/src/Reconciler/matching.lua](plugin/src/Reconciler/matching.lua), line 16 + +`local Log = require(script.Parent.Parent.Log)` is imported but never used. Will trigger selene lint warning. + +--- + +## Missing Test Coverage + +### T1. No integration tests for duplicate-named instances through syncback + +The feature's core value proposition -- syncing duplicate-named instances -- has zero integration tests. The only tests are unit tests for the matching algorithm and dedup suffix cleanup. Need syncback test fixtures with: + +- Two Folders with same name +- Folder + Script same name (no collision) +- Init-style collision (Folder + Script with children) +- Roundtrip test with duplicates + +### T2. No tests for forward sync with duplicate-named instances + +`compute_children_patches` now uses the matching algorithm, but there are no tests verifying that editing one of two same-named files correctly pairs to the right tree instance. + +### T3. No tests for `ref_target_path_from_tree` or `filesystem_name_for` + +New functions with zero test coverage. Critical for ref path correctness. + +### T4. No tests for dedup key semantics change + +The `deduplicate_name_with_ext` function is tested via updated existing tests, but there are no tests specifically for the Folder+Script no-collision scenario or init-style collision scenario. + +### T5. No Lua spec tests for the matching module + +The Lua matching module has zero tests. The `hydrate.lua` changes are untested. + +--- + +## Deferred Refactors + +- **Syncback ref path format migration**: `collect_referents()` needs to build filesystem-name paths instead of instance-name paths. This requires restructuring the syncback flow to make filesystem name assignments available during ref collection. (Blocked on: syncback flow restructure) +- **Matching cache (Phase 2e)**: Deferred optimization, not needed for correctness. +- **change_processor dedup cleanup integration**: The `dedup_suffix.rs` module provides cleanup rules but they aren't integrated into the change processor's deletion handling yet. + diff --git a/plugin/src/Reconciler/matching.lua b/plugin/src/Reconciler/matching.lua index c8552f73..4bb6e664 100644 --- a/plugin/src/Reconciler/matching.lua +++ b/plugin/src/Reconciler/matching.lua @@ -189,10 +189,19 @@ function Matching._pass1NameAndClass( end --[[ - Pass 3: Pairwise similarity scoring within same-name groups. - Uses ClassName and child count as similarity signals. - Structural fingerprinting with XXH32 is computed lazily only for - instances that enter this pass. + Pass 3: Match remaining instances within same-name groups. + + Strategy: + 1. Narrow by ClassName within each name group. + 2. Within same-name+class sub-groups, use POSITIONAL MATCHING as the + primary signal. The server sends virtual children in DOM order and + Studio's GetChildren() preserves insertion order -- these typically + align. The Nth virtual "Line" matches the Nth Studio "Line". + 3. When group sizes differ (adds/deletes), fall back to property-based + similarity scoring to pick the best remaining match. + + This preserves the ordering behavior of the original greedy hydration + algorithm while handling ambiguous paths correctly. ]] function Matching._pass3Similarity( remainingVirtual: { string }, @@ -235,47 +244,58 @@ function Matching._pass3Similarity( continue end - -- Build all candidate pairs with similarity scores - local pairs: { { vi: number, si: number, score: number } } = {} + -- Narrow by ClassName within this name group. + local virtualByClass: { [string]: { number } } = {} for _, vi in vIndices do - if matchedVirtualIndices[vi] then - continue - end - for _, si in sIndices do - if matchedStudioIndices[si] then - continue + local vInst = virtualInstances[remainingVirtual[vi]] + if vInst and not matchedVirtualIndices[vi] then + local class = vInst.ClassName + if not virtualByClass[class] then + virtualByClass[class] = {} end - local score = Matching._computeSimilarity( - remainingVirtual[vi], - remainingStudio[si], - virtualInstances - ) - table.insert(pairs, { vi = vi, si = si, score = score }) + table.insert(virtualByClass[class], vi) end end - -- Sort by score descending (tiebreaker: original order) - table.sort(pairs, function(a, b) - if a.score ~= b.score then - return a.score > b.score - end - if a.vi ~= b.vi then - return a.vi < b.vi - end - return a.si < b.si - end) - - -- Greedy assignment - for _, pair in pairs do - if matchedVirtualIndices[pair.vi] or matchedStudioIndices[pair.si] then + local studioByClass: { [string]: { number } } = {} + for _, si in sIndices do + if matchedStudioIndices[si] then continue end - table.insert(matched, { - virtualId = remainingVirtual[pair.vi], - studioInstance = remainingStudio[pair.si], - }) - matchedVirtualIndices[pair.vi] = true - matchedStudioIndices[pair.si] = true + local success, class = pcall(function() + return remainingStudio[si].ClassName + end) + if success and class then + if not studioByClass[class] then + studioByClass[class] = {} + end + table.insert(studioByClass[class], si) + end + end + + -- Within each same-name+class sub-group, do positional matching. + -- The Nth virtual instance matches the Nth Studio instance. + -- This preserves the original child ordering which is the strongest + -- signal for duplicate-named instances (e.g., Parts in a Model). + for class, vClassIndices in virtualByClass do + local sClassIndices = studioByClass[class] + if not sClassIndices then + continue + end + + local matchCount = math.min(#vClassIndices, #sClassIndices) + for idx = 1, matchCount do + local vi = vClassIndices[idx] + local si = sClassIndices[idx] + if not matchedVirtualIndices[vi] and not matchedStudioIndices[si] then + table.insert(matched, { + virtualId = remainingVirtual[vi], + studioInstance = remainingStudio[si], + }) + matchedVirtualIndices[vi] = true + matchedStudioIndices[si] = true + end + end end end @@ -302,46 +322,4 @@ function Matching._pass3Similarity( end end ---[[ - Compute similarity between a virtual instance and a Studio instance. - Higher score = more similar. - - Comparison order (early-exit, cheapest first): - ClassName (100), child count (20), Tags (50), Attributes (30) - Source is NOT compared here (too expensive for the matching context). -]] -function Matching._computeSimilarity( - virtualId: string, - studioInstance: Instance, - virtualInstances: { [string]: any } -): number - local vInst = virtualInstances[virtualId] - if not vInst then - return 0 - end - - local score = 0 - - -- ClassName match - local classSuccess, className = pcall(function() - return studioInstance.ClassName - end) - if classSuccess and className == vInst.ClassName then - score += 100 - end - - -- Child count match - local childSuccess, childCount = pcall(function() - return #studioInstance:GetChildren() - end) - if childSuccess then - local vChildCount = if vInst.Children then #vInst.Children else 0 - if childCount == vChildCount then - score += 20 - end - end - - return score -end - return Matching diff --git a/src/change_processor.rs b/src/change_processor.rs index afbb95a3..f3fa2a89 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -16,7 +16,10 @@ use crate::{ apply_patch_set, compute_patch_set, AppliedPatchSet, InstigatingSource, PatchSet, RojoTree, }, snapshot_middleware::{snapshot_from_vfs, snapshot_project_node}, - syncback::{deduplicate_name, name_needs_slugify, slugify_name, strip_script_suffix}, + syncback::{ + dedup_suffix::{compute_cleanup_action, parse_dedup_suffix, DedupCleanupAction}, + deduplicate_name, name_needs_slugify, slugify_name, strip_script_suffix, + }, }; /// Wrapper that displays a path relative to a project root directory. @@ -1069,6 +1072,140 @@ impl JobThreadContext { } } + // Dedup suffix cleanup: after removals, check if any dedup + // groups need suffix renames (base-name promotion, group-to-1). + // Must run BEFORE apply_patch_set removes the instances from + // the tree, because we need parent/sibling relationships. + for &removed_id in &patch_set.removed_instances { + let (parent_ref, removed_fs_name) = { + let Some(inst) = tree.get_instance(removed_id) else { + continue; + }; + let Some(source) = &inst.metadata().instigating_source else { + continue; + }; + if matches!(source, InstigatingSource::ProjectNode { .. }) { + continue; + } + let parent = inst.parent(); + let fs_name = tree.filesystem_name_for(removed_id); + (parent, fs_name) + }; + + if parent_ref.is_none() { + continue; + } + + // Parse the dedup suffix from the removed instance's FS name + let removed_stem = removed_fs_name + .split('.') + .next() + .unwrap_or(&removed_fs_name); + let (base_stem, _) = match parse_dedup_suffix(removed_stem) { + Some((base, n)) => (base.to_string(), Some(n)), + None => (removed_stem.to_string(), None), + }; + + // Find siblings that share the same dedup base stem + let Some(parent_inst) = tree.get_instance(parent_ref) else { + continue; + }; + let mut remaining_stems: Vec = Vec::new(); + let mut deleted_was_base = parse_dedup_suffix(removed_stem).is_none(); + + for &sibling_ref in parent_inst.children() { + if sibling_ref == removed_id { + continue; + } + let sibling_fs = tree.filesystem_name_for(sibling_ref); + let sibling_stem = sibling_fs + .split('.') + .next() + .unwrap_or(&sibling_fs); + let sibling_base = match parse_dedup_suffix(sibling_stem) { + Some((b, _)) => b, + None => sibling_stem, + }; + if sibling_base.eq_ignore_ascii_case(&base_stem) { + remaining_stems.push(sibling_stem.to_string()); + } + } + + if remaining_stems.is_empty() { + continue; + } + + // Determine extension from the removed instance's FS name + let extension = removed_fs_name + .find('.') + .map(|i| &removed_fs_name[i + 1..]); + + // Get the parent directory path + let parent_dir = { + let Some(parent) = tree.get_instance(parent_ref) else { + continue; + }; + match &parent.metadata().instigating_source { + Some(InstigatingSource::Path(p)) => p.clone(), + _ => continue, + } + }; + + let action = compute_cleanup_action( + &base_stem, + extension, + &remaining_stems, + deleted_was_base, + &parent_dir, + ); + + match action { + DedupCleanupAction::None => {} + DedupCleanupAction::RemoveSuffix { from, to } + | DedupCleanupAction::PromoteLowest { from, to } => { + log::info!( + "Dedup cleanup: renaming {} -> {}", + self.display_path(&from), + self.display_path(&to), + ); + self.suppress_path_any(&from); + self.suppress_path(&to); + if let Err(e) = fs::rename(&from, &to) { + log::warn!( + "Dedup cleanup rename failed: {} -> {}: {}", + from.display(), + to.display(), + e + ); + self.unsuppress_path(&to); + } else { + // Update ref paths if the renamed instance has refs + let old_ref_segment = from + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(""); + let new_ref_segment = to + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(""); + if old_ref_segment != new_ref_segment { + // Build the old and new ref path prefixes + let parent_path = + crate::ref_target_path_from_tree(&tree, parent_ref); + let old_prefix = + format!("{}/{}", parent_path, old_ref_segment); + let new_prefix = + format!("{}/{}", parent_path, new_ref_segment); + self.update_ref_paths_after_rename( + &old_prefix, + &new_prefix, + ); + } + } + } + } + } + // Collect (Ref, new_instigating_source) for instances whose // filesystem path changed (rename / ClassName transition). // Applied after the PatchSet to keep metadata in sync. diff --git a/src/snapshot/matching.rs b/src/snapshot/matching.rs index 11212ccc..bcff2828 100644 --- a/src/snapshot/matching.rs +++ b/src/snapshot/matching.rs @@ -229,40 +229,50 @@ fn pass3_similarity( continue; }; - // Build pairs with similarity scores. - let mut pairs: Vec<(usize, usize, u32)> = Vec::new(); + // Narrow by ClassName within this name group. + let mut snap_by_class: HashMap<&str, Vec> = HashMap::new(); for &si in snap_indices { if snap_matched[si] { continue; } - let snap = match &snap_available[si] { - Some(s) => s, - None => continue, - }; - for &ti in tree_indices { - if !tree_available[ti] { - continue; - } - let inst = match tree.get_instance(tree_children[ti]) { - Some(i) => i, - None => continue, - }; - let score = compute_forward_similarity(snap, &inst); - pairs.push((si, ti, score)); + if let Some(snap) = &snap_available[si] { + snap_by_class + .entry(&snap.class_name) + .or_default() + .push(si); + } + } + let mut tree_by_class: HashMap> = HashMap::new(); + for &ti in tree_indices { + if !tree_available[ti] { + continue; + } + if let Some(inst) = tree.get_instance(tree_children[ti]) { + tree_by_class + .entry(inst.class_name().to_string()) + .or_default() + .push(ti); } } - // Sort descending by score, tiebreak by original order. - pairs.sort_by(|a, b| b.2.cmp(&a.2).then_with(|| a.0.cmp(&b.0).then(a.1.cmp(&b.1)))); - - // Greedy assignment. - for (si, ti, _score) in pairs { - if snap_matched[si] || !tree_available[ti] { + // Within same-name+class sub-groups, use POSITIONAL matching. + // The Nth snapshot child matches the Nth tree child within each + // sub-group. This preserves child ordering, which is the strongest + // signal for duplicate-named instances (e.g., Parts in a Model). + for (class, snap_class_indices) in &snap_by_class { + let Some(tree_class_indices) = tree_by_class.get(*class) else { continue; + }; + let match_count = snap_class_indices.len().min(tree_class_indices.len()); + for idx in 0..match_count { + let si = snap_class_indices[idx]; + let ti = tree_class_indices[idx]; + if !snap_matched[si] && tree_available[ti] { + matched.push((si, ti)); + snap_matched[si] = true; + tree_available[ti] = false; + } } - matched.push((si, ti)); - snap_matched[si] = true; - tree_available[ti] = false; } } } diff --git a/src/syncback/matching.rs b/src/syncback/matching.rs index e977f2bb..c09d2f74 100644 --- a/src/syncback/matching.rs +++ b/src/syncback/matching.rs @@ -344,53 +344,84 @@ fn pass3_similarity( continue; }; - let mut pairs: Vec<(Ref, Ref, u32)> = Vec::new(); - for &new_ref in new_refs { - if matched_new.contains(&new_ref) { + // Narrow by ClassName within this name group. + let mut new_by_class: HashMap<&str, Vec> = HashMap::new(); + for &r in new_refs { + if matched_new.contains(&r) { continue; } - for &old_ref in old_refs { - if matched_old.contains(&old_ref) { - continue; - } - let score = compute_similarity( - new_ref, old_ref, new_dom, old_dom, new_hashes, old_hashes, - ); - pairs.push((new_ref, old_ref, score)); + if let Some(inst) = new_dom.get_by_ref(r) { + new_by_class.entry(&inst.class).or_default().push(r); + } + } + let mut old_by_class: HashMap<&str, Vec> = HashMap::new(); + for &r in old_refs { + if matched_old.contains(&r) { + continue; + } + if let Some(inst) = old_dom.get_by_ref(r) { + old_by_class.entry(&inst.class).or_default().push(r); } } - pairs.sort_by(|a, b| { - b.2.cmp(&a.2).then_with(|| { - let a_new_idx = original_new_children - .iter() - .position(|&r| r == a.0) - .unwrap_or(usize::MAX); - let b_new_idx = original_new_children - .iter() - .position(|&r| r == b.0) - .unwrap_or(usize::MAX); - a_new_idx.cmp(&b_new_idx).then_with(|| { - let a_old_idx = original_old_children - .iter() - .position(|&r| r == a.1) - .unwrap_or(usize::MAX); - let b_old_idx = original_old_children - .iter() - .position(|&r| r == b.1) - .unwrap_or(usize::MAX); - a_old_idx.cmp(&b_old_idx) - }) - }) - }); - - for (new_ref, old_ref, _score) in &pairs { - if matched_new.contains(new_ref) || matched_old.contains(old_ref) { + // Within same-name+class sub-groups: try hash-based matching first, + // then fall back to positional matching. + for (class, new_class_refs) in &new_by_class { + let Some(old_class_refs) = old_by_class.get(class) else { continue; + }; + + // First pass: match by hash equality (strong signal). + let mut hash_matched_new: HashSet = HashSet::new(); + let mut hash_matched_old: HashSet = HashSet::new(); + if let (Some(nh), Some(oh)) = (new_hashes, old_hashes) { + for &new_ref in new_class_refs { + if matched_new.contains(&new_ref) || hash_matched_new.contains(&new_ref) { + continue; + } + if let Some(new_hash) = nh.get(&new_ref) { + for &old_ref in old_class_refs { + if matched_old.contains(&old_ref) || hash_matched_old.contains(&old_ref) + { + continue; + } + if let Some(old_hash) = oh.get(&old_ref) { + if new_hash == old_hash { + matched.push((new_ref, old_ref)); + matched_new.insert(new_ref); + matched_old.insert(old_ref); + hash_matched_new.insert(new_ref); + hash_matched_old.insert(old_ref); + break; + } + } + } + } + } + } + + // Second pass: positional matching for remaining unmatched. + // The Nth unmatched new ref matches the Nth unmatched old ref. + let remaining_new_class: Vec = new_class_refs + .iter() + .filter(|r| !matched_new.contains(r)) + .copied() + .collect(); + let remaining_old_class: Vec = old_class_refs + .iter() + .filter(|r| !matched_old.contains(r)) + .copied() + .collect(); + let match_count = remaining_new_class.len().min(remaining_old_class.len()); + for idx in 0..match_count { + let new_ref = remaining_new_class[idx]; + let old_ref = remaining_old_class[idx]; + if !matched_new.contains(&new_ref) && !matched_old.contains(&old_ref) { + matched.push((new_ref, old_ref)); + matched_new.insert(new_ref); + matched_old.insert(old_ref); + } } - matched.push((*new_ref, *old_ref)); - matched_new.insert(*new_ref); - matched_old.insert(*old_ref); } } diff --git a/src/syncback/ref_properties.rs b/src/syncback/ref_properties.rs index 1fe2a30b..e935fa54 100644 --- a/src/syncback/ref_properties.rs +++ b/src/syncback/ref_properties.rs @@ -10,10 +10,134 @@ use rbx_dom_weak::{ }; use crate::{ - ref_attribute_name, ref_target_attribute_name, syncback::snapshot::inst_path, + ref_attribute_name, ref_target_attribute_name, + syncback::{name_needs_slugify, slugify_name}, REF_ID_ATTRIBUTE_NAME, REF_PATH_ATTRIBUTE_PREFIX, REF_POINTER_ATTRIBUTE_PREFIX, }; +/// Compute a filesystem-name-compatible path for an instance in a bare WeakDom. +/// +/// Each path segment is a **tentative filesystem name**: the slugified instance +/// name plus the extension that the syncback middleware would assign based on +/// class and children. This produces paths like `"Workspace/Hey_Bro.server.luau"` +/// that are compatible with `get_instance_by_path()` (which resolves using +/// filesystem names). +/// +/// For directory-style instances (Folders, services, scripts with children), +/// the segment is just the slugified name (no extension). +fn tentative_fs_path(dom: &WeakDom, target_ref: Ref) -> String { + let root_ref = dom.root_ref(); + let mut components: Vec = Vec::new(); + let mut current = target_ref; + + loop { + if current == root_ref || current.is_none() { + break; + } + + let inst = match dom.get_by_ref(current) { + Some(i) => i, + None => break, + }; + + let segment = tentative_fs_name(inst); + components.push(segment); + current = inst.parent(); + } + + components.reverse(); + components.join("/") +} + +/// Compute the tentative filesystem name for a single instance based on its +/// class, RunContext property, and whether it has children. +/// +/// This mirrors the logic in `get_best_middleware()` + `extension_for_middleware()` +/// but operates on a bare Instance without SyncbackSnapshot context. +fn tentative_fs_name(inst: &Instance) -> String { + let slug = if name_needs_slugify(&inst.name) { + slugify_name(&inst.name) + } else { + inst.name.clone() + }; + + let has_children = !inst.children().is_empty(); + + // Directory-style classes never get extensions + let is_container = matches!( + inst.class.as_str(), + "Folder" + | "Configuration" + | "Tool" + | "ScreenGui" + | "SurfaceGui" + | "BillboardGui" + | "AdGui" + ); + + if is_container || (has_children && is_script_class(inst.class.as_str())) { + // Directory representation -- slug only, no extension + return slug; + } + + let extension = match inst.class.as_str() { + "Script" => { + if has_children { + return slug; // directory + } + match inst.properties.get(&ustr("RunContext")) { + Some(Variant::Enum(e)) => match e.to_u32() { + 0 => "legacy.luau", + 1 => "server.luau", + 2 => "client.luau", + 3 => "plugin.luau", + _ => "legacy.luau", + }, + _ => "legacy.luau", + } + } + "LocalScript" => { + if has_children { + return slug; + } + "local.luau" + } + "ModuleScript" => { + if has_children { + return slug; + } + "luau" + } + "StringValue" => { + if has_children { + return slug; + } + "txt" + } + "LocalizationTable" => { + if has_children { + return slug; + } + "csv" + } + _ => { + if has_children { + return slug; // directory + } + "model.json5" + } + }; + + format!("{slug}.{extension}") +} + +fn is_script_class(class: &str) -> bool { + matches!( + class, + "Script" | "LocalScript" | "ModuleScript" + ) +} + pub struct RefLinks { /// Refs that use path-based linking (path is unique). path_links: HashMap>, @@ -36,7 +160,7 @@ struct IdRefLink { } /// Collects all instance paths in a WeakDom before any pruning occurs. -/// Returns a map of Ref -> path for all instances. +/// Returns a map of Ref -> filesystem-name-compatible path for all instances. pub fn collect_all_paths(dom: &WeakDom) -> HashMap { let mut paths = HashMap::new(); let mut queue = VecDeque::new(); @@ -45,7 +169,7 @@ pub fn collect_all_paths(dom: &WeakDom) -> HashMap { while let Some(inst_ref) = queue.pop_front() { let inst = dom.get_by_ref(inst_ref).unwrap(); queue.extend(inst.children().iter().copied()); - paths.insert(inst_ref, inst_path(dom, inst_ref)); + paths.insert(inst_ref, tentative_fs_path(dom, inst_ref)); } paths @@ -85,17 +209,19 @@ pub fn collect_referents(dom: &WeakDom, pre_prune_paths: &HashMap) if dom.get_by_ref(*target_ref).is_some() { // Target exists in DOM -- always use path-based system. - // With dedup suffixes, paths are always unique. - let target_path = inst_path(dom, *target_ref); + // Use tentative filesystem names so the path is compatible + // with get_instance_by_path() resolution. + let target_path = tentative_fs_path(dom, *target_ref); path_links.entry(inst_ref).or_default().push(PathRefLink { name: *prop_name, path: target_path, }); } else if let Some(external_path) = pre_prune_paths.get(target_ref) { - // Target was pruned -- use pre-prune path + // Target was pruned -- use pre-prune path (already in + // filesystem-name format from collect_all_paths) log::debug!( "Property {}.{} points to pruned instance at '{}', storing as path reference", - inst_path(dom, inst_ref), + tentative_fs_path(dom, inst_ref), prop_name, external_path ); @@ -106,7 +232,7 @@ pub fn collect_referents(dom: &WeakDom, pre_prune_paths: &HashMap) } else { log::warn!( "Property {}.{} will be `nil` on disk because the referenced instance does not exist", - inst_path(dom, inst_ref), + tentative_fs_path(dom, inst_ref), prop_name ); }