---
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<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](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.