mirror of
https://github.com/rojo-rbx/rojo.git
synced 2026-10-23 05:51:39 +00:00
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.
This commit is contained in:
Joe
parent
e617e8fcda
commit
012c20b1b9
1 changed files with 335 additions and 0 deletions
335
.cursor/plans/fix_ref_audit_round_3_f8ae0e39.plan.md
Normal file
335
.cursor/plans/fix_ref_audit_round_3_f8ae0e39.plan.md
Normal file
|
|
@ -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::<serde_json::Value>(&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::<serde_json::Value>(&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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue