Add CI pipeline documentation and enhance syncback architecture

- Introduced a new CI pipeline documentation file outlining steps for auto-fixing formatting, running linters, building, and testing.
- Documented the Rojo syncback architecture, detailing improvements to prevent duplicate file creation and format mismatches.
- Updated syncback logic to ensure existing instances are updated instead of creating duplicates, enhancing project file management.
- Added tests for encoding instances and handling nested children to improve reliability in syncback operations.
This commit is contained in:
Joe 2026-02-01 18:18:31 -06:00
parent edc09741bf
commit b368eb2401
46 changed files with 3688 additions and 112 deletions

74
.cursor/commands/ci.md Normal file
View file

@ -0,0 +1,74 @@
# /ci - Complete CI Pipeline
Run the complete CI pipeline: auto-fix all formatting, run all linters, build everything, and run ALL tests.
## Instructions
Execute ALL of the following steps in order. Fix issues automatically where possible.
### 1. Auto-fix Lua Formatting (Stylua)
```powershell
stylua plugin/src
```
### 2. Auto-fix Rust Formatting
```powershell
cargo fmt
```
### 3. Lua Static Analysis (Selene)
```powershell
selene plugin/src
```
If there are errors, report them and offer to help fix.
### 4. Rust Linting (Clippy) - Auto-fix where possible
```powershell
cargo clippy --fix --allow-dirty --allow-staged
```
Then verify with:
```powershell
cargo clippy --all-targets --all-features
```
If there are remaining warnings/errors, report them and offer to help fix.
### 5. Build Everything
```powershell
cargo build --locked --all-targets --all-features
```
### 6. Run ALL Rust Tests
```powershell
cargo test --locked --all-targets --all-features
```
### 7. Run Roblox Plugin Tests (requires Roblox Studio)
```powershell
.\scripts\unit-test-plugin.ps1
```
## Reporting
After ALL steps complete, provide a summary:
```
=== CI COMPLETE ===
Formatting:
- Stylua: [fixed X files / no changes needed]
- Rustfmt: [fixed X files / no changes needed]
Linting:
- Selene: PASS / FAIL (X issues)
- Clippy: PASS / FAIL (X warnings)
Build: PASS / FAIL
Tests:
- Rust: X passed, Y failed
- Plugin: X passed, Y failed, Z skipped
Overall: PASS / FAIL
```
If any step fails, continue running the remaining steps to get a complete picture, then report all failures at the end.

View file

@ -0,0 +1,441 @@
---
name: Rojo Syncback Architecture Fix
overview: Document Rojo's two-way sync architecture and fix the plugin API syncback to properly consult the tree/VFS instead of relying solely on plugin data, preventing duplicate file creation and format mismatches.
todos:
- id: tree-lookup
content: Add tree-based instance lookup in syncback_added_instance() to find existing instances by name+parent before deciding file format
status: completed
- id: use-instigating-source
content: When existing instance found in tree, write to its instigating_source.path instead of creating new file
status: completed
- id: verify-plugin-children-tracking
content: Verify plugin encodeInstance correctly includes/excludes children and test children deletion scenarios
status: completed
- id: test-plugin-cli-parity
content: Create test verifying plugin syncback produces same result as CLI syncback for identical Studio state
status: completed
isProject: false
---
# Rojo Syncback Architecture Fix
## Goal
Enable perfect two-way synchronization between Roblox Studio and the filesystem via the Rojo plugin, without requiring constant `rojo serve` restarts and manual `rojo syncback` operations. The plugin should accurately keep the project up to date as if running `rojo syncback` from scratch.
This builds on the [Pull Instances to Rojo Feature](../.cursor/plans/pull_removed_scripts_feature_dd790311.plan.md) which added support for pulling ALL instance types from Studio to Rojo.
---
## Background: How Two-Way Sync Works
### Original Design (commit a398338)
The original two-way sync only handled **property updates**:
```
Plugin ──[WriteRequest.updated]──> API Server
│
▼
tree_mutation_sender
│
▼
ChangeProcessor.handle_tree_event()
│
▼ (for each update)
fs::write(instigating_source.path, new_value)
│
▼
VFS detects file change
│
▼
Tree updated from VFS snapshot
```
Key insight: **The tree knows the correct file path via `instigating_source**`. Updates write to that path, preserving the existing file structure.
The original `/api/write` only handled `updated` instances (property changes). It did NOT handle `added` or `removed` instances - those were delegated to CLI syncback.
### Extended Design (Pull Instances Feature)
We extended `/api/write` to handle three types of operations:
| Operation | Plugin sends | Server action |
| ---------------------- | ------------------------ | ------------------------------------------------------ |
| **Update** | `WriteRequest.updated[]` | Write to `instigating_source.path` via ChangeProcessor |
| **Pull (create file)** | `WriteRequest.added{}` | Create new files via `syncback_added_instance()` |
| **Pull (delete file)** | `WriteRequest.removed[]` | Delete files via `syncback_removed_instance()` |
---
## How the Plugin Encodes Instances
### encodeInstance.lua
When the plugin sends an instance via "Pull", it uses `encodeInstance()` which:
1. **Recursively includes ALL children** (lines 247-268)
2. **Filters duplicate-named siblings** (can't reliably sync)
3. **Encodes all properties** (Attributes, Tags, etc.)
```lua
-- From plugin/src/ChangeBatcher/encodeInstance.lua
return {
parent = parentId,
name = instance.Name,
className = instance.ClassName,
properties = properties,
children = children, -- ALL children recursively encoded
}
```
### When Plugin Sends Data
| Scenario | Plugin action | Data sent |
| -------------------------- | ---------------- | ------------------------------------------- |
| Property edit (Source) | `createPatchSet` | `updated[]` with changed properties only |
| User "Pulls" removed item | `encodeInstance` | `added{}` with FULL instance + all children |
| Instance deleted in Studio | `createPatchSet` | `removed[]` with instance ID |
---
## The Core Problems
### Problem 1: Server ignores existing tree state for "added" instances
When plugin sends an "added" instance, the server decides file format based on `has_children` from the plugin data, NOT the tree state:
```
Existing tree: EventService -> instigating_source: "EventService/init.luau"
Plugin sends: AddedInstance { name: "EventService", children: [...] }
(user pulled it because it appeared as "to delete")
Current behavior: Creates NEW files at EventService/init.luau
(even though files already exist!)
```
### Problem 2: Why items appear as "to delete" when files exist
If duplicate files exist (e.g., `BombardilloEvent.luau` AND `BombardilloEvent/`):
1. Rojo can't determine which representation is correct
2. Skips loading these instances ("duplicate-named siblings")
3. Instance exists in Studio but NOT in Rojo tree
4. Appears as "to be deleted" in sync diff
5. User pulls it, creating MORE files
6. Cycle continues
### Problem 3: Format transitions (children added/removed)
| Situation | Expected file format | Current plugin behavior |
| ----------------------------- | -------------------- | ------------------------------------------ |
| ModuleScript with children | `Name/init.luau` | encodeInstance includes children correctly |
| ModuleScript children deleted | `Name.luau` | encodeInstance sends with `children: []` |
| User edits Source only | Keep existing format | Sent as UPDATE, not ADD - works correctly |
The UPDATE path (via ChangeProcessor) is correct. The ADD path needs to check tree state first.
---
## The Duplicate Creation Chain
```
1. Initial state: EventService/init.luau exists, works fine
2. Bug triggers: Something creates EventService.luau (duplicate)
- Could be: plugin syncback bug, manual creation, etc.
3. Rojo startup: Sees duplicate, can't load either
- Warning: "Skipped 2 location(s) with duplicate-named siblings"
4. Sync diff: EventService appears as "to be deleted"
- (Exists in Studio, not in Rojo tree due to duplicate skip)
5. User pulls: Plugin sends encodeInstance(EventService)
- Server creates MORE files at EventService/...
6. Result: Even more duplicates, cycle continues
```
---
## Architectural Principle: Tree is Source of Truth
For plugin syncback, the **tree/VFS** should be the source of truth for existing file structure, not:
- The raw filesystem (our current fix checks this, but it's indirect)
- The plugin data (incomplete/partial)
### Correct Flow for "Added" Instances
```
Plugin sends AddedInstance for "EventService"
│
▼
Does instance already exist in tree at this path?
│
┌───────┴───────┐
▼ ▼
YES NO
│ │
▼ ▼
Use tree's Use has_children
instigating_ to decide format
source.path (truly new instance)
│ │
▼ ▼
UPDATE existing CREATE new files
file in place with format based
on children
```
---
## Files and Their Roles
### Server-Side (Rust)
| File | Role | Status |
| -------------------------------------------------- | ------------------------------- | ----------------------------- |
| [src/web/api.rs](src/web/api.rs) | Plugin API `/api/write` handler | NEEDS FIX: tree lookup |
| [src/change_processor.rs](src/change_processor.rs) | Two-way sync UPDATE handler | OK: uses `instigating_source` |
| [src/syncback/mod.rs](src/syncback/mod.rs) | CLI syncback clean mode | FIXED: filesystem scan |
### Plugin-Side (Lua)
| File | Role | Status |
| ------------------------------------------------------------------------------------------ | ---------------------------- | ------------------------- |
| [plugin/src/ServeSession.lua](plugin/src/ServeSession.lua) | Orchestrates sync operations | OK: correct flow |
| [plugin/src/ChangeBatcher/encodeInstance.lua](plugin/src/ChangeBatcher/encodeInstance.lua) | Encodes instances for pull | OK: includes children |
| [plugin/src/ChangeBatcher/createPatchSet.lua](plugin/src/ChangeBatcher/createPatchSet.lua) | Creates patches for updates | OK: property changes only |
---
## Detailed Implementation
### 1. [src/web/api.rs](src/web/api.rs) - Tree Lookup for Added Instances
**Current approach**: `detect_existing_script_format()` checks raw filesystem
**Better approach**: Check the tree FIRST
```rust
fn syncback_added_instance(
&self,
added: &AddedInstance,
tree: &RojoTree,
// ...
) -> anyhow::Result<()> {
let parent_ref = added.parent.context("must have parent")?;
// NEW: Check if instance already exists in tree
if let Some(existing_ref) = find_child_by_name(tree, parent_ref, &added.name) {
if let Some(existing) = tree.get_instance(existing_ref) {
if let Some(source) = &existing.metadata().instigating_source {
// Instance exists - update in place instead of creating new
return self.syncback_update_existing(added, source.path());
}
}
}
// Instance doesn't exist - create new (existing code)
self.syncback_instance_to_path_with_stats(added, &parent_dir, stats)
}
fn find_child_by_name(tree: &RojoTree, parent: Ref, name: &str) -> Option<Ref> {
let parent_inst = tree.get_instance(parent)?;
parent_inst.children().iter()
.find(|&&child_ref| {
tree.get_instance(child_ref)
.map(|c| c.name() == name)
.unwrap_or(false)
})
.copied()
}
```
### 2. [src/syncback/mod.rs](src/syncback/mod.rs) - Already Fixed
The clean mode fix (scanning actual filesystem for `existing_paths`) is correct:
```rust
// In clean mode, scan the actual filesystem to find ALL existing files
let existing_paths: HashSet<PathBuf> = if !incremental {
let mut paths = HashSet::new();
// Recursively scan source directories
scan_directory(dir, &mut paths, &ignore_patterns, project_path);
paths
} else {
HashSet::new()
};
```
### 3. [src/change_processor.rs](src/change_processor.rs) - No Changes Needed
The UPDATE path correctly uses `instigating_source`:
```rust
for (key, changed_value) in &update.changed_properties {
if key == "Source" {
if let Some(InstigatingSource::Path(path)) = &instance.metadata().instigating_source {
fs::write(path, value)?; // Writes to correct existing file
}
}
}
```
---
## Implementation Tasks
### Task 1: Tree-Based Instance Lookup (Priority: HIGH)
**File**: [src/web/api.rs](src/web/api.rs)
In `syncback_added_instance()`, add tree lookup BEFORE format detection:
```rust
// Add helper function
fn find_child_by_name(tree: &RojoTree, parent_ref: Ref, name: &str) -> Option<Ref> {
let parent = tree.get_instance(parent_ref)?;
parent.children().iter()
.find(|&&child_ref| {
tree.get_instance(child_ref)
.map(|c| c.name() == name)
.unwrap_or(false)
})
.copied()
}
// In syncback_added_instance(), before filesystem operations:
if let Some(existing_ref) = find_child_by_name(tree, parent_ref, &added.name) {
if let Some(existing) = tree.get_instance(existing_ref) {
if let Some(source) = &existing.metadata().instigating_source {
// Found existing instance - write to its path, don't create new
log::info!("Syncback: Updating existing instance at {}", source.path().display());
return self.syncback_update_existing_instance(added, existing, source.path());
}
}
}
// Not found - continue with current new-instance creation logic
```
### Task 2: Handle Format Transitions (Priority: MEDIUM)
When instance has children in tree but plugin sends with empty children:
**Option A (Conservative - Recommended)**: Preserve existing format
- If tree has `Name/init.luau`, always update `Name/init.luau`
- User must use CLI syncback for format transitions
**Option B (Future)**: Add explicit flag
```rust
pub struct AddedInstance {
// ... existing fields ...
#[serde(default)]
pub force_format_change: bool, // If true, allow format transition
}
```
### Task 3: Verify Plugin Children Tracking (Priority: HIGH)
**File**: [plugin/src/ChangeBatcher/encodeInstance.lua](plugin/src/ChangeBatcher/encodeInstance.lua)
Create test scenarios:
1. **ModuleScript with children** - verify `children` array populated
2. **ModuleScript with children deleted** - verify `children: []`
3. **Nested children** - verify recursive encoding
4. **Duplicate siblings** - verify correctly skipped with warning
Add test file: `plugin/src/ChangeBatcher/encodeInstance.spec.lua`
### Task 4: Integration Test (Priority: MEDIUM)
Create end-to-end test:
```
1. Setup:
- Create EventService/init.luau with children
- Start rojo serve
2. Simulate plugin syncback:
- Send AddedInstance for EventService with children
3. Verify:
- No duplicate files created
- EventService/init.luau updated (not EventService.luau created)
4. Compare:
- Plugin syncback result == CLI syncback result
```
---
## Summary of Changes
### Already Fixed
| Issue | Fix | File |
| ----------------------------- | -------------------------------------- | --------------------------------- |
| reify.lua crash | Gracefully handle missing children IDs | `plugin/src/Reconciler/reify.lua` |
| CLI clean mode misses orphans | Scan actual filesystem | `src/syncback/mod.rs` |
| Plugin creates wrong format | Check filesystem for existing format | `src/web/api.rs` |
| Hidden services marked delete | Expose ignoreHiddenServices to plugin | `src/web/interface.rs`, `plugin/src/Reconciler/diff.lua` |
### Still Needed
| Issue | Fix | File |
| ------------------------ | -------------------------------------- | ---------------- |
| Ignores tree state | Add tree lookup before format decision | `src/web/api.rs` |
| Duplicate creation cycle | Update existing instead of create new | `src/web/api.rs` |
| Test coverage | Add integration tests | `tests/` |
---
## Design Principles
1. **Tree is source of truth** for existing file structure
2. **instigating_source.path** is THE path for an instance's file
3. **New instances** use `has_children` to decide format
4. **Existing instances** always update in place, never create duplicates
5. **Format transitions** only via CLI syncback (safe, explicit)
6. **Clean mode** scans filesystem to remove orphans tree doesn't know about
---
## File Format Reference
This applies to BOTH plugin syncback AND CLI syncback:
| Instance Type | No Children | With Children |
| ----------------- | ------------------ | ----------------------- |
| ModuleScript | `Name.luau` | `Name/init.luau` |
| Script (Server) | `Name.server.luau` | `Name/init.server.luau` |
| Script (Client) | `Name.client.luau` | `Name/init.client.luau` |
| LocalScript | `Name.client.luau` | `Name/init.client.luau` |
| Folder | directory | directory |
| StringValue | `Name.txt` | `Name/init.meta.json5` |
| LocalizationTable | `Name.csv` | `Name/init.csv` |
| Other | `Name.model.json5` | `Name/init.meta.json5` |
**Note**: `init.meta.json5` only created if instance has properties (Attributes, etc.)

View file

@ -156,5 +156,154 @@ return function()
folder:Destroy() folder:Destroy()
end) end)
end) end)
describe("children encoding", function()
it("should include children array even when empty", function()
local moduleScript = Instance.new("ModuleScript")
moduleScript.Name = "TestModule"
moduleScript.Source = "return {}"
local encoded = encodeInstance(moduleScript, "PARENT_ID")
expect(encoded).to.be.ok()
expect(encoded.children).to.be.ok()
expect(type(encoded.children)).to.equal("table")
expect(#encoded.children).to.equal(0)
moduleScript:Destroy()
end)
it("should encode ModuleScript with children", function()
local parent = Instance.new("ModuleScript")
parent.Name = "ParentModule"
parent.Source = "return {}"
local child1 = Instance.new("ModuleScript")
child1.Name = "ChildModule1"
child1.Source = "return 1"
child1.Parent = parent
local child2 = Instance.new("ModuleScript")
child2.Name = "ChildModule2"
child2.Source = "return 2"
child2.Parent = parent
local encoded = encodeInstance(parent, "PARENT_ID")
expect(encoded).to.be.ok()
expect(encoded.children).to.be.ok()
expect(#encoded.children).to.equal(2)
-- Verify children are properly encoded
local childNames = {}
for _, child in encoded.children do
childNames[child.name] = true
expect(child.className).to.equal("ModuleScript")
expect(child.properties.Source).to.be.ok()
end
expect(childNames["ChildModule1"]).to.equal(true)
expect(childNames["ChildModule2"]).to.equal(true)
parent:Destroy()
end)
it("should encode nested children recursively", function()
local root = Instance.new("Folder")
root.Name = "Root"
local level1 = Instance.new("Folder")
level1.Name = "Level1"
level1.Parent = root
local level2 = Instance.new("ModuleScript")
level2.Name = "Level2"
level2.Source = "return 'nested'"
level2.Parent = level1
local encoded = encodeInstance(root, "PARENT_ID")
expect(encoded).to.be.ok()
expect(#encoded.children).to.equal(1)
expect(encoded.children[1].name).to.equal("Level1")
expect(#encoded.children[1].children).to.equal(1)
expect(encoded.children[1].children[1].name).to.equal("Level2")
expect(encoded.children[1].children[1].className).to.equal("ModuleScript")
root:Destroy()
end)
it("should skip children with duplicate names", function()
local parent = Instance.new("Folder")
parent.Name = "Parent"
local child1 = Instance.new("ModuleScript")
child1.Name = "DuplicateName"
child1.Source = "return 1"
child1.Parent = parent
local child2 = Instance.new("ModuleScript")
child2.Name = "DuplicateName" -- Same name!
child2.Source = "return 2"
child2.Parent = parent
local child3 = Instance.new("ModuleScript")
child3.Name = "UniqueName"
child3.Source = "return 3"
child3.Parent = parent
local encoded = encodeInstance(parent, "PARENT_ID")
expect(encoded).to.be.ok()
-- Should only have 1 child (UniqueName), duplicates skipped
expect(#encoded.children).to.equal(1)
expect(encoded.children[1].name).to.equal("UniqueName")
parent:Destroy()
end)
it("should return empty children array when all children deleted", function()
-- Simulate: parent had children, now they're all gone
local parent = Instance.new("ModuleScript")
parent.Name = "ParentModule"
parent.Source = "return {}"
-- No children parented
local encoded = encodeInstance(parent, "PARENT_ID")
expect(encoded).to.be.ok()
expect(encoded.children).to.be.ok()
expect(#encoded.children).to.equal(0)
parent:Destroy()
end)
it("should encode Script with Folder child", function()
-- This tests the init.server.luau + child directory scenario
local script = Instance.new("Script")
script.Name = "EventService"
script.Source = "-- Event service"
local events = Instance.new("Folder")
events.Name = "Events"
events.Parent = script
local event1 = Instance.new("ModuleScript")
event1.Name = "AcidRainEvent"
event1.Source = "return {}"
event1.Parent = events
local encoded = encodeInstance(script, "PARENT_ID")
expect(encoded).to.be.ok()
expect(encoded.className).to.equal("Script")
expect(#encoded.children).to.equal(1)
expect(encoded.children[1].name).to.equal("Events")
expect(encoded.children[1].className).to.equal("Folder")
expect(#encoded.children[1].children).to.equal(1)
expect(encoded.children[1].children[1].name).to.equal("AcidRainEvent")
script:Destroy()
end)
end)
end) end)
end end

View file

@ -222,7 +222,7 @@ function PatchTree.build(patch, instanceMap, changeListHeaders)
local parentObject = instance.Parent local parentObject = instance.Parent
local parentId = instanceMap.fromInstances[parentObject] local parentId = instanceMap.fromInstances[parentObject]
local previousId = nil local previousId = nil
while parentObject do while parentObject and parentId do
if knownAncestors[parentId] then if knownAncestors[parentId] then
-- We've already added this ancestor -- We've already added this ancestor
previousId = parentId previousId = parentId

View file

@ -175,13 +175,21 @@ local function shouldDeleteChild(virtualInstance, childInstance)
return false return false
end end
local function diff(instanceMap, virtualInstances, rootId) local function diff(instanceMap, virtualInstances, rootId, serverInfo)
local patch = { local patch = {
removed = {}, removed = {},
added = {}, added = {},
updated = {}, updated = {},
} }
-- Build a lookup table for visible services (for ignoreHiddenServices check)
local visibleServicesSet: { [string]: boolean } = {}
if serverInfo and serverInfo.ignoreHiddenServices and serverInfo.visibleServices then
for _, serviceName in ipairs(serverInfo.visibleServices) do
visibleServicesSet[serviceName] = true
end
end
-- Count of locations with duplicate-named siblings -- Count of locations with duplicate-named siblings
local skippedDuplicateCount = 0 local skippedDuplicateCount = 0
@ -403,6 +411,20 @@ local function diff(instanceMap, virtualInstances, rootId)
continue continue
end end
-- Skip hidden services when ignoreHiddenServices is enabled.
-- Services are direct children of game (DataModel), and if they're
-- not in the visible services list, they weren't synced back to disk,
-- so we shouldn't mark them for deletion during forward sync.
if serverInfo and serverInfo.ignoreHiddenServices and instance == game then
if not visibleServicesSet[childInstance.Name] then
Log.trace(
"Skipping hidden service {} (ignoreHiddenServices is enabled)",
childInstance.Name
)
continue
end
end
-- This is an existing instance not present in the virtual DOM. -- This is an existing instance not present in the virtual DOM.
-- We can mark it for deletion unless the user has asked us not -- We can mark it for deletion unless the user has asked us not
-- to delete unknown stuff. Scripts are always eligible for deletion -- to delete unknown stuff. Scripts are always eligible for deletion

View file

@ -1,7 +1,4 @@
return function() return function()
local Packages = script.Parent.Parent.Parent.Packages
local Log = require(Packages.Log)
local InstanceMap = require(script.Parent.Parent.InstanceMap) local InstanceMap = require(script.Parent.Parent.InstanceMap)
local PatchSet = require(script.Parent.Parent.PatchSet) local PatchSet = require(script.Parent.Parent.PatchSet)
@ -164,15 +161,7 @@ return function()
assert(isEmpty(patch.updated)) assert(isEmpty(patch.updated))
end) end)
--[[ it("should ignore unreadable properties", function()
Because rbx_dom_lua resolves non-canonical properties to their canonical
variants, this test does not work as intended.
Instead, heat_xml is diffed with Heat, the canonical property variant,
and a patch trying to assign to heat_xml is generated. This is
incorrect, but will require more invasive changes to fix later.
]]
itFIXME("should ignore unreadable properties", function()
local knownInstances = InstanceMap.new() local knownInstances = InstanceMap.new()
local virtualInstances = { local virtualInstances = {
ROOT = { ROOT = {
@ -194,8 +183,6 @@ return function()
local ok, patch = diff(knownInstances, virtualInstances, "ROOT") local ok, patch = diff(knownInstances, virtualInstances, "ROOT")
Log.warn("{:#?}", patch)
assert(ok, tostring(patch)) assert(ok, tostring(patch))
assert(isEmpty(patch.removed)) assert(isEmpty(patch.removed))

View file

@ -21,6 +21,18 @@ local function getProperty(instance, propertyName)
}) })
end end
-- If the property name we were given is not the canonical name, treat it
-- as unknown. This handles serialization-only aliases like `heat_xml`
-- which resolve to `Heat` - we should skip these and let the canonical
-- property name be used instead.
if descriptor.name ~= propertyName then
return false,
Error.new(Error.UnknownProperty, {
className = instance.ClassName,
propertyName = propertyName,
})
end
if descriptor.scriptability == "None" or descriptor.scriptability == "Write" then if descriptor.scriptability == "None" or descriptor.scriptability == "Write" then
return false, return false,
Error.new(Error.UnreadableProperty, { Error.new(Error.UnreadableProperty, {

View file

@ -41,9 +41,9 @@ function Reconciler:hydrate(virtualInstances, rootId, rootInstance)
return result return result
end end
function Reconciler:diff(virtualInstances, rootId) function Reconciler:diff(virtualInstances, rootId, serverInfo)
Timer.start("Reconciler:diff") Timer.start("Reconciler:diff")
local success, result = diff(self.__instanceMap, virtualInstances, rootId) local success, result = diff(self.__instanceMap, virtualInstances, rootId, serverInfo)
Timer.stop() Timer.stop()
return success, result return success, result

View file

@ -612,7 +612,7 @@ function ServeSession:__computeInitialPatch(serverInfo)
Log.trace("Computing changes that plugin needs to make to catch up to server...") Log.trace("Computing changes that plugin needs to make to catch up to server...")
self:setLoadingText("Finding differences between server and Studio...") self:setLoadingText("Finding differences between server and Studio...")
local success, catchUpPatch = local success, catchUpPatch =
self.__reconciler:diff(readResponseBody.instances, serverInfo.rootInstanceId, game) self.__reconciler:diff(readResponseBody.instances, serverInfo.rootInstanceId, serverInfo)
if not success then if not success then
Log.error("Could not compute a diff to catch up to the Rojo server: {:#?}", catchUpPatch) Log.error("Could not compute a diff to catch up to the Rojo server: {:#?}", catchUpPatch)

View file

@ -0,0 +1,10 @@
{
"name": "api_write_existing",
"tree": {
"$className": "DataModel",
"ServerScriptService": {
"$className": "ServerScriptService",
"$path": "src"
}
}
}

View file

@ -0,0 +1,5 @@
-- AcidRainEvent module
return {
name = "AcidRainEvent",
duration = 30,
}

View file

@ -0,0 +1,8 @@
-- EventService: handles game events
local EventService = {}
function EventService:Start()
print("EventService started")
end
return EventService

View file

@ -0,0 +1,10 @@
{
"name": "syncback_format_transitions",
"tree": {
"$className": "DataModel",
"ReplicatedStorage": {
"$className": "ReplicatedStorage",
"$path": "src"
}
}
}

View file

@ -0,0 +1,2 @@
-- UI Helper module under LocalScript
return {}

View file

@ -0,0 +1,2 @@
-- Parent LocalScript with children
print("DirLocalScriptWithChildren started")

View file

@ -0,0 +1,2 @@
-- Child module script
return { child = true }

View file

@ -0,0 +1 @@
This is a child StringValue

View file

@ -0,0 +1,6 @@
{
"className": "Configuration",
"properties": {
"Name": "DirModelWithChildren"
}
}

View file

@ -0,0 +1,2 @@
-- Child A module
return "ChildA"

View file

@ -0,0 +1,2 @@
-- Child B module
return "ChildB"

View file

@ -0,0 +1,5 @@
-- Parent ModuleScript with children
return {
name = "DirModuleWithChildren",
hasChildren = true,
}

View file

@ -0,0 +1,2 @@
-- Handler module under Script
return function() end

View file

@ -0,0 +1,2 @@
-- Parent Script with children
print("DirScriptWithChildren started")

View file

@ -0,0 +1,2 @@
-- Standalone LocalScript (no children)
print("StandaloneLocalScript running")

View file

@ -0,0 +1,6 @@
{
"className": "Configuration",
"properties": {
"Name": "StandaloneModel"
}
}

View file

@ -0,0 +1,2 @@
-- Standalone ModuleScript (no children)
return { standalone = true }

View file

@ -0,0 +1,2 @@
-- Standalone Script (no children)
print("StandaloneScript running")

View file

@ -0,0 +1 @@
Standalone StringValue content

View file

@ -0,0 +1,11 @@
{
"name": "syncback_scripts_only",
"syncScriptsOnly": true,
"tree": {
"$className": "DataModel",
"ServerScriptService": {
"$className": "ServerScriptService",
"$path": "src"
}
}
}

View file

@ -0,0 +1,4 @@
{
"className": "Configuration",
"properties": {}
}

View file

@ -0,0 +1,2 @@
-- Helper module
return {}

View file

@ -0,0 +1,2 @@
-- Server script with children
print("ScriptWithChildren started")

View file

@ -0,0 +1,2 @@
-- Standalone module script
return { standalone = true }

View file

@ -0,0 +1,10 @@
# Run the Rojo plugin unit tests
# Requires: run-in-roblox (via rokit) and Roblox Studio installed
$ErrorActionPreference = "Stop"
Write-Host "Building test place..." -ForegroundColor Cyan
rojo build plugin/test-place.project.json -o TestPlace.rbxl
Write-Host "Running tests in Roblox..." -ForegroundColor Cyan
run-in-roblox --script plugin/run-tests.server.lua --place TestPlace.rbxl

View file

@ -0,0 +1,7 @@
# Watch and rebuild the Rojo plugin on file changes
# Make sure you've enabled the Studio setting to reload plugins on file change!
$ErrorActionPreference = "Stop"
Write-Host "Watching for plugin changes... (Ctrl+C to stop)" -ForegroundColor Cyan
rojo build plugin.project.json --plugin Rojo.rbxm --watch

View file

@ -41,7 +41,7 @@ impl Options {
pub fn run(self) -> anyhow::Result<()> { pub fn run(self) -> anyhow::Result<()> {
match self.subcommand { match self.subcommand {
Subcommand::Init(subcommand) => subcommand.run(), Subcommand::Init(subcommand) => subcommand.run(),
Subcommand::Serve(subcommand) => subcommand.run(self.global), Subcommand::Serve(subcommand) => subcommand.run(),
Subcommand::Build(subcommand) => subcommand.run(), Subcommand::Build(subcommand) => subcommand.run(),
Subcommand::Upload(subcommand) => subcommand.run(), Subcommand::Upload(subcommand) => subcommand.run(),
Subcommand::Sourcemap(subcommand) => subcommand.run(), Subcommand::Sourcemap(subcommand) => subcommand.run(),

View file

@ -1,5 +1,4 @@
use std::{ use std::{
io::{self, Write},
net::{IpAddr, Ipv4Addr}, net::{IpAddr, Ipv4Addr},
path::PathBuf, path::PathBuf,
sync::Arc, sync::Arc,
@ -7,11 +6,10 @@ use std::{
use clap::Parser; use clap::Parser;
use memofs::Vfs; use memofs::Vfs;
use termcolor::{BufferWriter, Color, ColorChoice, ColorSpec, WriteColor};
use crate::{serve_session::ServeSession, web::LiveServer}; use crate::{serve_session::ServeSession, web::LiveServer};
use super::{resolve_path, GlobalOptions}; use super::resolve_path;
const DEFAULT_BIND_ADDRESS: Ipv4Addr = Ipv4Addr::new(127, 0, 0, 1); const DEFAULT_BIND_ADDRESS: Ipv4Addr = Ipv4Addr::new(127, 0, 0, 1);
const DEFAULT_PORT: u16 = 34872; const DEFAULT_PORT: u16 = 34872;
@ -34,7 +32,7 @@ pub struct ServeCommand {
} }
impl ServeCommand { impl ServeCommand {
pub fn run(self, global: GlobalOptions) -> anyhow::Result<()> { pub fn run(self) -> anyhow::Result<()> {
let project_path = resolve_path(&self.project); let project_path = resolve_path(&self.project);
let vfs = Vfs::new_default(); let vfs = Vfs::new_default();
@ -53,49 +51,10 @@ impl ServeCommand {
let server = LiveServer::new(session); let server = LiveServer::new(session);
let _ = show_start_message(ip, port, global.color.into()); let host = if ip.is_loopback() { "localhost".to_owned() } else { ip.to_string() };
log::info!("Listening: http://{}:{}", host, port);
server.start((ip, port).into()); server.start((ip, port).into());
Ok(()) Ok(())
} }
} }
fn show_start_message(bind_address: IpAddr, port: u16, color: ColorChoice) -> io::Result<()> {
let mut green = ColorSpec::new();
green.set_fg(Some(Color::Green)).set_bold(true);
let writer = BufferWriter::stdout(color);
let mut buffer = writer.buffer();
let address_string = if bind_address.is_loopback() {
"localhost".to_owned()
} else {
bind_address.to_string()
};
writeln!(&mut buffer, "Rojo server listening:")?;
write!(&mut buffer, " Address: ")?;
buffer.set_color(&green)?;
writeln!(&mut buffer, "{}", address_string)?;
buffer.set_color(&ColorSpec::new())?;
write!(&mut buffer, " Port: ")?;
buffer.set_color(&green)?;
writeln!(&mut buffer, "{}", port)?;
writeln!(&mut buffer)?;
buffer.set_color(&ColorSpec::new())?;
write!(&mut buffer, "Visit ")?;
buffer.set_color(&green)?;
write!(&mut buffer, "http://{}:{}/", address_string, port)?;
buffer.set_color(&ColorSpec::new())?;
writeln!(&mut buffer, " in your browser for more information.")?;
writer.print(&buffer)?;
Ok(())
}

View file

@ -129,6 +129,19 @@ pub struct Project {
#[serde(skip_serializing_if = "Option::is_none")] #[serde(skip_serializing_if = "Option::is_none")]
pub sync_scripts_only: Option<bool>, pub sync_scripts_only: Option<bool>,
/// When enabled, hidden/internal services (like AdService, AnalyticsService,
/// Chat, HttpService, etc.) are ignored during sync operations. Only "visible"
/// services like Workspace, ReplicatedStorage, ServerScriptService will be
/// synced. Defaults to `true`.
///
/// This prevents the plugin from trying to delete services that were never
/// synced to disk in the first place.
///
/// Note: This setting can also be specified in `syncbackRules.ignoreHiddenServices`
/// for backward compatibility, but the root-level setting takes precedence.
#[serde(skip_serializing_if = "Option::is_none")]
pub ignore_hidden_services: Option<bool>,
/// The path to the file that this project came from. Relative paths in the /// The path to the file that this project came from. Relative paths in the
/// project should be considered relative to the parent of this field, also /// project should be considered relative to the parent of this field, also
/// given by `Project::folder_location`. /// given by `Project::folder_location`.

View file

@ -226,6 +226,26 @@ impl ServeSession {
pub fn sync_scripts_only(&self) -> bool { pub fn sync_scripts_only(&self) -> bool {
self.root_project.sync_scripts_only.unwrap_or(false) self.root_project.sync_scripts_only.unwrap_or(false)
} }
/// Returns whether hidden/internal services should be ignored during sync.
/// When enabled, only "visible" services like Workspace, ReplicatedStorage, etc.
/// are included in sync operations.
///
/// Checks root-level `ignoreHiddenServices` first, then falls back to
/// `syncbackRules.ignoreHiddenServices` for backward compatibility.
/// Defaults to `true` if neither is specified.
pub fn ignore_hidden_services(&self) -> bool {
// Root-level setting takes precedence
if let Some(value) = self.root_project.ignore_hidden_services {
return value;
}
// Fall back to syncbackRules for backward compatibility
self.root_project
.syncback_rules
.as_ref()
.map(|rules| rules.ignore_hidden_services())
.unwrap_or(true)
}
} }
#[derive(Debug, Error)] #[derive(Debug, Error)]

View file

@ -50,7 +50,7 @@ const DEBUG_MODEL_FORMAT_VAR: &str = "ROJO_SYNCBACK_DEBUG";
/// Services that are considered "visible" and will be included when /// Services that are considered "visible" and will be included when
/// `ignoreHiddenServices` is enabled. All other services will be ignored. /// `ignoreHiddenServices` is enabled. All other services will be ignored.
const VISIBLE_SERVICES: &[&str] = &[ pub const VISIBLE_SERVICES: &[&str] = &[
"Lighting", "Lighting",
"MaterialService", "MaterialService",
"NetworkClient", "NetworkClient",
@ -134,12 +134,17 @@ pub fn syncback_loop_with_stats(
} }
// If ignoreHiddenServices is enabled (default: true), filter to only visible services // If ignoreHiddenServices is enabled (default: true), filter to only visible services
if project // Check root-level setting first, then fall back to syncbackRules for backward compatibility
.syncback_rules let ignore_hidden = project
.as_ref() .ignore_hidden_services
.map(|rules| rules.ignore_hidden_services()) .or_else(|| {
.unwrap_or(true) project
{ .syncback_rules
.as_ref()
.map(|rules| rules.ignore_hidden_services())
})
.unwrap_or(true);
if ignore_hidden {
log::debug!("Filtering hidden services"); log::debug!("Filtering hidden services");
strip_hidden_services(&mut new_tree); strip_hidden_services(&mut new_tree);
} }
@ -212,25 +217,75 @@ pub fn syncback_loop_with_stats(
let project_path = project.folder_location(); let project_path = project.folder_location();
// In clean mode, collect all existing paths from old tree for cleanup. // In clean mode, we scan the actual filesystem to find ALL existing files,
// These will be compared against new paths to remove orphaned files. // not just what Rojo loaded into its tree. This ensures orphaned files
// (like duplicates Rojo couldn't load) get cleaned up properly.
// Clean mode is essentially: "delete everything and rebuild from Studio"
let existing_paths: HashSet<PathBuf> = if !incremental { let existing_paths: HashSet<PathBuf> = if !incremental {
log::debug!("Clean mode: collecting existing paths for cleanup"); log::debug!("Clean mode: scanning filesystem for existing paths");
let mut paths = HashSet::new(); let mut paths = HashSet::new();
for ref_id in descendants(old_tree.inner(), old_tree.get_root_id()) {
if let Some(inst) = old_tree.get_instance(ref_id) { // Get the source directories from the project's tree structure.
for path in inst.metadata().relevant_paths.iter() { // We scan these directories recursively to find all files on disk.
// Skip paths matching ignore patterns let mut dirs_to_scan: Vec<PathBuf> = Vec::new();
if is_valid_path(&ignore_patterns, project_path, path) { for ref_id in old_tree.inner().root().children() {
// Only include paths that actually exist on disk if let Some(inst) = old_tree.get_instance(*ref_id) {
if path.exists() { if let Some(source) = &inst.metadata().instigating_source {
paths.insert(path.clone()); let path = source.path();
} if path.exists() {
dirs_to_scan.push(path.to_path_buf());
} }
} }
} }
} }
log::debug!("Collected {} existing paths", paths.len());
// Recursively scan each source directory
fn scan_directory(
dir: &Path,
paths: &mut HashSet<PathBuf>,
ignore_patterns: &Option<Vec<Glob>>,
project_path: &Path,
) {
let entries = match std::fs::read_dir(dir) {
Ok(e) => e,
Err(_) => return,
};
for entry in entries.flatten() {
let path = entry.path();
// Skip paths matching ignore patterns
if !is_valid_path(ignore_patterns, project_path, &path) {
continue;
}
// Skip hidden files/directories (starting with .)
if let Some(name) = path.file_name().and_then(|n| n.to_str()) {
if name.starts_with('.') {
continue;
}
}
paths.insert(path.clone());
if path.is_dir() {
scan_directory(&path, paths, ignore_patterns, project_path);
}
}
}
for dir in &dirs_to_scan {
if dir.is_dir() {
scan_directory(dir, &mut paths, &ignore_patterns, project_path);
} else if dir.exists() {
// It's a file, add it directly
if is_valid_path(&ignore_patterns, project_path, dir) {
paths.insert(dir.clone());
}
}
}
log::debug!("Scanned {} existing paths from filesystem", paths.len());
paths paths
} else { } else {
HashSet::new() HashSet::new()
@ -649,7 +704,7 @@ pub struct SyncbackRules {
/// Whether to encode Windows-invalid characters in file names during /// Whether to encode Windows-invalid characters in file names during
/// syncback. Characters like `<`, `>`, `:`, `"`, `/`, `\`, `|`, `?`, `*` /// syncback. Characters like `<`, `>`, `:`, `"`, `/`, `\`, `|`, `?`, `*`
/// will be encoded as `%LT%`, `%GT%`, `%COLON%`, etc. /// will be encoded as `%LT%`, `%GT%`, `%COLON%`, etc.
/// Defaults to `false`. /// Defaults to `true`.
#[serde(skip_serializing_if = "Option::is_none")] #[serde(skip_serializing_if = "Option::is_none")]
encode_windows_invalid_chars: Option<bool>, encode_windows_invalid_chars: Option<bool>,
/// When enabled, only "visible" services will be synced back. This includes /// When enabled, only "visible" services will be synced back. This includes

View file

@ -21,6 +21,7 @@ use rbx_dom_weak::{
use crate::{ use crate::{
serve_session::ServeSession, serve_session::ServeSession,
snapshot::{InstanceWithMeta, InstigatingSource, PatchSet, PatchUpdate}, snapshot::{InstanceWithMeta, InstigatingSource, PatchSet, PatchUpdate},
syncback::VISIBLE_SERVICES,
web::{ web::{
interface::{ interface::{
ErrorResponse, Instance, InstanceMetadata, MessagesPacket, OpenResponse, ReadResponse, ErrorResponse, Instance, InstanceMetadata, MessagesPacket, OpenResponse, ReadResponse,
@ -32,6 +33,22 @@ use crate::{
web_api::{InstanceUpdate, RefPatchResponse, SerializeResponse}, web_api::{InstanceUpdate, RefPatchResponse, SerializeResponse},
}; };
/// Represents the existing file format for a script/instance on disk.
/// Used to preserve the current format when doing partial updates from the plugin.
///
/// When the plugin sends a script update, it might not include children info.
/// We check the filesystem to see if this is already a directory (with init file)
/// or a standalone file, and preserve that format to avoid creating duplicates.
#[derive(Debug, Clone, Copy, PartialEq)]
enum ExistingFileFormat {
/// No existing file found - use has_children to decide
None,
/// Standalone file exists (e.g., Name.luau)
Standalone,
/// Directory with init file exists (e.g., Name/init.luau)
Directory,
}
/// Convert a Variant to a JSON-compatible value for .model.json5 files /// Convert a Variant to a JSON-compatible value for .model.json5 files
fn variant_to_json(variant: &Variant) -> Option<serde_json::Value> { fn variant_to_json(variant: &Variant) -> Option<serde_json::Value> {
use serde_json::{json, Value}; use serde_json::{json, Value};
@ -150,6 +167,13 @@ impl ApiService {
let tree = self.serve_session.tree(); let tree = self.serve_session.tree();
let root_instance_id = tree.get_root_id(); let root_instance_id = tree.get_root_id();
let ignore_hidden_services = self.serve_session.ignore_hidden_services();
let visible_services = if ignore_hidden_services {
VISIBLE_SERVICES.iter().map(|s| s.to_string()).collect()
} else {
Vec::new()
};
msgpack_ok(&ServerInfoResponse { msgpack_ok(&ServerInfoResponse {
server_version: SERVER_VERSION.to_owned(), server_version: SERVER_VERSION.to_owned(),
protocol_version: PROTOCOL_VERSION, protocol_version: PROTOCOL_VERSION,
@ -161,6 +185,8 @@ impl ApiService {
game_id: self.serve_session.game_id(), game_id: self.serve_session.game_id(),
root_instance_id, root_instance_id,
sync_source_only: true, sync_source_only: true,
ignore_hidden_services,
visible_services,
}) })
} }
@ -403,11 +429,36 @@ impl ApiService {
} }
} }
/// Find a child instance by name under a given parent in the tree.
/// Returns the Ref of the child if found, None otherwise.
fn find_child_by_name(
tree: &crate::snapshot::RojoTree,
parent_ref: Ref,
name: &str,
) -> Option<Ref> {
let parent = tree.get_instance(parent_ref)?;
parent
.children()
.iter()
.find(|&&child_ref| {
tree.get_instance(child_ref)
.map(|c| c.name() == name)
.unwrap_or(false)
})
.copied()
}
/// This follows the same middleware selection logic as the dedicated syncback /// This follows the same middleware selection logic as the dedicated syncback
/// system in `src/syncback/mod.rs` to ensure consistent behavior. /// system in `src/syncback/mod.rs` to ensure consistent behavior.
/// ///
/// Takes the tree reference and duplicate siblings cache as parameters to ensure /// Takes the tree reference and duplicate siblings cache as parameters to ensure
/// consistency - the cache was computed from this exact tree state. /// consistency - the cache was computed from this exact tree state.
///
/// IMPORTANT: Before creating new files, this function checks if an instance
/// with the same name already exists in the tree. If so, it updates the existing
/// file at its `instigating_source.path` instead of creating a new file.
/// This prevents duplicate file creation when the plugin sends an instance
/// that already exists on the filesystem.
fn syncback_added_instance( fn syncback_added_instance(
&self, &self,
added: &crate::web::interface::AddedInstance, added: &crate::web::interface::AddedInstance,
@ -446,6 +497,26 @@ impl ApiService {
); );
} }
// CRITICAL: Check if instance already exists in tree before creating new files.
// This prevents duplicate file creation when the plugin sends an instance
// that already exists (e.g., user "pulls" an instance that appeared as
// "to delete" due to duplicate detection issues).
if let Some(existing_ref) = Self::find_child_by_name(tree, parent_ref, &added.name) {
if let Some(existing) = tree.get_instance(existing_ref) {
if let Some(source) = &existing.metadata().instigating_source {
let existing_path = source.path();
log::info!(
"Syncback: Instance '{}' already exists in tree at {}, updating in place",
added.name,
existing_path.display()
);
// Update the existing instance instead of creating new files
return self.syncback_update_existing_instance(added, existing_path, stats);
}
}
}
// Instance doesn't exist in tree - create new files
// Get the parent's filesystem path from its metadata // Get the parent's filesystem path from its metadata
let parent_path = parent_instance let parent_path = parent_instance
.metadata() .metadata()
@ -468,6 +539,152 @@ impl ApiService {
self.syncback_instance_to_path_with_stats(added, &parent_dir, stats) self.syncback_instance_to_path_with_stats(added, &parent_dir, stats)
} }
/// Update an existing instance in place instead of creating new files.
/// This is called when the plugin sends an "added" instance that already
/// exists in the tree. We update the existing file at its instigating_source
/// path to preserve the file format and avoid creating duplicates.
///
/// For children, we use the normal syncback path which will check for
/// existing files via `detect_existing_script_format`.
fn syncback_update_existing_instance(
&self,
added: &crate::web::interface::AddedInstance,
existing_path: &std::path::Path,
stats: &crate::syncback::SyncbackStats,
) -> anyhow::Result<()> {
use anyhow::Context;
let class_name = &added.class_name;
// For scripts, update the Source property at the existing path
match class_name.as_str() {
"ModuleScript" | "Script" | "LocalScript" => {
let source = self.get_source_property(added);
// Determine the actual file to write based on existing structure
// If existing_path is a directory, look for init file inside
let file_path = if existing_path.is_dir() {
// Find the init file based on class
let init_name = match class_name.as_str() {
"ModuleScript" => {
if existing_path.join("init.luau").exists() {
"init.luau"
} else {
"init.lua"
}
}
"Script" => {
// Check which init file exists
if existing_path.join("init.server.luau").exists() {
"init.server.luau"
} else if existing_path.join("init.client.luau").exists() {
"init.client.luau"
} else if existing_path.join("init.server.lua").exists() {
"init.server.lua"
} else {
"init.server.luau" // Default
}
}
"LocalScript" => {
if existing_path.join("init.client.luau").exists() {
"init.client.luau"
} else if existing_path.join("init.local.luau").exists() {
"init.local.luau"
} else if existing_path.join("init.client.lua").exists() {
"init.client.lua"
} else {
"init.client.luau" // Default
}
}
_ => unreachable!(),
};
existing_path.join(init_name)
} else {
// It's already a file path
existing_path.to_path_buf()
};
fs::write(&file_path, source.as_bytes()).with_context(|| {
format!("Failed to write file: {}", file_path.display())
})?;
log::info!(
"Syncback: Updated existing {} at {}",
class_name,
file_path.display()
);
// Handle children - they go into the directory
if !added.children.is_empty() {
let children_dir = if existing_path.is_dir() {
existing_path.to_path_buf()
} else {
// For standalone scripts, children would need directory conversion
// For now, use the parent directory as the base
existing_path
.parent()
.map(|p| p.to_path_buf())
.unwrap_or_else(|| existing_path.to_path_buf())
};
// Filter duplicate children
let inst_path = format!("{}", existing_path.display());
let unique_children =
self.filter_duplicate_children(&added.children, &inst_path, stats);
// Process children using normal syncback path
// This will use detect_existing_script_format to check existing files
for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &children_dir, stats)?;
}
}
}
// For non-script types, update the appropriate file
_ => {
if existing_path.is_dir() {
// Update init.meta.json5 if needed
if !added.properties.is_empty() {
self.write_init_meta_json(existing_path, added)?;
log::info!(
"Syncback: Updated existing {} at {}/init.meta.json5",
class_name,
existing_path.display()
);
}
// Handle children
if !added.children.is_empty() {
let inst_path = format!("{}", existing_path.display());
let unique_children =
self.filter_duplicate_children(&added.children, &inst_path, stats);
for child in &unique_children {
self.syncback_instance_to_path_with_stats(
child,
existing_path,
stats,
)?;
}
}
} else {
// It's a standalone file (e.g., .model.json5)
let content = self.serialize_instance_to_model_json(added)?;
fs::write(existing_path, &content).with_context(|| {
format!("Failed to write file: {}", existing_path.display())
})?;
log::info!(
"Syncback: Updated existing {} at {}",
class_name,
existing_path.display()
);
}
}
}
Ok(())
}
/// Syncback a removed instance by deleting its file(s) from the filesystem. /// Syncback a removed instance by deleting its file(s) from the filesystem.
/// This is called when the user selects "pull" for an instance that exists in Rojo /// This is called when the user selects "pull" for an instance that exists in Rojo
/// but has been deleted in Studio. /// but has been deleted in Studio.
@ -600,6 +817,78 @@ impl ApiService {
.collect() .collect()
} }
/// Detects what file format currently exists on disk for a given instance.
/// This is used to preserve the existing format during partial updates.
///
/// When the plugin sends a script update, it might not include children info.
/// We check the filesystem to see if this is already a directory (with init file)
/// or a standalone file, and preserve that format.
fn detect_existing_script_format(
parent_dir: &std::path::Path,
name: &str,
class_name: &str,
) -> ExistingFileFormat {
let dir_path = parent_dir.join(name);
// Check for directory with init file first (takes precedence)
if dir_path.is_dir() {
// Check for any init file based on class type
let init_files = match class_name {
"ModuleScript" => vec!["init.luau", "init.lua"],
"Script" => vec![
"init.server.luau",
"init.server.lua",
"init.client.luau",
"init.client.lua",
],
"LocalScript" => vec![
"init.client.luau",
"init.client.lua",
"init.local.luau",
"init.local.lua",
],
// For non-scripts, check for init.model.json5 or similar
_ => vec!["init.model.json5", "init.model.json", "init.meta.json5"],
};
for init_file in init_files {
if dir_path.join(init_file).exists() {
return ExistingFileFormat::Directory;
}
}
}
// Check for standalone file
let standalone_patterns = match class_name {
"ModuleScript" => vec![format!("{}.luau", name), format!("{}.lua", name)],
"Script" => vec![
format!("{}.server.luau", name),
format!("{}.server.lua", name),
format!("{}.client.luau", name),
format!("{}.client.lua", name),
],
"LocalScript" => vec![
format!("{}.client.luau", name),
format!("{}.client.lua", name),
format!("{}.local.luau", name),
format!("{}.local.lua", name),
],
// For non-scripts, check for model files
_ => vec![
format!("{}.model.json5", name),
format!("{}.model.json", name),
],
};
for pattern in standalone_patterns {
if parent_dir.join(&pattern).exists() {
return ExistingFileFormat::Standalone;
}
}
ExistingFileFormat::None
}
/// Recursively syncback an instance and its children to the filesystem. /// Recursively syncback an instance and its children to the filesystem.
/// This is the internal implementation that handles the actual file creation. /// This is the internal implementation that handles the actual file creation.
#[allow(dead_code)] #[allow(dead_code)]
@ -642,13 +931,70 @@ impl ApiService {
// Determine the appropriate middleware/file format based on class name // Determine the appropriate middleware/file format based on class name
// This matches the logic in src/syncback/mod.rs::get_best_middleware // This matches the logic in src/syncback/mod.rs::get_best_middleware
// Scripts with children become directories with init files //
// IMPORTANT: When an instance already exists on disk, preserve its format.
// The plugin might send a partial update (e.g., just the script source without
// children), so we can't rely solely on has_children to decide the format.
// Only transition formats when creating truly new instances.
let existing_format =
Self::detect_existing_script_format(parent_dir, &added.name, &added.class_name);
match added.class_name.as_str() { match added.class_name.as_str() {
// Script types: .luau files, or directories with init files if has children // Script types: .luau files, or directories with init files if has children
// Non-Source properties (like Attributes) go into init.meta.json5 // Non-Source properties (like Attributes) go into init.meta.json5
"ModuleScript" => { "ModuleScript" => {
let source = self.get_source_property(added); let source = self.get_source_property(added);
if has_children {
// Decide whether to use directory or standalone format:
// - If directory already exists on disk, preserve it (update init file)
// - If standalone already exists on disk, preserve it (update standalone)
// - If nothing exists, use has_children to decide (new instance)
let use_directory = match existing_format {
ExistingFileFormat::Directory => {
log::debug!(
"Syncback: Preserving existing directory format for {}",
added.name
);
true
}
ExistingFileFormat::Standalone => {
log::debug!(
"Syncback: Preserving existing standalone format for {}",
added.name
);
false
}
ExistingFileFormat::None => {
// New instance - use has_children to decide
has_children
}
};
if use_directory {
// Directory format: Name/init.luau
// Only remove standalone if this is a new instance transitioning to directory
if existing_format == ExistingFileFormat::None {
let standalone_path = parent_dir.join(format!("{}.luau", added.name));
if standalone_path.exists() {
if let Err(err) = fs::remove_file(&standalone_path) {
log::warn!(
"Syncback: Failed to remove old standalone file {}: {}",
standalone_path.display(),
err
);
} else {
log::info!(
"Syncback: Removed old standalone file {} (script now has children)",
standalone_path.display()
);
}
let meta_path = parent_dir.join(format!("{}.meta.json5", added.name));
if meta_path.exists() {
let _ = fs::remove_file(&meta_path);
}
}
}
let dir_path = parent_dir.join(&added.name); let dir_path = parent_dir.join(&added.name);
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
@ -657,32 +1003,83 @@ impl ApiService {
fs::write(&init_path, source.as_bytes()).with_context(|| { fs::write(&init_path, source.as_bytes()).with_context(|| {
format!("Failed to write file: {}", init_path.display()) format!("Failed to write file: {}", init_path.display())
})?; })?;
// Write init.meta.json5 if there are non-Source properties (like Attributes)
self.write_script_meta_json_if_needed(&dir_path, added)?; self.write_script_meta_json_if_needed(&dir_path, added)?;
log::info!( log::info!(
"Syncback: Created ModuleScript directory at {}", "Syncback: Updated ModuleScript at {}",
dir_path.display() init_path.display()
); );
// Recurse into children if any were provided
for child in &unique_children { for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?;
} }
} else { } else {
// Standalone format: Name.luau
// Only remove directory if this is a new instance transitioning to standalone
if existing_format == ExistingFileFormat::None {
let dir_path = parent_dir.join(&added.name);
if dir_path.is_dir() {
if let Err(err) = fs::remove_dir_all(&dir_path) {
log::warn!(
"Syncback: Failed to remove old directory {}: {}",
dir_path.display(),
err
);
} else {
log::info!(
"Syncback: Removed old directory {} (script no longer has children)",
dir_path.display()
);
}
}
}
let file_path = parent_dir.join(format!("{}.luau", added.name)); let file_path = parent_dir.join(format!("{}.luau", added.name));
fs::write(&file_path, source.as_bytes()).with_context(|| { fs::write(&file_path, source.as_bytes()).with_context(|| {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
// Create adjacent meta file if there are non-Source properties
self.write_adjacent_script_meta_if_needed(parent_dir, &added.name, added)?; self.write_adjacent_script_meta_if_needed(parent_dir, &added.name, added)?;
log::info!("Syncback: Created ModuleScript at {}", file_path.display()); log::info!("Syncback: Updated ModuleScript at {}", file_path.display());
} }
} }
"Script" => { "Script" => {
let source = self.get_source_property(added); let source = self.get_source_property(added);
// Determine script type based on RunContext property
// This ensures proper round-tripping regardless of emitLegacyScripts mode
let script_suffix = self.get_script_suffix_for_run_context(added); let script_suffix = self.get_script_suffix_for_run_context(added);
if has_children { // Use existing format if present, otherwise decide based on has_children
let use_directory = match existing_format {
ExistingFileFormat::Directory => {
log::debug!(
"Syncback: Preserving existing directory format for Script {}",
added.name
);
true
}
ExistingFileFormat::Standalone => {
log::debug!(
"Syncback: Preserving existing standalone format for Script {}",
added.name
);
false
}
ExistingFileFormat::None => has_children,
};
if use_directory {
// Only clean up if this is a new instance
if existing_format == ExistingFileFormat::None {
for suffix in &["server", "client"] {
let standalone_path =
parent_dir.join(format!("{}.{}.luau", added.name, suffix));
if standalone_path.exists() {
let _ = fs::remove_file(&standalone_path);
}
}
let meta_path = parent_dir.join(format!("{}.meta.json5", added.name));
if meta_path.exists() {
let _ = fs::remove_file(&meta_path);
}
}
let dir_path = parent_dir.join(&added.name); let dir_path = parent_dir.join(&added.name);
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
@ -692,27 +1089,66 @@ impl ApiService {
format!("Failed to write file: {}", init_path.display()) format!("Failed to write file: {}", init_path.display())
})?; })?;
self.write_script_meta_json_if_needed(&dir_path, added)?; self.write_script_meta_json_if_needed(&dir_path, added)?;
log::info!( log::info!("Syncback: Updated Script at {}", init_path.display());
"Syncback: Created Script directory at {}",
dir_path.display()
);
for child in &unique_children { for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?;
} }
} else { } else {
// Only clean up if this is a new instance
if existing_format == ExistingFileFormat::None {
let dir_path = parent_dir.join(&added.name);
if dir_path.is_dir() {
let _ = fs::remove_dir_all(&dir_path);
}
}
let file_path = let file_path =
parent_dir.join(format!("{}.{}.luau", added.name, script_suffix)); parent_dir.join(format!("{}.{}.luau", added.name, script_suffix));
fs::write(&file_path, source.as_bytes()).with_context(|| { fs::write(&file_path, source.as_bytes()).with_context(|| {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
// Create adjacent meta file if there are non-Source properties
self.write_adjacent_script_meta_if_needed(parent_dir, &added.name, added)?; self.write_adjacent_script_meta_if_needed(parent_dir, &added.name, added)?;
log::info!("Syncback: Created Script at {}", file_path.display()); log::info!("Syncback: Updated Script at {}", file_path.display());
} }
} }
"LocalScript" => { "LocalScript" => {
let source = self.get_source_property(added); let source = self.get_source_property(added);
if has_children {
// Use existing format if present, otherwise decide based on has_children
let use_directory = match existing_format {
ExistingFileFormat::Directory => {
log::debug!(
"Syncback: Preserving existing directory format for LocalScript {}",
added.name
);
true
}
ExistingFileFormat::Standalone => {
log::debug!(
"Syncback: Preserving existing standalone format for LocalScript {}",
added.name
);
false
}
ExistingFileFormat::None => has_children,
};
if use_directory {
// Only clean up if this is a new instance
if existing_format == ExistingFileFormat::None {
for suffix in &["local", "client"] {
let standalone_path =
parent_dir.join(format!("{}.{}.luau", added.name, suffix));
if standalone_path.exists() {
let _ = fs::remove_file(&standalone_path);
}
}
let meta_path = parent_dir.join(format!("{}.meta.json5", added.name));
if meta_path.exists() {
let _ = fs::remove_file(&meta_path);
}
}
let dir_path = parent_dir.join(&added.name); let dir_path = parent_dir.join(&added.name);
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
@ -722,21 +1158,25 @@ impl ApiService {
format!("Failed to write file: {}", init_path.display()) format!("Failed to write file: {}", init_path.display())
})?; })?;
self.write_script_meta_json_if_needed(&dir_path, added)?; self.write_script_meta_json_if_needed(&dir_path, added)?;
log::info!( log::info!("Syncback: Updated LocalScript at {}", init_path.display());
"Syncback: Created LocalScript directory at {}",
dir_path.display()
);
for child in &unique_children { for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?;
} }
} else { } else {
// Only clean up if this is a new instance
if existing_format == ExistingFileFormat::None {
let dir_path = parent_dir.join(&added.name);
if dir_path.is_dir() {
let _ = fs::remove_dir_all(&dir_path);
}
}
let file_path = parent_dir.join(format!("{}.local.luau", added.name)); let file_path = parent_dir.join(format!("{}.local.luau", added.name));
fs::write(&file_path, source.as_bytes()).with_context(|| { fs::write(&file_path, source.as_bytes()).with_context(|| {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
// Create adjacent meta file if there are non-Source properties
self.write_adjacent_script_meta_if_needed(parent_dir, &added.name, added)?; self.write_adjacent_script_meta_if_needed(parent_dir, &added.name, added)?;
log::info!("Syncback: Created LocalScript at {}", file_path.display()); log::info!("Syncback: Updated LocalScript at {}", file_path.display());
} }
} }
@ -843,8 +1283,29 @@ impl ApiService {
} }
// All other classes -> .model.json5 or directory if has children // All other classes -> .model.json5 or directory if has children
// Preserves existing format to avoid creating duplicates during partial updates
_ => { _ => {
if has_children { let use_directory = match existing_format {
ExistingFileFormat::Directory => {
log::debug!(
"Syncback: Preserving existing directory format for {} {}",
added.class_name,
added.name
);
true
}
ExistingFileFormat::Standalone => {
log::debug!(
"Syncback: Preserving existing standalone format for {} {}",
added.class_name,
added.name
);
false
}
ExistingFileFormat::None => has_children,
};
if use_directory {
// Create directory and put the instance as init.meta.json5 // Create directory and put the instance as init.meta.json5
let dir_path = parent_dir.join(&added.name); let dir_path = parent_dir.join(&added.name);
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
@ -855,7 +1316,7 @@ impl ApiService {
self.write_init_meta_json(&dir_path, added)?; self.write_init_meta_json(&dir_path, added)?;
log::info!( log::info!(
"Syncback: Created {} directory at {} with init.meta.json5", "Syncback: Updated {} at {}/init.meta.json5",
added.class_name, added.class_name,
dir_path.display() dir_path.display()
); );
@ -872,7 +1333,7 @@ impl ApiService {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
log::info!( log::info!(
"Syncback: Created {} at {}", "Syncback: Updated {} at {}",
added.class_name, added.class_name,
file_path.display() file_path.display()
); );

View file

@ -193,6 +193,14 @@ pub struct ServerInfoResponse {
/// Other property changes will be ignored. /// Other property changes will be ignored.
#[serde(default)] #[serde(default)]
pub sync_source_only: bool, pub sync_source_only: bool,
/// When true, hidden/internal services should be ignored during sync.
/// The plugin should not mark these services as "to delete" during forward sync.
#[serde(default)]
pub ignore_hidden_services: bool,
/// List of service names that are considered "visible" when ignoreHiddenServices is true.
/// Services not in this list should be ignored during sync operations.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
pub visible_services: Vec<String>,
} }
// Serialize place IDs as f64 to avoid msgpack uint64 encoding issues with Lua // Serialize place IDs as f64 to avoid msgpack uint64 encoding issues with Lua

View file

@ -242,6 +242,40 @@ impl TestServeSession {
Ok(deserialize_msgpack(&body).expect("Server returned malformed response")) Ok(deserialize_msgpack(&body).expect("Server returned malformed response"))
} }
/// Post to /api/write to simulate plugin syncback operations.
/// Uses the library's WriteRequest type for proper serialization.
/// Uses human-readable msgpack format to match server expectations.
pub fn post_api_write(
&self,
request: &librojo::web_api::WriteRequest,
) -> Result<(), reqwest::Error> {
use serde::Serialize;
let url = format!("http://localhost:{}/api/write", self.port);
// Serialize with human-readable mode to match server expectations
let mut body = Vec::new();
let mut serializer = rmp_serde::Serializer::new(&mut body)
.with_human_readable()
.with_struct_map();
request
.serialize(&mut serializer)
.expect("Failed to serialize WriteRequest");
let client = reqwest::blocking::Client::new();
let response = client.post(url).body(body).send()?;
if !response.status().is_success() {
panic!(
"Write request failed with status {}: {}",
response.status(),
response.text().unwrap_or_default()
);
}
Ok(())
}
} }
fn deserialize_msgpack<'a, T: Deserialize<'a>>( fn deserialize_msgpack<'a, T: Deserialize<'a>>(

View file

@ -2,3 +2,4 @@ mod build;
mod clean_mode; mod clean_mode;
mod serve; mod serve;
mod syncback; mod syncback;
mod syncback_format_transitions;

View file

@ -681,3 +681,115 @@ fn forced_parent() {
assert_snapshot!("forced_parent_serialize_model", model); assert_snapshot!("forced_parent_serialize_model", model);
}); });
} }
/// Test that plugin syncback via /api/write doesn't create duplicate files
/// when sending an AddedInstance that already exists on the filesystem.
///
/// This tests the fix for the issue where pulling an instance that appeared
/// as "to delete" (due to duplicate detection issues) would create additional
/// duplicate files instead of updating the existing ones.
///
/// The fix has two layers:
/// 1. Tree lookup: Check if instance exists in Rojo's tree, update in place
/// 2. Filesystem check: Detect existing file format and preserve it
#[test]
fn api_write_existing_instance() {
use librojo::web_api::{AddedInstance, WriteRequest};
use rbx_dom_weak::types::Variant;
use std::collections::HashMap;
run_serve_test("api_write_existing", |session, _redactions| {
let info = session.get_api_rojo().unwrap();
let root_id = info.root_instance_id;
// Read the tree to find ServerScriptService
let read_response = session.get_api_read(root_id).unwrap();
// Find the ServerScriptService instance
let sss_id = read_response
.instances
.iter()
.find(|(_, inst)| inst.class_name == "ServerScriptService")
.map(|(id, _)| *id)
.expect("ServerScriptService should exist");
// Verify initial state on filesystem
let event_service_dir = session.path().join("src").join("EventService");
let init_file = event_service_dir.join("init.server.luau");
assert!(
event_service_dir.exists(),
"EventService directory should exist before test"
);
assert!(
init_file.exists(),
"init.server.luau should exist before test"
);
// This is the critical file we're checking - it should NOT be created
let standalone_file = session.path().join("src").join("EventService.server.luau");
assert!(
!standalone_file.exists(),
"EventService.server.luau should NOT exist initially"
);
// Simulate plugin sending an AddedInstance for EventService
// This happens when user "pulls" an instance that appeared as "to delete"
let mut properties = HashMap::new();
properties.insert(
"Source".to_string(),
Variant::String("-- Updated EventService\nreturn {}".to_string()),
);
let added_instance = AddedInstance {
parent: Some(sss_id),
name: "EventService".to_string(),
class_name: "Script".to_string(),
properties,
children: vec![AddedInstance {
parent: None,
name: "AcidRainEvent".to_string(),
class_name: "ModuleScript".to_string(),
properties: {
let mut p = HashMap::new();
p.insert(
"Source".to_string(),
Variant::String("-- Updated AcidRainEvent\nreturn {}".to_string()),
);
p
},
children: vec![],
}],
};
let instance_ref = rbx_dom_weak::types::Ref::new();
let mut added_map = HashMap::new();
added_map.insert(instance_ref, added_instance);
let write_request = WriteRequest {
session_id: info.session_id.clone(),
removed: vec![],
added: added_map,
updated: vec![],
};
session
.post_api_write(&write_request)
.expect("Write request should succeed");
// Give the server time to process
std::thread::sleep(std::time::Duration::from_millis(200));
// CRITICAL ASSERTION: No duplicate standalone file should be created
// The existing directory structure (EventService/init.server.luau) should be preserved
assert!(
!standalone_file.exists(),
"EventService.server.luau should NOT be created - the existing directory structure must be preserved"
);
// The directory should still exist
assert!(
event_service_dir.exists(),
"EventService directory should still exist after syncback"
);
});
}

File diff suppressed because it is too large Load diff