From 9aa3145b0607883f80d7e67223fc5ab4f0e4c864 Mon Sep 17 00:00:00 2001 From: Joe Date: Sun, 1 Mar 2026 09:19:41 -0600 Subject: [PATCH] Enhance MCP sync functionality and improve documentation - Updated the TODO list to include a task for ensuring well-formed MCP route documentation, enhancing agent usability. - Modified the MCP stream handling to support additional parameters for sync commands, including mode and overrides for better control during sync operations. - Introduced a new function to enrich sync changes with filesystem paths, improving traceability of changes. - Updated API documentation to reflect new sync modes and override capabilities, providing clearer guidance for users. These changes aim to improve the overall functionality and user experience of the MCP integration and sync process. --- TODO.txt | 2 + plugin/src/App/init.lua | 208 ++++++++++++-- plugin/src/McpStream.lua | 8 +- src/web/api.rs | 42 ++- src/web/mcp.rs | 583 +++++++++++++++++++++++++++++++++++++-- 5 files changed, 801 insertions(+), 42 deletions(-) diff --git a/TODO.txt b/TODO.txt index f011c141..c3a83af5 100644 --- a/TODO.txt +++ b/TODO.txt @@ -351,6 +351,8 @@ can we allow the terminal to stay so i can like run some git commands myself wit - ensure this doesn't interrupt the connection of the mcp, since i think we shutdown the api? we should just not shutdown the api or something * add logging for all agent connect/disconnects in the terminal - also add logging for commands (including the json) it runs and the resulting json. try to keep this on one line please, json compact. +* ensure all mcp route documentation is well formed and gives the agent knowledge on how to use this system to the fullest. + - I did .md based docs over with my D:\UserGenerated\studio-rust-mcp-server codebase, we should probably switch to this as it's cleaner [sync] right now sync is running REALLY slow diff --git a/plugin/src/App/init.lua b/plugin/src/App/init.lua index 1c581b26..98c3d7a0 100644 --- a/plugin/src/App/init.lua +++ b/plugin/src/App/init.lua @@ -49,6 +49,12 @@ local Theme = require(script.Theme) local Http = require(Packages.Http) local encodeService = require(Plugin.ChangeBatcher.encodeService) +local encodeProperty = require(Plugin.ChangeBatcher.encodeProperty) +local SHA1 = require(Plugin.SHA1) +local RbxDom = require(Packages.RbxDom) +local decodeValue = require(Plugin.Reconciler.decodeValue) +local getProperty = require(Plugin.Reconciler.getProperty) +local trueEquals = require(Plugin.Reconciler.trueEquals) local Page = require(script.Page) local Notifications = require(script.Components.Notifications) @@ -202,8 +208,8 @@ function App:init() if RunService:IsEdit() then self.mcpStream = McpStream.new({ - onSyncCommand = function(requestId) - return self:startMcpSync(requestId) + onSyncCommand = function(requestId, mode, overrides) + return self:startMcpSync(requestId, mode, overrides) end, getPluginConfig = function(key) return Settings:get(key) @@ -1117,7 +1123,10 @@ function App:endSession() Log.trace("Session terminated by user") end -function App:startMcpSync(requestId) +function App:startMcpSync(requestId, mode, overrides) + mode = mode or "standard" + overrides = overrides or {} + return Promise.new(function(resolve, _reject) if self.state.appStatus == AppStatus.Connected then resolve({ @@ -1192,13 +1201,71 @@ function App:startMcpSync(requestId) local patchTree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }, gitMetadata) mcpPatchTree = patchTree - local allPreSelected = true + -- Apply overrides: look up each override by id in the tree, verify, set selection + local overrideSelections = {} + for _, override in overrides do + local node = patchTree:getNode(override.id) + if not node or not node.patchType then + Log.trace("MCP sync: override id {} not found in patch tree, skipping", override.id) + continue + end + + local verified = true + + if override.studioHash and node.instance and node.instance:IsA("LuaSourceContainer") then + local source = node.instance.Source + local gitBlob = "blob " .. tostring(#source) .. "\0" .. source + local currentHash = SHA1(buffer.fromstring(gitBlob)) + if currentHash ~= override.studioHash then + Log.info( + "MCP sync: override {} studioHash mismatch (expected={}, got={})", + override.id, + override.studioHash, + currentHash + ) + verified = false + end + end + + if verified and override.expectedProperties and node.instance then + for propName, expectedEncoded in override.expectedProperties do + local decodeOk, expectedValue = decodeValue(expectedEncoded, instanceMap) + if not decodeOk then + Log.info( + "MCP sync: override {} could not decode expected value for {}", + override.id, + propName + ) + verified = false + break + end + local readOk, currentValue = getProperty(node.instance, propName) + if not readOk then + Log.info("MCP sync: override {} could not read property {}", override.id, propName) + verified = false + break + end + if not trueEquals(expectedValue, currentValue) then + Log.info("MCP sync: override {} property {} mismatch", override.id, propName) + verified = false + break + end + end + end + + if verified then + overrideSelections[override.id] = override.direction + end + end + + -- Compute resolution state: check defaults + overrides + local allResolved = true local hasSelectableNodes = false patchTree:forEach(function(node) if node.patchType then hasSelectableNodes = true - if node.defaultSelection == nil then - allPreSelected = false + if node.defaultSelection == nil and overrideSelections[node.id] == nil then + allResolved = false end end end) @@ -1212,14 +1279,55 @@ function App:startMcpSync(requestId) return "Accept" end + -- DRYRUN: return full change list without applying + if mode == "dryrun" then + Log.info("MCP sync: dryrun mode, returning changes without applying") + local changes = self:_buildMcpChangeList(patchTree, patch, instanceMap, nil, true) + resolveOnce({ + status = "dryrun", + changes = changes, + }) + return "Abort" + end + + -- FASTFAIL: if unresolved changes remain, fail immediately + if mode == "fastfail" and not allResolved then + Log.info("MCP sync: fastfail mode, unresolved changes exist") + local changes = self:_buildMcpChangeList(patchTree, patch, instanceMap, nil, true) + resolveOnce({ + status = "fastfail_unresolved", + changes = changes, + message = "Unresolved changes exist. Provide overrides or use standard mode for user review.", + }) + return "Abort" + end + + -- Merge overrides into default selections + local selections = PatchTree.buildInitialSelections(patchTree) + for nodeId, direction in overrideSelections do + selections[nodeId] = direction + end + + -- Check if all nodes are now resolved (defaults + overrides) + local allPreSelected = true + patchTree:forEach(function(node) + if node.patchType and selections[node.id] == nil then + allPreSelected = false + end + end) + + -- MANUAL: always show UI + if mode == "manual" then + allPreSelected = false + end + if allPreSelected then - Log.info("MCP sync: all changes pre-selected, fast-forwarding") - local selections = PatchTree.buildInitialSelections(patchTree) - local changes = self:_buildMcpChangeList(patchTree, selections) + Log.info("MCP sync: all changes resolved, fast-forwarding") + local changes = self:_buildMcpChangeList(patchTree, patch, instanceMap, selections, false) local autoSelectedIds = {} patchTree:forEach(function(node) - if node.patchType and node.defaultSelection and node.defaultSelection ~= "ignore" then + if node.patchType and selections[node.id] then autoSelectedIds[node.id] = true end end) @@ -1254,7 +1362,7 @@ function App:startMcpSync(requestId) local result = self.confirmationEvent:Wait() if result == "Abort" then - local changes = self:_buildMcpChangeList(patchTree, nil) + local changes = self:_buildMcpChangeList(patchTree, patch, instanceMap, nil, true) resolveOnce({ status = "rejected", changes = changes, @@ -1264,7 +1372,7 @@ function App:startMcpSync(requestId) end if type(result) == "table" and result.type == "Confirm" then - local changes = self:_buildMcpChangeList(patchTree, result.selections) + local changes = self:_buildMcpChangeList(patchTree, patch, instanceMap, result.selections, false) resolveOnce({ status = "success", changes = changes, @@ -1321,16 +1429,36 @@ function App:startMcpSync(requestId) end) end -function App:_buildMcpChangeList(patchTree, selections) +local SCRIPT_CLASS_SET = { + Script = true, + LocalScript = true, + ModuleScript = true, +} + +function App:_buildMcpChangeList(patchTree, patch, instanceMap, selections, includeAll) local changes = {} + local patchUpdatedLookup = nil + if patch and patch.updated then + patchUpdatedLookup = {} + for _, change in patch.updated do + patchUpdatedLookup[change.id] = change + end + end + patchTree:forEach(function(node) if not node.patchType then return end local direction = nil - if selections then + if includeAll then + if selections and selections[node.id] then + direction = selections[node.id] + elseif node.defaultSelection then + direction = node.defaultSelection + end + elseif selections then local sel = selections[node.id] if sel == "push" then direction = "push" @@ -1355,10 +1483,56 @@ function App:_buildMcpChangeList(patchTree, selections) end local path = table.concat(segments, "/") - table.insert(changes, { + + local entry: any = { path = path, - direction = direction, - }) + direction = direction or "unresolved", + id = node.id, + className = node.className, + patchType = node.patchType, + defaultSelection = node.defaultSelection, + } + + if node.instance and node.instance:IsA("LuaSourceContainer") then + local source = node.instance.Source + local gitBlob = "blob " .. tostring(#source) .. "\0" .. source + entry.studioHash = SHA1(buffer.fromstring(gitBlob)) + end + + if node.patchType == "Edit" and patchUpdatedLookup and node.instance then + local patchChange = patchUpdatedLookup[node.id] + if patchChange and patchChange.changedProperties then + local props = {} + local isScript = SCRIPT_CLASS_SET[node.className] + + for propName, incomingEncoded in patchChange.changedProperties do + if isScript and propName == "Source" then + continue + end + + local descriptor = RbxDom.findCanonicalPropertyDescriptor(node.className, propName) + if not descriptor then + continue + end + + local currentOk, currentEncoded = encodeProperty(node.instance, propName, descriptor) + if not currentOk then + continue + end + + props[propName] = { + current = currentEncoded, + incoming = incomingEncoded, + } + end + + if next(props) then + entry.properties = props + end + end + end + + table.insert(changes, entry) end) return changes diff --git a/plugin/src/McpStream.lua b/plugin/src/McpStream.lua index 614c7cc3..1a446bbd 100644 --- a/plugin/src/McpStream.lua +++ b/plugin/src/McpStream.lua @@ -94,9 +94,13 @@ function McpStream:_tryConnect() end if data.type == "sync" and data.requestId then - Log.info("MCP stream: received sync command (requestId={})", data.requestId) + Log.info( + "MCP stream: received sync command (requestId={}, mode={})", + data.requestId, + data.mode or "standard" + ) - local resultPromise = self._onSyncCommand(data.requestId) + local resultPromise = self._onSyncCommand(data.requestId, data.mode or "standard", data.overrides or {}) resultPromise :andThen(function(result) diff --git a/src/web/api.rs b/src/web/api.rs index 3c3d7f6b..8205d7e8 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -3518,6 +3518,42 @@ fn pick_script_path(instance: InstanceWithMeta<'_>) -> Option { .map(|path| path.to_owned()) } +/// Enrich each SyncChange with `fs_path` by looking up the instance Ref in the +/// RojoTree and reading its InstigatingSource path. +fn enrich_sync_changes_with_fs_path( + changes: &mut [super::mcp::SyncChange], + serve_session: &ServeSession, +) { + use crate::snapshot::InstigatingSource; + use std::str::FromStr; + + let tree = serve_session.tree(); + let project_path = serve_session.root_project().folder_location(); + + for change in changes.iter_mut() { + let ref_str = match &change.id { + Some(id) => id.as_str(), + None => continue, + }; + + let r = match rbx_dom_weak::types::Ref::from_str(ref_str) { + Ok(r) => r, + Err(_) => continue, + }; + + let meta = match tree.get_metadata(r) { + Some(m) => m, + None => continue, + }; + + if let Some(InstigatingSource::Path(path)) = &meta.instigating_source { + if let Ok(relative) = path.strip_prefix(project_path) { + change.fs_path = Some(relative.to_string_lossy().into_owned()); + } + } + } +} + // --------------------------------------------------------------------------- // MCP stream WebSocket: plugin connects here to receive sync commands // --------------------------------------------------------------------------- @@ -3701,7 +3737,11 @@ async fn handle_mcp_stream_connection( msg = websocket.next() => { match msg { Some(Ok(Message::Text(text))) => { - if let Ok(result) = serde_json::from_str::(&text) { + if let Ok(mut result) = serde_json::from_str::(&text) { + enrich_sync_changes_with_fs_path( + &mut result.changes, + &serve_session, + ); let mut slot = mcp_state .result_tx .lock() diff --git a/src/web/mcp.rs b/src/web/mcp.rs index 1970162e..ef0981e8 100644 --- a/src/web/mcp.rs +++ b/src/web/mcp.rs @@ -25,6 +25,18 @@ pub struct PluginConfig { pub place_id: Option, } +/// Agent-specified auto-accept directive for a single instance change. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct SyncOverride { + pub id: String, + pub direction: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub studio_hash: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub expected_properties: Option, +} + /// Command sent from the MCP handler to the plugin via the MCP stream WebSocket. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -32,14 +44,32 @@ pub struct McpSyncCommand { #[serde(rename = "type")] pub command_type: String, pub request_id: String, + #[serde(default)] + pub mode: String, + #[serde(default)] + pub overrides: Vec, } /// A single change entry in the sync result. -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct SyncChange { pub path: String, pub direction: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub id: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub class_name: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub patch_type: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub studio_hash: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub default_selection: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub fs_path: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub properties: Option, } /// Result sent from the plugin back through the MCP stream WebSocket. @@ -229,11 +259,43 @@ fn handle_tools_list(id: Option) -> Response> { "tools": [ { "name": "atlas_sync", - "description": "Sync filesystem changes to Roblox Studio. Equivalent to clicking the Atlas sync button. If all changes are pre-selected by the git system, they are auto-accepted (fast-forward). Otherwise, the user is prompted to review changes in Studio and the call blocks until they accept or reject.", + "description": "Sync filesystem changes to Roblox Studio. Supports modes: 'standard' (default, auto-accepts if all git-resolved, else shows UI), 'manual' (always shows UI), 'fastfail' (fails immediately if unresolved changes exist), 'dryrun' (returns what would change without applying). Supports overrides to auto-accept specific changes by instance id with hash/property verification.", "inputSchema": { "type": "object", - "properties": {}, - "required": [] + "properties": { + "mode": { + "type": "string", + "enum": ["standard", "manual", "fastfail", "dryrun"], + "description": "Sync mode. 'standard' (default): auto-accept if all changes are git-resolved, otherwise show Studio UI. 'manual': always show Studio UI. 'fastfail': fail immediately if any changes are unresolved. 'dryrun': return what would change without applying." + }, + "overrides": { + "type": "array", + "description": "Auto-accept directives for specific instance changes, matched by id from a previous sync response.", + "items": { + "type": "object", + "properties": { + "id": { + "type": "string", + "description": "Server Ref (32-char hex) from a previous sync response." + }, + "direction": { + "type": "string", + "enum": ["push", "pull"], + "description": "Direction to accept: 'push' (Atlas to Studio) or 'pull' (Studio to Atlas)." + }, + "studioHash": { + "type": "string", + "description": "For scripts: SHA1 of git blob of current Studio Source. Override is rejected if hash doesn't match (concurrent edit detected)." + }, + "expectedProperties": { + "type": "object", + "description": "Map of property name to expected current Studio value (RbxDom encoded). Override is rejected if any value doesn't match." + } + }, + "required": ["id", "direction"] + } + } + } } } ] @@ -259,15 +321,32 @@ async fn handle_tools_call( return json_response(&resp, StatusCode::OK); } - handle_atlas_sync(id, mcp_state, active_api_connections).await + let arguments = params + .as_ref() + .and_then(|p| p.get("arguments")) + .cloned() + .unwrap_or(Value::Object(Default::default())); + + handle_atlas_sync(id, arguments, mcp_state, active_api_connections).await } async fn handle_atlas_sync( id: Option, + arguments: Value, mcp_state: Arc, active_api_connections: Arc, ) -> Response> { - // Check if the plugin is connected via the regular API WebSocket (connected mode). + let mode = arguments + .get("mode") + .and_then(|v| v.as_str()) + .unwrap_or("standard") + .to_string(); + + let overrides: Vec = arguments + .get("overrides") + .and_then(|v| serde_json::from_value(v.clone()).ok()) + .unwrap_or_default(); + if active_api_connections.load(Ordering::Relaxed) > 0 { let config_info = { let config = mcp_state @@ -293,7 +372,6 @@ async fn handle_atlas_sync( ); } - // Check if a sync is already in progress (from another agent invocation). if mcp_state .sync_in_progress .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) @@ -306,7 +384,6 @@ async fn handle_atlas_sync( ); } - // Check if the plugin MCP stream is connected. if !mcp_state.plugin_stream_connected.load(Ordering::Relaxed) { mcp_state.sync_in_progress.store(false, Ordering::SeqCst); return tool_response( @@ -317,7 +394,6 @@ async fn handle_atlas_sync( ); } - // Create a oneshot channel for receiving the result from the plugin. let (result_tx, result_rx) = tokio::sync::oneshot::channel(); { let mut slot = mcp_state @@ -331,15 +407,15 @@ async fn handle_atlas_sync( let command = McpSyncCommand { command_type: "sync".to_string(), request_id: request_id.clone(), + mode, + overrides, }; - // Send the sync command to the plugin via the watch channel. if mcp_state.command_tx.send(Some(command)).is_err() { mcp_state.sync_in_progress.store(false, Ordering::SeqCst); return tool_response(id, true, "Failed to send sync command to the plugin."); } - // Await the plugin's result (blocks until plugin responds or disconnects). let result = match result_rx.await { Ok(result) => result, Err(_) => { @@ -354,15 +430,19 @@ async fn handle_atlas_sync( mcp_state.sync_in_progress.store(false, Ordering::SeqCst); - // Clear the command so the watch channel doesn't re-fire on reconnect. let _ = mcp_state.command_tx.send(None); - let is_error = result.status != "success" && result.status != "empty"; + let is_error = !matches!(result.status.as_str(), "success" | "empty" | "dryrun"); let mut text = match result.status.as_str() { "success" => "Sync completed successfully.".to_string(), "empty" => "No changes to sync. Studio is already up to date.".to_string(), "rejected" => "Sync was rejected by the user.".to_string(), + "dryrun" => "Dry run complete. No changes were applied.".to_string(), + "fastfail_unresolved" => { + "Sync failed: unresolved changes exist that require manual review or overrides." + .to_string() + } "already_connected" => "Atlas is already connected to Studio in live sync mode. \ All changes are syncing automatically." .to_string(), @@ -382,8 +462,31 @@ async fn handle_atlas_sync( }; text.push_str(&format!("\n\n{label}:")); for change in &result.changes { - text.push_str(&format!("\n- [{}] {}", change.direction, change.path)); + let fs_info = change + .fs_path + .as_deref() + .map(|p| format!(", fs={p}")) + .unwrap_or_default(); + let type_info = match (change.patch_type.as_deref(), change.class_name.as_deref()) { + (Some(pt), Some(cn)) => format!(" ({pt}, {cn}{fs_info})"), + (Some(pt), None) => format!(" ({pt}{fs_info})"), + _ if !fs_info.is_empty() => format!(" ({fs_info})"), + _ => String::new(), + }; + text.push_str(&format!( + "\n- [{}] {}{}", + change.direction, change.path, type_info + )); } + + let json_block = serde_json::json!({ + "status": result.status, + "changes": result.changes, + }); + text.push_str(&format!( + "\n\n\n{}\n", + serde_json::to_string(&json_block).unwrap_or_default() + )); } tool_response(id, is_error, &text) @@ -448,10 +551,13 @@ mod tests { let cmd = McpSyncCommand { command_type: "sync".to_string(), request_id: "abc-123".to_string(), + mode: "standard".to_string(), + overrides: vec![], }; let json = serde_json::to_value(&cmd).unwrap(); assert_eq!(json["type"], "sync"); assert_eq!(json["requestId"], "abc-123"); + assert_eq!(json["mode"], "standard"); } #[test] @@ -499,6 +605,7 @@ mod tests { two_way_sync: true, one_shot_sync: false, confirmation_behavior: "Always".to_string(), + place_id: None, }; let json = serde_json::to_value(&cfg).unwrap(); assert_eq!(json["twoWaySync"], true); @@ -549,6 +656,8 @@ mod tests { let cmd = McpSyncCommand { command_type: "sync".to_string(), request_id: "test-1".to_string(), + mode: "standard".to_string(), + overrides: vec![], }; state.command_tx.send(Some(cmd)).unwrap(); @@ -583,6 +692,7 @@ mod tests { two_way_sync: true, one_shot_sync: false, confirmation_behavior: "Initial".to_string(), + place_id: None, }; *state.plugin_config.lock().unwrap() = Some(cfg); @@ -652,12 +762,16 @@ mod tests { mod atlas_sync_guards { use super::*; + fn empty_args() -> Value { + serde_json::json!({}) + } + #[tokio::test] async fn rejects_when_api_connected() { let state = Arc::new(McpSyncState::new()); let conns = Arc::new(AtomicUsize::new(1)); - let resp = handle_atlas_sync(Some(Value::from(1)), state, conns).await; + let resp = handle_atlas_sync(Some(Value::from(1)), empty_args(), state, conns).await; let bytes = resp.into_body().collect().await.unwrap().to_bytes(); let json: Value = serde_json::from_slice(&bytes).unwrap(); @@ -673,10 +787,11 @@ mod tests { two_way_sync: true, one_shot_sync: false, confirmation_behavior: "Always".to_string(), + place_id: None, }); let conns = Arc::new(AtomicUsize::new(1)); - let resp = handle_atlas_sync(Some(Value::from(1)), state, conns).await; + let resp = handle_atlas_sync(Some(Value::from(1)), empty_args(), state, conns).await; let bytes = resp.into_body().collect().await.unwrap().to_bytes(); let json: Value = serde_json::from_slice(&bytes).unwrap(); @@ -691,7 +806,7 @@ mod tests { state.sync_in_progress.store(true, Ordering::SeqCst); let conns = Arc::new(AtomicUsize::new(0)); - let resp = handle_atlas_sync(Some(Value::from(2)), state, conns).await; + let resp = handle_atlas_sync(Some(Value::from(2)), empty_args(), state, conns).await; let bytes = resp.into_body().collect().await.unwrap().to_bytes(); let json: Value = serde_json::from_slice(&bytes).unwrap(); @@ -706,7 +821,8 @@ mod tests { state.plugin_stream_connected.store(false, Ordering::SeqCst); let conns = Arc::new(AtomicUsize::new(0)); - let resp = handle_atlas_sync(Some(Value::from(3)), state.clone(), conns).await; + let resp = + handle_atlas_sync(Some(Value::from(3)), empty_args(), state.clone(), conns).await; let bytes = resp.into_body().collect().await.unwrap().to_bytes(); let json: Value = serde_json::from_slice(&bytes).unwrap(); @@ -736,6 +852,7 @@ mod tests { changes: vec![SyncChange { path: "ServerScriptService/Main.server.luau".to_string(), direction: "push".to_string(), + ..Default::default() }], message: None, }; @@ -744,7 +861,8 @@ mod tests { tx.send(result).unwrap(); }); - let resp = handle_atlas_sync(Some(Value::from(5)), state.clone(), conns).await; + let resp = + handle_atlas_sync(Some(Value::from(5)), empty_args(), state.clone(), conns).await; let bytes = resp.into_body().collect().await.unwrap().to_bytes(); let json: Value = serde_json::from_slice(&bytes).unwrap(); @@ -774,10 +892,12 @@ mod tests { SyncChange { path: "ReplicatedStorage/Foo.luau".to_string(), direction: "push".to_string(), + ..Default::default() }, SyncChange { path: "Workspace/Bar.model.json5".to_string(), direction: "pull".to_string(), + ..Default::default() }, ], message: Some("User rejected the sync changes.".to_string()), @@ -787,7 +907,7 @@ mod tests { tx.send(result).unwrap(); }); - let resp = handle_atlas_sync(Some(Value::from(6)), state, conns).await; + let resp = handle_atlas_sync(Some(Value::from(6)), empty_args(), state, conns).await; let bytes = resp.into_body().collect().await.unwrap().to_bytes(); let json: Value = serde_json::from_slice(&bytes).unwrap(); @@ -813,7 +933,8 @@ mod tests { let _tx = state2.result_tx.lock().unwrap().take(); }); - let resp = handle_atlas_sync(Some(Value::from(7)), state.clone(), conns).await; + let resp = + handle_atlas_sync(Some(Value::from(7)), empty_args(), state.clone(), conns).await; let bytes = resp.into_body().collect().await.unwrap().to_bytes(); let json: Value = serde_json::from_slice(&bytes).unwrap(); @@ -845,7 +966,7 @@ mod tests { .unwrap(); }); - let resp = handle_atlas_sync(Some(Value::from(8)), state, conns).await; + let resp = handle_atlas_sync(Some(Value::from(8)), empty_args(), state, conns).await; let bytes = resp.into_body().collect().await.unwrap().to_bytes(); let json: Value = serde_json::from_slice(&bytes).unwrap(); @@ -853,5 +974,423 @@ mod tests { let text = json["result"]["content"][0]["text"].as_str().unwrap(); assert!(text.contains("No changes to sync")); } + + #[tokio::test] + async fn dryrun_status_is_not_an_error() { + let state = Arc::new(McpSyncState::new()); + state.plugin_stream_connected.store(true, Ordering::SeqCst); + let conns = Arc::new(AtomicUsize::new(0)); + + let state2 = Arc::clone(&state); + tokio::spawn(async move { + let mut rx = state2.command_rx.clone(); + rx.changed().await.unwrap(); + let cmd = rx.borrow().clone().unwrap(); + + let tx = state2.result_tx.lock().unwrap().take().unwrap(); + tx.send(McpSyncResult { + request_id: cmd.request_id, + status: "dryrun".to_string(), + changes: vec![SyncChange { + path: "Workspace/Part".to_string(), + direction: "unresolved".to_string(), + id: Some("aabb".to_string()), + class_name: Some("Part".to_string()), + patch_type: Some("Edit".to_string()), + ..Default::default() + }], + message: None, + }) + .unwrap(); + }); + + let args = serde_json::json!({"mode": "dryrun"}); + let resp = handle_atlas_sync(Some(Value::from(9)), args, state, conns).await; + let bytes = resp.into_body().collect().await.unwrap().to_bytes(); + let json: Value = serde_json::from_slice(&bytes).unwrap(); + + assert_eq!(json["result"]["isError"], false); + let text = json["result"]["content"][0]["text"].as_str().unwrap(); + assert!(text.contains("Dry run complete")); + assert!(text.contains("")); + } + + #[tokio::test] + async fn fastfail_unresolved_is_error() { + let state = Arc::new(McpSyncState::new()); + state.plugin_stream_connected.store(true, Ordering::SeqCst); + let conns = Arc::new(AtomicUsize::new(0)); + + let state2 = Arc::clone(&state); + tokio::spawn(async move { + let mut rx = state2.command_rx.clone(); + rx.changed().await.unwrap(); + let cmd = rx.borrow().clone().unwrap(); + + let tx = state2.result_tx.lock().unwrap().take().unwrap(); + tx.send(McpSyncResult { + request_id: cmd.request_id, + status: "fastfail_unresolved".to_string(), + changes: vec![SyncChange { + path: "ServerScriptService/Script".to_string(), + direction: "unresolved".to_string(), + ..Default::default() + }], + message: Some("Unresolved changes exist.".to_string()), + }) + .unwrap(); + }); + + let args = serde_json::json!({"mode": "fastfail"}); + let resp = handle_atlas_sync(Some(Value::from(10)), args, state, conns).await; + let bytes = resp.into_body().collect().await.unwrap().to_bytes(); + let json: Value = serde_json::from_slice(&bytes).unwrap(); + + assert_eq!(json["result"]["isError"], true); + let text = json["result"]["content"][0]["text"].as_str().unwrap(); + assert!(text.contains("unresolved")); + } + + #[tokio::test] + async fn mode_and_overrides_forwarded_to_command() { + let state = Arc::new(McpSyncState::new()); + state.plugin_stream_connected.store(true, Ordering::SeqCst); + let conns = Arc::new(AtomicUsize::new(0)); + + let state2 = Arc::clone(&state); + tokio::spawn(async move { + let mut rx = state2.command_rx.clone(); + rx.changed().await.unwrap(); + let cmd = rx.borrow().clone().unwrap(); + + assert_eq!(cmd.mode, "fastfail"); + assert_eq!(cmd.overrides.len(), 1); + assert_eq!(cmd.overrides[0].id, "aabbccdd"); + assert_eq!(cmd.overrides[0].direction, "push"); + assert_eq!(cmd.overrides[0].studio_hash.as_deref(), Some("abc123")); + + let tx = state2.result_tx.lock().unwrap().take().unwrap(); + tx.send(McpSyncResult { + request_id: cmd.request_id, + status: "success".to_string(), + changes: vec![], + message: None, + }) + .unwrap(); + }); + + let args = serde_json::json!({ + "mode": "fastfail", + "overrides": [{ + "id": "aabbccdd", + "direction": "push", + "studioHash": "abc123" + }] + }); + let resp = handle_atlas_sync(Some(Value::from(11)), args, state, conns).await; + let bytes = resp.into_body().collect().await.unwrap().to_bytes(); + let json: Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(json["result"]["isError"], false); + } + + #[tokio::test] + async fn mode_defaults_to_standard() { + let state = Arc::new(McpSyncState::new()); + state.plugin_stream_connected.store(true, Ordering::SeqCst); + let conns = Arc::new(AtomicUsize::new(0)); + + let state2 = Arc::clone(&state); + tokio::spawn(async move { + let mut rx = state2.command_rx.clone(); + rx.changed().await.unwrap(); + let cmd = rx.borrow().clone().unwrap(); + + assert_eq!(cmd.mode, "standard"); + assert!(cmd.overrides.is_empty()); + + let tx = state2.result_tx.lock().unwrap().take().unwrap(); + tx.send(McpSyncResult { + request_id: cmd.request_id, + status: "empty".to_string(), + changes: vec![], + message: None, + }) + .unwrap(); + }); + + let resp = handle_atlas_sync(Some(Value::from(12)), empty_args(), state, conns).await; + let bytes = resp.into_body().collect().await.unwrap().to_bytes(); + let json: Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(json["result"]["isError"], false); + } + + #[tokio::test] + async fn enriched_changes_include_json_block() { + let state = Arc::new(McpSyncState::new()); + state.plugin_stream_connected.store(true, Ordering::SeqCst); + let conns = Arc::new(AtomicUsize::new(0)); + + let state2 = Arc::clone(&state); + tokio::spawn(async move { + let mut rx = state2.command_rx.clone(); + rx.changed().await.unwrap(); + let cmd = rx.borrow().clone().unwrap(); + + let tx = state2.result_tx.lock().unwrap().take().unwrap(); + tx.send(McpSyncResult { + request_id: cmd.request_id, + status: "success".to_string(), + changes: vec![SyncChange { + path: "Workspace/Part".to_string(), + direction: "push".to_string(), + id: Some("00aabb".to_string()), + class_name: Some("Part".to_string()), + patch_type: Some("Edit".to_string()), + default_selection: Some("push".to_string()), + fs_path: Some("src/workspace/Part.model.json5".to_string()), + studio_hash: None, + properties: Some(serde_json::json!({ + "Anchored": { + "current": false, + "incoming": true + } + })), + }], + message: None, + }) + .unwrap(); + }); + + let resp = handle_atlas_sync(Some(Value::from(13)), empty_args(), state, conns).await; + let bytes = resp.into_body().collect().await.unwrap().to_bytes(); + let json: Value = serde_json::from_slice(&bytes).unwrap(); + + let text = json["result"]["content"][0]["text"].as_str().unwrap(); + assert!(text.contains("")); + assert!(text.contains("")); + assert!(text.contains("Edit")); + assert!(text.contains("Part")); + assert!(text.contains("fs=src/workspace/Part.model.json5")); + + let json_start = text.find("\n").unwrap() + 7; + let json_end = text.find("\n").unwrap(); + let inner: Value = serde_json::from_str(&text[json_start..json_end]).unwrap(); + assert_eq!(inner["status"], "success"); + assert_eq!(inner["changes"][0]["id"], "00aabb"); + assert_eq!(inner["changes"][0]["className"], "Part"); + assert_eq!( + inner["changes"][0]["fsPath"], + "src/workspace/Part.model.json5" + ); + assert_eq!( + inner["changes"][0]["properties"]["Anchored"]["current"], + false + ); + assert_eq!( + inner["changes"][0]["properties"]["Anchored"]["incoming"], + true + ); + } + } + + // -- New wire type tests for added structs -------------------------------- + + mod new_wire_types { + use super::*; + + #[test] + fn sync_override_serializes_camel_case() { + let ov = SyncOverride { + id: "aabbccdd".to_string(), + direction: "push".to_string(), + studio_hash: Some("abc123".to_string()), + expected_properties: Some(serde_json::json!({"Anchored": true})), + }; + let json = serde_json::to_value(&ov).unwrap(); + assert_eq!(json["id"], "aabbccdd"); + assert_eq!(json["direction"], "push"); + assert_eq!(json["studioHash"], "abc123"); + assert_eq!(json["expectedProperties"]["Anchored"], true); + } + + #[test] + fn sync_override_defaults_optional_fields() { + let json = serde_json::json!({ + "id": "aabb", + "direction": "pull" + }); + let ov: SyncOverride = serde_json::from_value(json).unwrap(); + assert_eq!(ov.id, "aabb"); + assert_eq!(ov.direction, "pull"); + assert!(ov.studio_hash.is_none()); + assert!(ov.expected_properties.is_none()); + } + + #[test] + fn sync_override_omits_none_on_serialize() { + let ov = SyncOverride { + id: "aabb".to_string(), + direction: "push".to_string(), + studio_hash: None, + expected_properties: None, + }; + let json = serde_json::to_value(&ov).unwrap(); + assert!(json.get("studioHash").is_none()); + assert!(json.get("expectedProperties").is_none()); + } + + #[test] + fn mcp_sync_command_includes_mode_and_overrides() { + let cmd = McpSyncCommand { + command_type: "sync".to_string(), + request_id: "r1".to_string(), + mode: "dryrun".to_string(), + overrides: vec![SyncOverride { + id: "x".to_string(), + direction: "pull".to_string(), + studio_hash: None, + expected_properties: None, + }], + }; + let json = serde_json::to_value(&cmd).unwrap(); + assert_eq!(json["mode"], "dryrun"); + assert_eq!(json["overrides"][0]["id"], "x"); + } + + #[test] + fn mcp_sync_command_defaults_mode_and_overrides() { + let json = serde_json::json!({ + "type": "sync", + "requestId": "r1" + }); + let cmd: McpSyncCommand = serde_json::from_value(json).unwrap(); + assert_eq!(cmd.mode, ""); + assert!(cmd.overrides.is_empty()); + } + + #[test] + fn sync_change_serializes_all_fields() { + let change = SyncChange { + path: "Workspace/Part".to_string(), + direction: "push".to_string(), + id: Some("aabb".to_string()), + class_name: Some("Part".to_string()), + patch_type: Some("Edit".to_string()), + studio_hash: Some("hash123".to_string()), + default_selection: Some("push".to_string()), + fs_path: Some("src/workspace/Part.model.json5".to_string()), + properties: Some( + serde_json::json!({"Anchored": {"current": false, "incoming": true}}), + ), + }; + let json = serde_json::to_value(&change).unwrap(); + assert_eq!(json["id"], "aabb"); + assert_eq!(json["className"], "Part"); + assert_eq!(json["patchType"], "Edit"); + assert_eq!(json["studioHash"], "hash123"); + assert_eq!(json["defaultSelection"], "push"); + assert_eq!(json["fsPath"], "src/workspace/Part.model.json5"); + assert!(json["properties"]["Anchored"].is_object()); + } + + #[test] + fn sync_change_omits_none_fields() { + let change = SyncChange { + path: "Test".to_string(), + direction: "push".to_string(), + ..Default::default() + }; + let json = serde_json::to_value(&change).unwrap(); + assert!(json.get("id").is_none()); + assert!(json.get("className").is_none()); + assert!(json.get("patchType").is_none()); + assert!(json.get("studioHash").is_none()); + assert!(json.get("defaultSelection").is_none()); + assert!(json.get("fsPath").is_none()); + assert!(json.get("properties").is_none()); + } + + #[test] + fn enriched_sync_result_roundtrip() { + let result = McpSyncResult { + request_id: "r1".to_string(), + status: "dryrun".to_string(), + changes: vec![ + SyncChange { + path: "ServerScriptService/Main".to_string(), + direction: "push".to_string(), + id: Some("00aa".to_string()), + class_name: Some("Script".to_string()), + patch_type: Some("Edit".to_string()), + studio_hash: Some("hashval".to_string()), + default_selection: Some("push".to_string()), + fs_path: None, + properties: None, + }, + SyncChange { + path: "Workspace/Part".to_string(), + direction: "unresolved".to_string(), + id: Some("00bb".to_string()), + class_name: Some("Part".to_string()), + patch_type: Some("Edit".to_string()), + studio_hash: None, + default_selection: None, + fs_path: Some("src/workspace/Part.model.json5".to_string()), + properties: Some( + serde_json::json!({"Position": {"current": [0,0,0], "incoming": [1,2,3]}}), + ), + }, + ], + message: None, + }; + let serialized = serde_json::to_string(&result).unwrap(); + let deserialized: McpSyncResult = serde_json::from_str(&serialized).unwrap(); + assert_eq!(deserialized.changes.len(), 2); + assert_eq!( + deserialized.changes[0].studio_hash.as_deref(), + Some("hashval") + ); + assert_eq!( + deserialized.changes[1].fs_path.as_deref(), + Some("src/workspace/Part.model.json5") + ); + assert!(deserialized.changes[1].properties.is_some()); + } + } + + // -- Input schema tests --------------------------------------------------- + + mod input_schema_tests { + use super::*; + + #[test] + fn tools_list_has_mode_and_overrides_in_schema() { + let resp = handle_tools_list(Some(Value::from(1))); + let rt = tokio::runtime::Runtime::new().unwrap(); + let bytes = rt.block_on(async { resp.into_body().collect().await.unwrap().to_bytes() }); + let json: Value = serde_json::from_slice(&bytes).unwrap(); + + let schema = &json["result"]["tools"][0]["inputSchema"]; + assert!(schema["properties"]["mode"].is_object()); + assert!(schema["properties"]["overrides"].is_object()); + + let mode_enum = schema["properties"]["mode"]["enum"].as_array().unwrap(); + let mode_strs: Vec<&str> = mode_enum.iter().filter_map(|v| v.as_str()).collect(); + assert!(mode_strs.contains(&"standard")); + assert!(mode_strs.contains(&"manual")); + assert!(mode_strs.contains(&"fastfail")); + assert!(mode_strs.contains(&"dryrun")); + + let override_items = &schema["properties"]["overrides"]["items"]; + assert_eq!(override_items["properties"]["id"]["type"], "string"); + assert!(override_items["required"] + .as_array() + .unwrap() + .contains(&Value::from("id"))); + assert!(override_items["required"] + .as_array() + .unwrap() + .contains(&Value::from("direction"))); + } } }