rojo/.cursor/plans/ambiguous_paths_audit_fixes_9d3ed6da.plan.md

name	overview	todos	isProject
Ambiguous paths audit fixes	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.	id content status fix1-recursive-matching 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. completed id content status fix2-added-paths Fix 2: Replace escape_ref_path_segment with slugify_name in api.rs added_paths to fix ref path format mismatch. completed id content status fix3-dedup-refs Fix 3: Restructure syncback ref collection to use final dedup'd filesystem names instead of tentative paths. Fix misleading comment. completed id content status fix4-dedup-cleanup Fix 4: Change dedup cleanup in change_processor to match by full filesystem name (stem + extension) instead of stem only. completed id content status fix5-shared-trueequals Fix 5: Extract trueEquals to a shared Lua module to eliminate duplication between matching.lua and diff.lua. completed id content status run-tests Write all required tests and run CI to verify everything passes. completed	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 -- forward sync matching
  • 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 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:

  // 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 -- tentative_fs_path, collect_referents, comment
  • src/syncback/mod.rs -- syncback_loop orchestration
  • src/syncback/file_names.rs -- name_for_inst (produces dedup'd names)
  • 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:

  // 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 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:

  // 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/matching.lua -- remove local copy, require shared
  • 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.