rojo/.cursor/plans/audit_ambiguous_path_handling_30e99d9a.plan.md

name	overview	todos	isProject
Audit Ambiguous Path Handling	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."	id content status c1-pass3-double-match Fix: Forward sync pass3 can re-match pass1-matched items (snap_available not consumed) completed id content status c2-case-sensitivity Fix: get_instance_by_path inconsistent case sensitivity between primary and fallback completed id content status c3-projectnode-path Fix: ref_target_path_from_tree uses project filename instead of instance name for ProjectNode completed id content status c5-return-value Verify: encodeInstance return value change doesn't break callers completed id content status c6-dead-code Fix: Remove dead code in ref_properties.rs (compute_refs_with_duplicate_siblings, is_path_unique_with_cache) completed id content status c7-ref-path-format Critical: ref_properties.rs writes instance-name paths but tree.rs expects filesystem-name paths completed id content status q2-hashset-perf Fix: Use HashSet instead of Vec for matched tracking in syncback matching completed id content status c8-matching-lua-require-crash Critical: matching.lua has wrong require path for Log module - crashes on load, breaks hydration entirely completed id content status q3-unused-import Fix: Remove unused Log import in Lua matching module (after fixing c8, Log is still unused) completed id content status t1-syncback-integration Test: Add syncback integration tests for duplicate-named instances completed id content status t3-new-fn-tests Test: Add tests for ref_target_path_from_tree and filesystem_name_for completed	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, 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, 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, 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, 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 continues), 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, 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, 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, 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, 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:

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<name, Vec<index>> 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, 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, 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.