---
name: Ambiguous paths audit fixes
overview: "Fix 6 issues found during the ambiguous-paths branch audit: make Rust matching recursive, fix ref path format in api.rs added_paths, restructure syncback ref collection for dedup-aware paths, improve dedup cleanup grouping precision, extract shared trueEquals module, and correct a misleading comment."
todos:
  - id: fix1-recursive-matching
    content: "Fix 1: Make Rust matching implementations recursive (both forward sync and syncback) to match the Lua plugin behavior. Add depth limit and best_so_far pruning."
    status: completed
  - id: fix2-added-paths
    content: "Fix 2: Replace escape_ref_path_segment with slugify_name in api.rs added_paths to fix ref path format mismatch."
    status: completed
  - id: fix3-dedup-refs
    content: "Fix 3: Restructure syncback ref collection to use final dedup'd filesystem names instead of tentative paths. Fix misleading comment."
    status: completed
  - id: fix4-dedup-cleanup
    content: "Fix 4: Change dedup cleanup in change_processor to match by full filesystem name (stem + extension) instead of stem only."
    status: completed
  - id: fix5-shared-trueequals
    content: "Fix 5: Extract trueEquals to a shared Lua module to eliminate duplication between matching.lua and diff.lua."
    status: completed
  - id: run-tests
    content: Write all required tests and run CI to verify everything passes.
    status: completed
isProject: false
---

# Ambiguous Path Handling Audit Fixes

> This plan was generated by `/audit` from a Plan-mode session.
> Refer to `.cursor/rules/atlas.mdc` for full project standards.

## Standards

These standards govern every fix in this plan. An implementer MUST read `.cursor/rules/atlas.mdc` before starting.

### Round-Trip Identity (from `atlas.mdc`)

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** -- same names, same classes, same properties, same hierarchy, same ref targets. 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 until proven otherwise.

### Code Quality (from `atlas.mdc`)

DRY code that is easy to reason about and hard to get wrong. The same slugify/dedup/meta-update pattern should not be copy-pasted across files. Small refactors (2-3 call sites) are included as fixes. Major rewrites (10+ call sites) are deferred.

## Context

Auditing all changes on `ambiguous-paths` branch (10 commits, 41 files, +4275/-1020 lines) against `origin/master`. The feature replaces the duplicate-skip blacklist with a matching algorithm (3 implementations), filesystem-collision-based dedup, filesystem-name-based ref paths, dedup suffix cleanup, and plugin updates.

The audit traced round-trip identity through all sync paths and found 3 critical issues, 3 correctness concerns, and 2 code quality items.

---

## Fixes

Each fix below was approved by the user during the audit quiz. Implement them in order. Each fix includes its test requirements -- a fix is not complete until its tests pass.

### Fix 1: Make Rust matching implementations recursive

- **Status:** Approved
- **Finding:** C1/CC3 -- Rust matching is flat (non-recursive) while Lua is recursive, causing pairing divergence
- **Files:**
  - [src/snapshot/matching.rs](src/snapshot/matching.rs) -- forward sync matching
  - [src/syncback/matching.rs](src/syncback/matching.rs) -- syncback matching
- **Problem:** Both Rust `count_own_diffs` functions only compare top-level properties and add +1 for children count difference. They do NOT recursively score subtrees. The `UNMATCHED_PENALTY` constant (10,000) is defined but never used. For ambiguous groups where instances have identical top-level properties but different children, Rust scores them identically (0 cost), falling back to child order. The Lua implementation correctly recurses into subtrees and applies `UNMATCHED_PENALTY` for unmatched children.
- **Solution:**
**For `src/snapshot/matching.rs` (forward sync):**
  1. Rename `count_own_diffs` to `count_own_diffs_only` (internal helper).
  2. Create new `compute_change_count(snap: &InstanceSnapshot, inst: &InstanceWithMeta, tree: &RojoTree, best_so_far: u32) -> u32`:
    - Call `count_own_diffs_only` for the flat property cost.
    - If cost >= best_so_far, early return.
    - Recursively call `match_forward_internal` on children (a new variant that returns matched pairs + unmatched counts without consuming snapshots -- use `&[InstanceSnapshot]` and `&[Ref]` refs).
    - For each matched pair, recursively call `compute_change_count`. If cost >= best_so_far, early return.
    - Add `UNMATCHED_PENALTY * unmatched_count` to cost.
  3. In the ambiguous group scoring loop, replace `count_own_diffs(snap, &inst)` with `compute_change_count(snap, &inst, tree, best_so_far)`.
  4. The `match_forward` function needs a variant that doesn't consume snapshots (borrows instead). Create `match_forward_scoring(snap_children: &[InstanceSnapshot], tree_children: &[Ref], tree: &RojoTree) -> ScoringResult` that returns `(matched_count, unmatched_snap_count, unmatched_tree_count, Vec<(usize, usize)>)` -- index pairs for recursive scoring.
  5. **Depth limit:** Add a `depth: u32` parameter to `compute_change_count`. At depth > 3 (or configurable), fall back to flat comparison to prevent O(n^k) explosion on deeply nested identical trees.
  **For `src/syncback/matching.rs` (syncback):**
  Same transformation: create `compute_change_count` that recursively matches children and scores subtrees. The hash fast-path remains: if new_hash == old_hash, return 0 immediately (skips recursion). When hashes differ, recurse into children.
  **Key constraint:** The recursive scoring functions must NOT allocate heavily in the inner loops. Use stack-based recursion with early exit on `best_so_far`. The hash fast-path in syncback makes this efficient -- most subtrees are identical and short-circuit at depth 0.
- **Risk:** Medium. Recursive matching is more expensive than flat matching. The `best_so_far` pruning and depth limit mitigate this. Hash fast-path in syncback means most comparisons never recurse. Forward sync doesn't have hashes but the (Name, ClassName) grouping limits the cross-product size.
- **Verify round-trip:** Create 10 same-named Parts with different subtrees, syncback them, rebuild. The pairings should be stable across syncback + forward sync.
- **Verify syncback parity:** N/A (matching algorithm is internal, output is the same files)
- **Tests required:**
  - Rust unit test in `src/snapshot/matching.rs`: 10 Parts named "Line" with different Transparency values, reversed order in tree. Verify each pair has matching Transparency (proves recursive scoring produces correct pairing).
  - Rust unit test in `src/syncback/matching.rs`: Same test with WeakDom instances.
  - Rust unit test: Identical instances (all properties same, same children) -- verify greedy falls back to child order (stable).
  - Rust unit test: Depth limit test -- deeply nested tree (depth > 5) completes in reasonable time.

### Fix 2: Replace `escape_ref_path_segment` with filesystem-compatible names in `added_paths`

- **Status:** Approved
- **Finding:** C2 -- `added_paths` mixes filesystem-name parent path with escape-encoded child name
- **Files:**
  - [src/web/api.rs](src/web/api.rs) lines 685-686
- **Problem:** `crate::escape_ref_path_segment(&added.name)` produces escaped instance names (e.g., `"Hey\/Bro"`) but `get_instance_by_path` splits on `/` without unescaping. An instance named `"Hey/Bro"` gets path segment `"Hey\/Bro"` which splits into `["Hey\", "Bro"]`, failing resolution. Should use the same filesystem name format as `ref_target_path_from_tree`.
- **Solution:** Replace lines 685-686 with:

```rust
  // Use slugified name (matching filesystem conventions) instead of
  // escape_ref_path_segment (which uses legacy \/ escaping).
  let fs_name = if crate::syncback::name_needs_slugify(&added.name) {
      crate::syncback::slugify_name(&added.name)
  } else {
      added.name.clone()
  };
  let path = if parent_path.is_empty() {
      fs_name
  } else {
      format!("{}/{}", parent_path, fs_name)
  };
  

```

  Note: This doesn't include the file extension (e.g., `.server.luau`) because added instances don't have middleware info at this point. The `get_instance_by_path` fallback uses case-insensitive instance name matching, which will handle the resolution. For a more complete fix, compute the tentative extension based on `added.class_name` and `added.children.is_empty()`, similar to `tentative_fs_name` in `ref_properties.rs`.

- **Risk:** Low. Only affects ref properties referencing just-added instances with `/` or `\` in their names.
- **Verify round-trip:** Add an instance named `"Hey/Bro"` via two-way sync, then verify a ref property pointing to it resolves correctly after a server restart.
- **Verify syncback parity:** N/A (two-way sync only, CLI syncback doesn't use added_paths)
- **Tests required:**
  - Rust integration test in `tests/tests/two_way_sync.rs`: Send a `/api/write` with an added instance named `"A/B"` and an update that references it via Ref. Verify the Ref resolves to the correct tree instance after patch application.

### Fix 3: Restructure syncback ref collection for dedup-aware paths

- **Status:** Approved
- **Finding:** C3 -- `tentative_fs_path` doesn't include dedup suffixes, producing ambiguous paths for duplicate-named instances
- **Files:**
  - [src/syncback/ref_properties.rs](src/syncback/ref_properties.rs) -- `tentative_fs_path`, `collect_referents`, comment
  - [src/syncback/mod.rs](src/syncback/mod.rs) -- `syncback_loop` orchestration
  - [src/syncback/file_names.rs](src/syncback/file_names.rs) -- `name_for_inst` (produces dedup'd names)
  - [src/snapshot_middleware/dir.rs](src/snapshot_middleware/dir.rs) -- `syncback_dir_no_meta` (calls `name_for_inst`)
- **Problem:** `collect_referents()` calls `tentative_fs_path()` which computes paths BEFORE dedup suffix assignment. Two instances with the same name/class get identical paths (e.g., both `"Workspace/Data"`). The `Rojo_Ref`_* attribute stores an ambiguous path that resolves to the first matching child, potentially the wrong one.
The comment at line ~147 says "ALL filesystem paths are unique" which is misleading -- tentative paths aren't unique for duplicate-named instances.
- **Solution:**
**Phase 1: Correct the comment (immediate)**
Replace the misleading comment in `collect_referents` with:

```rust
  // NOTE: tentative_fs_path() does not include dedup suffixes (~N).
  // For instances with duplicate names under the same parent, this
  // produces ambiguous paths. Resolution via get_instance_by_path()
  // returns the first matching child, which may be wrong.
  //
  // To fully fix this, collect_referents must run AFTER dedup suffix
  // assignment, with access to the final filesystem names. See Phase 2.
  

```

  **Phase 2: Pass dedup assignments to ref collection**

1. In `syncback_loop()` (`src/syncback/mod.rs`), after the recursive `syncback_dir_no_meta` / middleware dispatch completes (which assigns filenames via `name_for_inst`), build a `HashMap<Ref, String>` mapping each instance's Ref to its FINAL dedup'd filesystem path.
2. Pass this map to `collect_referents()` as a new parameter `final_paths: &HashMap<Ref, String>`. In `collect_referents`, when building the target path, look up the target Ref in `final_paths` first. If found, use that path. If not found (external ref, pruned instance), fall back to `tentative_fs_path`.
3. The `final_paths` map is built bottom-up during the syncback walk. Each middleware's `syncback()` call returns a `SyncbackReturn` which contains the `fs_snapshot` with file paths. Extract the Ref→path mapping from the `SyncbackSnapshot` path + the child assignments.
  **Implementation detail:** The simplest approach is to add a `ref_path_map: &mut HashMap<Ref, String>` parameter to `SyncbackSnapshot::with_joined_path` and record the mapping there. Each time a child gets a filesystem name assigned, record `(child_ref, parent_path + "/" + filename)` in the map.

- **Risk:** Medium. Changes the syncback flow to thread a map through the recursive walk. Must be careful not to break incremental mode. The `final_paths` map is only populated during syncback, so forward sync is unaffected.
- **Verify round-trip:** Create two Folders named "Data" under Workspace, add a Script with PrimaryPart pointing to the second "Data". Syncback, rebuild, verify PrimaryPart points to the correct "Data" (the one with dedup suffix `~1`).
- **Verify syncback parity:** The CLI syncback output must include the correct `Rojo_Ref_`* attribute with the dedup'd path (e.g., `"Workspace/Data~1"`). Two-way sync should produce the same path.
- **Tests required:**
  - Rust integration test in `tests/tests/syncback_roundtrip.rs`: Two same-named Folders with a ref property pointing to the second one. Verify the `Rojo_Ref_`* attribute contains the dedup'd path (e.g., `"Workspace/Data~1"`).
  - Rust unit test: `collect_referents` with `final_paths` containing dedup'd paths. Verify the output uses dedup'd paths instead of tentative paths.

### Fix 4: Dedup cleanup should match by full filesystem name, not stem alone

- **Status:** Approved
- **Finding:** CC1 -- Change processor groups dedup siblings by stem only, ignoring extension
- **Files:**
  - [src/change_processor.rs](src/change_processor.rs) lines 1098-1125
- **Problem:** The dedup cleanup code extracts the stem by splitting on `'.'` and groups siblings by base stem. `Foo.server.luau` (stem `Foo`) and `Foo.luau` (stem `Foo`) are grouped together even though they have different dedup keys (`foo.server.luau` vs `foo.luau`) and are NOT in the same dedup group. This inflates `remaining_stems.len()` and can change the cleanup action.
- **Solution:** When finding siblings in the dedup group, also compare extensions. Extract the extension from the removed instance's filesystem name, and only include siblings whose extension matches. Specifically:

```rust
  // Extract the compound extension (everything after the stem)
  let removed_extension = removed_fs_name
      .find('.')
      .map(|i| &removed_fs_name[i + 1..]);

  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_ext = sibling_fs.find('.').map(|i| &sibling_fs[i + 1..]);

      // Only group siblings with the same extension
      if sibling_ext != removed_extension { continue; }

      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());
      }
  }
  

```

- **Risk:** Very low. Makes grouping more precise, no behavior change for the common case (all siblings in a dedup group have the same extension by definition).
- **Verify round-trip:** Create siblings `Foo.server.luau` (Script) and `Foo.luau` (ModuleScript) and `Foo~1.server.luau` (Script). Delete `Foo.server.luau` via two-way sync. Verify `Foo~1.server.luau` is renamed to `Foo.server.luau` and `Foo.luau` is untouched.
- **Verify syncback parity:** N/A (dedup cleanup is a two-way sync operation)
- **Tests required:**
  - Rust unit test in `src/syncback/dedup_suffix.rs`: Verify `compute_cleanup_action` with mixed-extension input (though this tests the helper, the real fix is in change_processor).
  - Rust integration test or manual verification: Mixed-extension siblings deletion scenario as described above.

### Fix 5: Extract `trueEquals` to a shared module

- **Status:** Approved
- **Finding:** CC2 -- `trueEquals` (and `fuzzyEq`) duplicated between `matching.lua` and `diff.lua`
- **Files:**
  - New: [plugin/src/Reconciler/trueEquals.lua](plugin/src/Reconciler/trueEquals.lua)
  - [plugin/src/Reconciler/matching.lua](plugin/src/Reconciler/matching.lua) -- remove local copy, require shared
  - [plugin/src/Reconciler/diff.lua](plugin/src/Reconciler/diff.lua) -- remove local copy, require shared
- **Problem:** `trueEquals` is copy-pasted between two files. If either is updated without the other, matching and diff will disagree on value equality, causing phantom changes (matching says "equal" but diff says "different").
- **Solution:**
  1. Create `plugin/src/Reconciler/trueEquals.lua` containing `fuzzyEq` and `trueEquals`. Export `trueEquals` as the module return value.
  2. In `matching.lua`, remove the local `fuzzyEq` and `trueEquals` definitions (lines 27-111). Add `local trueEquals = require(script.Parent.trueEquals)`.
  3. In `diff.lua`, remove the local `fuzzyEq` and `trueEquals` definitions (lines 19-115). Add `local trueEquals = require(script.Parent.trueEquals)`.
- **Risk:** Very low. Pure refactor, no behavior change.
- **Verify round-trip:** Existing tests cover both callers.
- **Verify syncback parity:** N/A (plugin-only change)
- **Tests required:**
  - Existing diff.spec.lua and diff.stress.spec.lua tests verify equality behavior. No new tests needed.

---

## Skipped Fixes

### Skipped: Remove unused `UNMATCHED_PENALTY` from Rust matching files

- **Finding:** Q1 -- dead constant in both Rust matching files
- **Problem:** `UNMATCHED_PENALTY` defined but never used
- **Reason skipped:** Fix 1 (recursive matching) will start using this constant, making removal unnecessary.

---

## Accepted Limitations

None -- all limitations flagged were approved for fixing.

---

## Deferred Refactors

- **Matching cache (Phase 2e from original plan):** Add optional cache mapping `parent_ref -> (children_list_hash, match_assignments)` to avoid re-running the full algorithm when children haven't changed. Not needed for correctness, optimization only.
- **Duplicated name-to-index grouping pattern:** All three matching implementations repeat the same "build HashMap of name to indices" pattern. Could be factored into a shared helper, but the three implementations operate on different types (InstanceSnapshot, WeakDom, Lua tables) making a shared abstraction non-trivial. Flagged for consideration after the matching implementations stabilize.

---

## Test Plan

Tests are not optional. Every approved fix must have test coverage. A fix is not complete until its tests pass (or intentionally fail, for known limitations).

### Rust Unit Tests (`#[cfg(test)]` blocks)

- **10 same-named Parts with different properties, reversed order:** Verify recursive matching pairs correctly by property content, not position. (Fix 1)
- **Identical instances (all same):** Verify stable child-order tiebreaker. (Fix 1)
- **Depth limit test:** Deeply nested tree (depth > 5) completes without hanging. (Fix 1)
- `**collect_referents` with `final_paths`:** Verify dedup'd paths used instead of tentative. (Fix 3)

### Rust Integration Tests (`tests/tests/`)

- **Two-way sync with added instance named `"A/B"`:** Verify ref resolution works with slugified names. (Fix 2)
- **Syncback roundtrip with duplicate-named Folders + ref:** Verify `Rojo_Ref`_* contains dedup'd path. (Fix 3)

### Lua Spec Tests (`.spec.lua` files)

- Existing diff.spec.lua and diff.stress.spec.lua cover `trueEquals` behavior. (Fix 5)

### Snapshot Tests (`insta` crate)

- Existing snapshot tests should be re-verified after recursive matching change. Run `cargo insta review` after Fix 1. (Fix 1)

---

## Test Rules

The implementer MUST follow these rules when writing tests for this plan:

### For bugs found and fixed

Every bug fix MUST have a test that:

1. Would have **FAILED** before the fix
2. **PASSES** after the fix
3. Prevents the bug from regressing

### For missing coverage identified

For each "missing test coverage" gap:

1. Write the test
2. Verify it passes against the current implementation
3. If it fails, investigate -- it may have found another bug

### Scope: test every affected layer

For each finding, determine which test layers are affected and write tests in ALL of them:

- Does the bug manifest at the plugin level? -> Lua spec test
- Does it manifest at the server level? -> Rust unit test or integration test
- Does it affect the round-trip? -> Integration test with filesystem verification
- Could it regress from a different code path? -> Test that code path too

## Final Step: Run CI

After ALL fixes and tests are implemented, run the `/ci` command (see `.cursor/commands/ci.md`) to execute the full CI pipeline. Every fix must pass CI before the plan is considered complete. Do not skip this step.