diff --git a/.cursor/plans/ambiguous_paths_audit_fixes_9d3ed6da.plan.md b/.cursor/plans/ambiguous_paths_audit_fixes_9d3ed6da.plan.md new file mode 100644 index 00000000..a26e32c7 --- /dev/null +++ b/.cursor/plans/ambiguous_paths_audit_fixes_9d3ed6da.plan.md @@ -0,0 +1,310 @@ +--- +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` 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`. 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` 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. \ No newline at end of file diff --git a/.cursor/plans/audit_ambiguous_paths_followup_00831072.plan.md b/.cursor/plans/audit_ambiguous_paths_followup_00831072.plan.md new file mode 100644 index 00000000..cbe009ee --- /dev/null +++ b/.cursor/plans/audit_ambiguous_paths_followup_00831072.plan.md @@ -0,0 +1,181 @@ +--- +name: audit ambiguous paths followup +overview: 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. +todos: + - id: fix-metadata-drift + content: Update dedup cleanup rename flow to keep tree metadata/path indexes in sync + status: pending +isProject: 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 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: Build correct same-batch added Ref paths + +- **Status:** Approved +- **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; it 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:** Derive same-batch added target path using the same filesystem naming pipeline as syncback write (class/middleware extension + dedup suffix + directory-vs-file representation). Reuse/shared helper preferred to avoid divergence. +- **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 3: Replace global ref-path text rewriting with attribute-scoped final paths + +- **Status:** Approved +- **Finding:** Step 2a / Step 9 / Step 11h (round-trip identity, parity, determinism) +- **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)`, `[src/syncback/snapshot.rs](d:/UserGenerated/rojo/src/syncback/snapshot.rs)` +- **Problem:** Syncback currently calls `collect_referents(..., None)` (`~177`) then performs broad post-hoc string replacement (`~614-637`) via `fix_ref_paths` (`~456-492`) across entire meta/model file text. This is not attribute-scoped and can alter unrelated strings. +- **Solution:** Feed definitive final paths into ref linking directly (e.g., pass populated `final_paths` to `collect_referents`) so `Rojo_Ref_`* attributes are written correctly first time. Remove or strictly confine generic text replacement path. +- **Risk:** Medium-high; touches syncback ref pipeline and ordering. +- **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 4: 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 three approved bug classes. +- **Solution:** Add targeted regression tests tied to Fixes 1-3 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-3. + - 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. +- Revisit optimal assignment algorithm for large ambiguous groups (major rewrite; out of current scope). + +## 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 2). + +### 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). +- Same `/api/write` request: add target + set Ref to target, verify persisted path includes expected extension/suffix (Fix 2). +- Ref rewrite safety: non-ref string fields are unchanged while `Rojo_Ref_`* gets final dedup path (Fix 3). + +### 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 3). + +### 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. \ No newline at end of file