diff --git a/.cursor/plans/audit_ref_two-way_sync_9e8ce7a8.plan.md b/.cursor/plans/audit_ref_two-way_sync_9e8ce7a8.plan.md index 00c4f512..4e6aef92 100644 --- a/.cursor/plans/audit_ref_two-way_sync_9e8ce7a8.plan.md +++ b/.cursor/plans/audit_ref_two-way_sync_9e8ce7a8.plan.md @@ -63,13 +63,13 @@ isProject: false ## Quality Standard -> **Round-trip identity**: Two-way sync writes a `Rojo_Ref_*` attribute to a meta/model file. Building an rbxl from that directory tree and forward-syncing it back must produce a **bit-identical Ref target** -- same property name, same target instance. Any deviation is a bug. +> **Round-trip identity**: Two-way sync writes a `Rojo_Ref_`* attribute to a meta/model file. Building an rbxl from that directory tree and forward-syncing it back must produce a **bit-identical Ref target** -- same property name, same target instance. Any deviation is a bug. Every audit item below must be evaluated against this standard. If there is ANY code path where a Ref property can be lost, point to the wrong target, or silently become nil through a two-way-sync + rebuild cycle, flag it as **critical**. ## Code Quality Standard -Watch for duplicated logic between [ref_properties.rs](src/syncback/ref_properties.rs) (CLI syncback bulk mode) and [api.rs](src/web/api.rs) (two-way sync per-instance mode). Both compute paths and format `Rojo_Ref_*` attributes. The shared helpers in [rojo_ref.rs](src/rojo_ref.rs) should be the single source of truth. +Watch for duplicated logic between [ref_properties.rs](src/syncback/ref_properties.rs) (CLI syncback bulk mode) and [api.rs](src/web/api.rs) (two-way sync per-instance mode). Both compute paths and format `Rojo_Ref_`* attributes. The shared helpers in [rojo_ref.rs](src/rojo_ref.rs) should be the single source of truth. --- @@ -183,7 +183,7 @@ Every code path in [api.rs](src/web/api.rs) that handles Ref properties must cor The forward-sync direction (filesystem -> server -> plugin) must correctly resolve `Rojo_Ref_*` attributes. - [patch_compute.rs](src/snapshot/patch_compute.rs) `compute_ref_properties`: - - Correctly strips `Rojo_Ref_` prefix to get property name? + - Correctly strips `Rojo_Ref`_ prefix to get property name? - Handles both `Variant::String` and `Variant::BinaryString` attribute values? - Logs warning for non-string attribute types? - `tree.get_instance_by_path(path)` returns `None` for invalid paths without crashing? @@ -219,10 +219,10 @@ The new `Rojo_Ref_*` system coexists with the legacy `Rojo_Target_*` + `Rojo_Id` Ref attributes add a new dimension to meta file management. - **Creation**: When the first Ref is set on an instance with no meta file -> meta file created with just `attributes: { Rojo_Ref_PropertyName: "path" }`? Does `build_meta_object` handle this (called when file doesn't exist)? -- **Merge**: When Ref attribute is added to an existing meta file with `className`, `properties`, other `attributes` -> existing content preserved? `Rojo_Ref_*` merged into attributes alongside user attributes? +- **Merge**: When Ref attribute is added to an existing meta file with `className`, `properties`, other `attributes` -> existing content preserved? `Rojo_Ref_`* merged into attributes alongside user attributes? - **Update**: When Ref target changes -> attribute value overwritten? Old value not left alongside new? - **Deletion**: When Ref set to nil -> attribute removed from file? If no other content remains in `attributes`, is the empty `attributes: {}` section left or cleaned up? If meta file has no remaining content at all (no className, no properties, no attributes), should the file be deleted? -- **Orphaned Rojo_Ref_***: If an instance is deleted, its meta file is deleted too (existing behavior). No orphaned `Rojo_Ref_*` attributes should remain. Verify this path. +- **Orphaned Rojo_Ref_***: If an instance is deleted, its meta file is deleted too (existing behavior). No orphaned `Rojo_Ref_`* attributes should remain. Verify this path. --- @@ -302,7 +302,7 @@ These are all updates to existing instances (not additions), so suppression is c Verify that the two-way sync Ref output matches CLI syncback (`ref_properties.rs`) output for equivalent scenarios: -- Same `Rojo_Ref_*` attribute name format +- Same `Rojo_Ref_`* attribute name format - Same path format (slash-separated, root excluded) - Same JSON5 attribute placement (in `attributes` section, not `properties`) - Same handling of nil Refs (attribute absent, not present with empty value) @@ -399,11 +399,11 @@ Evaluate existing tests against the audit areas above. For each gap, flag as mis - **Round-trip tests**: Do integration tests verify that setting a Ref via `/api/write`, then building from filesystem, produces the same Ref? - **Nil Ref cleanup**: Is the nil Ref -> attribute removal -> property removal chain tested end-to-end? -- **Forward-sync resolution**: Are there serve tests that verify `Rojo_Ref_*` attributes resolve to `Variant::Ref` in the tree? +- **Forward-sync resolution**: Are there serve tests that verify `Rojo_Ref_`* attributes resolve to `Variant::Ref` in the tree? - **Path edge cases**: Instance names with `/` in them? Deep nesting? Root-level Ref targets? - **Concurrent changes**: Multiple Ref changes in one batch? - **Error paths**: Ref to non-existent instance? Ref on ProjectNode instance? -- **CLI syncback parity**: Does the test suite verify that two-way sync and CLI syncback produce identical `Rojo_Ref_*` output for the same input? +- **CLI syncback parity**: Does the test suite verify that two-way sync and CLI syncback produce identical `Rojo_Ref_`* output for the same input? --- diff --git a/.cursor/plans/fix_ref_audit_issues_746c0c25.plan.md b/.cursor/plans/fix_ref_audit_issues_746c0c25.plan.md index 496f3a9a..55ae9aaf 100644 --- a/.cursor/plans/fix_ref_audit_issues_746c0c25.plan.md +++ b/.cursor/plans/fix_ref_audit_issues_746c0c25.plan.md @@ -66,14 +66,14 @@ Also grep for any OTHER callers of `encodePatchUpdate` that may be missing the 4 ## Issue 2: Stale Rojo_Ref_* Paths After Rename (CRITICAL) -When an instance is renamed, all `Rojo_Ref_*` attributes across the tree that reference the old path must be updated to the new path. +When an instance is renamed, all `Rojo_Ref_`* attributes across the tree that reference the old path must be updated to the new path. ### Design When `handle_tree_event` processes a rename: 1. Compute the **old path** and **new path** of the renamed instance (using `full_path_of` before and after rename) -2. Scan ALL descendants of the tree root for `Rojo_Ref_*` attributes +2. Scan ALL descendants of the tree root for `Rojo_Ref_`* attributes 3. For each attribute whose path **starts with** the old path (prefix match handles children too), replace the old prefix with the new prefix 4. Update the in-memory tree's Attributes property 5. Write the updated meta/model file to disk (with `suppress_path`) @@ -89,7 +89,7 @@ Add a new method `update_ref_paths_after_rename(&self, tree, old_path, new_path) - For each attribute starting with `REF_PATH_ATTRIBUTE_PREFIX`, checks if the path value starts with `old_path` - If match: replaces the prefix with `new_path` - Updates the instance's Attributes property in the tree -- Finds the instance's meta/model file via `InstigatingSource::Path` and rewrites the `Rojo_Ref_*` attribute on disk +- Finds the instance's meta/model file via `InstigatingSource::Path` and rewrites the `Rojo_Ref_`* attribute on disk Call this method from the rename handler, AFTER the filesystem rename is complete and the tree path has been updated. Both the directory-format rename path (line ~885) and the regular file rename path (line ~973) need it. @@ -208,13 +208,13 @@ pub fn split_ref_path(path: &str) -> Vec { ## Issue 4: compute_ref_properties Priority Conflict (CRITICAL) -If both `Rojo_Ref_PrimaryPart` and `Rojo_Target_PrimaryPart` exist, `Rojo_Target_*` silently overwrites `Rojo_Ref_*` because it's processed second. +If both `Rojo_Ref_PrimaryPart` and `Rojo_Target_PrimaryPart` exist, `Rojo_Target_`* silently overwrites `Rojo_Ref_*` because it's processed second. ### Fix **File:** [patch_compute.rs](src/snapshot/patch_compute.rs) `compute_ref_properties` -Swap the processing order: process `Rojo_Target_*` FIRST, then `Rojo_Ref_*`. Since `map.insert` overwrites, the last insert wins. Processing `Rojo_Ref_*` second means the path-based system has priority (which is correct since it's the preferred system). +Swap the processing order: process `Rojo_Target_`* FIRST, then `Rojo_Ref_*`. Since `map.insert` overwrites, the last insert wins. Processing `Rojo_Ref_*` second means the path-based system has priority (which is correct since it's the preferred system). ```rust for (attr_name, attr_value) in attributes.iter() { @@ -301,7 +301,7 @@ Add a helper `is_ref_path_unique(tree, target_ref)` that walks ancestors checkin **File:** [ref_properties.rs](src/syncback/ref_properties.rs) line 251 -The ID-based `Rojo_Target_*` system uses inline `format!("{REF_POINTER_ATTRIBUTE_PREFIX}{}", link.name)` while the path-based system uses the shared `ref_attribute_name()` helper. Add a matching helper: +The ID-based `Rojo_Target_`* system uses inline `format!("{REF_POINTER_ATTRIBUTE_PREFIX}{}", link.name)` while the path-based system uses the shared `ref_attribute_name()` helper. Add a matching helper: ```rust // In rojo_ref.rs: diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_change_target_patch.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_change_target_patch.snap index efea91c5..aaa75a01 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_change_target_patch.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_change_target_patch.snap @@ -1,5 +1,6 @@ --- source: tests/tests/serve.rs +assertion_line: 869 expression: redacted --- body: @@ -15,7 +16,9 @@ body: Attributes: "!Attributes": Rojo_Ref_PrimaryPart: - "!String": Workspace/MyModel/Target + "!String": Workspace/MyModel/OtherPart + PrimaryPart: + "!Ref": id-4 id: id-3 packetType: messages sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_multiple_attrs_patch.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_multiple_attrs_patch.snap index efea91c5..b4b34abd 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_multiple_attrs_patch.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_multiple_attrs_patch.snap @@ -1,5 +1,6 @@ --- source: tests/tests/serve.rs +assertion_line: 988 expression: redacted --- body: @@ -14,8 +15,12 @@ body: changedProperties: Attributes: "!Attributes": + Rojo_Ref_CustomRef: + "!String": Workspace/MyModel/OtherPart Rojo_Ref_PrimaryPart: "!String": Workspace/MyModel/Target + CustomRef: + "!Ref": id-4 id: id-3 packetType: messages sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_new_file_patch.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_new_file_patch.snap index efea91c5..c38c4726 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_new_file_patch.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_new_file_patch.snap @@ -1,11 +1,25 @@ --- source: tests/tests/serve.rs +assertion_line: 928 expression: redacted --- body: messageCursor: 1 messages: - - added: {} + - added: + id-4: + Children: [] + ClassName: ObjectValue + Id: id-4 + Metadata: + ignoreUnknownInstances: false + Name: RefValue + Parent: id-3 + Properties: + Attributes: + "!Attributes": {} + Value: + "!Ref": id-5 removed: [] updated: - changedClassName: ~ diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_nonexistent_patch.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_nonexistent_patch.snap index efea91c5..d973ba9b 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_nonexistent_patch.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_nonexistent_patch.snap @@ -1,5 +1,6 @@ --- source: tests/tests/serve.rs +assertion_line: 959 expression: redacted --- body: @@ -15,7 +16,8 @@ body: Attributes: "!Attributes": Rojo_Ref_PrimaryPart: - "!String": Workspace/MyModel/Target + "!String": Workspace/NonExistent/Part + PrimaryPart: ~ id: id-3 packetType: messages sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_remove_attr_patch.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_remove_attr_patch.snap index efea91c5..cf3c77c3 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_remove_attr_patch.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_path_remove_attr_patch.snap @@ -1,5 +1,6 @@ --- source: tests/tests/serve.rs +assertion_line: 897 expression: redacted --- body: @@ -12,10 +13,8 @@ body: changedMetadata: ~ changedName: ~ changedProperties: - Attributes: - "!Attributes": - Rojo_Ref_PrimaryPart: - "!String": Workspace/MyModel/Target + Attributes: ~ + PrimaryPart: ~ id: id-3 packetType: messages sessionId: id-1 diff --git a/scripts/ci.sh b/scripts/ci.sh old mode 100644 new mode 100755