hakowork/rojo
3
0
Fork 0
mirror of https://github.com/rojo-rbx/rojo.git synced 2026-10-23 05:51:39 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Add audit documentation and enhance two-way sync handling

Browse source 
- Introduced a comprehensive audit guide for the production-grade sync feature, detailing the evaluation criteria for round-trip identity and code quality.
- Added a new plan for auditing the two-way sync Ref property feature, outlining specific tasks and expected behaviors.
- Updated `ServeSession.lua` to include the instance map in patch updates, ensuring accurate property encoding.
- Enhanced `encodeInstance.lua` to prevent encoding Ref properties during instance addition, delegating this to the patch update process.
- Implemented regression tests to highlight known limitations in Ref handling, ensuring better coverage and identification of existing bugs.

These changes improve the clarity of the sync feature's audit process and strengthen the handling of Ref properties in the system.
This commit is contained in:
Joe 2026-02-14 12:08:12 -06:00
parent c181172460
commit 4070b48308
15 changed files with 1559 additions and 138 deletions
Download patch file Download diff file Expand all files Collapse all files

323
.cursor/commands/audit.md Normal file
View file

@ -0,0 +1,323 @@
# /audit - Production-Grade Sync Feature Audit
Perform a systematic, production-grade audit of a sync feature implementation. This command is specifically designed for Atlas's bidirectional synchronization system (filesystem <-> Roblox Studio).
## The Invariant
Every audit item MUST be evaluated against this standard:
> **Round-trip identity:** Two-way sync (or syncback) 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.
A "works most of the time" finding is NOT acceptable. If there is ANY code path where an instance name, property, or hierarchy relationship can be lost, mangled, or silently altered through a syncback/two-way-sync -> rebuild cycle, flag it as **critical**.
## CLI Syncback Parity
From `atlas.mdc`: "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 between the two-way sync path and the CLI syncback path is a bug until proven otherwise. The CLI syncback is the ground truth.
## Code Quality
Watch for duplicated logic -- the same pattern copy-pasted across `change_processor.rs`, `api.rs`, `dir.rs`, `project.rs`, etc. If the same sequence appears in 3+ places, that's a refactor candidate.
- **Small refactor** (helper function, 2-3 call sites): flag it AND fix it.
- **Major rewrite** (10+ call sites, pipeline redesign): flag it in a "Deferred Refactors" section.
---
## Instructions
### 1. Identify the Feature Scope
**Default behavior:** Audit all changes in the current workspace against `origin/master`. This captures everything: committed branch changes, unpushed commits, staged changes, and unstaged changes. Run:
```bash
git log origin/master..HEAD --oneline
git diff origin/master --stat
git diff origin/master
```
Read the commit log (for intent) and the full diff (for code). If the user provides plan files (via `@` reference), read those for additional context. Do NOT search for or read plan files on your own -- plans may be stale and describe outdated designs or already-fixed issues.
**If the user specifies a feature or branch:** Scope the audit to that feature's changes only.
From the diff, identify:
- Which files were modified (plugin Lua, server Rust, test fixtures)
- What the feature does (new property type support, naming change, sync flow change, etc.)
- What the correct round-trip behavior should be
- Which sync paths are affected (forward sync, reverse sync, two-way sync, all three)
### 2. Trace Round-Trip Identity (MOST CRITICAL)
For each operation the feature introduces, trace the **complete lifecycle** through every path:
#### 2a. Two-Way Sync -> Forward Sync Round-Trip
Pick a concrete example. Trace:
1. **Plugin encodes** -- How does the plugin read the property/change from Studio? What format does it send to the server? What encoding is used (msgpack, JSON, etc.)?
2. **Server receives** -- How does `handle_api_write` deserialize the data? Any type conversions?
3. **Server writes to disk** -- Which function writes to the filesystem? What file format (meta.json5, model.json5, .luau, etc.)? What path computation is used?
4. **Server applies to tree** -- How is the PatchSet built? Does `apply_patch_set` handle this data type correctly? Any special-case skips (like `Ref::none()` skip in `apply_update_child`)?
5. **VFS echo prevention** -- Is `suppress_path()` called before `fs::write()` for updates? Is suppression correctly NOT used for additions?
6. **Forward sync reads** -- When the file is read back by the snapshot system, does it produce the exact same property value? Any lossy conversion?
7. **Plugin receives** -- Does `decodeValue.lua` correctly decode the value back to a Studio-compatible type?
**Verify:** After the full round-trip, is the value identical? If you killed the server and ran `atlas build`, would the property have the correct value?
#### 2b. Nil/Removal Round-Trip
Trace what happens when the property is set to nil or removed:
- Plugin encoding of nil values
- Server handling (property removal vs. nil sentinel)
- Filesystem representation (attribute removed? file deleted? empty value?)
- Forward sync behavior (absent property = default value?)
#### 2c. Change/Update Round-Trip
Trace what happens when the value changes from A to B:
- Does the old value get properly overwritten?
- Any merge conflicts with existing file content?
- Ordering issues (removal before addition? addition before removal?)
#### 2d. Rename/Move Interactions
Trace what happens when an instance involved in the feature is renamed or moved:
- Do stored paths/references become stale?
- Does the ChangeProcessor update dependent data?
- Is there a window of inconsistency during the rename?
### 3. Audit Plugin-Side Encoding
Every code path in the plugin that encodes the feature's data must be checked:
- **Property filter** (`propertyFilter.lua`): Is the data type correctly included/excluded?
- **Patch update encoding** (`encodePatchUpdate.lua`): Does the encoding handle all cases (nil, valid, edge)?
- **Instance encoding** (`encodeInstance.lua`): For newly added instances, is the data type handled correctly? Any fragile error paths (pcall catching errors instead of explicit checks)?
- **Patch creation** (`createPatchSet.lua`): Are all required parameters passed through?
- **All callers**: Search for ALL callers of modified functions. Any call site that was NOT updated when a function signature changed? (Use grep to find every caller.)
**CRITICAL -- Lua silent argument mismatch:** In Lua/Luau, calling `foo(a, b, c)` when the function signature is `function(a, b, c, d)` does NOT produce any error -- `d` is simply `nil`. This means adding a new parameter to a function signature produces **zero errors** at callers that weren't updated. The missing argument silently becomes `nil`, and the function may take a fallback path that drops data without warning. **Grep for every caller of any modified function and verify the argument count matches.** This is the #1 source of silent data loss bugs in the plugin.
**Multiple plugin entry points:** Property changes flow through the plugin via TWO separate paths:
1. **ChangeBatcher flow** (live two-way sync): `Instance.Changed` -> `InstanceMap.__connectSignals` -> `ChangeBatcher:add()` -> `createPatchSet()` -> `encodePatchUpdate()` -> `ApiContext:write()`
2. **Confirmation dialog flow** (initial sync pull): `ServeSession.__confirmAndApplyInitialPatch` -> `encodePatchUpdate()` -> `ApiContext:write()`
Both paths must be audited. A function modified in path 1 may not have been updated in path 2.
**Change detection signals:** `Instance.Changed` fires for all scriptable properties on regular instances, but ValueBase instances (ObjectValue, IntValue, etc.) override Changed to only fire for `Value`. The InstanceMap connects `GetPropertyChangedSignal` for `Name`, `Value`, and `Parent` on ValueBase instances. Verify the feature's data type is detected by the correct signal.
### 4. Audit Server-Side Processing
Every code path in `api.rs` and `change_processor.rs`:
- **Data extraction**: Is the feature's data extracted correctly from the write request?
- **Path/value computation**: Are computed values correct? **Verify inverse consistency:** if you compute a value from data (e.g., `full_path_of(ref)` -> path), then resolve it back (e.g., `get_instance_by_path(path)` -> ref), do you get the same original data? Any case where the round-trip fails (e.g., instance names containing the path separator `/`)?
- **Meta/model file writing**: Correct file format? Correct section (properties vs. attributes)? Merge with existing content? Creation from scratch?
- **PatchSet construction**: Data included correctly? Any type conversions that lose information?
- **Filter functions**: Any filter that should now pass the data type but was not updated?
- **Dual write path consistency**: `handle_api_write` does TWO things: (1) writes files to disk via `syncback_updated_properties`, and (2) sends a PatchSet to the ChangeProcessor to update the in-memory tree. These MUST stay consistent. If the file write succeeds but the PatchSet doesn't include the same change (or vice versa), the tree and filesystem diverge silently.
- **Processing order**: `handle_api_write` processes removals, then additions, then updates. Property writes (`syncback_updated_properties`) happen BEFORE the PatchSet is sent to the ChangeProcessor. This ordering matters: if a property change references an instance that was renamed in the same batch, the path is computed from the PRE-rename tree state.
- **Sentinel values in patch system**: Check `apply_update_child` and `apply_add_child` for any data type that is silently skipped or treated as a no-op. Example: `Variant::Ref(Ref::none())` is skipped with `continue` at line 281-282 of `patch_apply.rs`. If the feature's nil/empty representation matches a sentinel, the patch system may silently ignore it instead of applying it.
### 5. Audit Forward-Sync Resolution
The filesystem -> server -> plugin direction:
- **Snapshot middleware**: Does the middleware that reads the file format correctly parse the feature's data?
- **Patch computation**: Does `compute_patch_set` handle the data type? Any special-case resolution (like `compute_ref_properties` for Rojo_Ref_* attributes)?
- **Patch application**: Does `apply_patch_set` correctly apply the data? Any sentinel values that are skipped?
- **Attribute cleanup**: Are internal attributes (Rojo_Ref_*, Rojo_Target_*, etc.) cleaned from the Attributes property before being sent to Studio?
- **Plugin decoding**: Does `decodeValue.lua` handle the type?
### 6. Audit Interactions with Existing Systems
- **Legacy systems**: Does the new feature coexist with legacy systems? Priority conflicts? Duplicate data?
- **Other callers**: Are there callers of modified functions from OUTSIDE the feature's scope? Were they updated?
- **Shared filters/helpers**: Were shared data structures (like `UNENCODABLE_DATA_TYPES`) modified? Does this affect other code paths that import them?
### 7. Audit Meta File Lifecycle
- **Creation**: New file with just the feature's data works?
- **Merge**: Adding to existing file with other content preserves everything?
- **Update**: Changing the value overwrites correctly?
- **Deletion**: Removing the value cleans up correctly? Empty sections removed?
- **Orphaned data**: Instance deleted -> file deleted -> no orphaned data?
### 8. Audit VFS Echo Prevention
The suppression system uses per-path counters. Verify:
- **Updates to existing files**: MUST call `suppress_path()` before `fs::write()`
- **New instance additions**: Must NOT suppress (VFS watcher needs to pick up new files)
- **All write branches**: Every branch that writes a file must be checked
### 9. Audit CLI Syncback Parity
Compare two-way sync output with CLI syncback output:
- Same data format on disk?
- Same file placement (attributes section vs. properties)?
- Same nil handling?
- Shared helpers used consistently?
- Any divergence is a bug per `atlas.mdc`
### 10. Audit Wire Format
- **Msgpack serialization**: Does the data type survive the plugin -> server msgpack round-trip?
- **Type parsing**: Does the server's deserializer correctly parse the wire format?
- **Edge values**: Does nil/zero/empty round-trip correctly?
### 11. Check Sync-Specific Patterns
These are patterns specific to Atlas's sync system that every audit must verify:
#### 11a. The 4 File Format Branches
`syncback_updated_properties` has 4 branches for writing property changes. EVERY audit must verify the feature works in ALL 4:
1. **Directory format**: `init.meta.json5` inside the instance's directory
2. **Standalone script**: adjacent `ScriptName.meta.json5` (strips `.server`/`.client`/`.plugin`/`.local`/`.legacy` suffix from stem)
3. **Model file**: inline in `.model.json5` (writes directly into the file, NOT adjacent meta)
4. **Other file types** (`.txt`, `.csv`, `.toml`): adjacent `FileName.meta.json5`
Common bug: treating model files like scripts (writing adjacent meta instead of inline).
#### 11b. Compound File Extensions
`.server.luau`, `.client.luau`, `.model.json5`, `.meta.json5`, `.project.json5` are multi-part extensions that need special handling:
- **Rename**: Must preserve the compound extension. The suffix list in `change_processor.rs` must include all compound parts. Missing `.model` from the suffix list caused a critical bug where `.model.json5` became `.json5` on rename.
- **Meta pairing**: `Foo.server.luau` pairs with `Foo.meta.json5` (NOT `Foo.server.meta.json5`). Script suffixes are stripped from the stem before computing the meta path.
- **Model files store `name` inline**: `.model.json5` has `"name"` in the root JSON object. `.meta.json5` has `"name"` as a top-level field. These are different locations.
#### 11c. InstigatingSource
Every instance in the RojoTree tracks where it came from:
- `InstigatingSource::Path(PathBuf)` -- created from a file, can be written back
- `InstigatingSource::ProjectNode { path, name, node, parent_class }` -- defined in project file, CANNOT be modified via two-way sync
**Verify:** Does the feature correctly guard against writing to ProjectNode instances? Is `InstigatingSource` set correctly for new instances created by the feature?
#### 11d. Pre-seeding Dedup Sets
When adding instances, `sibling_slugs` (or `taken_names`) must be seeded from existing tree children via their `instigating_source` paths (not instance names). This prevents slug collisions with existing files on disk.
**Verify:** Does the feature seed the dedup set correctly? Does it use `strip_middleware_extension` to derive the dedup key from filesystem paths?
#### 11e. Init File Detection
`init.luau`, `init.server.luau`, etc. inside directories make the directory represent the script instance. When auditing:
- **Renames**: Renaming a directory instance must rename the directory AND its init file path. The meta file is `init.meta.json5` inside the directory.
- **Deletions**: Deleting a directory instance deletes the entire directory. Also clean up adjacent `DirName.meta.json5` at the grandparent level.
- **Property writes**: Properties for directory instances go into `init.meta.json5` inside the directory, NOT an adjacent file.
#### 11f. Atomicity and File Watcher Races
When an operation involves multiple filesystem changes (delete old file + create new file, or rename + meta update), there is a window where the VFS watcher could fire between operations and produce incorrect intermediate state.
**Verify:** Are multi-step operations ordered to minimize race windows? Does `suppress_path` cover all intermediate states? Could the ChangeProcessor snapshot an inconsistent state?
#### 11g. Determinism
Given the same instance tree, does the feature ALWAYS produce the same filesystem output?
- Is child ordering stable (sorted, not random)?
- Are generated identifiers (dedup suffixes, IDs) deterministic?
- Non-determinism = different builds from the same source = git churn = invariant violation.
#### 11h. Syncback Idempotency
Running the operation twice should produce zero changes on the second run. If re-running writes the same files again (even with identical content), something is unstable.
#### 11i. Edge Cases
- **ProjectNode instances**: Can the feature interact with instances defined in project files? Is there a guard?
- **Nested projects**: Does data span project boundaries correctly?
- **Concurrent changes**: Multiple changes in one batch handled correctly?
- **Non-existent targets**: Graceful handling when referenced data doesn't exist?
- **Name conflicts**: Instance names with special characters (`/`, `:`, `?`, `*`, `<`, `>`, `|`, `"`, `\`) that affect the feature?
- **Script type transitions**: ClassName changes (ModuleScript -> Script) cause file extension changes. Does the feature handle this?
- **One-shot mode**: Does `Settings:get("oneShotSync")` correctly block the feature's outgoing writes?
### 12. Run Static Analysis
Run `cargo clippy` on the modified code to catch Rust issues the audit might miss:
```bash
cargo clippy --all-targets 2>&1
```
Focus on warnings in files modified by the feature. Clippy catches: unused variables, dead code, redundant clones, suspicious patterns, missing error handling, and type conversion issues that could silently lose data.
Also run `selene plugin/src` for Lua linting if plugin files were modified.
### 13. Write Tests for Every Finding
Tests are not optional. Every issue found during the audit must have test coverage. This applies to ALL test layers:
- **Rust unit tests** (`#[cfg(test)]` blocks) -- for isolated logic (helpers, path computation, patch compute/apply)
- **Rust integration tests** (`tests/tests/`) -- for end-to-end server behavior (serve tests, two-way sync via `/api/write`, connected mode, syncback)
- **Lua spec tests** (`.spec.lua` files) -- for plugin-side encoding, decoding, batching, reconciliation
- **Snapshot tests** (`insta` crate) -- for forward-sync patch output verification
#### 13a. Tests for bugs found and fixed
Every bug fixed during the audit MUST have a test that:
1. Would have FAILED before the fix
2. PASSES after the fix
3. Prevents the bug from regressing
#### 13b. Tests for missing coverage identified
For each "missing test coverage" gap identified in the audit:
1. Write the test
2. Verify it passes against the current implementation
3. If it fails, investigate -- it may have found another bug
#### 13c. Tests for known limitations
For each known limitation that violates round-trip identity:
1. Write a test that asserts the **correct** behavior
2. The test SHOULD FAIL against the current implementation
3. **Leave the test failing** -- do NOT mark it `#[ignore]` or fix the implementation
4. If a test unexpectedly PASSES, analyze why. If correct, keep it as free coverage
5. These tests serve as acceptance criteria for future fixes
#### 13d. 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 `/api/write` + filesystem verification
- Does it affect forward-sync? -> Serve test with `recv_socket_packet`
- Could it regress from a different code path? -> Test that code path too
### 14. Challenge Known Limitations
Do NOT blindly accept "known limitations" from implementation plans. For every limitation that violates round-trip identity:
1. **Present each limitation to the user** using the AskQuestion tool. State:
- What the limitation is
- The concrete scenario where data is lost or corrupted
- The severity (how likely is this to hit users?)
- What fixing it would require
2. **Ask the user to explicitly confirm** it's acceptable for this release, or whether it should be fixed now.
3. **Never silently skip a round-trip violation** just because a plan document says "known limitation." Plans are written before implementation -- the user may have changed their mind, or the severity may be worse than anticipated.
### 15. Produce the Report
Structured report with:
- **Critical issues** -- data loss, incorrect values, silent corruption on round-trip
- **Correctness concerns** -- edge cases that might not manifest immediately
- **Missing test coverage** -- specific test cases needed, prioritized by risk
- **Challenged limitations** -- for each limitation: what it is, user's decision (accept/fix), and if accepted, confirmation that warnings are logged and no silent corruption occurs beyond the accepted scope
- **Code quality items** -- DRY violations, dead code, fragile error paths
- **Deferred refactors** -- major structural improvements too large to do inline
- For each issue: **file path, line numbers, description, and suggested fix**

420
.cursor/plans/audit_ref_two-way_sync_9e8ce7a8.plan.md Normal file
View file

@ -0,0 +1,420 @@
---
name: Audit Ref Two-Way Sync
overview: "Production-grade audit of the two-way sync Ref property feature. The invariant: for every Ref property change (set, change target, set to nil), the filesystem must represent the Ref as a Rojo_Ref_* path-based attribute such that building an rbxl from the filesystem and forward-syncing it back produces a bit-identical Ref target. Every audit item is evaluated against this round-trip fidelity standard."
todos:
- id: round-trip-identity
content: "Verify round-trip identity for all Ref lifecycle paths: set, nil, change target, ObjectValue.Value, script ref, stale path after rename"
status: completed
- id: plugin-encoding
content: "Audit plugin-side encoding: propertyFilter, encodePatchUpdate, createPatchSet, encodeInstance, ChangeBatcher for Ref correctness"
status: completed
- id: server-resolution
content: "Audit server-side path resolution: syncback_updated_properties, filter_properties_for_meta, merge_or_build_meta, handle_api_write nil Ref conversion"
status: completed
- id: forward-sync
content: "Audit forward-sync: compute_ref_properties, patch_apply defer/finalize, decodeValue.lua for Rojo_Ref_* attribute resolution"
status: completed
- id: existing-ref-systems
content: "Audit interaction with legacy Rojo_Target_*/Rojo_Id system: coexistence, priority, DRY concerns"
status: completed
- id: meta-lifecycle
content: "Audit meta file lifecycle with Refs: creation, merge, update, deletion, orphaned attributes"
status: completed
- id: path-correctness
content: "Audit path computation: full_path_of/get_instance_by_path inverse consistency, instance names containing /, deep nesting, root target"
status: completed
- id: encode-instance-leak
content: "Audit encodeInstance.lua Ref leak: Ref no longer in UNENCODABLE_DATA_TYPES means Refs reach encodeProperty and fail via pcall instead of explicit filter"
status: completed
- id: vfs-echo
content: Verify suppress_path() is called on all meta/model file writes with Rojo_Ref_* attributes (all 4 format branches)
status: completed
- id: syncback-parity
content: "Verify two-way sync Ref output matches CLI syncback output: same attribute format, same path format, same nil handling, shared helpers"
status: completed
- id: msgpack-wire
content: "Verify Variant::Ref msgpack round-trip: plugin { Ref = hexid } deserializes to correct Ref, Ref::none() round-trips as Ref(None)"
status: completed
- id: edge-cases
content: "Audit edge cases: ProjectNode targets, nested projects, concurrent changes, non-existent property names, multiple instances"
status: completed
- id: known-limitations
content: "Verify known limitations are documented and handled gracefully: ambiguous paths, stale paths, no immediate ID for new instances"
status: completed
- id: prove-stale-path
content: "Write ignored regression tests proving stale-path-after-rename bug: set Ref, rename target, assert stored path updated (should fail)"
status: completed
- id: prove-untracked-ref
content: "Write ignored regression tests proving untracked-instance-ref-dropped bug: add instance + set Ref to it in same batch, assert Ref persisted (should fail)"
status: completed
- id: prove-missing-instancemap
content: "Write ignored Lua spec test proving ServeSession.lua:738 missing instanceMap arg: call encodePatchUpdate without 4th arg, assert Ref encoded (should fail)"
status: completed
- id: test-coverage
content: Review test coverage against audit areas, flag gaps with specific test descriptions
status: completed
- id: write-report
content: "Produce final audit report: critical issues, correctness concerns, missing coverage, known limitations, code quality items"
status: completed
isProject: false
---
# Audit: Two-Way Sync Ref Properties -- Round-Trip Fidelity
## 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.
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.
---
## 1. Round-Trip Identity Verification
Trace the **complete lifecycle** of a Ref property through every path.
### 1a. Set PrimaryPart Round-Trip
Concrete example: `Model.PrimaryPart = Part1` (Part1 is at `Workspace/TestModel/Part1`).
- **Plugin encodes**: reads `instance.PrimaryPart` -> Instance -> `instanceMap.fromInstances[Part1]` -> server Ref ID -> `{ Ref = "hexid" }`
- **Server receives**: `Variant::Ref(part1_ref)` in `changed_properties`
- **Server writes**: `syncback_updated_properties` extracts Ref, computes path via `full_path_of`, writes `Rojo_Ref_PrimaryPart = "Workspace/TestModel/Part1"` to `init.meta.json5`
- **Server applies**: PatchSet includes `PrimaryPart = Variant::Ref(part1_ref)` to in-memory tree
- **Forward sync reads**: `compute_ref_properties` finds `Rojo_Ref_PrimaryPart` attribute, calls `get_instance_by_path("Workspace/TestModel/Part1")`, resolves to `Variant::Ref`
- **Verify**: Is the resolved Ref the same target instance? Does `full_path_of` produce a path that `get_instance_by_path` can resolve back? Any off-by-one in root handling?
### 1b. Nil PrimaryPart Round-Trip
`Model.PrimaryPart = nil`:
- **Plugin encodes**: reads nil -> `{ Ref = "000...0" }`
- **Server receives**: `Variant::Ref(Ref::none())`
- **handle_api_write converts**: nil Ref -> `None` (property removal) in PatchSet
- **Server writes**: `remove_attributes` includes `"Rojo_Ref_PrimaryPart"`
- **Server applies**: property removed from in-memory tree
- **Forward sync reads**: no `Rojo_Ref_PrimaryPart` attribute -> no PrimaryPart property
- **Verify**: Is the nil Ref -> `None` conversion in `handle_api_write` correct? Does `merge_or_build_meta` actually remove the attribute? Does `remove_attributes` run BEFORE `new_attributes` merge (ordering matters if both lists have the same key)?
### 1c. Change Target Round-Trip
`Model.PrimaryPart = Part1` then `Model.PrimaryPart = Part2`:
- First write: `Rojo_Ref_PrimaryPart = "Workspace/TestModel/Part1"`
- Second write: `merge_or_build_meta` overwrites attribute to `"Workspace/TestModel/Part2"`
- **Verify**: Does the merge correctly overwrite? Or does the old value persist alongside the new? Is the attribute key case-sensitive?
### 1d. ObjectValue.Value Round-Trip
`ObjectValue.Value = SomePart` where ObjectValue is defined in `.model.json5`:
- **Server writes**: attribute goes into the `.model.json5` file directly (is_model_file = true)
- **Verify**: Does `merge_or_build_meta` correctly add `Rojo_Ref_Value` to a model file's attributes? Does the snapshot middleware read attributes from `.model.json5` the same way as from `.meta.json5`?
### 1e. Ref on Script Round-Trip
Script has a custom Ref property set via two-way sync:
- **Server writes**: adjacent `ScriptName.meta.json5` gets the `Rojo_Ref_*` attribute
- **Verify**: Is the meta file path computed correctly (strip `.server`/`.client` suffix from stem)? Does the adjacent meta correctly pair with the script file on read?
### 1f. Stale Path After Rename
Set `Model.PrimaryPart = Part1`, then rename Model from `TestModel` to `RenamedModel`:
- Meta file written with `Rojo_Ref_PrimaryPart = "Workspace/TestModel/Part1"`
- ChangeProcessor renames directory to `RenamedModel`
- Path in attribute is now stale
- **Verify**: What happens on rebuild? `get_instance_by_path("Workspace/TestModel/Part1")` returns None because `TestModel` no longer exists. PrimaryPart silently becomes nil. Is this documented as a known limitation? Is a warning logged?
---
## 2. Plugin-Side Encoding Completeness
Every code path in the plugin that encodes property changes must correctly handle Ref properties.
- [propertyFilter.lua](plugin/src/ChangeBatcher/propertyFilter.lua): `Ref` removed from `UNENCODABLE_DATA_TYPES`? Only `UniqueId` remains?
- [encodePatchUpdate.lua](plugin/src/ChangeBatcher/encodePatchUpdate.lua):
- `descriptor.dataType == "Ref"` branch handles nil, valid, and unresolvable cases?
- `descriptor:read(instance)` correctly reads Ref properties on all instance types (Model.PrimaryPart, ObjectValue.Value, Weld.Part0)?
- Does the `instanceMap.fromInstances[readResult]` lookup use the Instance as key (not a string)?
- When `instanceMap` is nil (should never happen but defensively), does the code log and skip without crashing?
- [createPatchSet.lua](plugin/src/ChangeBatcher/createPatchSet.lua): `instanceMap` passed to `encodePatchUpdate` in both normal and `syncSourceOnly` paths?
- [encodeInstance.lua](plugin/src/ChangeBatcher/encodeInstance.lua): Still filters out Ref properties during instance addition encoding? (Refs on newly added instances are NOT encoded -- they'd have no target ID yet)
- **Edge cases**:
- Instance's `Changed` event fires for Ref property but the target was just destroyed -> `readResult` is nil -> correctly encoded as null ref?
- ValueBase instances (ObjectValue) -> `GetPropertyChangedSignal("Value")` fires, `descriptor:read` returns the Instance reference?
---
## 3. Server-Side Path Resolution Completeness
Every code path in [api.rs](src/web/api.rs) that handles Ref properties must correctly compute paths.
- `syncback_updated_properties`:
- Ref extraction happens BEFORE `filter_properties_for_meta`?
- `crate::ref_attribute_name(key)` produces correct attribute name?
- `crate::ref_target_path(tree.inner(), *target_ref)` produces correct path?
- `tree.get_instance(*target_ref).is_some()` correctly checks target existence?
- Nil Ref -> `remove_attributes` list, not `ref_attributes` map?
- Target not in tree -> warning logged, not in either list?
- Ref entries removed from `props` before `filter_properties_for_meta`? (verify Refs don't fall through to `variant_to_json` which returns None)
- `filter_properties_for_meta`:
- `Variant::Ref` is no longer skipped? Only `Variant::UniqueId` is skipped?
- If a `Variant::Ref` somehow reaches `variant_to_json`, it returns `None` (safe fallback)?
- `merge_or_build_meta`:
- `remove_attributes` processed BEFORE `new_attributes` merge? (important: if the same key is in both, the add should win)
- Removal works on existing files (parses JSON5, modifies object, writes back)?
- Removal is no-op on non-existent files?
- Removal is no-op when the attribute doesn't exist in the file?
- `handle_api_write`:
- Nil Refs (`Ref::none()`) converted to `None` (property removal) in the PatchSet? (the fix we applied)
- Conversion happens for ALL updated instances, not just some?
- Non-nil Refs pass through unchanged?
---
## 4. Forward-Sync Resolution Completeness
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?
- 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?
- `None` return for unresolvable paths -- does this cause property removal on the tree instance? (could silently clear a Ref that was set during the session but hasn't been written to disk yet)
- [patch_apply.rs](src/snapshot/patch_apply.rs):
- `defer_ref_properties` correctly collects `Rojo_Ref_*` attributes for later resolution?
- `finalize_patch_application` resolves path refs via `get_instance_by_path`?
- Resolved refs are set as `Variant::Ref` on the instance?
- `Rojo_Ref_*` attributes are cleaned from the Attributes property after resolution? (they should NOT appear as instance attributes in Studio)
- [decodeValue.lua](plugin/src/Reconciler/decodeValue.lua):
- `Ref` type correctly handled: resolves via `instanceMap.fromIds[value]`?
- Null ref (`"000...0"`) returns nil?
- Unknown ID returns error?
---
## 5. Interaction with Existing Ref Systems
The new `Rojo_Ref_*` system coexists with the legacy `Rojo_Target_*` + `Rojo_Id` system.
- [ref_properties.rs](src/syncback/ref_properties.rs): CLI syncback still uses both path-based (`Rojo_Ref_*`) and ID-based (`Rojo_Target_*` + `Rojo_Id`) systems?
- The shared `ref_attribute_name` helper is used for path-based refs?
- The `Rojo_Target_*` formatting still uses inline `format!` (not shared)?
- Is this inconsistency a DRY concern?
- `link_referents`: Old attributes (`Rojo_Ref_*` and `Rojo_Target_*`) are filtered out before re-inserting? No duplicate attributes?
- `compute_ref_properties` in patch_compute.rs: handles BOTH `Rojo_Ref_*` and `Rojo_Target_*`? Both resolve correctly? Priority if both exist for the same property?
- Two-way sync: `filter_properties_for_meta` no longer skips `Variant::Ref` -- does this mean `Variant::Ref` values could reach `variant_to_json` if a Ref property change comes from a path OTHER than `syncback_updated_properties`? Are there other callers?
---
## 6. Meta File Lifecycle with Refs
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?
- **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.
---
## 7. Path Computation Correctness
The path computation is the heart of the system. Any bug here silently corrupts Ref targets.
- `ref_target_path` calls `dom.full_path_of(target_ref, "/")`:
- Root instance -> empty string ""? Does `get_instance_by_path("")` correctly resolve to root?
- Single depth -> `"Workspace"`? Resolves back?
- Deep nesting -> `"Workspace/Model/SubFolder/Part"`? Resolves back?
- Instance name contains `/` -> path becomes ambiguous! `full_path_of` uses `/` as separator but instance names can contain `/` (slugified in filesystem but NOT in the tree). **This could be a critical issue.** A Part named `"A/B"` at `Workspace` would produce path `"Workspace/A/B"` which resolves to `Workspace > A > B` (3 levels) instead of `Workspace > "A/B"` (2 levels).
- Instance name contains special chars -> path may not match filesystem slug. But paths are computed from tree names, not filesystem names, so this should be OK for resolution within the same session. Cross-session, paths are stored on disk and resolved from the tree -- tree names should be consistent.
---
## 7b. Known Potential Issue: Missing instanceMap in ServeSession Pull Path
**File:** [ServeSession.lua](plugin/src/ServeSession.lua) line 738
`encodePatchUpdate` was updated to accept an `instanceMap` parameter (4th arg) for Ref encoding. `createPatchSet.lua` passes it correctly. However, the call in `ServeSession.lua` line 738 was NOT updated:
```lua
local update = encodePatchUpdate(instance, change.id, propertiesToSync)
```
This is the "pull" path in `__confirmAndApplyInitialPatch` -- when users select "pull" in the initial patch confirmation dialog to sync Studio property changes back to Atlas. The missing 4th argument means `instanceMap` is `nil` inside `encodePatchUpdate`, causing all Ref property changes (PrimaryPart, ObjectValue.Value, constraint refs, etc.) to be silently dropped with a warning instead of encoded. This is a round-trip identity violation for the initial sync pull path.
**Expected fix:** Pass `self.__instanceMap` as the 4th argument:
```lua
local update = encodePatchUpdate(instance, change.id, propertiesToSync, self.__instanceMap)
```
**Audit should verify:** Are there any OTHER callers of `encodePatchUpdate` beyond `createPatchSet.lua` and `ServeSession.lua:738` that are also missing the `instanceMap` argument?
---
## 7c. encodeInstance.lua Ref Leak
**File:** [encodeInstance.lua](plugin/src/ChangeBatcher/encodeInstance.lua)
`encodeInstance.lua` imports `UNENCODABLE_DATA_TYPES` from `propertyFilter.lua` and uses it to filter properties before calling `encodeProperty`. We removed `Ref = true` from that filter. Now Ref properties on newly added instances pass through to `encodeProperty`, which calls `RbxDom.EncodedValue.encode(readResult, dataType)`. For Ref properties, `readResult` is a Roblox Instance, and `RbxDom.EncodedValue.encode` likely throws (Instance is not an encodable value). The `pcall` wrapper catches the error and the property is silently dropped.
**The behavior is correct** (you can't encode a Ref without the InstanceMap during instance addition), **but the code path is fragile**. It relies on `pcall` catching an error thrown by `RbxDom.EncodedValue.encode` rather than an explicit check. If `RbxDom.EncodedValue.encode` is updated to handle Refs in the future, the behavior would change silently.
**Audit should verify:** Does `encodeInstance.lua` need its own Ref-specific filter (like `encodePatchUpdate.lua` has), or is the pcall fallback acceptable?
---
## 7d. VFS Echo Prevention for Ref Meta Writes
**File:** [api.rs](src/web/api.rs) `syncback_updated_properties()`
The suppression system uses per-path counters: `suppress_path()` increments a create/write counter, `suppress_path_remove()` increments a remove counter. The change processor checks and decrements these when VFS events arrive.
**Modern pattern:**
- **Updates to existing files** (meta writes, model updates, source writes): MUST suppress to prevent feedback loops
- **New instance additions** (`syncback_added_instance`): do NOT suppress -- the VFS watcher must pick up new files to add them to the tree
- **Folder creation**: suppression varies by context (conversion vs. new creation)
**Verify for Ref changes specifically:** All four file format branches in `syncback_updated_properties` call `suppress_path()` before `fs::write()`:
1. Directory init.meta.json5 (line ~2630)
2. Adjacent script .meta.json5 (line ~2663)
3. Inline .model.json5 (line ~2690)
4. Adjacent non-script .meta.json5 (line ~2713)
These are all updates to existing instances (not additions), so suppression is correct. Verify no branch was missed and that the `suppress_path` calls use the correct path (the path being written, not the instance path).
---
## 7e. CLI Syncback Parity
**From atlas.mdc:** "Plugin-based sync must produce exactly the same filesystem output that atlas syncback would give for the same input. Byte-for-byte identical files."
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 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)
- Both use the shared `ref_attribute_name()` helper from [rojo_ref.rs](src/rojo_ref.rs)
Any divergence between the two paths is a bug per atlas.mdc.
---
## 7f. Msgpack Wire Format for Refs
The plugin sends `{ Ref = "hexid" }` via msgpack to the server. The server deserializes this as `Variant::Ref(Ref)`.
**Verify:** Does the msgpack deserialization in `handle_api_write` correctly round-trip `Variant::Ref` values? Check that `Ref::from_str` parses the hex string from the plugin and produces the correct `NonZeroU128`. Check that `Ref::none()` (hex `"000...0"`) deserializes as `Ref(None)`.
---
## 8. Edge Cases and Error Handling
- **Ref to ProjectNode instance**: Can a Ref property target an instance defined in a project file? Path resolution should work since ProjectNode instances ARE in the tree. But if the target is removed from the project file, the path becomes invalid.
- **Ref to instance in nested project**: Path computed by `full_path_of` spans the entire tree. But nested projects have their own sync boundaries. Is the path still valid after a project reload?
- **Concurrent Ref changes**: Two Ref properties changed in the same batch -> both written to meta file? Does `merge_or_build_meta` handle multiple attribute additions in one call?
- **Ref property that doesn't exist in reflection**: If the plugin sends a Ref for a property name that doesn't exist in rbx_reflection -> attribute is written but never resolved on forward-sync?
- **Multiple instances with same Ref property change**: Different instances, each with PrimaryPart changes -> each gets their own meta file written correctly?
---
## 9. Known Limitations (Document, Don't Fix)
These were explicitly accepted in the implementation plan. Confirm they are documented, log appropriate warnings, and do NOT produce silent data corruption.
- **Ambiguous paths**: Duplicate-named siblings make paths non-unique. Path-based refs may resolve to wrong sibling. No crash, but round-trip violation.
- **Stale paths after rename**: Stored path becomes invalid when target is renamed/moved.
- **No immediate ID for new instances**: Pulled instances don't get IDs until VFS watcher processes files. Ref properties targeting new instances are dropped with warning.
---
## 10. Prove Known Limitations (Regression Tests Left Failing)
Write tests that **assert the correct round-trip behavior** for the known limitations. These tests SHOULD FAIL against the current implementation, proving the bugs are real. **Leave them failing -- do NOT mark them `#[ignore]` or `SKIP` or try to fix the implementation to make them pass.** They document real violations of the round-trip invariant and will serve as acceptance criteria when fixes are implemented later.
If a test unexpectedly PASSES, analyze why. If the behavior is genuinely correct, leave the test -- it's free coverage.
### 10a. Stale Path After Target Rename (Rust integration)
In `tests/tests/two_way_sync.rs`:
1. Set `Model.PrimaryPart = Part1` via `/api/write`
2. Wait for `Rojo_Ref_PrimaryPart = "Workspace/TestModel/Part1"` in meta file
3. Rename `Part1` to `RenamedPart` via `/api/write` (changed_name)
4. Wait for rename to process
5. Read meta file -- assert `Rojo_Ref_PrimaryPart` is now `"Workspace/TestModel/RenamedPart"`
6. EXPECTED: FAIL (stored path is stale)
Add variations:
- Rename the MODEL (parent) instead of the target Part
- Rename an ancestor higher in the tree
### 10b. Ref to Instance Added in Same Request (Rust integration)
In `tests/tests/two_way_sync.rs`:
1. Add a new Part via `/api/write` (added instances) with a temp GUID key
2. In the SAME write request, set `Model.PrimaryPart = newPartRef` (using the temp GUID)
3. Assert that `Rojo_Ref_PrimaryPart` appears in the meta file with the correct path to the new Part
4. EXPECTED: FAIL (target not in tree when `syncback_updated_properties` runs)
### 10c. Ref to Untracked Instance Dropped (Lua spec)
In `plugin/src/ChangeBatcher/encodePatchUpdate.spec.lua`:
1. Create a Model and a Part in Studio
2. Set `Model.PrimaryPart = Part`
3. Insert Model into InstanceMap but NOT Part
4. Call `encodePatchUpdate` with `PrimaryPart` change and the instanceMap
5. Assert that PrimaryPart IS encoded (with Part's server ID)
6. EXPECTED: FAIL (Part not in InstanceMap, property dropped with warning)
### 10d. ServeSession Pull Path Missing instanceMap (Lua spec)
In `plugin/src/ChangeBatcher/encodePatchUpdate.spec.lua`:
1. Create a Model with PrimaryPart set to a tracked Part (both in InstanceMap)
2. Call `encodePatchUpdate(model, "MODEL_ID", { PrimaryPart = true })` -- WITHOUT the 4th instanceMap arg
3. Assert that PrimaryPart IS encoded as `{ Ref = serverId }`
4. EXPECTED: FAIL (instanceMap is nil, triggers warning path, property dropped)
---
## 11. Test Coverage Review
Evaluate existing tests against the audit areas above. For each gap, flag as missing coverage with specific test description.
- **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?
- **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?
---
## 12. Deliverables
Produce a structured report with:
- **Critical issues** -- bugs that would cause incorrect Ref targets, lost Refs, or silent data corruption on round-trip
- **Correctness concerns** -- logic that might not manifest immediately but could cause problems (e.g., the `/` in instance names issue in section 7)
- **Missing test coverage** -- specific test cases needed, prioritized by risk
- **Known limitations** -- confirmed as documented and handled gracefully (warning logged, no crash, no silent corruption beyond the documented scope)
- **Code quality items** -- DRY violations, dead code, stale comments
- For each issue: file path, line numbers, description, and suggested fix