rojo/.cursor/plans/audit_ambiguous_paths_followup_00831072.plan.md

name	overview	todos	isProject
audit ambiguous paths followup	Production audit of the ambiguous-paths branch against origin/master. Documents critical round-trip issues, approved fixes, accepted limitations, and required regression tests for implementation in a follow-up Agent-mode session.	id content status fix-metadata-drift Fix 1: Update dedup cleanup rename flow to keep tree metadata/path indexes in sync in_progress id content status fix-batch-removal-dedup Fix 2: Exclude co-removed siblings from dedup cleanup sibling enumeration pending id content status fix-added-paths Fix 3: Extend added_paths in api.rs to include middleware extension and dedup suffix pending id content status fix-ref-path-final Fix 4: Complete Phase 2 of previous audit Fix 3 -- pass final_paths to collect_referents instead of global text rewrite pending id content status fix-regression-tests Fix 5: Add regression tests for Fixes 1-4 pending	false

Ambiguous Paths Audit Fix Plan

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 §Quality Standard)

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 §Two-Way Sync Strategy)

Plugin-based sync must produce exactly the same filesystem output that atlas syncback would give for the same input. Byte-for-byte identical files, identical directory structures, identical naming. Any divergence is a bug until proven otherwise.

Code Quality (from atlas.mdc §Code Quality Standard)

DRY code that is easy to reason about and hard to get wrong. Small helper refactors are in scope; major pipeline redesign is deferred.

Context

Audited branch ambiguous-paths against origin/master using commit range origin/master..HEAD.

Primary findings:

• Critical path drift in dedup cleanup rename flow ([src/change_processor.rs](d:/UserGenerated/rojo/src/change_processor.rs)).
• Critical batch-removal dedup cleanup counts co-removed siblings as "remaining", defeating group-to-1 cleanup ([src/change_processor.rs](d:/UserGenerated/rojo/src/change_processor.rs)).
• Critical same-batch Ref target path synthesis mismatch for added instances ([src/web/api.rs](d:/UserGenerated/rojo/src/web/api.rs)).
• Critical ref-path post-processing strategy relies on global text replacement ([src/syncback/mod.rs](d:/UserGenerated/rojo/src/syncback/mod.rs), [src/syncback/fs_snapshot.rs](d:/UserGenerated/rojo/src/syncback/fs_snapshot.rs)).
• Known limitation accepted for this release: duplicate-named children under ProjectNode currently skipped in [src/snapshot_middleware/project.rs](d:/UserGenerated/rojo/src/snapshot_middleware/project.rs).
• Known limitation accepted for this release: greedy matcher is not globally optimal ([src/snapshot/matching.rs](d:/UserGenerated/rojo/src/snapshot/matching.rs), [src/syncback/matching.rs](d:/UserGenerated/rojo/src/syncback/matching.rs), [plugin/src/Reconciler/matching.lua](d:/UserGenerated/rojo/plugin/src/Reconciler/matching.lua)).

Fixes

Each fix below was approved by the user during the audit quiz. Implement in order.

Fix 1: Keep tree metadata consistent after dedup cleanup rename

• Status: Approved
• Finding: Step 2d / Step 8 (rename/delete interactions + VFS suppression)
• Files: [src/change_processor.rs](d:/UserGenerated/rojo/src/change_processor.rs)
• Problem: In dedup cleanup (~1073-1203), the code renames files (fs::rename) and suppresses both old/new paths, but does not update the renamed sibling's InstigatingSource::Path or metadata path indexes. Subsequent writes can target stale paths.
• Solution: After successful DedupCleanupAction::RemoveSuffix / PromoteLowest, locate the sibling instance being renamed, update its metadata source path and relevant path indexes in-memory, and keep suppression counters consistent with the updated path.
• Risk: Medium; touches tree mutation bookkeeping and rename lifecycle.
• Verify round-trip: Create duplicate siblings (Foo, Foo~1), delete Foo, ensure Foo~1 -> Foo rename occurs, then issue a property update to the survivor in same session and verify disk write targets new path and survives rebuild.
• Verify syncback parity: Compare output of two-way deletion flow to CLI syncback on equivalent tree state; both must produce the same remaining filename/meta layout.
• Tests required:
  • Rust integration test in [tests/tests/two_way_sync.rs](d:/UserGenerated/rojo/tests/tests/two_way_sync.rs) covering delete-triggered dedup cleanup followed by update.
  • Optional focused unit test around cleanup metadata path update helper in [src/change_processor.rs](d:/UserGenerated/rojo/src/change_processor.rs).

Fix 2: Exclude co-removed siblings from dedup cleanup sibling enumeration

• Status: Approved
• Files: [src/change_processor.rs](d:/UserGenerated/rojo/src/change_processor.rs)
• Problem: In the dedup cleanup loop (lines 1077-1203), the sibling enumeration (line 1120) only skips sibling_ref == removed_id -- the single instance being processed in the current iteration. It does NOT skip other instances that are also in patch_set.removed_instances. When multiple siblings from the same dedup group are removed in a single patch, the co-removed instances are incorrectly counted as "remaining", inflating the sibling count and defeating cleanup rules.
• Failure path: Remove Foo (A) and Foo~1 (B) from a group of 3, leaving Foo~2 (C) as the sole survivor. api.rs deletes all files first. In the dedup cleanup loop, iteration for A finds B and C as remaining (count=2), so group-to-1 does not fire; instead PromoteLowest tries to rename the already-deleted Foo~1 → Foo (fails). Iteration for B finds A and C as remaining (count=2), group-to-1 again does not fire. Result: C keeps its ~2 suffix permanently when it should have been renamed to Foo.
• Solution: Before the dedup cleanup loop, build a HashSet<Ref> from patch_set.removed_instances. In the sibling enumeration (line 1121), change the skip condition from sibling_ref == removed_id to removed_set.contains(&sibling_ref):

let removed_set: HashSet<Ref> = patch_set.removed_instances.iter().copied().collect();

// ... inside sibling loop:
for &sibling_ref in parent_inst.children() {
    if removed_set.contains(&sibling_ref) {
        continue;
    }
    // ... rest of sibling processing
}

• Risk: Low; narrow, mechanical change. The HashSet construction is O(n) and the lookup is O(1).
• Verify round-trip: Remove 2 of 3 duplicate siblings via two-way sync. Verify the sole survivor has its dedup suffix removed and survives rebuild with the clean name.
• Verify syncback parity: N/A (dedup cleanup is a two-way sync operation only).
• Tests required:
  • Rust integration test in [tests/tests/two_way_sync.rs](d:/UserGenerated/rojo/tests/tests/two_way_sync.rs): batch-remove 2 of 3 same-named siblings, verify survivor is renamed from Foo~2 to Foo.
  • Edge test: batch-remove ALL members of a dedup group (no survivor); verify no spurious rename attempts.

Fix 3: Build correct same-batch added Ref paths

• Status: Approved
• Lineage: Extends ambiguous_paths_audit_fixes_9d3ed6da Fix 2 (completed), which replaced escape_ref_path_segment with slugify_name in added_paths but explicitly acknowledged the missing extension as a limitation. The current code at lines 637-641 of api.rs uses slugify_name; this fix adds the remaining segments.
• Finding: Step 2a / Step 10 (plugin->server wire format and write processing)
• Files: [src/web/api.rs](d:/UserGenerated/rojo/src/web/api.rs)
• Problem: added_paths map (~622-649) uses only slugified instance name (from the previous fix); it still omits middleware extension and dedup suffix. syncback_updated_properties then writes incorrect Rojo_Ref_* when target is newly added in same request (~2694-2706).
• Solution: Extend the existing added_paths builder (lines 637-649) to derive the full filesystem name: slugified name + middleware extension based on added.class_name and added.children.is_empty() + dedup suffix if colliding with siblings. Reuse tentative_fs_name logic from ref_properties.rs or the shared helper from the Deferred Refactors section.
• Risk: Medium; affects same-request Ref persistence edge path.
• Verify round-trip: Send /api/write with add(target script/model) + update(ref property pointing to target) in one batch; verify persisted path includes correct segment (.server.luau, .model.json5, or dedup suffix) and resolves after rebuild.
• Verify syncback parity: Run equivalent scenario via CLI syncback fixture and compare written Rojo_Ref_* attribute string.
• Tests required:
  • Rust integration test in [tests/tests/two_way_sync.rs](d:/UserGenerated/rojo/tests/tests/two_way_sync.rs) for same-batch add+ref on file-backed and directory-backed targets.
  • Edge test for dedup suffix target (e.g., Foo~1).

Fix 4: Complete attribute-scoped final paths (previously incomplete)

• Status: Approved
• Finding: Step 2a / Step 9 / Step 11h (round-trip identity, parity, determinism)
• Lineage: ambiguous_paths_audit_fixes_9d3ed6da Fix 3 defined a two-phase fix. Phase 1 (correct misleading comment) was completed -- visible at lines 194-199 of ref_properties.rs. Phase 2 (pass final_paths to collect_referents) was NOT completed despite the todo being marked "completed". The API was prepared (final_paths: Option<&HashMap<Ref, String>> parameter exists at line 188 of ref_properties.rs) but the call site at line 177 of mod.rs still passes None. The ref_path_map infrastructure was built by ref_path_and_dedup_integration_81a47dfe and is populated during the syncback walk (used at lines 614-637 of mod.rs for the text replacement). This fix completes that unfinished Phase 2.
• Files: [src/syncback/mod.rs](d:/UserGenerated/rojo/src/syncback/mod.rs), [src/syncback/ref_properties.rs](d:/UserGenerated/rojo/src/syncback/ref_properties.rs), [src/syncback/fs_snapshot.rs](d:/UserGenerated/rojo/src/syncback/fs_snapshot.rs)
• Problem: collect_referents(&new_tree, &pre_prune_paths, None) is called at line 177 of mod.rs BEFORE the syncback walk that populates ref_path_map. The None means tentative paths (without dedup suffixes) are written into Rojo_Ref_* attributes. Then the post-hoc fix_ref_paths (lines 614-637) performs global text replacement across entire meta/model file contents to substitute tentative paths with final dedup'd paths. This is not attribute-scoped and can alter unrelated strings that happen to contain the same substring as a tentative path.
• Existing infrastructure to build on:
  1. collect_referents already accepts final_paths: Option<&HashMap<Ref, String>> (line 188 of ref_properties.rs).
  2. ref_path_map is already populated during the syncback walk with (Ref, final_path) entries.
  3. tentative_fs_path_public is already used at line 623 of mod.rs to compute tentative paths for the substitution diff.
• Solution: Move the collect_referents call from line 177 (before the syncback walk) to after the walk completes (after line 612). Pass Some(&ref_path_map.borrow()) instead of None. Then remove or guard the fix_ref_paths block (lines 614-637) -- it should no longer be needed for any path that has a final_paths entry.
• Ordering constraint: collect_referents is currently called before property filtering (lines 179-198). Moving it after the syncback walk means it runs after filtering. Verify that collect_referents does not depend on properties that are removed by filtering (it should not -- it only reads Ref-type properties, which are not in the filtered set unless ignore_referents is true, in which case refs are skipped entirely).
• Risk: Medium; changes the ordering of the syncback pipeline. The infrastructure is already in place, reducing implementation risk.
• Verify round-trip: Scenario with duplicate-named ref target and non-ref string property containing same substring as tentative path; ensure only Rojo_Ref_* changes and non-ref string is untouched.
• Verify syncback parity: Compare CLI syncback and two-way outputs for refs targeting dedup-suffixed instances; paths must be byte-identical.
• Tests required:
  • Rust integration test in [tests/tests/syncback_roundtrip.rs](d:/UserGenerated/rojo/tests/tests/syncback_roundtrip.rs) validating non-ref string preservation while refs are rewritten correctly.
  • Rust integration test in [tests/tests/two_way_sync.rs](d:/UserGenerated/rojo/tests/tests/two_way_sync.rs) for dedup-aware ref persistence without text rewrite side effects.

Fix 5: Add regression coverage for approved fixes

• Status: Approved
• Finding: Step 13 (missing coverage)
• Files: [tests/tests/two_way_sync.rs](d:/UserGenerated/rojo/tests/tests/two_way_sync.rs), [tests/tests/syncback_roundtrip.rs](d:/UserGenerated/rojo/tests/tests/syncback_roundtrip.rs), optionally helper fixtures under [rojo-test/syncback-tests](d:/UserGenerated/rojo/rojo-test/syncback-tests).
• Problem: Current tests do not fully guard the four approved bug classes.
• Solution: Add targeted regression tests tied to Fixes 1-4 and ensure they fail pre-fix and pass post-fix.
• Risk: Low.
• Verify round-trip: Covered by added integration scenarios.
• Verify syncback parity: Add assertions comparing resulting file paths/attribute values against expected CLI-format output.
• Tests required:
  • All tests listed under Fixes 1-4.
  • One deterministic re-run assertion (second run no changes) for affected scenarios.

Skipped Fixes

Skipped: ProjectNode duplicate-child handling rewrite

• Finding: Step 2d / Step 11k
• Problem: Duplicate-named children under ProjectNode are currently skipped with warnings ([src/snapshot_middleware/project.rs](d:/UserGenerated/rojo/src/snapshot_middleware/project.rs) around ~587-683).
• Reason skipped: User accepted as release limitation for now.

Skipped: Globally optimal matcher replacement

• Finding: Step 11h (determinism/assignment quality)
• Problem: Greedy assignment may produce suboptimal pairing in rare ambiguous groups.
• Reason skipped: User accepted greedy behavior for this release.

Accepted Limitations

• Limitation: ProjectNode duplicate-named children are skipped during syncback.
• Scenario: Under project-backed nodes, duplicate names are detected and omitted from child maps; warning logged.
• User decision: Accepted for this release.
• Mitigation: Keep explicit warning logs; add failing acceptance test (below) to track until fixed.
• Limitation: Greedy matching is not globally optimal.
• Scenario: Ambiguous duplicate groups can choose a local minimum causing rename churn.
• User decision: Accepted for this release.
• Mitigation: Add stress/failing test for known counterexample and keep deterministic tie behavior.

Deferred Refactors

• Consolidate filesystem-segment derivation into one shared helper used by [src/rojo_ref.rs](d:/UserGenerated/rojo/src/rojo_ref.rs), [src/snapshot/tree.rs](d:/UserGenerated/rojo/src/snapshot/tree.rs), and [src/web/api.rs](d:/UserGenerated/rojo/src/web/api.rs) to eliminate duplicate naming logic. (Continuing from ambiguous_paths_audit_fixes_9d3ed6da Deferred Refactors -- still open.)
• Revisit optimal assignment algorithm for large ambiguous groups (major rewrite; out of current scope). (Continuing from ambiguous_paths_audit_fixes_9d3ed6da Deferred Refactors -- still open.)
• Phase 2e matching cache from the original plan (ambiguous_path_handling_f2eaef11): add optional cache mapping parent_ref -> (children_list_hash, match_assignments) to avoid re-running the full algorithm when children haven't changed. Optimization only, not needed for correctness. (Never completed or cancelled across any subsequent plan -- explicitly deferred here.)

Test Plan

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

• Dedup cleanup metadata update helper behavior after rename (Fix 1).
• Added-path segment generation helper returns correct extension/suffix across middleware types (Fix 3).

Rust Integration Tests (tests/tests/)

• Delete base dedup name then update promoted sibling in same serve session; verify writes hit new path and persist after rebuild (Fix 1).
• Batch-remove 2 of 3 same-named siblings; verify sole survivor is renamed from Foo~2 to Foo (Fix 2).
• Batch-remove all members of a dedup group; verify no spurious rename attempts (Fix 2).
• Same /api/write request: add target + set Ref to target, verify persisted path includes expected extension/suffix (Fix 3).
• Ref rewrite safety: non-ref string fields are unchanged while Rojo_Ref_* gets final dedup path (Fix 4).

Lua Spec Tests (.spec.lua)

• No mandatory Lua-side changes for approved fixes; optional guard test for same-batch add+ref behavior if plugin-side helper touched.

Snapshot Tests (insta)

• Add or update syncback snapshot fixture showing final dedup-aware Rojo_Ref_* output without incidental string changes (Fix 4).

Failing Tests for Known Limitations

• ProjectNode duplicate-named child round-trip identity test asserting correct behavior (expected to fail until limitation removed).
• Ambiguous-group matching counterexample test asserting globally optimal pairing (expected to fail with greedy algorithm).

Test Rules

For bugs found and fixed

• Each approved bug fix must have a regression test that fails pre-fix and passes post-fix.

For missing coverage identified

• Add coverage in affected layer(s); investigate any unexpected failure as a potential additional bug.

For known limitations (Accepted Limitations section)

• Write correctness-asserting tests even if they fail now.
• Leave them failing (do not ignore), to serve as future acceptance criteria.

Scope

• Cover both persistence correctness (filesystem outputs) and in-memory tree consistency where applicable.

Final Step: Run CI

After implementing all approved fixes and tests, run /ci and require a clean pass before considering this plan complete.