From 012c20b1b940641ffa9dbb617cd8f4a863a84a21 Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 14 Feb 2026 15:56:29 -0600 Subject: [PATCH] Add audit plan for Fix Ref Audit Round 3 addressing correctness and code quality issues - Introduced a new audit plan detailing six fixes identified in the third audit of two-way sync Ref properties, including populating the RefPathIndex on startup, refining the removal process for attributes, and updating stale comments. - Enhanced documentation to clarify the context and standards for the fixes, ensuring adherence to project guidelines. - Completed integration and unit tests for all identified fixes to verify functionality and maintain robustness. These changes improve the reliability and clarity of the synchronization system's handling of Ref properties. --- .../fix_ref_audit_round_3_f8ae0e39.plan.md | 335 ++++++++++++++++++ 1 file changed, 335 insertions(+) create mode 100644 .cursor/plans/fix_ref_audit_round_3_f8ae0e39.plan.md diff --git a/.cursor/plans/fix_ref_audit_round_3_f8ae0e39.plan.md b/.cursor/plans/fix_ref_audit_round_3_f8ae0e39.plan.md new file mode 100644 index 00000000..004e8677 --- /dev/null +++ b/.cursor/plans/fix_ref_audit_round_3_f8ae0e39.plan.md @@ -0,0 +1,335 @@ +--- +name: Fix Ref Audit Round 3 +overview: "Fix 3 correctness bugs and 3 code quality issues from the third audit of two-way sync Ref properties: empty RefPathIndex on startup, overbroad remove_file in index, wrong filesystem name in path remapping, stale test comments, dead parameter, misleading variable name." +todos: + - id: fix-startup-index + content: "Fix 1: Populate RefPathIndex on server startup by scanning meta/model files for Rojo_Ref_* attributes" + status: completed + - id: fix-precise-removal + content: "Fix 2: Replace overbroad remove_file with re-index-after-write approach; add remove_all_for_file method" + status: completed + - id: fix-slugified-remap + content: "Fix 3: Use slugified_new() instead of new_segment in primary match case of path remapping" + status: completed + - id: fix-stale-comments + content: "Fix 4: Update EXPECTED: FAIL comments on ref_stale_path_after_target_rename and ref_stale_path_after_parent_rename" + status: completed + - id: fix-dead-param + content: "Fix 5: Remove unused _tree parameter from update_ref_paths_after_rename" + status: completed + - id: fix-variable-name + content: "Fix 6: Remove redundant removed_ref_values variable (subsumed by Fix 2)" + status: completed + - id: tests-all + content: Write all required integration and unit tests for Fixes 1-3 + status: completed + - id: run-ci + content: Run /ci to verify all tests pass + status: completed +isProject: false +--- + +# Fix Ref Two-Way Sync Audit Round 3 + +> 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 refactors (2-3 call sites) are included as fixes. Major rewrites (10+ call sites) are deferred. + +## Context + +Third audit of the two-way sync Ref property feature (branch `refs` vs `origin/master`). Previous audits found and fixed: ServeSession missing instanceMap, stale paths after rename, slash escaping, priority conflict, encodeInstance leak, filter_properties_for_meta, slugified name fallback, immediate ID assignment, and RefPathIndex implementation. This audit found 3 correctness bugs and 3 code quality issues in the current implementation. + +## Fixes + +### Fix 1: Populate RefPathIndex on Server Startup + +- **Status:** Approved +- **Finding:** Step 2d: Rename/Move Interactions -- RefPathIndex starts empty, pre-existing Rojo_Ref_* attributes are never indexed. +- **Files:** `src/change_processor.rs` +- **Problem:** `RefPathIndex` is initialized empty (`RefPathIndex::new()`) in `src/serve_session.rs:160`. It's only populated when `syncback_updated_properties` in `api.rs` writes new Rojo_Ref_* attributes. Pre-existing attributes in meta/model files (from CLI syncback, manual edits, or previous sessions) are never indexed. If a rename happens before any two-way sync writes, the index finds nothing, and stale ref paths in existing meta files are not updated. +- **Solution:** After the initial tree is built and the ChangeProcessor starts, scan all tree instances that have `InstigatingSource::Path` pointing to a `.meta.json5` or `.model.json5` file. For each such file, read it and look for `Rojo_Ref_*` attributes. Add entries to the RefPathIndex. +Implementation: Add a method `populate_initial_index` to `JobThreadContext` (or a standalone function). Call it at the beginning of `main_task()` in the ChangeProcessor, before the main loop. The function should: + 1. Lock the tree + 2. Iterate all instances via `tree.descendants(root_id)` + 3. For each instance with `InstigatingSource::Path(path)`, check if the path ends in `.meta.json5`, `.model.json5`, `.meta.json`, or `.model.json` (or if it's a directory, check for `init.meta.json5` inside it) + 4. Read the file, parse JSON5, look for `attributes` object entries starting with `Rojo_Ref_` + 5. For each matching attribute, call `index.add(path_value, meta_file_path)` + Alternatively (simpler and more robust): Walk the `InstanceContext` or the VFS to find all `*.meta.json5` and `*.model.json5` files under the project root. For each, parse and scan for `Rojo_Ref_*` attributes. This avoids depending on tree metadata. + The simpler approach (file-system scan) is recommended because: + - It catches ALL meta files, not just those with InstigatingSource + - It handles nested projects correctly + - It's a one-time cost at startup + ```rust + fn populate_initial_ref_path_index(&self) { + let project_root = &self.project_root; + // Walk all .meta.json5 and .model.json5 files under project root + for entry in walkdir::WalkDir::new(project_root) + .into_iter() + .filter_map(|e| e.ok()) + .filter(|e| { + let name = e.file_name().to_string_lossy(); + name.ends_with(".meta.json5") || name.ends_with(".model.json5") + || name.ends_with(".meta.json") || name.ends_with(".model.json") + }) + { + let path = entry.path(); + if let Ok(bytes) = std::fs::read(path) { + if let Ok(val) = crate::json::from_slice::(&bytes) { + if let Some(attrs) = val.get("attributes").and_then(|a| a.as_object()) { + let mut index = self.ref_path_index.lock().unwrap(); + for (key, value) in attrs { + if key.starts_with(crate::REF_PATH_ATTRIBUTE_PREFIX) { + if let Some(path_str) = value.as_str() { + index.add(path_str, path); + } + } + } + } + } + } + } + } + ``` + **Dependency check:** `walkdir` is already a dependency of the project (used by memofs). If not directly available, use `std::fs::read_dir` recursively or `glob`. +- **Risk:** Low. Additive change that runs once at startup. Does not modify existing behavior. May add a few ms to startup time for large projects. +- **Verify round-trip:** Start server with a pre-existing meta file containing `Rojo_Ref_PrimaryPart = "Workspace/Model/Part1"`. Rename `Part1` to `NewPart` via Studio. Verify meta file updates to `"Workspace/Model/NewPart"`. +- **Verify syncback parity:** N/A -- this is index population only. +- **Tests required:** + - Integration test: Create a fixture with pre-existing Rojo_Ref_* in a meta file. Start server. Rename target. Verify meta file path updated. + +--- + +### Fix 2: Precise RefPathIndex Removal for Partial Attribute Deletion + +- **Status:** Approved +- **Finding:** Step 7: Audit Meta File Lifecycle -- `remove_file()` deletes all index entries for a file instead of just the removed attribute. +- **Files:** `src/web/api.rs` (lines 2887-2903) +- **Problem:** When removing one Rojo_Ref_* attribute from a file that has multiple, `index.remove_file(&written_meta_path, attr_name)` removes the meta file from ALL index entries. Remaining Rojo_Ref_* attributes lose their index entries, so future renames won't find them. +- **Solution:** Replace the current removal-then-add approach with a complete re-index of the written file. After `merge_or_build_meta` writes the file, re-read it and re-index ALL remaining Rojo_Ref_* attributes. This is simpler and more correct than trying to track individual removals. +Replace the current index maintenance block (lines 2887-2904 in `api.rs`) with: + ```rust + // Re-index the written meta file: remove all old entries for this file, + // then add entries for all remaining Rojo_Ref_* attributes. + { + let mut index = self.ref_path_index.lock().unwrap(); + + // Remove ALL old entries for this file (clean slate) + index.remove_all_for_file(&written_meta_path); + + // Re-scan the file for remaining Rojo_Ref_* attributes + if let Ok(bytes) = std::fs::read(&written_meta_path) { + if let Ok(val) = crate::json::from_slice::(&bytes) { + if let Some(attrs) = val.get("attributes").and_then(|a| a.as_object()) { + for (key, value) in attrs { + if key.starts_with(crate::REF_PATH_ATTRIBUTE_PREFIX) { + if let Some(path_str) = value.as_str() { + index.add(path_str, &written_meta_path); + } + } + } + } + } + } + } + ``` + Add a `remove_all_for_file` method to `RefPathIndex` that removes a file from all entries (rename from `remove_file` to be explicit): + ```rust + pub fn remove_all_for_file(&mut self, meta_file: &Path) { + let mut empty_keys = Vec::new(); + for (path, files) in &mut self.paths_to_files { + files.remove(meta_file); + if files.is_empty() { + empty_keys.push(path.clone()); + } + } + for key in empty_keys { + self.paths_to_files.remove(&key); + } + } + ``` + Remove the old `remove_file` method (or rename it to `remove_all_for_file`) and remove the `removed_ref_values` and `ref_path_values` intermediate variables since they're no longer needed. +- **Risk:** Low. The re-read of the just-written file adds negligible I/O (file is in OS cache). The approach is simpler and more correct than precise tracking. +- **Verify round-trip:** Set two Rojo_Ref_* attributes on the same instance. Remove one (set to nil). Rename the target of the remaining one. Verify the remaining ref path updates. +- **Verify syncback parity:** N/A +- **Tests required:** + - Integration test: Set PrimaryPart and CustomRef on same Model. Set PrimaryPart to nil. Rename CustomRef's target. Verify CustomRef's path updates in meta file. + +--- + +### Fix 3: Use Slugified Filesystem Name in Path Remapping + +- **Status:** Approved +- **Finding:** Step 11b: Compound File Extensions / naming -- filesystem path remapping uses tree name instead of slugified filesystem name. +- **Files:** `src/change_processor.rs` (line 461) +- **Problem:** In `update_ref_paths_after_rename`, the primary match case (line 460-463) pushes `new_segment` (the tree name) as the replacement directory component. When the new name has forbidden filesystem characters, the ChangeProcessor renames the directory to the slugified version, but the remapping produces a path using the unslugified tree name. The file at the wrong path is not found, and ref paths stay stale. +- **Solution:** Replace `result.push(new_segment)` with `result.push(slugified_new())` on line 461. The `slugified_new` closure already handles both cases: returns the slugified name when needed, or the original name when no slugification is required. + ```rust + // BEFORE (line 460-463): + if s == old_segment { + result.push(new_segment); + replaced = true; + continue; + } + + // AFTER: + if s == old_segment { + result.push(slugified_new()); + replaced = true; + continue; + } + ``` +- **Risk:** Very low. Single-line change. The `slugified_new()` function is already used in the fallback path (line 467) and is known to work correctly. +- **Verify round-trip:** Rename an instance from "OldName" to "Bad:Name". Verify Rojo_Ref_* paths in children's meta files are updated correctly. The directory on disk should be "Bad_Name" and the meta file at that path should have updated ref paths. +- **Verify syncback parity:** N/A +- **Tests required:** + - Integration test: Create instance "OldModel" with child Part. Set Ref to Part. Rename OldModel to "New:Model". Verify meta file at `.../New_Model/init.meta.json5` has updated Rojo_Ref_* path. + +--- + +### Fix 4: Update Stale "EXPECTED: FAIL" Test Comments + +- **Status:** Approved +- **Finding:** Step 14: Challenge Known Limitations -- test comments from "prove the bug" phase are now incorrect. +- **Files:** `tests/tests/two_way_sync.rs` +- **Problem:** Two tests have stale doc-comments: + - `ref_stale_path_after_target_rename` (around line 6758): Says "EXPECTED: FAIL -- stored path becomes stale after target rename." But `update_ref_paths_after_rename` now handles this. + - `ref_stale_path_after_parent_rename` (around line 6807): Says "EXPECTED: FAIL -- stored path becomes stale after parent rename." Same fix applies. +- **Solution:** Update both doc-comments to explain that `update_ref_paths_after_rename` handles this case. Remove "EXPECTED: FAIL" markers. +- **Risk:** None -- documentation-only changes. +- **Verify round-trip:** Run both tests, verify they pass. +- **Verify syncback parity:** N/A +- **Tests required:** None -- existing tests ARE the coverage. + +--- + +### Fix 5: Remove Unused `_tree` Parameter from `update_ref_paths_after_rename` + +- **Status:** Approved +- **Finding:** Code quality -- dead parameter from previous design iteration. +- **Files:** `src/change_processor.rs` (line 407-408) +- **Problem:** `update_ref_paths_after_rename(&self, _tree: &mut RojoTree, old_path, new_path)` takes a mutable tree reference but never uses it. The function operates entirely through `self.ref_path_index` and filesystem I/O. +- **Solution:** Remove the `_tree` parameter from the function signature. Update the single call site (line 1538-1542) to not pass `&mut tree`. + ```rust + // BEFORE: + fn update_ref_paths_after_rename( + &self, + _tree: &mut crate::snapshot::RojoTree, + old_path: &str, + new_path: &str, + ) + + // AFTER: + fn update_ref_paths_after_rename( + &self, + old_path: &str, + new_path: &str, + ) + ``` + Call site: + ```rust + // BEFORE: + self.update_ref_paths_after_rename(&mut tree, old_ref_path, &new_ref_path); + + // AFTER: + self.update_ref_paths_after_rename(old_ref_path, &new_ref_path); + ``` +- **Risk:** None -- removing unused parameter. +- **Verify round-trip:** Existing rename tests still pass. +- **Verify syncback parity:** N/A +- **Tests required:** None. + +--- + +### Fix 6: Rename Misleading Variable and Remove Redundant Filter + +- **Status:** Approved +- **Finding:** Code quality -- misleading name and redundant filter. +- **Files:** `src/web/api.rs` (lines 2756-2760, 2887-2903) +- **Problem:** `removed_ref_values` (line 2756) contains attribute NAMES (e.g., "Rojo_Ref_PrimaryPart"), not path values. The `.filter(|a| a.starts_with(REF_PATH_ATTRIBUTE_PREFIX))` is redundant since all items in `remove_attributes` already start with that prefix (they all come from `ref_attribute_name(key)`). +- **Solution:** This variable and its associated code are removed entirely by Fix 2 (which replaces the whole index maintenance block with re-scanning). If Fix 2 is applied first, this fix is a no-op. If applied independently: rename `removed_ref_values` to `removed_ref_attr_names` and remove the redundant filter. +- **Risk:** None. +- **Verify round-trip:** N/A +- **Verify syncback parity:** N/A +- **Tests required:** None -- naming change only. + +--- + +## Skipped Fixes + +None. All findings were approved. + +## Accepted Limitations + +- **Ambiguous paths (duplicate-named siblings):** Properly warned via `is_ref_path_unique`. Ref is written but may resolve to wrong sibling on rebuild. Accepted in all three audits. Warning is logged, no silent corruption beyond documented scope. +- **Plugin reconciler duplicate on pull:** When PatchSet includes added instances, the plugin reconciler may create duplicate Studio instances. Pre-existing issue with the pull flow, not introduced by this feature. + +## Deferred Refactors + +- **ChangeProcessor::start parameter count:** Now at 8 parameters (clippy warns at 7). Consider grouping into a config struct. This is a pre-existing issue made marginally worse by adding `ref_path_index`. + +## Test Plan + +Tests are not optional. Every approved fix must have test coverage. A fix is not complete until its tests pass. + +### Rust Integration Tests (`tests/tests/two_way_sync.rs`) + +- *Fixture with pre-existing Rojo_Ref_ + rename** (Fix 1): Start server with a meta file that already contains a Rojo_Ref_PrimaryPart attribute. Rename the target instance. Verify the meta file's Rojo_Ref_* path is updated. This tests startup index population. +- **Multi-ref file partial removal + rename** (Fix 2): Set two Rojo_Ref_* attributes on the same instance (PrimaryPart and a second ref). Set PrimaryPart to nil. Rename the second ref's target. Verify the second ref's path updates in the meta file. This tests precise index maintenance. +- *Rename to slugified name + Rojo_Ref_ path update** (Fix 3): Set a Ref on a child instance. Rename the parent to a name with forbidden characters (e.g., "New:Model"). Verify the meta file at the slugified directory path has updated Rojo_Ref_* paths. + +### Rust Unit Tests (`#[cfg(test)]`) + +- **RefPathIndex `remove_all_for_file**` (Fix 2): Add test for the new method, verifying it removes a specific file from all entries without affecting other files' entries. + +### No New Tests Required + +- Fix 4: Existing tests are the coverage (update comments only). +- Fix 5: Existing rename tests verify behavior (remove dead parameter only). +- Fix 6: Subsumed by Fix 2. + +## 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 server level? -> Rust unit test or integration test +- Does it affect the round-trip? -> Integration test with `/api/write` + 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