mirror of
https://github.com/rojo-rbx/rojo.git
synced 2026-10-23 05:51:39 +00:00
Add audit plan for Ref property fixes and enhance CI test execution
- Introduced a new audit plan detailing fixes for four issues identified in the second audit of two-way sync Ref properties, including stale comments, slugified name mismatches, immediate ID assignments, and performance optimizations. - Updated CI scripts to run Rust tests with increased parallelism by specifying `--test-threads=16`, improving test execution efficiency. - Enhanced error handling in `update_ref_paths_in_file` to provide clearer context on failures during file reading and JSON parsing. These changes improve the documentation for Ref property audits and optimize the CI testing process.
This commit is contained in:
Joe
parent
faa12dc573
commit
075cca4c96
4 changed files with 321 additions and 14 deletions
304
.cursor/plans/fix_ref_audit_round_2_07d615d1.plan.md
Normal file
304
.cursor/plans/fix_ref_audit_round_2_07d615d1.plan.md
Normal file
|
|
@ -0,0 +1,304 @@
|
|||
---
|
||||
name: Fix Ref Audit Round 2
|
||||
overview: "Fix 4 issues found in the second audit of two-way sync Ref properties: stale test comment, slugified name mismatch during renames, immediate ID assignment for same-request additions, and performance optimization for Rojo_Ref_* path updates after renames."
|
||||
todos:
|
||||
- id: fix-test-comment
|
||||
content: "Fix 1: Update stale test comment on ref_to_instance_added_in_same_request"
|
||||
status: completed
|
||||
- id: fix-slugified-rename
|
||||
content: "Fix 2: Add slugified name fallback in update_ref_paths_after_rename"
|
||||
status: completed
|
||||
- id: fix-immediate-id
|
||||
content: "Fix 3: Implement immediate ID assignment -- include added instances in PatchSet with plugin GUID as snapshot_id"
|
||||
status: completed
|
||||
- id: fix-ref-path-index
|
||||
content: "Fix 4: Implement RefPathIndex for O(1) rename path lookups, add to JobThreadContext + ApiService"
|
||||
status: completed
|
||||
- id: tests-all
|
||||
content: Write all required integration and unit tests for Fixes 1-4
|
||||
status: completed
|
||||
- id: run-ci
|
||||
content: Run /ci to verify all tests pass
|
||||
status: completed
|
||||
isProject: false
|
||||
---
|
||||
|
||||
# Fix Ref Two-Way Sync Audit Round 2
|
||||
|
||||
> 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
|
||||
|
||||
Second audit of the two-way sync Ref property feature (branch `refs` vs `origin/master`). The first audit and fix plan addressed critical issues (ServeSession missing instanceMap, stale paths, slash escaping, priority conflict, encodeInstance leak, filter_properties_for_meta). This audit found 4 additional issues.
|
||||
|
||||
## Fixes
|
||||
|
||||
### Fix 1: Stale Test Comment on `ref_to_instance_added_in_same_request`
|
||||
|
||||
- **Status:** Approved
|
||||
- **Finding:** Step 2b: Nil/Removal Round-Trip
|
||||
- **Files:** `tests/tests/two_way_sync.rs`
|
||||
- **Problem:** The test `ref_to_instance_added_in_same_request` (around line ~6780) has comments saying "EXPECTED: FAIL" and explaining that the new Part is not in the tree. However, the `added_paths` mechanism in `syncback_updated_properties` (`src/web/api.rs` lines 596-620) was specifically added to handle this case. It pre-computes paths for instances from `request.added`, so the `Rojo_Ref_PrimaryPart` attribute IS written correctly to the meta file. The test should PASS.
|
||||
- **Solution:** Update the test doc-comment to explain that `added_paths` handles same-request additions. Remove the "EXPECTED: FAIL" marker and "This SHOULD FAIL" assertion comment. The actual assertion (`assert_meta_has_ref_attr`) should remain as-is since it asserts correct behavior.
|
||||
- **Risk:** None -- documentation-only change.
|
||||
- **Verify round-trip:** Run `cargo test ref_to_instance_added_in_same_request` -- it should pass.
|
||||
- **Verify syncback parity:** N/A
|
||||
- **Tests required:**
|
||||
- No new tests. The existing test IS the coverage.
|
||||
|
||||
---
|
||||
|
||||
### Fix 2: Slugified Name Mismatch in `update_ref_paths_after_rename`
|
||||
|
||||
- **Status:** Approved
|
||||
- **Finding:** Step 2d: Rename/Move Interactions
|
||||
- **Files:** `src/change_processor.rs`
|
||||
- **Problem:** `update_ref_paths_after_rename` (lines 434-458) replaces path components in InstigatingSource paths by comparing the UNESCAPED tree instance name (`old_segment`) to filesystem path components. But the filesystem uses SLUGIFIED names when the instance name has forbidden characters. Example: instance "Test:Model" has filesystem directory "Test_Model". After rename, `old_segment = "Test:Model"` but filesystem component is `"Test_Model"` -- they don't match, so meta files inside the renamed directory aren't found, and Rojo_Ref_* attributes aren't updated.
|
||||
- **Solution:** When the direct name comparison fails, fall back to comparing against the slugified name. Import `name_needs_slugify` and `slugify_name` from `crate::syncback`. After the `if s == old_segment` check at line 441, add:
|
||||
|
||||
```rust
|
||||
if s == old_segment {
|
||||
// ... existing match logic ...
|
||||
} else if name_needs_slugify(old_segment) {
|
||||
let slugified_old = slugify_name(old_segment);
|
||||
if s == slugified_old.as_str() {
|
||||
let new_fs_name = if name_needs_slugify(new_segment) {
|
||||
slugify_name(new_segment)
|
||||
} else {
|
||||
new_segment.to_string()
|
||||
};
|
||||
*comp = std::path::Component::Normal(
|
||||
std::ffi::OsStr::new(&new_fs_name),
|
||||
);
|
||||
replaced = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Note: The `slugify_name` return value needs to outlive the component assignment. Store it in a local variable before the loop, or use a `Cow` pattern. The implementer should verify the lifetime works (may need to collect into a PathBuf outside the loop).
|
||||
|
||||
- **Risk:** Low. Only affects the edge case of slugified names + rename. The slugify/compare is stateless and deterministic.
|
||||
- **Verify round-trip:** Create an instance named "Test:Model" with a child Part, set PrimaryPart to the Part, then rename "Test:Model" to "New:Model". Verify the meta file's Rojo_Ref_PrimaryPart is updated from "Workspace/Test:Model/Part1" to "Workspace/New:Model/Part1".
|
||||
- **Verify syncback parity:** N/A -- this is change_processor.rs only.
|
||||
- **Tests required:**
|
||||
- Integration test in `tests/tests/two_way_sync.rs`: Create an instance with forbidden characters, set a Ref, rename it, verify meta file updated.
|
||||
|
||||
---
|
||||
|
||||
### Fix 3: Immediate ID Assignment for Same-Request Additions
|
||||
|
||||
- **Status:** Approved
|
||||
- **Finding:** Step 14: Known Limitation Challenge (transient dangling Ref)
|
||||
- **Files:** `src/web/api.rs`
|
||||
- **Problem:** When `/api/write` both adds a new instance AND sets a Ref targeting it, the filesystem is correct (via `added_paths`), but the in-memory tree gets a dangling Ref. The plugin receives an unresolvable Ref ID ("Unknown ID" error in `decodeValue.lua`), causing a brief nil period (~200-500ms) until VFS watcher correction.
|
||||
- **Solution:** Include added instances in the PatchSet's `added_instances` using the plugin's GUID as `snapshot_id`. The existing `snapshot_id_to_instance_id` mapping in `apply_patch_set` handles GUID -> tree ID rewriting for Ref properties in `updated_instances`. No VFS suppression needed -- the VFS watcher will match the tree child by name+className via `compute_children_patches` and generate an UPDATE (not duplicate ADD) that fills in InstigatingSource and metadata.
|
||||
|
||||
**Implementation steps:**
|
||||
|
||||
1. **Build InstanceSnapshots from AddedInstance data** -- After the `syncback_added_instance` loop (around line 590), convert each `AddedInstance` to an `InstanceSnapshot`:
|
||||
|
||||
```rust
|
||||
fn added_instance_to_snapshot(
|
||||
guid: Ref,
|
||||
added: &AddedInstance,
|
||||
) -> InstanceSnapshot {
|
||||
let properties: UstrMap<Variant> = added
|
||||
.properties
|
||||
.iter()
|
||||
.map(|(k, v)| (ustr(k), v.clone()))
|
||||
.collect();
|
||||
|
||||
InstanceSnapshot {
|
||||
snapshot_id: guid,
|
||||
name: Cow::Owned(added.name.clone()),
|
||||
class_name: ustr(&added.class_name),
|
||||
properties,
|
||||
children: added
|
||||
.children
|
||||
.iter()
|
||||
.map(|child| added_instance_to_snapshot(Ref::new(), child))
|
||||
.collect(),
|
||||
metadata: Default::default(), // VFS will fill in InstigatingSource
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
1. **Include in PatchSet** -- Replace `added_instances: Vec::new()` (currently around line 700 in the PatchSet construction) with:
|
||||
|
||||
```rust
|
||||
added_instances: request
|
||||
.added
|
||||
.iter()
|
||||
.filter_map(|(guid, added)| {
|
||||
let parent_ref = added.parent?;
|
||||
Some(PatchAdd {
|
||||
parent_id: parent_ref,
|
||||
instance: added_instance_to_snapshot(*guid, added),
|
||||
})
|
||||
})
|
||||
.collect(),
|
||||
```
|
||||
|
||||
1. **No VFS suppression for additions** -- Keep existing behavior (additions are NOT suppressed). The VFS watcher will fire, `compute_children_patches` will match the existing tree child by name+className, and generate a metadata UPDATE (setting InstigatingSource::Path) rather than a duplicate ADD.
|
||||
2. **Transient metadata gap** -- Between PatchSet application and VFS correction (~200ms), the tree instance has no InstigatingSource. Rename, property write, and deletion operations that check InstigatingSource would fail during this window. This is acceptable because the user cannot make additional changes in Studio during this brief period.
|
||||
|
||||
- **Risk:** Moderate. Changes how added instances enter the tree (now via PatchSet instead of only via VFS). Edge cases:
|
||||
- VFS watcher fires BEFORE PatchSet is processed: ChangeProcessor processes PatchSet and VFS patches sequentially on the same thread. If VFS fires first, the PatchSet would find the instance already in the tree and the `snapshot_id_to_instance_id` map would still work (matching by tree ID).
|
||||
- Actually, the ChangeProcessor processes patches in order. The PatchSet from `handle_api_write` is sent to the `tree_mutation_sender` channel. VFS-triggered patches are also sent to this channel. The ChangeProcessor processes them in order. Since `handle_api_write` sends the PatchSet BEFORE returning (and the VFS watcher fires after the file write), the PatchSet should be processed first. But verify by checking the channel order.
|
||||
- Plugin reconciler receives the ADD and creates a new Studio instance. But the Studio instance already exists (it was "pulled" from Studio). This is a PRE-EXISTING issue with the pull flow and is not made worse by this change.
|
||||
- **Verify round-trip:** Run `ref_to_instance_added_in_same_request` test -- it should pass AND the in-memory tree should have the correct Ref (not dangling).
|
||||
- **Verify syncback parity:** N/A -- this is api.rs PatchSet construction only.
|
||||
- **Tests required:**
|
||||
- Update `ref_to_instance_added_in_same_request` test to also verify the Ref resolves correctly in the tree (read the instance back via `/api/read` after a short delay).
|
||||
- Integration test: Add two instances in the same request with Ref properties referencing each other. Verify both Refs resolve correctly.
|
||||
|
||||
---
|
||||
|
||||
### Fix 4: Pre-computed Index for Rename Path Updates
|
||||
|
||||
- **Status:** Approved
|
||||
- **Finding:** Step 11f: Atomicity and File Watcher Races (performance aspect)
|
||||
- **Files:** `src/change_processor.rs`, `src/web/api.rs` (or new module)
|
||||
- **Problem:** `update_ref_paths_after_rename` iterates ALL tree descendants (O(tree_size)) for each rename. For large trees with many renames in a batch, this is O(N * tree_size).
|
||||
- **Solution:** Maintain a `HashMap<String, HashSet<PathBuf>>` mapping Rojo_Ref_* path prefixes to meta/model files that contain them. Update the index when:
|
||||
- `syncback_updated_properties` writes a Rojo_Ref_* attribute to a meta/model file (add to index)
|
||||
- `merge_or_build_meta` removes a Rojo_Ref_* attribute (remove from index if no refs remain)
|
||||
- `update_ref_paths_in_file` updates a path (update index keys)
|
||||
- VFS watcher processes a meta/model file with Rojo_Ref_* attributes (add to index)
|
||||
|
||||
On rename, look up `old_path` prefix in the index to find affected files directly.
|
||||
|
||||
**Data structure:**
|
||||
|
||||
```rust
|
||||
/// Index of meta/model files that contain Rojo_Ref_* attributes.
|
||||
/// Maps the Rojo_Ref_* attribute VALUE (the path string) to the set of
|
||||
/// filesystem paths where that attribute appears.
|
||||
struct RefPathIndex {
|
||||
/// path_value -> set of meta/model files containing a Rojo_Ref_*
|
||||
/// attribute with that path value.
|
||||
paths_to_files: HashMap<String, HashSet<PathBuf>>,
|
||||
}
|
||||
|
||||
impl RefPathIndex {
|
||||
fn add(&mut self, ref_path: &str, meta_file: &Path) { ... }
|
||||
fn remove(&mut self, ref_path: &str, meta_file: &Path) { ... }
|
||||
/// Find all files with Rojo_Ref_* paths that start with `prefix`.
|
||||
fn find_by_prefix(&self, prefix: &str) -> Vec<PathBuf> { ... }
|
||||
}
|
||||
```
|
||||
|
||||
**Storage location:** Add as a field on `JobThreadContext` (change_processor) behind `Arc<Mutex<>>` (shared with api.rs via the same pattern as `suppressed_paths`). Also accessible from `ApiService`.
|
||||
|
||||
**Index maintenance points:**
|
||||
|
||||
- `syncback_updated_properties` in api.rs: after writing Rojo_Ref_* to a meta file, call `index.add(path_value, meta_path)`
|
||||
- `merge_or_build_meta` in api.rs: after removing attributes, call `index.remove(old_path_value, meta_path)`
|
||||
- `update_ref_paths_in_file` in meta.rs: after updating, call `index.remove(old) + index.add(new)`
|
||||
- Initial population: when the server starts and loads the tree, scan meta files for existing Rojo_Ref_* attributes (or lazily populate on first rename)
|
||||
|
||||
**Rename handler update:** Replace the `tree.descendants(root_id)` scan with:
|
||||
|
||||
```rust
|
||||
let affected_files = self.ref_path_index.lock().unwrap().find_by_prefix(old_path);
|
||||
for file_path in affected_files {
|
||||
// ... existing suppress + update_ref_paths_in_file logic ...
|
||||
}
|
||||
```
|
||||
|
||||
- **Risk:** Moderate. Index must stay synchronized with actual file contents. If the index gets out of sync (missed add/remove), renames would silently skip affected files. Mitigations: (1) the index is a performance optimization -- if a file is missed, the Ref becomes stale (same as the current behavior without the index for very large trees), (2) add debug logging when index is accessed to help diagnose sync issues.
|
||||
- **Verify round-trip:** All existing rename tests should pass with the index.
|
||||
- **Verify syncback parity:** N/A
|
||||
- **Tests required:**
|
||||
- Unit test for `RefPathIndex`: add, remove, find_by_prefix with various path patterns.
|
||||
- Integration test: set multiple Refs across different instances, rename one target, verify ALL affected meta files are updated (proves index maintenance works).
|
||||
|
||||
---
|
||||
|
||||
## 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 the first audit.
|
||||
- **Plugin reconciler duplicate on pull:** When the PatchSet includes added instances (Fix 3), the plugin reconciler creates new Studio instances. The original Studio instances (from the "pull" action) still exist. This is a pre-existing issue with the pull flow, not introduced by this feature. Deferred.
|
||||
|
||||
## Deferred Refactors
|
||||
|
||||
- **Lazy index population:** The `RefPathIndex` (Fix 4) could be populated lazily on first rename rather than scanning all files at server startup. This reduces startup time for large projects. Evaluate after Fix 4 is implemented.
|
||||
|
||||
## Test Plan
|
||||
|
||||
Tests are not optional. Every approved fix must have test coverage. A fix is not complete until its tests pass.
|
||||
|
||||
### Rust Unit Tests
|
||||
|
||||
- `RefPathIndex::add`, `remove`, `find_by_prefix` basic operations (Fix 4)
|
||||
- `RefPathIndex::find_by_prefix` with escaped paths (Fix 4)
|
||||
- `RefPathIndex::remove` when path not in index (no-op) (Fix 4)
|
||||
|
||||
### Rust Integration Tests (`tests/tests/two_way_sync.rs`)
|
||||
|
||||
- Update `ref_to_instance_added_in_same_request` comment + verify tree state via `/api/read` (Fix 1 + Fix 3)
|
||||
- Add two instances with cross-referencing Refs in same request (Fix 3)
|
||||
- Rename instance with slugified name, verify Rojo_Ref_* paths updated (Fix 2)
|
||||
- Set multiple Refs across different instances, rename target, verify all meta files updated (Fix 4)
|
||||
|
||||
### Lua Spec Tests
|
||||
|
||||
None required for these fixes (plugin-side code is unchanged).
|
||||
|
||||
### Snapshot Tests
|
||||
|
||||
None required for these fixes.
|
||||
|
||||
## 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.
|
||||
|
|
@ -68,7 +68,7 @@ $build = $LASTEXITCODE
|
|||
Record-Result "Build" $build
|
||||
|
||||
Write-Step 8 "Run ALL Rust Tests"
|
||||
$testOutput = cargo test --locked --all-targets --all-features 2>&1
|
||||
$testOutput = cargo test --locked --all-targets --all-features -- --test-threads=16 2>&1
|
||||
$rustTests = $LASTEXITCODE
|
||||
$testOutput | Write-Host
|
||||
Record-Result "Rust Tests" $rustTests
|
||||
|
|
@ -125,9 +125,9 @@ if ($RbxDom) {
|
|||
|
||||
Write-Step 15 "Run ALL rbx-dom Rust Tests"
|
||||
Push-Location rbx-dom
|
||||
cargo test --verbose
|
||||
cargo test --verbose -- --test-threads=16
|
||||
$testsRbxDom = $LASTEXITCODE
|
||||
cargo test --all-features --verbose
|
||||
cargo test --all-features --verbose -- --test-threads=16
|
||||
$testsRbxDomAll = $LASTEXITCODE
|
||||
Pop-Location
|
||||
Record-Result "Tests (rbx-dom)" $testsRbxDom
|
||||
|
|
|
|||
|
|
@ -67,7 +67,7 @@ build_exit=$?
|
|||
record "Build" $build_exit
|
||||
|
||||
step 8 "Run ALL Rust Tests"
|
||||
cargo test --locked --all-targets --all-features 2>&1
|
||||
cargo test --locked --all-targets --all-features -- --test-threads=16 2>&1
|
||||
rust_tests=$?
|
||||
record "Rust Tests" $rust_tests
|
||||
|
||||
|
|
@ -116,9 +116,9 @@ if [ "$RBX_DOM" = true ]; then
|
|||
record "Build (rbx-dom all-features)" $build_rbxdom_all
|
||||
|
||||
step 15 "Run ALL rbx-dom Rust Tests"
|
||||
(cd rbx-dom && cargo test --verbose)
|
||||
(cd rbx-dom && cargo test --verbose -- --test-threads=16)
|
||||
tests_rbxdom=$?
|
||||
(cd rbx-dom && cargo test --all-features --verbose)
|
||||
(cd rbx-dom && cargo test --all-features --verbose -- --test-threads=16)
|
||||
tests_rbxdom_all=$?
|
||||
record "Tests (rbx-dom)" $tests_rbxdom
|
||||
record "Tests (rbx-dom all-features)" $tests_rbxdom_all
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@
|
|||
//! `web/api.rs` (syncback added/removed instances) to keep meta file handling
|
||||
//! DRY and consistent.
|
||||
|
||||
use anyhow::Context;
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
|
||||
|
|
@ -106,14 +107,16 @@ pub fn update_ref_paths_in_file(
|
|||
return Ok(false);
|
||||
}
|
||||
|
||||
let bytes = match fs::read(file_path) {
|
||||
Ok(b) => b,
|
||||
Err(_) => return Ok(false),
|
||||
};
|
||||
let mut val = match crate::json::from_slice::<serde_json::Value>(&bytes) {
|
||||
Ok(v) if v.is_object() => v,
|
||||
_ => return Ok(false),
|
||||
};
|
||||
let bytes = fs::read(file_path)
|
||||
.with_context(|| format!("Failed to read {}", file_path.display()))?;
|
||||
let mut val: serde_json::Value = crate::json::from_slice(&bytes)
|
||||
.with_context(|| format!("Failed to parse JSON5 in {}", file_path.display()))?;
|
||||
if !val.is_object() {
|
||||
anyhow::bail!(
|
||||
"{} is not a JSON object, cannot update Rojo_Ref_* attributes",
|
||||
file_path.display()
|
||||
);
|
||||
}
|
||||
|
||||
let mut updated = false;
|
||||
if let Some(attrs) = val.get_mut("attributes").and_then(|a| a.as_object_mut()) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue